Message-ID: <PAXPR04MB918591AA3C3A41AE794DB41489059@PAXPR04MB9185.eurprd04.prod.outlook.com>
Date:   Mon, 14 Nov 2022 14:53:00 +0000
From:   Shenwei Wang <shenwei.wang@....com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Wei Fang <wei.fang@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
        kernel test robot <lkp@...el.com>
Subject: RE: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool
 statistics



> -----Original Message-----
> From: Alexander Lobakin <alexandr.lobakin@...el.com>
> Sent: Monday, November 14, 2022 7:46 AM
> To: Shenwei Wang <shenwei.wang@....com>
> Cc: Alexander Lobakin <alexandr.lobakin@...el.com>; David S. Miller
> <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub
> Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; Alexei
> Starovoitov <ast@...nel.org>; Daniel Borkmann <daniel@...earbox.net>;
> > @@ -29,6 +29,7 @@ config FEC
> >       select CRC32
> >       select PHYLIB
> >       select PAGE_POOL
> > +     select PAGE_POOL_STATS
> 
> Drivers should never select PAGE_POOL_STATS. This Kconfig option was made to
> allow user to choose whether he wants stats or better performance on slower
> systems. It's pure user choice, if something doesn't build or link, it must be
> guarded with IS_ENABLED(CONFIG_PAGE_POOL_STATS).

As the PAGE_POOL_STATS is becoming the infrastructure codes for many drivers, it is
redundant for every driver to implement the stub function in case it is not selected. These
stub functions should be provided by PAGE_POOL_STATS itself if the option is not selected.

> 
> >       imply NET_SELFTESTS
> >       help
> >         Say Y here if you want to use the built-in 10/100 Fast
> > ethernet diff --git a/drivers/net/ethernet/freescale/fec.h
> > b/drivers/net/ethernet/freescale/fec.h
> > index 61e847b18343..5ba1e0d71c68 100644
> > --- a/drivers/net/ethernet/freescale/fec.h
> > +++ b/drivers/net/ethernet/freescale/fec.h
> > @@ -526,6 +526,19 @@ struct fec_enet_priv_txrx_info {
> >       struct  sk_buff *skb;
> >  };
> >
> > +enum {
> > +     RX_XDP_REDIRECT = 0,
> > +     RX_XDP_PASS,
> > +     RX_XDP_DROP,
> > +     RX_XDP_TX,
> > +     RX_XDP_TX_ERRORS,
> > +     TX_XDP_XMIT,
> > +     TX_XDP_XMIT_ERRORS,
> > +
> > +     /* The following must be the last one */
> > +     XDP_STATS_TOTAL,
> > +};
> > +
> >  struct fec_enet_priv_tx_q {
> >       struct bufdesc_prop bd;
> >       unsigned char *tx_bounce[TX_RING_SIZE]; @@ -546,6 +559,7 @@
> > struct fec_enet_priv_rx_q {
> >       /* page_pool */
> >       struct page_pool *page_pool;
> >       struct xdp_rxq_info xdp_rxq;
> > +     u32 stats[XDP_STATS_TOTAL];
> 
> Still not convinced it is okay to deliberately provoke overflows here, maybe we
> need some more reviewers to help us agree on what is better?
> 
> >
> >       /* rx queue number, in the range 0-7 */
> >       u8 id;
> 
> [...]
> 
> >       case ETH_SS_STATS:
> > -             for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
> > -                     memcpy(data + i * ETH_GSTRING_LEN,
> > -                             fec_stats[i].name, ETH_GSTRING_LEN);
> > +             for (i = 0; i < ARRAY_SIZE(fec_stats); i++) {
> > +                     memcpy(data, fec_stats[i].name, ETH_GSTRING_LEN);
> > +                     data += ETH_GSTRING_LEN;
> > +             }
> > +             for (i = 0; i < ARRAY_SIZE(fec_xdp_stat_strs); i++) {
> > +                     strncpy(data, fec_xdp_stat_strs[i],
> > + ETH_GSTRING_LEN);
> 
> strncpy() is deprecated in favor of strscpy(), there were tons of commits which
> replace the former with the latter across the whole tree.
> 

Got it. 

Thanks.
Shenwei

> > +                     data += ETH_GSTRING_LEN;
> > +             }
> > +             page_pool_ethtool_stats_get_strings(data);
> > +
> >               break;
> >       case ETH_SS_TEST:
> >               net_selftest_get_strings(data);
> 
> [...]
> 
> > +     for (i = fep->num_rx_queues - 1; i >= 0; i--) {
> > +             rxq = fep->rx_queue[i];
> > +             for (j = 0; j < XDP_STATS_TOTAL; j++)
> > +                     rxq->stats[j] = 0;
> 
> (not critical) Just memset(&rxq->stats)?
> 
> > +     }
> > +
> >       /* Don't disable MIB statistics counters */
> >       writel(0, fep->hwp + FEC_MIB_CTRLSTAT);  } @@ -3084,6 +3156,9 @@
> > static void fec_enet_free_buffers(struct net_device *ndev)
> >               for (i = 0; i < rxq->bd.ring_size; i++)
> >                       page_pool_release_page(rxq->page_pool,
> > rxq->rx_skb_info[i].page);
> >
> > +             for (i = 0; i < XDP_STATS_TOTAL; i++)
> > +                     rxq->stats[i] = 0;
> > +
> >               if (xdp_rxq_info_is_reg(&rxq->xdp_rxq))
> >                       xdp_rxq_info_unreg(&rxq->xdp_rxq);
> >               page_pool_destroy(rxq->page_pool);
> > --
> > 2.34.1
> 
> Could you please send a follow-up maybe, fixing at least that
> PAGE_POOL_STATS select and strncpy()?
> 
> [0]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel
> .org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fnetdev%2Fnet-
> next.git%2Fcommit%2F%3Fh%3Dmain%26id%3D6970ef27ff7fd1ce3455b2c6960
> 81503d0c0f8ac&amp;data=05%7C01%7Cshenwei.wang%40nxp.com%7C0f6bfc
> 73c869426e59fc08dac64692d2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C638040303661645473%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&amp;sdata=8NbrJmoDnsbyb8WXU85OIq6BOYCOrXLBm1mjbTi%2Fam
> Q%3D&amp;reserved=0
> 
> Thanks,
> Olek

Powered by blists - more mailing lists