diff mbox series

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

Message ID 20211126032305.13571-1-xiangxia.m.yue@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2] net: ifb: support ethtools stats | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 9 maintainers not CCed: bpf@vger.kernel.org kafai@fb.com daniel@iogearbox.net songliubraving@fb.com kpsingh@kernel.org john.fastabend@gmail.com yhs@fb.com ast@kernel.org andrii@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch fail ERROR: space required before the open parenthesis '('
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline fail Was 0 now: 2

Commit Message

Tonghao Zhang Nov. 26, 2021, 3:23 a.m. UTC
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

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 <xiangxia.m.yue@gmail.com>
---
v2: rebased https://patchwork.kernel.org/project/netdevbpf/patch/20211125020155.6488-2-xiangxia.m.yue@gmail.com/
---
 drivers/net/ifb.c | 146 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 121 insertions(+), 25 deletions(-)

Comments

Jakub Kicinski Nov. 26, 2021, 9:42 p.m. UTC | #1
On Fri, 26 Nov 2021 11:23:05 +0800 xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> 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 <xiangxia.m.yue@gmail.com>

>  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).

> +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.

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

another inline

The logic itself LGTM.
Tonghao Zhang Nov. 29, 2021, 4:57 a.m. UTC | #2
On Sat, Nov 27, 2021 at 5:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 26 Nov 2021 11:23:05 +0800 xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > 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 <xiangxia.m.yue@gmail.com>
>
> >  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).
>
> > +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.
>
> > +     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
diff mbox series

Patch

diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 31f522b8e54e..3d88db708dc5 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -27,6 +27,7 @@ 
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
+#include <linux/ethtool.h>
 #include <linux/etherdevice.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -36,30 +37,55 @@ 
 #include <net/net_namespace.h>
 
 #define TX_Q_LIMIT    32
+
+struct ifb_q_stats {
+	u64 packets;
+	u64 bytes;
+	struct u64_stats_sync	sync;
+};
+
 struct ifb_q_private {
 	struct net_device	*dev;
 	struct tasklet_struct   ifb_tasklet;
 	int			tasklet_pending;
 	int			txqnum;
 	struct sk_buff_head     rq;
-	u64			rx_packets;
-	u64			rx_bytes;
-	struct u64_stats_sync	rsync;
-
-	struct u64_stats_sync	tsync;
-	u64			tx_packets;
-	u64			tx_bytes;
 	struct sk_buff_head     tq;
+	struct ifb_q_stats	rx_stats;
+	struct ifb_q_stats	tx_stats;
 } ____cacheline_aligned_in_smp;
 
 struct ifb_dev_private {
 	struct ifb_q_private *tx_private;
 };
 
+/* For ethtools stats. */
+struct ifb_q_stats_desc {
+	char	desc[ETH_GSTRING_LEN];
+	size_t	offset;
+};
+
+#define IFB_Q_STAT(m)	offsetof(struct ifb_q_stats, m)
+
+static const struct ifb_q_stats_desc ifb_q_stats_desc[] = {
+	{ "packets",	IFB_Q_STAT(packets) },
+	{ "bytes",	IFB_Q_STAT(bytes) },
+};
+
+#define IFB_Q_STATS_LEN	ARRAY_SIZE(ifb_q_stats_desc)
+
 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)
+{
+	u64_stats_update_begin(&stats->sync);
+	stats->packets++;
+	stats->bytes += len;
+	u64_stats_update_end(&stats->sync);
+}
+
 static void ifb_ri_tasklet(struct tasklet_struct *t)
 {
 	struct ifb_q_private *txp = from_tasklet(txp, t, ifb_tasklet);
@@ -83,10 +109,7 @@  static void ifb_ri_tasklet(struct tasklet_struct *t)
 #endif
 		nf_skip_egress(skb, true);
 
-		u64_stats_update_begin(&txp->tsync);
-		txp->tx_packets++;
-		txp->tx_bytes += skb->len;
-		u64_stats_update_end(&txp->tsync);
+		ifb_update_q_stats(&txp->tx_stats, skb->len);
 
 		rcu_read_lock();
 		skb->dev = dev_get_by_index_rcu(dev_net(txp->dev), skb->skb_iif);
@@ -139,18 +162,18 @@  static void ifb_stats64(struct net_device *dev,
 
 	for (i = 0; i < dev->num_tx_queues; i++,txp++) {
 		do {
-			start = u64_stats_fetch_begin_irq(&txp->rsync);
-			packets = txp->rx_packets;
-			bytes = txp->rx_bytes;
-		} while (u64_stats_fetch_retry_irq(&txp->rsync, start));
+			start = u64_stats_fetch_begin_irq(&txp->rx_stats.sync);
+			packets = txp->rx_stats.packets;
+			bytes = txp->rx_stats.bytes;
+		} while (u64_stats_fetch_retry_irq(&txp->rx_stats.sync, start));
 		stats->rx_packets += packets;
 		stats->rx_bytes += bytes;
 
 		do {
-			start = u64_stats_fetch_begin_irq(&txp->tsync);
-			packets = txp->tx_packets;
-			bytes = txp->tx_bytes;
-		} while (u64_stats_fetch_retry_irq(&txp->tsync, start));
+			start = u64_stats_fetch_begin_irq(&txp->tx_stats.sync);
+			packets = txp->tx_stats.packets;
+			bytes = txp->tx_stats.bytes;
+		} while (u64_stats_fetch_retry_irq(&txp->tx_stats.sync, start));
 		stats->tx_packets += packets;
 		stats->tx_bytes += bytes;
 	}
