Re: Per-cpu stats for network interfaces

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

On 2020/01/26 1:40, Jason Thorpe wrote:
> Another thing needed to make the network stack MP-safe is fixing the way interface statistics are kept.  We dealt with this some years ago for protocol statistics.  This change builds upon that.
>
> For network interfaces, it's a little trickier.
>
> 1- The stats are intermingled with other less-volatile information in "struct if_data".
>
> 2- There are a lot more network interface drivers than there are protocols, making the transition to the New Way harder.
>
> IMO, "struct if_data" should be used only for exporting the information to user-space, and this change starts us down this path (eventually, "struct ifnet" will no longer contain a "struct if_data").
>
> To ease the transition for drivers, the API is a little different than the other net_stats-based ones.  Specifically, it doesn't actually flip the switch on per-cpu stats just yet, although all of the stuff is there.  Instead, it allows un-converted drivers to co-exist with drivers that have been converted to the new API.  This is done with some preprocessor macros that are at the root of why it's a little different.  If we want, we can tidy these differences up later, or we can live with them... not a decision we need to make now.
>
> Attached is a preliminary diff, along with a couple of example driver conversions.  Feedback, please!  If there is consensus on this approach, I'll convert the drivers as time permits (it's pretty simple mechanical change).

I think it is better for device drivers to use new API such as
====================
	uint64_t *stats = IF_STAT_GETREF(ifp);
	if_statadd_ref(stats, if_collisions, CSR_READ(sc, WMREG_COLC)); // new API
	// some more code
	IF_STAT_PUTREF(ifp);
====================
instead of such as
====================
	uint64_t *stats = IF_STAT_GETREF(ifp);
	stats[IF_STAT_COLLISIONS] += CSR_READ(sc, WMREG_COLC);
	// some more code
	IF_STAT_PUTREF(ifp);
====================
code, because that can hide both 64bit counter in 32bit architecture
atomicity issue and memory ordering issue from device drivers.

It seems "__IF_STATS_PERCPU" is not defined.  "__IF_STATS_PERCPU" is
defined when MULTIPROCESSOR is defined, isn't it?

For other parts, looks good to me.  Thank you for your reworks!

By the way, it would be helpful to add new API which add both packets
counter and bytes counter.  Thought?


Thanks,

--
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>