Skip to contentSkip to conversation

add more xdp stats to veth driver

PATCH In review Lorenzo Bianconi 3 participants 17 messages Mar 19, 2020 – Mar 25, 2020

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Lorenzo Bianconi

author
#0 lore raw

cover add more xdp stats to veth driver

Align veth xdp stats accounting to mellanox, intel and marvell implementation. Introduce the following xdp counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Lorenzo Bianconi (5):
veth: move xdp stats in a dedicated structure
veth: introduce more specialized counters in veth_stats
veth: distinguish between rx_drops and xdp_drops
veth: introduce more xdp counters
veth: remove atomic64_add from veth_xdp_xmit hotpath

 drivers/net/veth.c | 174 ++++++++++++++++++++++++++++-----------------
 1 file changed, 110 insertions(+), 64 deletions(-)
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH 1/5 veth: move xdp stats in a dedicated structure

Move xdp stats in veth_stats data structure. This is a preliminary patch to align xdp statistics to mlx5, ixgbe and mvneta drivers

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/veth.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/net/veth.c +17 -13
index d4cbb9e8c63f..33e23bbde5bf 100644
@@ -40,10 +40,14 @@
 
 #define VETH_XDP_TX_BULK_SIZE	16
 
+struct veth_stats {
+	u64	xdp_packets;
+	u64	xdp_bytes;
+	u64	xdp_drops;
+};
+
 struct veth_rq_stats {
-	u64			xdp_packets;
-	u64			xdp_bytes;
-	u64			xdp_drops;
+	struct veth_stats	vs;
 	struct u64_stats_sync	syncp;
 };
 

@@ -80,7 +84,7 @@ struct veth_q_stat_desc {
 	size_t	offset;
 };
 
-#define VETH_RQ_STAT(m)	offsetof(struct veth_rq_stats, m)
+#define VETH_RQ_STAT(m)	offsetof(struct veth_stats, m)
 
 static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
 	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },

