Re: [PATCH v2 RESEND 1/1] net: fec: add xdp and page pool statistics

[Alexander Lobakin <alexandr.lobakin@intel.com>]

From: Shenwei Wang <shenwei.wang@nxp.com>
Date: Thu, 10 Nov 2022 21:40:21 +0000

> > -----Original Message-----
> > From: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Sent: Thursday, November 10, 2022 10:43 AM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>; Paolo Abeni
> > <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Alexei
> > > > at ../drivers/net/ethernet/freescale/fec_main.c:2788:4:
> > > > ../include/linux/fortify-string.h:413:25: warning: call to
> > '__read_overflow2_field'
> > > > declared with attribute warning: detected read beyond size of field
> > > > (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
> > > >   413 |                         __read_overflow2_field(q_size_field, size);
> > > >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > I think you can address it changing fec_xdp_stat_strs definition to:
> > > >
> > > > static const char
> > > > fec_xdp_stat_strs[XDP_STATS_TOTAL][ETH_GSTRING_LEN] =
> > >
> > > That does a problem. How about just change the memcpy to strncpy?
> > 
> > Don't use a static char array, it would consume more memory than the current
> > code. Just replace memcpy()s with strscpy().
> > 
> > Why u32 for the stats tho? It will overflow sooner or later. "To keep it simple
> > and compatible" you can use u64_stats API :)
> 
> The reason to use u32 here is : 1. It is simple to implement. 2. To follow the same
> behavior as the other MAC hardware statistic counters which are all 32bit. 3. I did
> investigate the u64_stats API, and think it is still a little expensive here.

1) u64_stats_t is not much harder.
2) This only means your HW statistics handling in the driver is
   wrong, as every driver which HW has 32-bit counter implements
   64-bit containers and a periodic task to take fresh HW numbers
   and clear them (so that the full stats are stored in the driver
   only).
3) Page Pool stats currently give you much more overhead as they are
   pure 64-bit, not u64_stats_t, with no synchronization.
   What is your machine and how fast your link is? Just curious, I
   never had any serious regressions using u64_stats_t on either
   high-end x86_64 servers or low-end MIPS32.

> 
> Thanks,
> Shenwei
> 
> > 
> > >
> > > Regards,
> > > Shenwei
> > >
> > > > { // ...
> > > >
> > > > Cheers,
> > > >
> > > > Paolo
> > 
> > Thanks,
> > Olek

Thanks,
Olek