Re: Per-cpu stats for network interfaces

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

HI,

On 2020/01/28 16:35, 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.
> >
> > 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 for your feedback!  I've incorporated your suggestions.  The pattern now actually looks like:
>
> 	net_stat_ref_t nsr = IF_STAT_GETREF(ifp);
> 	if_statinc_ref(nsr, if_opackets);
> 	// ...etc.
> 	IF_STAT_PUTREF(ifp);
>
> net_stat_ref_t is a new opaque type declared in <sys/net_stats.h> to prevent anything subscripting it directly.

Looks good to me! It seems very clean APIs.


> I have also added if_statadd2() for handling the very common pattern of bytes & packets in several places, e.g. instead of this:
>
> 	net_stat_ref_t nsr = IF_STAT_GETREF(ifp);
> 	if_statinc_ref(nsr, if_ipackets);
> 	if_statadd_ref(nsr, if_ibytes, m0->m_pkthdr.len);
> 	IF_STAT_PUTREF(ifp);
>
> ...it's just this:
>
> 	if_statadd2(ifp, if_ipackets, 1, if_ibytes, m0->m_pkthdr.len);
>
> I also simplified a few other bits, but other than the mechanical changes noted here, it's basically unchanged and I think ready to go.
>
> My integration plan:
>
> 1- Check in the basic if_stats.[ch] and if.[ch] changes.  Leave percpu stats *disabled* for now.
>
> 2- Check in covered drivers a few at a time, testing as many as I can for correct behavior.  The changes are very mechanical, and so easy to audit.
>
> 3- Finish converting some of the harder drivers that play nasty tricks with the existing if_data stats.
>
> 4- Verify all ports build releases correctly.
>
> 5- Flip the switch to enable percpu counters.

It's a good way.  I and my co-workers are willing help review and test.


Thanks,

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

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

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