Re: Per-cpu stats for network interfaces

[Jason Thorpe <thorpej%me.com@localhost>]

> On Jan 27, 2020, at 1:57 AM, Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost> wrote:
> 
> 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.

Yes, I agree the stats[...] thing is a little ugly.  That's the way e.g. netinet counters are done currently, however (when they're modified in a cluster).  The IF_STAT_COLLISIONS naming for this usage is also temporary until every driver is transitioned to the new API.  I suppose I could hide that in the API you suggest (and I would probably want to make a similar change to the other net_stats.h usage).

Curious what atomicity / memory ordering issues you're referring to... these are not global counters, but cpu-local, and so this is no worse than the existing use of e.g. "ifp->if_opackets++;" on a 32-bit system.

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

I have it defined in my local tree, along with a couple of other additional change to "struct ifnet" that makes non-converted drivers fail to compile.  My plan is for it to become always-on.  However, thinking about it a little more, it probably does make sense to conditional-ize it somewhat ... platforms like ARM, x86, sparc, etc. would define __HAVE_NET_STATS_PERCPU in <machine/types.h>, m68k (and sh3, ...) would not.  I wouldn't want to continue having "if_data" directly inside "struct ifnet", however, so I'll have to think about that part a little more.

If I did that, however, I would do it in a way that applied to other net_stats.h users (e.g. TCP stats).  So, I'll leave that for a future cleanup effort. This is going to be an iterative process so that not all drivers have to convert on day-zero.

> 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?

Yes, adding those two things together is a common pattern.  I seems reasonable to have something like:

static inline void
if_statadd2(ifnet *ifp, if_stat_t x1, uint64_t v1, if_stat_t x2, uint64_t v2)
{
	/* Do the obvious thing here. */
}

-- thorpej