RE: [EXT] Re: [PATCH 2/2] net: fec: add xdp and page pool statistics

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Monday, November 7, 2022 8:26 PM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> >
> > -#define FEC_STATS_SIZE               (ARRAY_SIZE(fec_stats) * sizeof(u64))
> > +static struct fec_xdp_stat {
> > +     char name[ETH_GSTRING_LEN];
> > +     u64 count;
> > +} fec_xdp_stats[XDP_STATS_TOTAL] = {
> > +     { "rx_xdp_redirect", 0 },           /* RX_XDP_REDIRECT = 0, */
> > +     { "rx_xdp_pass", 0 },               /* RX_XDP_PASS, */
> > +     { "rx_xdp_drop", 0 },               /* RX_XDP_DROP, */
> > +     { "rx_xdp_tx", 0 },                 /* RX_XDP_TX, */
> > +     { "rx_xdp_tx_errors", 0 },          /* RX_XDP_TX_ERRORS, */
> > +     { "tx_xdp_xmit", 0 },               /* TX_XDP_XMIT, */
> > +     { "tx_xdp_xmit_errors", 0 },        /* TX_XDP_XMIT_ERRORS, */
> > +};
> 
> Why do you mix the string and the count?

It was following the original coding style but it is not good. Will fix it in the next version.

> 
> > +
> > +#define FEC_STATS_SIZE       ((ARRAY_SIZE(fec_stats) + \
> > +                     ARRAY_SIZE(fec_xdp_stats)) * sizeof(u64))
> >
> >  static void fec_enet_update_ethtool_stats(struct net_device *dev)  {
> >       struct fec_enet_private *fep = netdev_priv(dev);
> > -     int i;
> > +     struct fec_xdp_stat xdp_stats[7];
> 
> You are allocating 7 x name[ETH_GSTRING_LEN], here, which you are not going
> to use. All you really need is u64 xdp_stats[XDP_STATS_TOTAL]
> 
> > +     int off = ARRAY_SIZE(fec_stats);
> > +     struct fec_enet_priv_rx_q *rxq;
> > +     int i, j;
> >
> >       for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
> >               fep->ethtool_stats[i] = readl(fep->hwp +
> > fec_stats[i].offset);
> > +
> > +     for (i = fep->num_rx_queues - 1; i >= 0; i--) {
> > +             rxq = fep->rx_queue[i];
> > +             for (j = 0; j < XDP_STATS_TOTAL; j++)
> > +                     xdp_stats[j].count += rxq->stats[j];
> > +     }
> > +
> > +     for (i = 0; i < XDP_STATS_TOTAL; i++)
> > +             fep->ethtool_stats[i + off] = xdp_stats[i].count;
> 
> It would be more logical to use j here.
> 
> It is also pretty messy. For fec_enet_get_strings() and
> fec_enet_get_sset_count() you deal with the three sets of stats individually. But
> here you combine normal stats and xdp stats in one. It will probably come out
> cleaner if you keep it all separate.

After you said so, I feel it messy too.  Will clean up the codes.

> 
> >  static void fec_enet_get_ethtool_stats(struct net_device *dev,
> >                                      struct ethtool_stats *stats, u64
> > *data)  {
> >       struct fec_enet_private *fep = netdev_priv(dev);
> > +     u64 *dst = data + FEC_STATS_SIZE / 8;
> 
> Why 8? sizeof(u64) would be a bit clearer. Or just use
> ARRAY_SIZE(fec_stats) which is what you are actually wanting.
> 
> >
> >       if (netif_running(dev))
> >               fec_enet_update_ethtool_stats(dev);
> >
> >       memcpy(data, fep->ethtool_stats, FEC_STATS_SIZE);
> > +
> > +     fec_enet_page_pool_stats(fep, dst);
> >  }
> >
> >  static void fec_enet_get_strings(struct net_device *netdev,
> >       u32 stringset, u8 *data)
> >  {
> > +     int off = ARRAY_SIZE(fec_stats);
> >       int i;
> >       switch (stringset) {
> >       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_xdp_stats); i++)
> > +                     memcpy(data + (i + off) * ETH_GSTRING_LEN,
> > +                            fec_xdp_stats[i].name, ETH_GSTRING_LEN);
> > +             off = (i + off) * ETH_GSTRING_LEN;
> > +             page_pool_ethtool_stats_get_strings(data + off);
> 
> Probably simpler is:
> 
>                 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_stats); i++) {
>                         memcpy(data, fec_xdp_stats[i].name, ETH_GSTRING_LEN);
>                         data += ETH_GSTRING_LEN;
>                 }
>                 page_pool_ethtool_stats_get_strings(data);

Yes, this looks better. 😊

Thanks,
Shenwei

> 
>         Andrew