@@ -173,14 +196,83 @@  static int ifb_dev_init(struct net_device *dev)
 		txp->dev = dev;
 		__skb_queue_head_init(&txp->rq);
 		__skb_queue_head_init(&txp->tq);
-		u64_stats_init(&txp->rsync);
-		u64_stats_init(&txp->tsync);
+		u64_stats_init(&txp->rx_stats.sync);
+		u64_stats_init(&txp->tx_stats.sync);
 		tasklet_setup(&txp->ifb_tasklet, ifb_ri_tasklet);
 		netif_tx_start_queue(netdev_get_tx_queue(dev, i));
 	}
 	return 0;
 }
 
+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);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static inline void ifb_fill_stats_data(u64 **data,
+				       struct ifb_q_stats *q_stats)
+{
+	void *stats_base = (void *)q_stats;
+	unsigned int start;
+	size_t offset;
+	int j;
+
+	do {
+		start = u64_stats_fetch_begin_irq(&q_stats->sync);
+		for (j = 0; j < IFB_Q_STATS_LEN; j++) {
+			offset = ifb_q_stats_desc[j].offset;
+			(*data)[j] = *(u64 *)(stats_base + offset);
+		}
+	} while (u64_stats_fetch_retry_irq(&q_stats->sync, start));
+
+	*data += IFB_Q_STATS_LEN;
+}
+
+static void ifb_get_ethtool_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
+{
+	struct ifb_dev_private *dp = netdev_priv(dev);
+	struct ifb_q_private *txp;
+	int i;
+
+	for (i = 0; i < dev->real_num_rx_queues; i++) {
+		txp = dp->tx_private + i;
+		ifb_fill_stats_data(&data, &txp->rx_stats);
+	}
+
+	for (i = 0; i < dev->real_num_tx_queues; i++) {
+		txp = dp->tx_private + i;
+		ifb_fill_stats_data(&data, &txp->tx_stats);
+	}
+}
+
 static const struct net_device_ops ifb_netdev_ops = {
 	.ndo_open	= ifb_open,
 	.ndo_stop	= ifb_close,
@@ -190,6 +282,12 @@  static const struct net_device_ops ifb_netdev_ops = {
 	.ndo_init	= ifb_dev_init,
 };
 
+static const struct ethtool_ops ifb_ethtool_ops = {
+	.get_strings		= ifb_get_strings,
+	.get_sset_count		= ifb_get_sset_count,
+	.get_ethtool_stats	= ifb_get_ethtool_stats,
+};
+
 #define IFB_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG  | NETIF_F_FRAGLIST	| \
 		      NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ENCAP_ALL	| \
 		      NETIF_F_HIGHDMA | NETIF_F_HW_VLAN_CTAG_TX		| \
@@ -213,6 +311,7 @@  static void ifb_setup(struct net_device *dev)
 {
 	/* Initialize the device structure. */
 	dev->netdev_ops = &ifb_netdev_ops;
+	dev->ethtool_ops = &ifb_ethtool_ops;
 
 	/* Fill in device structure with ethernet-generic values. */
 	ether_setup(dev);
@@ -241,10 +340,7 @@  static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct ifb_dev_private *dp = netdev_priv(dev);
 	struct ifb_q_private *txp = dp->tx_private + skb_get_queue_mapping(skb);
 
-	u64_stats_update_begin(&txp->rsync);
-	txp->rx_packets++;
-	txp->rx_bytes += skb->len;
-	u64_stats_update_end(&txp->rsync);
+	ifb_update_q_stats(&txp->rx_stats, skb->len);
 
 	if (!skb->redirected || !skb->skb_iif) {
 		dev_kfree_skb(skb);