Re: [net-next v2] net: ifb: support ethtools stats

On Sat, Nov 27, 2021 at 5:42 AM Jakub Kicinski [off-list ref] wrote:

On Fri, 26 Nov 2021 11:23:05 +0800 xiangxia.m.yue@gmail.com wrote:

quoted

From: Tonghao Zhang <redacted>

With this feature, we can use the ethtools to get tx/rx
queues stats. This patch, introduce the ifb_update_q_stats
helper to update the queues stats, and ifb_q_stats to simplify
the codes. In future, we can add more metrics in ifb_q_stats.

Cc: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Tonghao Zhang <redacted>

quoted

 static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev);
 static int ifb_open(struct net_device *dev);
 static int ifb_close(struct net_device *dev);

+static inline void ifb_update_q_stats(struct ifb_q_stats *stats, int len)

Please remove the "inline" keywords, we prefer to leave the choice to
the compiler (plus it hides "unused function" warnings if the caller
is ever removed).

quoted

+static void ifb_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
+{
+     u8 *p = buf;
+     int i, j;
+
+     switch(stringset) {
+     case ETH_SS_STATS:
+             for (i = 0; i < dev->real_num_rx_queues; i++)
+                     for (j = 0; j < IFB_Q_STATS_LEN; j++)
+                             ethtool_sprintf(&p, "rx_queue_%u_%.18s",
+                                             i, ifb_q_stats_desc[j].desc);
+
+             for (i = 0; i < dev->real_num_tx_queues; i++)
+                     for (j = 0; j < IFB_Q_STATS_LEN; j++)
+                             ethtool_sprintf(&p, "tx_queue_%u_%.18s",
+                                             i, ifb_q_stats_desc[j].desc);
+
+             break;
+     }
+}
+
+static int ifb_get_sset_count(struct net_device *dev, int sset)
+{
+     switch (sset) {
+     case ETH_SS_STATS:
+             return IFB_Q_STATS_LEN * (dev->real_num_rx_queues +
+                    dev->real_num_tx_queues);

Needs to align under opening bracket, try checkpatch --strict.

quoted

+     default:
+             return -EOPNOTSUPP;
+     }
+}
+
+static inline void ifb_fill_stats_data(u64 **data,
+                                    struct ifb_q_stats *q_stats)

another inline

Thanks, Jakub
v3 is sent, https://patchwork.kernel.org/project/netdevbpf/patch/20211128014631.43627-1-xiangxia.m.yue@gmail.com/

The logic itself LGTM.

--
Best regards, Tonghao