Re: [PATCH net-next v4 3/3] net: libwx: support vf per-queue statistics via ethtool -S

[Simon Horman <horms@kernel.org>]

On Thu, Jun 11, 2026 at 04:46:08PM +0800, mengyuanlou@net-swift.com wrote:
> 
> 
> > 2026年6月11日 14:48，Simon Horman <horms@kernel.org> 写道：
> > 
> > On Mon, Jun 08, 2026 at 06:39:46PM +0800, Mengyuan Lou wrote:
> >> Add per-queue TX/RX packet and byte counters to the VF ethtool stats
> >> table.
> >> The stats length is now dynamically determined based on whether the
> >> device is a VF or PF via wx_stats_len().
> >> 
> >> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
> >> ---
> >> .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 48 +++++++++++++++----
> >> drivers/net/ethernet/wangxun/libwx/wx_hw.c    |  4 ++
> >> .../net/ethernet/wangxun/libwx/wx_vf_common.c |  2 +
> >> 3 files changed, 44 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> >> index 30c6ef6103ac..86f6ac63acf7 100644
> >> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> >> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> >> @@ -48,6 +48,10 @@ static const struct wx_stats wx_gstrings_stats[] = {
> >> WX_STAT("rx_hwtstamp_cleared", rx_hwtstamp_cleared),
> >> };
> >> 
> >> +static const struct wx_stats wx_gstrings_stats_vf[] = {
> >> + WX_STAT("non_eop_descs", non_eop_descs),
> >> +};
> >> +
> > 
> > The AI-generated review available at https://netdev-ai.bots.linux.dev/sashiko/
> > flags that this is user-exposed and probably could benefit from a bit
> > more explanation in the patch description.
> > 
> >> static const struct wx_stats wx_gstrings_fdir_stats[] = {
> >> WX_STAT("fdir_match", stats.fdirmatch),
> >> WX_STAT("fdir_miss", stats.fdirmiss),
> >> @@ -69,15 +73,26 @@ static const struct wx_stats wx_gstrings_rsc_stats[] = {
> >> #define WX_QUEUE_STATS_LEN ( \
> >> (WX_NUM_TX_QUEUES + WX_NUM_RX_QUEUES) * \
> >> (sizeof(struct wx_queue_stats) / sizeof(u64)))
> >> -#define WX_GLOBAL_STATS_LEN  ARRAY_SIZE(wx_gstrings_stats)
> >> #define WX_FDIR_STATS_LEN  ARRAY_SIZE(wx_gstrings_fdir_stats)
> >> #define WX_RSC_STATS_LEN  ARRAY_SIZE(wx_gstrings_rsc_stats)
> >> -#define WX_STATS_LEN (WX_GLOBAL_STATS_LEN + WX_QUEUE_STATS_LEN)
> >> +
> >> +static inline unsigned int wx_global_stats_len(const struct wx *wx)
> >> +{
> >> + return wx->pdev->is_virtfn ?
> >> +        ARRAY_SIZE(wx_gstrings_stats_vf) : ARRAY_SIZE(wx_gstrings_stats);
> >> +}
> > 
> > Please don't use the inline keyword in .c files unless there is a
> > demonstrable - usually performance - reason to do so. Rather, please
> > let the compiler inline code (or not).
> > 
> > It is of course fine to use the inline keyword in .h files.
> 
> Ok，I will drop it.
> > 
> >> +
> >> +static inline unsigned int wx_stats_len(const struct wx *wx)
> >> +{
> >> + struct net_device *netdev = wx->netdev;
> > 
> > netdev seems unused.
> > 
> > Flagged by AI-generated review on sashiko.dev 
> 
> 
> #define WX_NUM_RX_QUEUES netdev->num_tx_queues
> #define WX_NUM_TX_QUEUES netdev->num_tx_queues
> 
> #define WX_QUEUE_STATS_LEN ( \
> (WX_NUM_TX_QUEUES + WX_NUM_RX_QUEUES) * \
> (sizeof(struct wx_queue_stats) / sizeof(u64)))
> 
> netdev is used.

Thanks, sorry for missing that.

Perhaps it would clearer if WX_NUM_RX_QUEUES and WX_NUM_TX_QUEUES took
arguments. And in that case, perhaps were functions rather than macros.
But maybe that is orthogonal to this patch-set.

> 
> > 
> >> +
> >> + return wx_global_stats_len(wx) + WX_QUEUE_STATS_LEN;
> >> +}
> >> 
> >> int wx_get_sset_count(struct net_device *netdev, int sset)
> >> {
> >> struct wx *wx = netdev_priv(netdev);
> >> - int len = WX_STATS_LEN;
> >> + int len = wx_stats_len(wx);
> >> 
> >> switch (sset) {
> >> case ETH_SS_STATS:
> >> @@ -100,8 +115,11 @@ void wx_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
> >> 
> >> switch (stringset) {
> >> case ETH_SS_STATS:
> >> - for (i = 0; i < WX_GLOBAL_STATS_LEN; i++)
> >> - ethtool_puts(&p, wx_gstrings_stats[i].stat_string);
> >> + for (i = 0; i < wx_global_stats_len(wx); i++)
> >> + if (wx->pdev->is_virtfn)
> >> + ethtool_puts(&p, wx_gstrings_stats_vf[i].stat_string);
> >> + else
> >> + ethtool_puts(&p, wx_gstrings_stats[i].stat_string);
> > 
> > The AI-generated review at https://netdev-ai.bots.linux.dev/sashiko/ also
> > flags that these seem to cover statistics that can be exposed via
> > netdev_stat_ops, which is preferred.
> > 
> > If consistency with the PF stats is desired for VF stats, then perhaps
> > netdev_stat_ops can also be implemented for PF queues.
> 
> Netdev_stat_ops will add in another. changlogs v3 just remove it.
> > 
> >> if (test_bit(WX_FLAG_FDIR_CAPABLE, wx->flags)) {
> >> for (i = 0; i < WX_FDIR_STATS_LEN; i++)
> >> ethtool_puts(&p, wx_gstrings_fdir_stats[i].stat_string);
> > 
> > ...
> 
>