Messages in this thread | | | From | Shenwei Wang <> | Subject | RE: [EXT] Re: [PATCH 2/2] net: fec: add xdp and page pool statistics | Date | Tue, 8 Nov 2022 13:32:35 +0000 |
| |
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Monday, November 7, 2022 8:26 PM > To: Shenwei Wang <shenwei.wang@nxp.com> > > > > -#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
| |