@@ -155,7 +159,7 @@ static void veth_get_ethtool_stats(struct net_device *dev,
 	idx = 1;
 	for (i = 0; i < dev->real_num_rx_queues; i++) {
 		const struct veth_rq_stats *rq_stats = &priv->rq[i].stats;
-		const void *stats_base = (void *)rq_stats;
+		const void *stats_base = (void *)&rq_stats->vs;
 		unsigned int start;
 		size_t offset;
 

@@ -283,7 +287,7 @@ static u64 veth_stats_tx(struct net_device *dev, u64 *packets, u64 *bytes)
 	return atomic64_read(&priv->dropped);
 }
 
-static void veth_stats_rx(struct veth_rq_stats *result, struct net_device *dev)
+static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 {
 	struct veth_priv *priv = netdev_priv(dev);
 	int i;

@@ -298,9 +302,9 @@ static void veth_stats_rx(struct veth_rq_stats *result, struct net_device *dev)
 
 		do {
 			start = u64_stats_fetch_begin_irq(&stats->syncp);
-			packets = stats->xdp_packets;
-			bytes = stats->xdp_bytes;
-			drops = stats->xdp_drops;
+			packets = stats->vs.xdp_packets;
+			bytes = stats->vs.xdp_bytes;
+			drops = stats->vs.xdp_drops;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
 		result->xdp_packets += packets;
 		result->xdp_bytes += bytes;

@@ -313,7 +317,7 @@ static void veth_get_stats64(struct net_device *dev,
 {
 	struct veth_priv *priv = netdev_priv(dev);
 	struct net_device *peer;
-	struct veth_rq_stats rx;
+	struct veth_stats rx;
 	u64 packets, bytes;
 
 	tot->tx_dropped = veth_stats_tx(dev, &packets, &bytes);

@@ -740,9 +744,9 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget, unsigned int *xdp_xmit,
 	}
 
 	u64_stats_update_begin(&rq->stats.syncp);
-	rq->stats.xdp_packets += done;
-	rq->stats.xdp_bytes += bytes;
-	rq->stats.xdp_drops += drops;
+	rq->stats.vs.xdp_packets += done;
+	rq->stats.vs.xdp_bytes += bytes;
+	rq->stats.vs.xdp_drops += drops;
 	u64_stats_update_end(&rq->stats.syncp);
 
 	return done;
Signature · 1 line
2.25.1
author
#2 lore raw

PATCH 2/5 veth: introduce more specialized counters in veth_stats

In reply to Lorenzo Bianconi

Introduce xdp_tx, xdp_redirect and rx_drops counters in veth_stats data structure. Move stats accounting in veth_poll. Remove xdp_xmit variable in veth_xdp_rcv_one/veth_xdp_rcv_skb and rely on veth_stats counters. This is a preliminary patch to align veth xdp statistics to mlx, intel and marvell xdp implementation

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/veth.c | 72 +++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 32 deletions(-)
drivers/net/veth.c +40 -32
index 33e23bbde5bf..bad8fd432067 100644
@@ -34,16 +34,16 @@
 #define VETH_RING_SIZE		256
 #define VETH_XDP_HEADROOM	(XDP_PACKET_HEADROOM + NET_IP_ALIGN)
 
-/* Separating two types of XDP xmit */
-#define VETH_XDP_TX		BIT(0)
-#define VETH_XDP_REDIR		BIT(1)
-
 #define VETH_XDP_TX_BULK_SIZE	16
 
 struct veth_stats {
+	u64	rx_drops;
+	/* xdp */
 	u64	xdp_packets;
 	u64	xdp_bytes;
+	u64	xdp_redirect;
 	u64	xdp_drops;
+	u64	xdp_tx;
 };
 
 struct veth_rq_stats {

@@ -493,8 +493,8 @@ static int veth_xdp_tx(struct net_device *dev, struct xdp_buff *xdp,
 
 static struct sk_buff *veth_xdp_rcv_one(struct veth_rq *rq,
 					struct xdp_frame *frame,
-					unsigned int *xdp_xmit,
-					struct veth_xdp_tx_bq *bq)
+					struct veth_xdp_tx_bq *bq,
+					struct veth_stats *stats)
 {
 	void *hard_start = frame->data - frame->headroom;
 	void *head = hard_start - sizeof(struct xdp_frame);

@@ -530,9 +530,10 @@ static struct sk_buff *veth_xdp_rcv_one(struct veth_rq *rq,
 			if (unlikely(veth_xdp_tx(rq->dev, &xdp, bq) < 0)) {
 				trace_xdp_exception(rq->dev, xdp_prog, act);
 				frame = &orig_frame;
+				stats->rx_drops++;
 				goto err_xdp;
 			}
-			*xdp_xmit |= VETH_XDP_TX;
+			stats->xdp_tx++;
 			rcu_read_unlock();
 			goto xdp_xmit;
 		case XDP_REDIRECT:

@@ -541,9 +542,10 @@ static struct sk_buff *veth_xdp_rcv_one(struct veth_rq *rq,
 			xdp.rxq->mem = frame->mem;
 			if (xdp_do_redirect(rq->dev, &xdp, xdp_prog)) {
 				frame = &orig_frame;
+				stats->rx_drops++;
 				goto err_xdp;
 			}
-			*xdp_xmit |= VETH_XDP_REDIR;
+			stats->xdp_redirect++;
 			rcu_read_unlock();
 			goto xdp_xmit;
 		default:

@@ -553,6 +555,7 @@ static struct sk_buff *veth_xdp_rcv_one(struct veth_rq *rq,
 			trace_xdp_exception(rq->dev, xdp_prog, act);
 			/* fall through */
 		case XDP_DROP:
+			stats->xdp_drops++;
 			goto err_xdp;
 		}
 	}

@@ -562,6 +565,7 @@ static struct sk_buff *veth_xdp_rcv_one(struct veth_rq *rq,
 	skb = veth_build_skb(head, headroom, len, 0);
 	if (!skb) {
 		xdp_return_frame(frame);
+		stats->rx_drops++;
 		goto err;
 	}
 

@@ -577,9 +581,10 @@ static struct sk_buff *veth_xdp_rcv_one(struct veth_rq *rq,
 	return NULL;
 }
 
-static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, struct sk_buff *skb,
-					unsigned int *xdp_xmit,
-					struct veth_xdp_tx_bq *bq)
+static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
+					struct sk_buff *skb,
+					struct veth_xdp_tx_bq *bq,
+					struct veth_stats *stats)
 {
 	u32 pktlen, headroom, act, metalen;
 	void *orig_data, *orig_data_end;

@@ -657,18 +662,21 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, struct sk_buff *skb,
 		xdp.rxq->mem = rq->xdp_mem;
 		if (unlikely(veth_xdp_tx(rq->dev, &xdp, bq) < 0)) {
 			trace_xdp_exception(rq->dev, xdp_prog, act);
+			stats->rx_drops++;
 			goto err_xdp;
 		}
-		*xdp_xmit |= VETH_XDP_TX;
+		stats->xdp_tx++;
 		rcu_read_unlock();
 		goto xdp_xmit;
 	case XDP_REDIRECT:
 		get_page(virt_to_page(xdp.data));
 		consume_skb(skb);
 		xdp.rxq->mem = rq->xdp_mem;
-		if (xdp_do_redirect(rq->dev, &xdp, xdp_prog))
+		if (xdp_do_redirect(rq->dev, &xdp, xdp_prog)) {
+			stats->rx_drops++;
 			goto err_xdp;
-		*xdp_xmit |= VETH_XDP_REDIR;
+		}
+		stats->xdp_redirect++;
 		rcu_read_unlock();
 		goto xdp_xmit;
 	default:

@@ -678,7 +686,8 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, struct sk_buff *skb,
 		trace_xdp_exception(rq->dev, xdp_prog, act);
 		/* fall through */
 	case XDP_DROP:
-		goto drop;
+		stats->xdp_drops++;
+		goto xdp_drop;
 	}
 	rcu_read_unlock();
 

@@ -700,6 +709,8 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, struct sk_buff *skb,
 out:
 	return skb;
 drop:
+	stats->rx_drops++;
+xdp_drop:
 	rcu_read_unlock();
 	kfree_skb(skb);
 	return NULL;

@@ -710,14 +721,14 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, struct sk_buff *skb,
 	return NULL;
 }
 
-static int veth_xdp_rcv(struct veth_rq *rq, int budget, unsigned int *xdp_xmit,
-			struct veth_xdp_tx_bq *bq)
+static int veth_xdp_rcv(struct veth_rq *rq, int budget,
+			struct veth_xdp_tx_bq *bq,
+			struct veth_stats *stats)
 {
-	int i, done = 0, drops = 0, bytes = 0;
+	int i, done = 0;
 
 	for (i = 0; i < budget; i++) {
 		void *ptr = __ptr_ring_consume(&rq->xdp_ring);
-		unsigned int xdp_xmit_one = 0;
 		struct sk_buff *skb;
 
 		if (!ptr)

@@ -726,27 +737,24 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget, unsigned int *xdp_xmit,
 		if (veth_is_xdp_frame(ptr)) {
 			struct xdp_frame *frame = veth_ptr_to_xdp(ptr);
 
-			bytes += frame->len;
-			skb = veth_xdp_rcv_one(rq, frame, &xdp_xmit_one, bq);
+			stats->xdp_bytes += frame->len;
+			skb = veth_xdp_rcv_one(rq, frame, bq, stats);
 		} else {
 			skb = ptr;
-			bytes += skb->len;
-			skb = veth_xdp_rcv_skb(rq, skb, &xdp_xmit_one, bq);
+			stats->xdp_bytes += skb->len;
+			skb = veth_xdp_rcv_skb(rq, skb, bq, stats);
 		}
-		*xdp_xmit |= xdp_xmit_one;
 
 		if (skb)
 			napi_gro_receive(&rq->xdp_napi, skb);
-		else if (!xdp_xmit_one)
-			drops++;
 
 		done++;
 	}
 
 	u64_stats_update_begin(&rq->stats.syncp);
+	rq->stats.vs.xdp_bytes += stats->xdp_bytes;
+	rq->stats.vs.xdp_drops += stats->xdp_drops + stats->rx_drops;
 	rq->stats.vs.xdp_packets += done;
-	rq->stats.vs.xdp_bytes += bytes;
-	rq->stats.vs.xdp_drops += drops;
 	u64_stats_update_end(&rq->stats.syncp);
 
 	return done;

@@ -756,14 +764,14 @@ static int veth_poll(struct napi_struct *napi, int budget)
 {
 	struct veth_rq *rq =
 		container_of(napi, struct veth_rq, xdp_napi);
-	unsigned int xdp_xmit = 0;
+	struct veth_stats stats = {};
 	struct veth_xdp_tx_bq bq;
 	int done;
 
 	bq.count = 0;
 
 	xdp_set_return_frame_no_direct();
-	done = veth_xdp_rcv(rq, budget, &xdp_xmit, &bq);
+	done = veth_xdp_rcv(rq, budget, &bq, &stats);
 
 	if (done < budget && napi_complete_done(napi, done)) {
 		/* Write rx_notify_masked before reading ptr_ring */

@@ -774,9 +782,9 @@ static int veth_poll(struct napi_struct *napi, int budget)
 		}
 	}
 
-	if (xdp_xmit & VETH_XDP_TX)
+	if (stats.xdp_tx > 0)
 		veth_xdp_flush(rq->dev, &bq);
-	if (xdp_xmit & VETH_XDP_REDIR)
+	if (stats.xdp_redirect > 0)
 		xdp_do_flush();
 	xdp_clear_return_frame_no_direct();
Signature · 1 line
2.25.1
author
#3 lore raw

PATCH 3/5 veth: distinguish between rx_drops and xdp_drops

In reply to Lorenzo Bianconi

Distinguish between rx_drops and xdp_drops since the latter is already reported in rx_packets. Report xdp_drops in ethtool statistics

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/veth.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/net/veth.c +7 -5
index bad8fd432067..2307696d4897 100644
@@ -90,6 +90,7 @@ static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
 	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
 	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
 	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
+	{ "rx_drops",		VETH_RQ_STAT(rx_drops) },
 };
 
 #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)

@@ -294,7 +295,7 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 
 	result->xdp_packets = 0;
 	result->xdp_bytes = 0;
-	result->xdp_drops = 0;
+	result->rx_drops = 0;
 	for (i = 0; i < dev->num_rx_queues; i++) {
 		struct veth_rq_stats *stats = &priv->rq[i].stats;
 		u64 packets, bytes, drops;

@@ -304,11 +305,11 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 			start = u64_stats_fetch_begin_irq(&stats->syncp);
 			packets = stats->vs.xdp_packets;
 			bytes = stats->vs.xdp_bytes;
-			drops = stats->vs.xdp_drops;
+			drops = stats->vs.rx_drops;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
 		result->xdp_packets += packets;
 		result->xdp_bytes += bytes;
-		result->xdp_drops += drops;
+		result->rx_drops += drops;
 	}
 }
 

@@ -325,7 +326,7 @@ static void veth_get_stats64(struct net_device *dev,
 	tot->tx_packets = packets;
 
 	veth_stats_rx(&rx, dev);
-	tot->rx_dropped = rx.xdp_drops;
+	tot->rx_dropped = rx.rx_drops;
 	tot->rx_bytes = rx.xdp_bytes;
 	tot->rx_packets = rx.xdp_packets;
 

@@ -753,7 +754,8 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 
 	u64_stats_update_begin(&rq->stats.syncp);
 	rq->stats.vs.xdp_bytes += stats->xdp_bytes;
-	rq->stats.vs.xdp_drops += stats->xdp_drops + stats->rx_drops;
+	rq->stats.vs.xdp_drops += stats->xdp_drops;
+	rq->stats.vs.rx_drops += stats->rx_drops;
 	rq->stats.vs.xdp_packets += done;
 	u64_stats_update_end(&rq->stats.syncp);
Signature · 1 line
2.25.1
author
#4 lore raw

PATCH 4/5 veth: introduce more xdp counters

In reply to Lorenzo Bianconi

Introduce xdp_xmit counter in order to distinguish between XDP_TX and ndo_xdp_xmit stats. Introduce the following ethtool counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/veth.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)
drivers/net/veth.c +35 -7
index 2307696d4897..093b55acedb1 100644
@@ -44,6 +44,9 @@ struct veth_stats {
 	u64	xdp_redirect;
 	u64	xdp_drops;
 	u64	xdp_tx;
+	u64	xdp_tx_err;
+	u64	xdp_xmit;
+	u64	xdp_xmit_err;
 };
 
 struct veth_rq_stats {

@@ -89,8 +92,13 @@ struct veth_q_stat_desc {
 static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
 	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
 	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
-	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
 	{ "rx_drops",		VETH_RQ_STAT(rx_drops) },
+	{ "rx_xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
+	{ "rx_xdp_drops",	VETH_RQ_STAT(xdp_drops) },
+	{ "rx_xdp_tx",		VETH_RQ_STAT(xdp_tx) },
+	{ "rx_xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
+	{ "tx_xdp_xmit",	VETH_RQ_STAT(xdp_xmit) },
+	{ "tx_xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
 };
 
 #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)

@@ -129,7 +137,7 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
 		for (i = 0; i < dev->real_num_rx_queues; i++) {
 			for (j = 0; j < VETH_RQ_STATS_LEN; j++) {
 				snprintf(p, ETH_GSTRING_LEN,
-					 "rx_queue_%u_%.11s",
+					 "rx_queue_%u_%.18s",
 					 i, veth_rq_stats_desc[j].desc);
 				p += ETH_GSTRING_LEN;
 			}

@@ -374,12 +382,13 @@ static int veth_select_rxq(struct net_device *dev)
 }
 
 static int veth_xdp_xmit(struct net_device *dev, int n,
-			 struct xdp_frame **frames, u32 flags)
+			 struct xdp_frame **frames,
+			 u32 flags, bool ndo_xmit)
 {
 	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
+	unsigned int qidx, max_len;
 	struct net_device *rcv;
 	int i, ret, drops = n;
-	unsigned int max_len;
 	struct veth_rq *rq;
 
 	rcu_read_lock();

@@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 	}
 
 	rcv_priv = netdev_priv(rcv);
-	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
+	qidx = veth_select_rxq(rcv);
+	rq = &rcv_priv->rq[qidx];
 	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
 	 * side. This means an XDP program is loaded on the peer and the peer
 	 * device is up.

@@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 	if (flags & XDP_XMIT_FLUSH)
 		__veth_xdp_flush(rq);
 
+	rq = &priv->rq[qidx];
+	u64_stats_update_begin(&rq->stats.syncp);
+	if (ndo_xmit) {
+		rq->stats.vs.xdp_xmit += n - drops;
+		rq->stats.vs.xdp_xmit_err += drops;
+	} else {
+		rq->stats.vs.xdp_tx += n - drops;
+		rq->stats.vs.xdp_tx_err += drops;
+	}
+	u64_stats_update_end(&rq->stats.syncp);
+
 	if (likely(!drops)) {
 		rcu_read_unlock();
 		return n;

@@ -437,11 +458,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 	return ret;
 }
 
+static int veth_ndo_xdp_xmit(struct net_device *dev, int n,
+			     struct xdp_frame **frames, u32 flags)
+{
+	return veth_xdp_xmit(dev, n, frames, flags, true);
+}
+
 static void veth_xdp_flush_bq(struct net_device *dev, struct veth_xdp_tx_bq *bq)
 {
 	int sent, i, err = 0;
 
-	sent = veth_xdp_xmit(dev, bq->count, bq->q, 0);
+	sent = veth_xdp_xmit(dev, bq->count, bq->q, 0, false);
 	if (sent < 0) {
 		err = sent;
 		sent = 0;

@@ -753,6 +780,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 	}
 
 	u64_stats_update_begin(&rq->stats.syncp);
+	rq->stats.vs.xdp_redirect += stats->xdp_redirect;
 	rq->stats.vs.xdp_bytes += stats->xdp_bytes;
 	rq->stats.vs.xdp_drops += stats->xdp_drops;
 	rq->stats.vs.rx_drops += stats->rx_drops;

@@ -1172,7 +1200,7 @@ static const struct net_device_ops veth_netdev_ops = {
 	.ndo_features_check	= passthru_features_check,
 	.ndo_set_rx_headroom	= veth_set_rx_headroom,
 	.ndo_bpf		= veth_xdp,
-	.ndo_xdp_xmit		= veth_xdp_xmit,
+	.ndo_xdp_xmit		= veth_ndo_xdp_xmit,
 };
 
 #define VETH_FEATURES (NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HW_CSUM | \
Signature · 1 line
2.25.1
#5 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 7 lines · Lorenzo Bianconi

Introduce xdp_xmit counter in order to distinguish between XDP_TX and ndo_xdp_xmit stats. Introduce the following ethtool counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Thank you for working on this!

Quoted 2 lines
Signed-off-by Lorenzo Bianconi <[email protected]>

---

...

Quoted 11 lines
drivers/net/veth.c +2 -1
@@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	}
 
  	rcv_priv = netdev_priv(rcv);
-	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
+	qidx = veth_select_rxq(rcv);
+	rq = &rcv_priv->rq[qidx];
  	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
  	 * side. This means an XDP program is loaded on the peer and the peer
  	 * device is up.

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 437

Quoted 4 lines
drivers/net/veth.c +1 -0
@@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	if (flags & XDP_XMIT_FLUSH)
  		__veth_xdp_flush(rq);
 
+	rq = &priv->rq[qidx];

I think there is no guarantee that this rq exists. Qidx is less than rcv->real_num_rx_queues, but not necessarily less than dev->real_num_rx_queues.

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 438

Quoted 1 line
drivers/net/veth.c +1 -0
+	u64_stats_update_begin(&rq->stats.syncp);

So this can cuase NULL pointer dereference.

Toshiaki Makita

Quoted 49 lines
drivers/net/veth.c +18 -2
+	if (ndo_xmit) {
+		rq->stats.vs.xdp_xmit += n - drops;
+		rq->stats.vs.xdp_xmit_err += drops;
+	} else {
+		rq->stats.vs.xdp_tx += n - drops;
+		rq->stats.vs.xdp_tx_err += drops;
+	}
+	u64_stats_update_end(&rq->stats.syncp);
+
  	if (likely(!drops)) {
  		rcu_read_unlock();
  		return n;

@@ -437,11 +458,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	return ret;
  }
 
+static int veth_ndo_xdp_xmit(struct net_device *dev, int n,
+			     struct xdp_frame **frames, u32 flags)
+{
+	return veth_xdp_xmit(dev, n, frames, flags, true);
+}
+
  static void veth_xdp_flush_bq(struct net_device *dev, struct veth_xdp_tx_bq *bq)
  {
  	int sent, i, err = 0;
 
-	sent = veth_xdp_xmit(dev, bq->count, bq->q, 0);
+	sent = veth_xdp_xmit(dev, bq->count, bq->q, 0, false);
  	if (sent < 0) {
  		err = sent;
  		sent = 0;

@@ -753,6 +780,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
  	}
 
  	u64_stats_update_begin(&rq->stats.syncp);
+	rq->stats.vs.xdp_redirect += stats->xdp_redirect;
  	rq->stats.vs.xdp_bytes += stats->xdp_bytes;
  	rq->stats.vs.xdp_drops += stats->xdp_drops;
  	rq->stats.vs.rx_drops += stats->rx_drops;

@@ -1172,7 +1200,7 @@ static const struct net_device_ops veth_netdev_ops = {
  	.ndo_features_check	= passthru_features_check,
  	.ndo_set_rx_headroom	= veth_set_rx_headroom,
  	.ndo_bpf		= veth_xdp,
-	.ndo_xdp_xmit		= veth_xdp_xmit,
+	.ndo_xdp_xmit		= veth_ndo_xdp_xmit,
  };
 
  #define VETH_FEATURES (NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HW_CSUM | \
author
#6 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 32 lines · Lorenzo Bianconi
Quoted 7 lines

Introduce xdp_xmit counter in order to distinguish between XDP_TX and ndo_xdp_xmit stats. Introduce the following ethtool counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Thank you for working on this!

Quoted 2 lines
Signed-off-by Lorenzo Bianconi <[email protected]>

---

...

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 436

Quoted 13 lines
drivers/net/veth.c +3 -1
@@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	}
  	rcv_priv = netdev_priv(rcv);
-	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
+	qidx = veth_select_rxq(rcv);
+	rq = &rcv_priv->rq[qidx];
  	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
  	 * side. This means an XDP program is loaded on the peer and the peer
  	 * device is up.

@@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	if (flags & XDP_XMIT_FLUSH)
  		__veth_xdp_flush(rq);
+	rq = &priv->rq[qidx];

I think there is no guarantee that this rq exists. Qidx is less than rcv->real_num_rx_queues, but not necessarily less than dev->real_num_rx_queues.

Quoted 3 lines

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 438

Quoted 1 line
drivers/net/veth.c +1 -0
+	u64_stats_update_begin(&rq->stats.syncp);

So this can cuase NULL pointer dereference.

oh right, thanks for spotting this.
I think we can recompute qidx for tx netdevice in this case, doing something like:

qidx = veth_select_rxq(dev);
rq = &priv->rq[qidx];

what do you think?

Regards,
Lorenzo

Quoted 48 lines

Toshiaki Makita

Quoted 45 lines
drivers/net/veth.c +18 -2
+	if (ndo_xmit) {
+		rq->stats.vs.xdp_xmit += n - drops;
+		rq->stats.vs.xdp_xmit_err += drops;
+	} else {
+		rq->stats.vs.xdp_tx += n - drops;
+		rq->stats.vs.xdp_tx_err += drops;
+	}
+	u64_stats_update_end(&rq->stats.syncp);
+
  	if (likely(!drops)) {
  		rcu_read_unlock();
  		return n;

@@ -437,11 +458,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	return ret;
  }
+static int veth_ndo_xdp_xmit(struct net_device *dev, int n,
+			     struct xdp_frame **frames, u32 flags)
+{
+	return veth_xdp_xmit(dev, n, frames, flags, true);
+}
+
  static void veth_xdp_flush_bq(struct net_device *dev, struct veth_xdp_tx_bq *bq)
  {
  	int sent, i, err = 0;
-	sent = veth_xdp_xmit(dev, bq->count, bq->q, 0);
+	sent = veth_xdp_xmit(dev, bq->count, bq->q, 0, false);
  	if (sent < 0) {
  		err = sent;
  		sent = 0;

@@ -753,6 +780,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
  	}
  	u64_stats_update_begin(&rq->stats.syncp);
+	rq->stats.vs.xdp_redirect += stats->xdp_redirect;
  	rq->stats.vs.xdp_bytes += stats->xdp_bytes;
  	rq->stats.vs.xdp_drops += stats->xdp_drops;
  	rq->stats.vs.rx_drops += stats->rx_drops;

@@ -1172,7 +1200,7 @@ static const struct net_device_ops veth_netdev_ops = {
  	.ndo_features_check	= passthru_features_check,
  	.ndo_set_rx_headroom	= veth_set_rx_headroom,
  	.ndo_bpf		= veth_xdp,
-	.ndo_xdp_xmit		= veth_xdp_xmit,
+	.ndo_xdp_xmit		= veth_ndo_xdp_xmit,
  };
  #define VETH_FEATURES (NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HW_CSUM | \
#7 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 36 lines · Lorenzo Bianconi
Quoted 32 lines · Lorenzo Bianconi
Quoted 7 lines

Introduce xdp_xmit counter in order to distinguish between XDP_TX and ndo_xdp_xmit stats. Introduce the following ethtool counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Thank you for working on this!

Quoted 2 lines
Signed-off-by Lorenzo Bianconi <[email protected]>

---

...

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 436

Quoted 13 lines
drivers/net/veth.c +3 -1
@@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	}
   	rcv_priv = netdev_priv(rcv);
-	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
+	qidx = veth_select_rxq(rcv);
+	rq = &rcv_priv->rq[qidx];
   	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
   	 * side. This means an XDP program is loaded on the peer and the peer
   	 * device is up.

@@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	if (flags & XDP_XMIT_FLUSH)
   		__veth_xdp_flush(rq);
+	rq = &priv->rq[qidx];

I think there is no guarantee that this rq exists. Qidx is less than rcv->real_num_rx_queues, but not necessarily less than dev->real_num_rx_queues.

Quoted 3 lines

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 438

Quoted 1 line
drivers/net/veth.c +1 -0
+	u64_stats_update_begin(&rq->stats.syncp);

So this can cuase NULL pointer dereference.

Quoted 8 lines

oh right, thanks for spotting this.
I think we can recompute qidx for tx netdevice in this case, doing something like:

qidx = veth_select_rxq(dev);
rq = &priv->rq[qidx];

what do you think?

This would not cause NULL pointer deref, but I wonder what counters you've added mean.

- rx_xdp_redirect, rx_xdp_drops, rx_xdp_tx

These counters names will be rx_queue_[i]_rx_xdp_[redirect|drops|tx].
"rx_" in their names looks redundant.
Also it looks like there is not "rx[i]_xdp_tx" counter but there is "rx[i]_xdp_tx_xmit" in mlx5 from this page.
https://community.mellanox.com/s/article/understanding-mlx5-ethtool-counters

- tx_xdp_xmit, tx_xdp_xmit_errors

These counters names will be rx_queue_[i]_tx_xdp_[xmit|xmit_errors].
Are these rx counters or tx counters?
If tx, currently there is no storage to store these tx counters so adding these would not be simple. If rx, I guess you should use peer rx queue counters instead of dev rx queue counters.

Toshiaki Makita

author
#8 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 46 lines · Lorenzo Bianconi
Quoted 36 lines
Quoted 32 lines · Lorenzo Bianconi
Quoted 7 lines

Introduce xdp_xmit counter in order to distinguish between XDP_TX and ndo_xdp_xmit stats. Introduce the following ethtool counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Thank you for working on this!

Quoted 2 lines
Signed-off-by Lorenzo Bianconi <[email protected]>

---

...

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 436

Quoted 13 lines
drivers/net/veth.c +3 -1
@@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	}
   	rcv_priv = netdev_priv(rcv);
-	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
+	qidx = veth_select_rxq(rcv);
+	rq = &rcv_priv->rq[qidx];
   	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
   	 * side. This means an XDP program is loaded on the peer and the peer
   	 * device is up.

@@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	if (flags & XDP_XMIT_FLUSH)
   		__veth_xdp_flush(rq);
+	rq = &priv->rq[qidx];

I think there is no guarantee that this rq exists. Qidx is less than rcv->real_num_rx_queues, but not necessarily less than dev->real_num_rx_queues.

Quoted 3 lines

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 438

Quoted 1 line
drivers/net/veth.c +1 -0
+	u64_stats_update_begin(&rq->stats.syncp);

So this can cuase NULL pointer dereference.

Quoted 8 lines

oh right, thanks for spotting this.
I think we can recompute qidx for tx netdevice in this case, doing something like:

qidx = veth_select_rxq(dev);
rq = &priv->rq[qidx];

what do you think?

Quoted 7 lines

This would not cause NULL pointer deref, but I wonder what counters you've added mean.

- rx_xdp_redirect, rx_xdp_drops, rx_xdp_tx

These counters names will be rx_queue_[i]_rx_xdp_[redirect|drops|tx]. "rx_" in their names looks redundant.

yes, we can drop the "rx" prefix in the stats name for them.

Quoted 3 lines

Also it looks like there is not "rx[i]_xdp_tx" counter but there is "rx[i]_xdp_tx_xmit" in mlx5 from this page.
https://community.mellanox.com/s/article/understanding-mlx5-ethtool-counters

rx[i]_xdp_tx_xmit and rx_xdp_tx are the same, we decided to use rx_xdp_tx for it since it seems more clear

Quoted 5 lines

- tx_xdp_xmit, tx_xdp_xmit_errors

These counters names will be rx_queue_[i]_tx_xdp_[xmit|xmit_errors]. Are these rx counters or tx counters?

tx_xdp_xmit[_errors] are used to count ndo_xmit stats so they are tx counters. I reused veth_stats for it just for convenience. Probably we can show them without rx suffix so it is clear they are transmitted by the current device. Another approach would be create per_cput struct to collect all tx stats. What do you think?

Regards,
Lorenzo

Quoted 6 lines

If tx, currently there is no storage to store these tx counters so adding these would not be simple.
If rx, I guess you should use peer rx queue counters instead of dev rx queue counters.

Toshiaki Makita

#9 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 54 lines · Lorenzo Bianconi
Quoted 46 lines · Lorenzo Bianconi
Quoted 36 lines
Quoted 32 lines · Lorenzo Bianconi
Quoted 7 lines

Introduce xdp_xmit counter in order to distinguish between XDP_TX and ndo_xdp_xmit stats. Introduce the following ethtool counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Thank you for working on this!

Quoted 2 lines
Signed-off-by Lorenzo Bianconi <[email protected]>

---

...

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 436

Quoted 13 lines
drivers/net/veth.c +3 -1
@@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
    	}
    	rcv_priv = netdev_priv(rcv);
-	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
+	qidx = veth_select_rxq(rcv);
+	rq = &rcv_priv->rq[qidx];
    	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
    	 * side. This means an XDP program is loaded on the peer and the peer
    	 * device is up.

@@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
    	if (flags & XDP_XMIT_FLUSH)
    		__veth_xdp_flush(rq);
+	rq = &priv->rq[qidx];

I think there is no guarantee that this rq exists. Qidx is less than rcv->real_num_rx_queues, but not necessarily less than dev->real_num_rx_queues.

Quoted 3 lines

drivers/net/veth.c static int veth_xdp_xmit(struct net_device *dev, int n, · line 438

Quoted 1 line
drivers/net/veth.c +1 -0
+	u64_stats_update_begin(&rq->stats.syncp);

So this can cuase NULL pointer dereference.

Quoted 8 lines

oh right, thanks for spotting this.
I think we can recompute qidx for tx netdevice in this case, doing something like:

qidx = veth_select_rxq(dev);
rq = &priv->rq[qidx];

what do you think?

Quoted 7 lines

This would not cause NULL pointer deref, but I wonder what counters you've added mean.

- rx_xdp_redirect, rx_xdp_drops, rx_xdp_tx

These counters names will be rx_queue_[i]_rx_xdp_[redirect|drops|tx]. "rx_" in their names looks redundant.

Quoted 8 lines

yes, we can drop the "rx" prefix in the stats name for them.

Quoted 3 lines

Also it looks like there is not "rx[i]_xdp_tx" counter but there is "rx[i]_xdp_tx_xmit" in mlx5 from this page.
https://community.mellanox.com/s/article/understanding-mlx5-ethtool-counters

rx[i]_xdp_tx_xmit and rx_xdp_tx are the same, we decided to use rx_xdp_tx for it since it seems more clear

OK.

Quoted 10 lines
Quoted 4 lines

- tx_xdp_xmit, tx_xdp_xmit_errors

These counters names will be rx_queue_[i]_tx_xdp_[xmit|xmit_errors]. Are these rx counters or tx counters?

tx_xdp_xmit[_errors] are used to count ndo_xmit stats so they are tx counters. I reused veth_stats for it just for convenience. Probably we can show them without rx suffix so it is clear they are transmitted by the current device. Another approach would be create per_cput struct to collect all tx stats. What do you think?

As veth_xdp_xmit really does not use tx queue but select peer rxq directly, per_cpu sounds more appropriate than per-queue.
One concern is consistency. Per-queue rx stats and per-cpu tx stats (or only sum of them?) looks inconsistent.
One alternative way is to change the queue selection login in veth_xdp_xmit and select txq instead of rxq. Then select peer rxq from txq, like veth_xmit. Accounting per queue tx stats is possible only when we can determine which txq is used.

Something like this:

static int veth_select_txq(struct net_device *dev)

{
	return smp_processor_id() % dev->real_num_tx_queues;
}

static int veth_xdp_xmit(struct net_device *dev, int n,
struct xdp_frame **frames, u32 flags)

{
	...
	txq = veth_select_txq(dev);
	rcv_rxq = txq; // 1-to-1 mapping from txq to peer rxq
	// Note: when XDP is enabled on rcv, this condition is always false
	if (rcv_rxq >= rcv->real_num_rx_queues)
		return -ENXIO;
	rcv_priv = netdev_priv(rcv);
	rq = &rcv_priv->rq[rcv_rxq];
	...
	// account txq stats in some way here
}

Thoughts?

Toshiaki Makita

author
#10 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 3 lines · Lorenzo Bianconi
Quoted 2 lines
Quoted 2 lines · Lorenzo Bianconi
Quoted 1 line
Quoted 1 line · Lorenzo Bianconi

[...]

Quoted 18 lines

As veth_xdp_xmit really does not use tx queue but select peer rxq directly, per_cpu sounds more appropriate than per-queue.
One concern is consistency. Per-queue rx stats and per-cpu tx stats (or only sum of them?) looks inconsistent.
One alternative way is to change the queue selection login in veth_xdp_xmit and select txq instead of rxq. Then select peer rxq from txq, like veth_xmit. Accounting per queue tx stats is possible only when we can determine which txq is used.

Something like this:

static int veth_select_txq(struct net_device *dev)

{
	return smp_processor_id() % dev->real_num_tx_queues;
}

static int veth_xdp_xmit(struct net_device *dev, int n,
struct xdp_frame **frames, u32 flags)

Quoted 12 lines
{
	...
	txq = veth_select_txq(dev);
	rcv_rxq = txq; // 1-to-1 mapping from txq to peer rxq
	// Note: when XDP is enabled on rcv, this condition is always false
	if (rcv_rxq >= rcv->real_num_rx_queues)
		return -ENXIO;
	rcv_priv = netdev_priv(rcv);
	rq = &rcv_priv->rq[rcv_rxq];
	...
	// account txq stats in some way here
}

actually I have a different idea..what about account tx stats on the peer rx queue as a result of XDP_TX or ndo_xdp_xmit and properly report this info in the ethool stats? In this way we do not have any locking issue and we still use the per-queue stats approach. Could you please take a look to the following patch?

Regards,
Lorenzo

drivers/net/veth.c +47 -16
index b6505a6c7102..f2acd2ee6287 100644
@@ -92,17 +92,22 @@ struct veth_q_stat_desc {
 static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
 	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
 	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
-	{ "rx_drops",		VETH_RQ_STAT(rx_drops) },
-	{ "rx_xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
-	{ "rx_xdp_drops",	VETH_RQ_STAT(xdp_drops) },
-	{ "rx_xdp_tx",		VETH_RQ_STAT(xdp_tx) },
-	{ "rx_xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
-	{ "tx_xdp_xmit",	VETH_RQ_STAT(xdp_xmit) },
-	{ "tx_xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+	{ "drops",		VETH_RQ_STAT(rx_drops) },
+	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
+	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
 };
 
 #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)
 
+static const struct veth_q_stat_desc veth_tq_stats_desc[] = {
+	{ "xdp_tx",		VETH_RQ_STAT(xdp_tx) },
+	{ "xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
+	{ "xdp_xmit",		VETH_RQ_STAT(xdp_xmit) },
+	{ "xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+};
+
+#define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
+
 static struct {
 	const char string[ETH_GSTRING_LEN];
 } ethtool_stats_keys[] = {

@@ -142,6 +147,14 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
 				p += ETH_GSTRING_LEN;
 			}
 		}
+		for (i = 0; i < dev->real_num_tx_queues; i++) {
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				snprintf(p, ETH_GSTRING_LEN,
+					 "tx_queue_%u_%.18s",
+					 i, veth_tq_stats_desc[j].desc);
+				p += ETH_GSTRING_LEN;
+			}
+		}
 		break;
 	}
 }

@@ -151,7 +164,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
 	switch (sset) {
 	case ETH_SS_STATS:
 		return ARRAY_SIZE(ethtool_stats_keys) +
-		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues;
+		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
+		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
 	default:
 		return -EOPNOTSUPP;
 	}

@@ -160,7 +174,7 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
 static void veth_get_ethtool_stats(struct net_device *dev,
 		struct ethtool_stats *stats, u64 *data)
 {
-	struct veth_priv *priv = netdev_priv(dev);
+	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
 	struct net_device *peer = rtnl_dereference(priv->peer);
 	int i, j, idx;
 

@@ -181,6 +195,26 @@ static void veth_get_ethtool_stats(struct net_device *dev,
 		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
 		idx += VETH_RQ_STATS_LEN;
 	}
+
+	if (!peer)
+		return;
+
+	rcv_priv = netdev_priv(peer);
+	for (i = 0; i < peer->real_num_rx_queues; i++) {
+		const struct veth_rq_stats *rq_stats = &rcv_priv->rq[i].stats;
+		const void *stats_base = (void *)&rq_stats->vs;
+		unsigned int start, tx_idx;
+		size_t offset;
+
+		tx_idx = (i % dev->real_num_tx_queues) * VETH_TQ_STATS_LEN;
+		do {
+			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				offset = veth_tq_stats_desc[j].offset;
+				data[tx_idx + idx + j] += *(u64 *)(stats_base + offset);
+			}
+		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
+	}
 }
 
 static const struct ethtool_ops veth_ethtool_ops = {

@@ -340,8 +374,7 @@ static void veth_get_stats64(struct net_device *dev,
 	tot->tx_packets = packets;
 
 	veth_stats_rx(&rx, dev);
-	tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
-	tot->rx_dropped = rx.rx_drops;
+	tot->rx_dropped = rx.rx_drops + rx.xdp_tx_err + rx.xdp_xmit_err;
 	tot->rx_bytes = rx.xdp_bytes;
 	tot->rx_packets = rx.xdp_packets;
 

@@ -353,7 +386,7 @@ static void veth_get_stats64(struct net_device *dev,
 		tot->rx_packets += packets;
 
 		veth_stats_rx(&rx, peer);
-		tot->rx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
+		tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
 		tot->tx_bytes += rx.xdp_bytes;
 		tot->tx_packets += rx.xdp_packets;
 	}

@@ -394,9 +427,9 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 			 u32 flags, bool ndo_xmit)
 {
 	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
-	unsigned int qidx, max_len;
 	struct net_device *rcv;
 	int i, ret, drops = n;
+	unsigned int max_len;
 	struct veth_rq *rq;
 
 	rcu_read_lock();

@@ -414,8 +447,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 	}
 
 	rcv_priv = netdev_priv(rcv);
-	qidx = veth_select_rxq(rcv);
-	rq = &rcv_priv->rq[qidx];
+	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
 	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
 	 * side. This means an XDP program is loaded on the peer and the peer
 	 * device is up.

@@ -446,7 +478,6 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 
 	ret = n - drops;
 drop:
-	rq = &priv->rq[qidx];
 	u64_stats_update_begin(&rq->stats.syncp);
 	if (ndo_xmit) {
 		rq->stats.vs.xdp_xmit += n - drops;
Quoted 4 lines

Thoughts?

Toshiaki Makita

#11 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 37 lines · Lorenzo Bianconi
Quoted 3 lines · Lorenzo Bianconi
Quoted 2 lines
Quoted 2 lines · Lorenzo Bianconi
Quoted 1 line
Quoted 1 line · Lorenzo Bianconi

[...]

Quoted 18 lines

As veth_xdp_xmit really does not use tx queue but select peer rxq directly, per_cpu sounds more appropriate than per-queue.
One concern is consistency. Per-queue rx stats and per-cpu tx stats (or only sum of them?) looks inconsistent.
One alternative way is to change the queue selection login in veth_xdp_xmit and select txq instead of rxq. Then select peer rxq from txq, like veth_xmit. Accounting per queue tx stats is possible only when we can determine which txq is used.

Something like this:

static int veth_select_txq(struct net_device *dev)

{
	return smp_processor_id() % dev->real_num_tx_queues;
}

static int veth_xdp_xmit(struct net_device *dev, int n,
struct xdp_frame **frames, u32 flags)

Quoted 12 lines
{
	...
	txq = veth_select_txq(dev);
	rcv_rxq = txq; // 1-to-1 mapping from txq to peer rxq
	// Note: when XDP is enabled on rcv, this condition is always false
	if (rcv_rxq >= rcv->real_num_rx_queues)
		return -ENXIO;
	rcv_priv = netdev_priv(rcv);
	rq = &rcv_priv->rq[rcv_rxq];
	...
	// account txq stats in some way here
}
Quoted 4 lines

actually I have a different idea..what about account tx stats on the peer rx queue as a result of XDP_TX or ndo_xdp_xmit and properly report this info in the ethool stats? In this way we do not have any locking issue and we still use the per-queue stats approach. Could you please take a look to the following patch?

Thanks I think your idea is nice.

Quoted 15 lines
drivers/net/veth.c +0 -7
index b6505a6c7102..f2acd2ee6287 100644
@@ -92,17 +92,22 @@ struct veth_q_stat_desc {
  static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
  	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
  	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
-	{ "rx_drops",		VETH_RQ_STAT(rx_drops) },
-	{ "rx_xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
-	{ "rx_xdp_drops",	VETH_RQ_STAT(xdp_drops) },
-	{ "rx_xdp_tx",		VETH_RQ_STAT(xdp_tx) },
-	{ "rx_xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
-	{ "tx_xdp_xmit",	VETH_RQ_STAT(xdp_xmit) },
-	{ "tx_xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },

drivers/net/veth.c struct veth_q_stat_desc { · line 104

Quoted 10 lines
drivers/net/veth.c +6 -0
+	{ "drops",		VETH_RQ_STAT(rx_drops) },
+	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
+	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
  };
 
  #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)
 
+static const struct veth_q_stat_desc veth_tq_stats_desc[] = {
+	{ "xdp_tx",		VETH_RQ_STAT(xdp_tx) },
+	{ "xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },

I'm wondering why xdp_tx is not accounted in rx_queue?
You can count xdp_tx/tx_error somewhere in rx path like veth_xdp_flush_bq(). xdp_redirect and xdp_drops are similar actions and in rx stats.

Quoted 49 lines
drivers/net/veth.c +19 -2
+	{ "xdp_xmit",		VETH_RQ_STAT(xdp_xmit) },
+	{ "xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+};
+
+#define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
+
  static struct {
  	const char string[ETH_GSTRING_LEN];
  } ethtool_stats_keys[] = {

@@ -142,6 +147,14 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
  				p += ETH_GSTRING_LEN;
  			}
  		}
+		for (i = 0; i < dev->real_num_tx_queues; i++) {
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				snprintf(p, ETH_GSTRING_LEN,
+					 "tx_queue_%u_%.18s",
+					 i, veth_tq_stats_desc[j].desc);
+				p += ETH_GSTRING_LEN;
+			}
+		}
  		break;
  	}
  }

@@ -151,7 +164,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
  	switch (sset) {
  	case ETH_SS_STATS:
  		return ARRAY_SIZE(ethtool_stats_keys) +
-		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues;
+		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
+		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
  	default:
  		return -EOPNOTSUPP;
  	}

@@ -160,7 +174,7 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
  static void veth_get_ethtool_stats(struct net_device *dev,
  		struct ethtool_stats *stats, u64 *data)
  {
-	struct veth_priv *priv = netdev_priv(dev);
+	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
  	struct net_device *peer = rtnl_dereference(priv->peer);
  	int i, j, idx;
 

@@ -181,6 +195,26 @@ static void veth_get_ethtool_stats(struct net_device *dev,
  		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
  		idx += VETH_RQ_STATS_LEN;
  	}
+
+	if (!peer)

drivers/net/veth.c static void veth_get_ethtool_stats(struct net_device *dev, · line 209

Quoted 10 lines
drivers/net/veth.c +10 -0
+		return;
+
+	rcv_priv = netdev_priv(peer);
+	for (i = 0; i < peer->real_num_rx_queues; i++) {
+		const struct veth_rq_stats *rq_stats = &rcv_priv->rq[i].stats;
+		const void *stats_base = (void *)&rq_stats->vs;
+		unsigned int start, tx_idx;
+		size_t offset;
+
+		tx_idx = (i % dev->real_num_tx_queues) * VETH_TQ_STATS_LEN;

I'm a bit concerned that this can fold multiple rx queue counters into one tx counter. But I cannot think of a better idea provided that we want to align XDP stats between drivers. So I'm OK with this.

Toshiaki Makita

Quoted 64 lines
drivers/net/veth.c +12 -7
+		do {
+			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				offset = veth_tq_stats_desc[j].offset;
+				data[tx_idx + idx + j] += *(u64 *)(stats_base + offset);
+			}
+		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
+	}
  }
 
  static const struct ethtool_ops veth_ethtool_ops = {

@@ -340,8 +374,7 @@ static void veth_get_stats64(struct net_device *dev,
  	tot->tx_packets = packets;
 
  	veth_stats_rx(&rx, dev);
-	tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
-	tot->rx_dropped = rx.rx_drops;
+	tot->rx_dropped = rx.rx_drops + rx.xdp_tx_err + rx.xdp_xmit_err;
  	tot->rx_bytes = rx.xdp_bytes;
  	tot->rx_packets = rx.xdp_packets;
 

@@ -353,7 +386,7 @@ static void veth_get_stats64(struct net_device *dev,
  		tot->rx_packets += packets;
 
  		veth_stats_rx(&rx, peer);
-		tot->rx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
+		tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
  		tot->tx_bytes += rx.xdp_bytes;
  		tot->tx_packets += rx.xdp_packets;
  	}

@@ -394,9 +427,9 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  			 u32 flags, bool ndo_xmit)
  {
  	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
-	unsigned int qidx, max_len;
  	struct net_device *rcv;
  	int i, ret, drops = n;
+	unsigned int max_len;
  	struct veth_rq *rq;
 
  	rcu_read_lock();

@@ -414,8 +447,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	}
 
  	rcv_priv = netdev_priv(rcv);
-	qidx = veth_select_rxq(rcv);
-	rq = &rcv_priv->rq[qidx];
+	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
  	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
  	 * side. This means an XDP program is loaded on the peer and the peer
  	 * device is up.

@@ -446,7 +478,6 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 
  	ret = n - drops;
  drop:
-	rq = &priv->rq[qidx];
  	u64_stats_update_begin(&rq->stats.syncp);
  	if (ndo_xmit) {
  		rq->stats.vs.xdp_xmit += n - drops;
Quoted 5 lines

Thoughts?

Toshiaki Makita

author
#12 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

[...]

Quoted 28 lines
Quoted 6 lines
Quoted 1 line

}

actually I have a different idea..what about account tx stats on the peer rx queue as a result of XDP_TX or ndo_xdp_xmit and properly report this info in the ethool stats? In this way we do not have any locking issue and we still use the per-queue stats approach. Could you please take a look to the following patch?

Thanks I think your idea is nice.

Quoted 19 lines
drivers/net/veth.c +3 -7
index b6505a6c7102..f2acd2ee6287 100644
@@ -92,17 +92,22 @@ struct veth_q_stat_desc {
  static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
  	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
  	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
-	{ "rx_drops",		VETH_RQ_STAT(rx_drops) },
-	{ "rx_xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
-	{ "rx_xdp_drops",	VETH_RQ_STAT(xdp_drops) },
-	{ "rx_xdp_tx",		VETH_RQ_STAT(xdp_tx) },
-	{ "rx_xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
-	{ "tx_xdp_xmit",	VETH_RQ_STAT(xdp_xmit) },
-	{ "tx_xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+	{ "drops",		VETH_RQ_STAT(rx_drops) },
+	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
+	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
  };
Quoted 8 lines

drivers/net/veth.c struct veth_q_stat_desc { · line 104

Quoted 4 lines
drivers/net/veth.c +3 -0
  #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)
+static const struct veth_q_stat_desc veth_tq_stats_desc[] = {
+	{ "xdp_tx",		VETH_RQ_STAT(xdp_tx) },
+	{ "xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },

I'm wondering why xdp_tx is not accounted in rx_queue?
You can count xdp_tx/tx_error somewhere in rx path like veth_xdp_flush_bq(). xdp_redirect and xdp_drops are similar actions and in rx stats.

Hi,

thanks for the review :)

I moved the accounting of xdp_tx/tx_error in veth_xdp_xmit for two reason:
1- veth_xdp_tx in veth_xdp_rcv_one or veth_xdp_rcv_skb returns an error
for XDP_TX just if xdp_frame pointer is invalid but the packet can be discarded in veth_xdp_xmit if the device is 'under-pressure' (and this can be a problem since in XDP there are no queueing mechanisms so far)
2- to be symmetric with ndo_xdp_xmit

Quoted 55 lines
Quoted 55 lines
drivers/net/veth.c +25 -2
+	{ "xdp_xmit",		VETH_RQ_STAT(xdp_xmit) },
+	{ "xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+};
+
+#define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
+
  static struct {
  	const char string[ETH_GSTRING_LEN];
  } ethtool_stats_keys[] = {

@@ -142,6 +147,14 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
  				p += ETH_GSTRING_LEN;
  			}
  		}
+		for (i = 0; i < dev->real_num_tx_queues; i++) {
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				snprintf(p, ETH_GSTRING_LEN,
+					 "tx_queue_%u_%.18s",
+					 i, veth_tq_stats_desc[j].desc);
+				p += ETH_GSTRING_LEN;
+			}
+		}
  		break;
  	}
  }

@@ -151,7 +164,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
  	switch (sset) {
  	case ETH_SS_STATS:
  		return ARRAY_SIZE(ethtool_stats_keys) +
-		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues;
+		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
+		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
  	default:
  		return -EOPNOTSUPP;
  	}

@@ -160,7 +174,7 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
  static void veth_get_ethtool_stats(struct net_device *dev,
  		struct ethtool_stats *stats, u64 *data)
  {
-	struct veth_priv *priv = netdev_priv(dev);
+	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
  	struct net_device *peer = rtnl_dereference(priv->peer);
  	int i, j, idx;

@@ -181,6 +195,26 @@ static void veth_get_ethtool_stats(struct net_device *dev,
  		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
  		idx += VETH_RQ_STATS_LEN;
  	}
+
+	if (!peer)
+		return;
+
+	rcv_priv = netdev_priv(peer);
+	for (i = 0; i < peer->real_num_rx_queues; i++) {
+		const struct veth_rq_stats *rq_stats = &rcv_priv->rq[i].stats;
+		const void *stats_base = (void *)&rq_stats->vs;
Quoted 8 lines

drivers/net/veth.c static void veth_get_ethtool_stats(struct net_device *dev, · line 209

Quoted 4 lines
drivers/net/veth.c +4 -0
+		unsigned int start, tx_idx;
+		size_t offset;
+
+		tx_idx = (i % dev->real_num_tx_queues) * VETH_TQ_STATS_LEN;

I'm a bit concerned that this can fold multiple rx queue counters into one tx counter. But I cannot think of a better idea provided that we want to align XDP stats between drivers. So I'm OK with this.

Since peer->real_num_rx_queues can be greater than dev->real_num_tx_queues, right? IIUC the only guarantee we have is:

peer->real_num_tx_queues < dev->real_num_rx_queues

If you are fine with that approach, I will post a patch before net-next closure.

Regards,
Lorenzo

Quoted 60 lines

Toshiaki Makita

Quoted 57 lines
drivers/net/veth.c +12 -7
+		do {
+			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				offset = veth_tq_stats_desc[j].offset;
+				data[tx_idx + idx + j] += *(u64 *)(stats_base + offset);
+			}
+		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
+	}
  }
  static const struct ethtool_ops veth_ethtool_ops = {

@@ -340,8 +374,7 @@ static void veth_get_stats64(struct net_device *dev,
  	tot->tx_packets = packets;
  	veth_stats_rx(&rx, dev);
-	tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
-	tot->rx_dropped = rx.rx_drops;
+	tot->rx_dropped = rx.rx_drops + rx.xdp_tx_err + rx.xdp_xmit_err;
  	tot->rx_bytes = rx.xdp_bytes;
  	tot->rx_packets = rx.xdp_packets;

@@ -353,7 +386,7 @@ static void veth_get_stats64(struct net_device *dev,
  		tot->rx_packets += packets;
  		veth_stats_rx(&rx, peer);
-		tot->rx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
+		tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
  		tot->tx_bytes += rx.xdp_bytes;
  		tot->tx_packets += rx.xdp_packets;
  	}

@@ -394,9 +427,9 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  			 u32 flags, bool ndo_xmit)
  {
  	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
-	unsigned int qidx, max_len;
  	struct net_device *rcv;
  	int i, ret, drops = n;
+	unsigned int max_len;
  	struct veth_rq *rq;
  	rcu_read_lock();

@@ -414,8 +447,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	}
  	rcv_priv = netdev_priv(rcv);
-	qidx = veth_select_rxq(rcv);
-	rq = &rcv_priv->rq[qidx];
+	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
  	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
  	 * side. This means an XDP program is loaded on the peer and the peer
  	 * device is up.

@@ -446,7 +478,6 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
  	ret = n - drops;
  drop:
-	rq = &priv->rq[qidx];
  	u64_stats_update_begin(&rq->stats.syncp);
  	if (ndo_xmit) {
  		rq->stats.vs.xdp_xmit += n - drops;
Quoted 4 lines

Thoughts?

Toshiaki Makita

#13 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 42 lines · Lorenzo Bianconi

[...]

Quoted 28 lines
Quoted 6 lines
Quoted 1 line

}

actually I have a different idea..what about account tx stats on the peer rx queue as a result of XDP_TX or ndo_xdp_xmit and properly report this info in the ethool stats? In this way we do not have any locking issue and we still use the per-queue stats approach. Could you please take a look to the following patch?

Thanks I think your idea is nice.

Quoted 19 lines
drivers/net/veth.c +3 -7
index b6505a6c7102..f2acd2ee6287 100644
@@ -92,17 +92,22 @@ struct veth_q_stat_desc {
   static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
   	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
   	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
-	{ "rx_drops",		VETH_RQ_STAT(rx_drops) },
-	{ "rx_xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
-	{ "rx_xdp_drops",	VETH_RQ_STAT(xdp_drops) },
-	{ "rx_xdp_tx",		VETH_RQ_STAT(xdp_tx) },
-	{ "rx_xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
-	{ "tx_xdp_xmit",	VETH_RQ_STAT(xdp_xmit) },
-	{ "tx_xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+	{ "drops",		VETH_RQ_STAT(rx_drops) },
+	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
+	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
   };
Quoted 8 lines

drivers/net/veth.c struct veth_q_stat_desc { · line 104

Quoted 4 lines
drivers/net/veth.c +3 -0
   #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)
+static const struct veth_q_stat_desc veth_tq_stats_desc[] = {
+	{ "xdp_tx",		VETH_RQ_STAT(xdp_tx) },
+	{ "xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },

I'm wondering why xdp_tx is not accounted in rx_queue?
You can count xdp_tx/tx_error somewhere in rx path like veth_xdp_flush_bq(). xdp_redirect and xdp_drops are similar actions and in rx stats.

Hi,

Quoted 7 lines

thanks for the review :)

I moved the accounting of xdp_tx/tx_error in veth_xdp_xmit for two reason:
1- veth_xdp_tx in veth_xdp_rcv_one or veth_xdp_rcv_skb returns an error
for XDP_TX just if xdp_frame pointer is invalid but the packet can be discarded in veth_xdp_xmit if the device is 'under-pressure' (and this can be a problem since in XDP there are no queueing mechanisms so far)

Right, but you can track the discard in veth_xdp_flush_bq().

Quoted 1 line

2- to be symmetric with ndo_xdp_xmit

I thought consistency between drivers is more important. What about other drivers?

Toshiaki Makita

Quoted 137 lines
Quoted 56 lines
Quoted 56 lines
drivers/net/veth.c +25 -2
+	{ "xdp_xmit",		VETH_RQ_STAT(xdp_xmit) },
+	{ "xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+};
+
+#define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
+
   static struct {
   	const char string[ETH_GSTRING_LEN];
   } ethtool_stats_keys[] = {

@@ -142,6 +147,14 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
   				p += ETH_GSTRING_LEN;
   			}
   		}
+		for (i = 0; i < dev->real_num_tx_queues; i++) {
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				snprintf(p, ETH_GSTRING_LEN,
+					 "tx_queue_%u_%.18s",
+					 i, veth_tq_stats_desc[j].desc);
+				p += ETH_GSTRING_LEN;
+			}
+		}
   		break;
   	}
   }

@@ -151,7 +164,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
   	switch (sset) {
   	case ETH_SS_STATS:
   		return ARRAY_SIZE(ethtool_stats_keys) +
-		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues;
+		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
+		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
   	default:
   		return -EOPNOTSUPP;
   	}

@@ -160,7 +174,7 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
   static void veth_get_ethtool_stats(struct net_device *dev,
   		struct ethtool_stats *stats, u64 *data)
   {
-	struct veth_priv *priv = netdev_priv(dev);
+	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
   	struct net_device *peer = rtnl_dereference(priv->peer);
   	int i, j, idx;

@@ -181,6 +195,26 @@ static void veth_get_ethtool_stats(struct net_device *dev,
   		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
   		idx += VETH_RQ_STATS_LEN;
   	}
+
+	if (!peer)
+		return;
+
+	rcv_priv = netdev_priv(peer);
+	for (i = 0; i < peer->real_num_rx_queues; i++) {
+		const struct veth_rq_stats *rq_stats = &rcv_priv->rq[i].stats;
+		const void *stats_base = (void *)&rq_stats->vs;
Quoted 8 lines

drivers/net/veth.c static void veth_get_ethtool_stats(struct net_device *dev, · line 209

Quoted 4 lines
drivers/net/veth.c +4 -0
+		unsigned int start, tx_idx;
+		size_t offset;
+
+		tx_idx = (i % dev->real_num_tx_queues) * VETH_TQ_STATS_LEN;

I'm a bit concerned that this can fold multiple rx queue counters into one tx counter. But I cannot think of a better idea provided that we want to align XDP stats between drivers. So I'm OK with this.

Since peer->real_num_rx_queues can be greater than dev->real_num_tx_queues, right? IIUC the only guarantee we have is:

peer->real_num_tx_queues < dev->real_num_rx_queues

If you are fine with that approach, I will post a patch before net-next closure.

Regards,
Lorenzo

Quoted 60 lines

Toshiaki Makita

Quoted 57 lines
drivers/net/veth.c +12 -7
+		do {
+			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				offset = veth_tq_stats_desc[j].offset;
+				data[tx_idx + idx + j] += *(u64 *)(stats_base + offset);
+			}
+		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
+	}
   }
   static const struct ethtool_ops veth_ethtool_ops = {

@@ -340,8 +374,7 @@ static void veth_get_stats64(struct net_device *dev,
   	tot->tx_packets = packets;
   	veth_stats_rx(&rx, dev);
-	tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
-	tot->rx_dropped = rx.rx_drops;
+	tot->rx_dropped = rx.rx_drops + rx.xdp_tx_err + rx.xdp_xmit_err;
   	tot->rx_bytes = rx.xdp_bytes;
   	tot->rx_packets = rx.xdp_packets;

@@ -353,7 +386,7 @@ static void veth_get_stats64(struct net_device *dev,
   		tot->rx_packets += packets;
   		veth_stats_rx(&rx, peer);
-		tot->rx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
+		tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
   		tot->tx_bytes += rx.xdp_bytes;
   		tot->tx_packets += rx.xdp_packets;
   	}

@@ -394,9 +427,9 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   			 u32 flags, bool ndo_xmit)
   {
   	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
-	unsigned int qidx, max_len;
   	struct net_device *rcv;
   	int i, ret, drops = n;
+	unsigned int max_len;
   	struct veth_rq *rq;
   	rcu_read_lock();

@@ -414,8 +447,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	}
   	rcv_priv = netdev_priv(rcv);
-	qidx = veth_select_rxq(rcv);
-	rq = &rcv_priv->rq[qidx];
+	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
   	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
   	 * side. This means an XDP program is loaded on the peer and the peer
   	 * device is up.

@@ -446,7 +478,6 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	ret = n - drops;
   drop:
-	rq = &priv->rq[qidx];
   	u64_stats_update_begin(&rq->stats.syncp);
   	if (ndo_xmit) {
   		rq->stats.vs.xdp_xmit += n - drops;
Quoted 4 lines

Thoughts?

Toshiaki Makita

author
#14 lore raw

Re: [PATCH net-next 4/5] veth: introduce more xdp counters

Quoted 1 line · Lorenzo Bianconi

[...]

Quoted 8 lines
Quoted 5 lines

I moved the accounting of xdp_tx/tx_error in veth_xdp_xmit for two reason:
1- veth_xdp_tx in veth_xdp_rcv_one or veth_xdp_rcv_skb returns an error
for XDP_TX just if xdp_frame pointer is invalid but the packet can be discarded in veth_xdp_xmit if the device is 'under-pressure' (and this can be a problem since in XDP there are no queueing mechanisms so far)

Right, but you can track the discard in veth_xdp_flush_bq().

ack, I guess it is just a matter of passing veth_rq to veth_xdp_flush_bq and account there for XDP_TX. I will post a patch.

Regards,
Lorenzo

Quoted 143 lines
Quoted 1 line

2- to be symmetric with ndo_xdp_xmit

I thought consistency between drivers is more important. What about other drivers?

Toshiaki Makita

Quoted 137 lines
Quoted 56 lines
Quoted 56 lines
drivers/net/veth.c +25 -2
+	{ "xdp_xmit",		VETH_RQ_STAT(xdp_xmit) },
+	{ "xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
+};
+
+#define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
+
   static struct {
   	const char string[ETH_GSTRING_LEN];
   } ethtool_stats_keys[] = {

@@ -142,6 +147,14 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
   				p += ETH_GSTRING_LEN;
   			}
   		}
+		for (i = 0; i < dev->real_num_tx_queues; i++) {
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				snprintf(p, ETH_GSTRING_LEN,
+					 "tx_queue_%u_%.18s",
+					 i, veth_tq_stats_desc[j].desc);
+				p += ETH_GSTRING_LEN;
+			}
+		}
   		break;
   	}
   }

@@ -151,7 +164,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
   	switch (sset) {
   	case ETH_SS_STATS:
   		return ARRAY_SIZE(ethtool_stats_keys) +
-		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues;
+		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
+		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
   	default:
   		return -EOPNOTSUPP;
   	}

@@ -160,7 +174,7 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
   static void veth_get_ethtool_stats(struct net_device *dev,
   		struct ethtool_stats *stats, u64 *data)
   {
-	struct veth_priv *priv = netdev_priv(dev);
+	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
   	struct net_device *peer = rtnl_dereference(priv->peer);
   	int i, j, idx;

@@ -181,6 +195,26 @@ static void veth_get_ethtool_stats(struct net_device *dev,
   		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
   		idx += VETH_RQ_STATS_LEN;
   	}
+
+	if (!peer)
+		return;
+
+	rcv_priv = netdev_priv(peer);
+	for (i = 0; i < peer->real_num_rx_queues; i++) {
+		const struct veth_rq_stats *rq_stats = &rcv_priv->rq[i].stats;
+		const void *stats_base = (void *)&rq_stats->vs;
Quoted 8 lines

drivers/net/veth.c static void veth_get_ethtool_stats(struct net_device *dev, · line 209

Quoted 4 lines
drivers/net/veth.c +4 -0
+		unsigned int start, tx_idx;
+		size_t offset;
+
+		tx_idx = (i % dev->real_num_tx_queues) * VETH_TQ_STATS_LEN;

I'm a bit concerned that this can fold multiple rx queue counters into one tx counter. But I cannot think of a better idea provided that we want to align XDP stats between drivers. So I'm OK with this.

Since peer->real_num_rx_queues can be greater than dev->real_num_tx_queues, right? IIUC the only guarantee we have is:

peer->real_num_tx_queues < dev->real_num_rx_queues

If you are fine with that approach, I will post a patch before net-next closure.

Regards,
Lorenzo

Quoted 60 lines

Toshiaki Makita

Quoted 57 lines
drivers/net/veth.c +12 -7
+		do {
+			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
+			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
+				offset = veth_tq_stats_desc[j].offset;
+				data[tx_idx + idx + j] += *(u64 *)(stats_base + offset);
+			}
+		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
+	}
   }
   static const struct ethtool_ops veth_ethtool_ops = {

@@ -340,8 +374,7 @@ static void veth_get_stats64(struct net_device *dev,
   	tot->tx_packets = packets;
   	veth_stats_rx(&rx, dev);
-	tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
-	tot->rx_dropped = rx.rx_drops;
+	tot->rx_dropped = rx.rx_drops + rx.xdp_tx_err + rx.xdp_xmit_err;
   	tot->rx_bytes = rx.xdp_bytes;
   	tot->rx_packets = rx.xdp_packets;

@@ -353,7 +386,7 @@ static void veth_get_stats64(struct net_device *dev,
   		tot->rx_packets += packets;
   		veth_stats_rx(&rx, peer);
-		tot->rx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
+		tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
   		tot->tx_bytes += rx.xdp_bytes;
   		tot->tx_packets += rx.xdp_packets;
   	}

@@ -394,9 +427,9 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   			 u32 flags, bool ndo_xmit)
   {
   	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
-	unsigned int qidx, max_len;
   	struct net_device *rcv;
   	int i, ret, drops = n;
+	unsigned int max_len;
   	struct veth_rq *rq;
   	rcu_read_lock();

@@ -414,8 +447,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	}
   	rcv_priv = netdev_priv(rcv);
-	qidx = veth_select_rxq(rcv);
-	rq = &rcv_priv->rq[qidx];
+	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
   	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
   	 * side. This means an XDP program is loaded on the peer and the peer
   	 * device is up.

@@ -446,7 +478,6 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
   	ret = n - drops;
   drop:
-	rq = &priv->rq[qidx];
   	u64_stats_update_begin(&rq->stats.syncp);
   	if (ndo_xmit) {
   		rq->stats.vs.xdp_xmit += n - drops;
Quoted 4 lines

Thoughts?

Toshiaki Makita

author
#15 lore raw

PATCH 5/5 veth: remove atomic64_add from veth_xdp_xmit hotpath

In reply to Lorenzo Bianconi

Remove atomic64_add from veth_xdp_xmit hotpath and rely on xdp_xmit_err/xdp_tx_err counters

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/veth.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/net/veth.c +17 -13
index 093b55acedb1..b6505a6c7102 100644
@@ -301,20 +301,26 @@ static void veth_stats_rx(struct veth_stats *result, struct net_device *dev)
 	struct veth_priv *priv = netdev_priv(dev);
 	int i;
 
+	result->xdp_xmit_err = 0;
 	result->xdp_packets = 0;
+	result->xdp_tx_err = 0;
 	result->xdp_bytes = 0;
 	result->rx_drops = 0;
 	for (i = 0; i < dev->num_rx_queues; i++) {
+		u64 packets, bytes, drops, xdp_tx_err, xdp_xmit_err;
 		struct veth_rq_stats *stats = &priv->rq[i].stats;
-		u64 packets, bytes, drops;
 		unsigned int start;
 
 		do {
 			start = u64_stats_fetch_begin_irq(&stats->syncp);
+			xdp_xmit_err = stats->vs.xdp_xmit_err;
+			xdp_tx_err = stats->vs.xdp_tx_err;
 			packets = stats->vs.xdp_packets;
 			bytes = stats->vs.xdp_bytes;
 			drops = stats->vs.rx_drops;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+		result->xdp_xmit_err += xdp_xmit_err;
+		result->xdp_tx_err += xdp_tx_err;
 		result->xdp_packets += packets;
 		result->xdp_bytes += bytes;
 		result->rx_drops += drops;

@@ -334,6 +340,7 @@ static void veth_get_stats64(struct net_device *dev,
 	tot->tx_packets = packets;
 
 	veth_stats_rx(&rx, dev);
+	tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
 	tot->rx_dropped = rx.rx_drops;
 	tot->rx_bytes = rx.xdp_bytes;
 	tot->rx_packets = rx.xdp_packets;

@@ -346,6 +353,7 @@ static void veth_get_stats64(struct net_device *dev,
 		tot->rx_packets += packets;
 
 		veth_stats_rx(&rx, peer);
+		tot->rx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
 		tot->tx_bytes += rx.xdp_bytes;
 		tot->tx_packets += rx.xdp_packets;
 	}

@@ -393,14 +401,16 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 
 	rcu_read_lock();
 	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
-		ret = -EINVAL;
-		goto drop;
+		rcu_read_unlock();
+		atomic64_add(drops, &priv->dropped);
+		return -EINVAL;
 	}
 
 	rcv = rcu_dereference(priv->peer);
 	if (unlikely(!rcv)) {
-		ret = -ENXIO;
-		goto drop;
+		rcu_read_unlock();
+		atomic64_add(drops, &priv->dropped);
+		return -ENXIO;
 	}
 
 	rcv_priv = netdev_priv(rcv);

@@ -434,6 +444,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 	if (flags & XDP_XMIT_FLUSH)
 		__veth_xdp_flush(rq);
 
+	ret = n - drops;
+drop:
 	rq = &priv->rq[qidx];
 	u64_stats_update_begin(&rq->stats.syncp);
 	if (ndo_xmit) {

@@ -445,15 +457,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
 	}
 	u64_stats_update_end(&rq->stats.syncp);
 
-	if (likely(!drops)) {
-		rcu_read_unlock();
-		return n;
-	}
-
-	ret = n - drops;
-drop:
 	rcu_read_unlock();
-	atomic64_add(drops, &priv->dropped);
 
 	return ret;
 }
Signature · 1 line
2.25.1
maintainer
#16 lore raw
In reply to Lorenzo Bianconi

From: Lorenzo Bianconi <[email protected]>
Date: Thu, 19 Mar 2020 17:41:24 +0100

Quoted 7 lines

Align veth xdp stats accounting to mellanox, intel and marvell implementation. Introduce the following xdp counters:
- rx_xdp_tx
- rx_xdp_tx_errors
- tx_xdp_xmit
- tx_xdp_xmit_errors
- rx_xdp_redirect

Series applied, thanks.