Re: Per-cpu stats for network interfaces

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

Hi,

Sorry, I am still reading newer mail...

On 2020/01/27 22:57, Jason Thorpe wrote:
> > 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).

I see.


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

Sorry for lack of my information.  I think the situation that CPU#0 calls
if_statadd() at the same time as CPU#1 calls if_stats_to_if_data().
I think atomic_{load,store}_relaxed() would be required.
Ah, however, if percpu_foreach() is done by xcall, the atomic operation
is not required, as you pointed out.


Thanks,

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

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

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

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