[PATCH net-next 0/5] add more xdp stats to veth driver

[PATCH net-next 0/5] add more xdp stats to veth driver

[Lorenzo Bianconi]

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

-- 
2.25.1

[PATCH net-next 1/5] veth: move xdp stats in a dedicated structure

[Lorenzo Bianconi]

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 <lorenzo@kernel.org>
---
 drivers/net/veth.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index d4cbb9e8c63f..33e23bbde5bf 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -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;
-- 
2.25.1

[PATCH net-next 2/5] veth: introduce more specialized counters in veth_stats

[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 <lorenzo@kernel.org>
---
 drivers/net/veth.c | 72 +++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 32 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 33e23bbde5bf..bad8fd432067 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -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();
 
-- 
2.25.1

[PATCH net-next 3/5] veth: distinguish between rx_drops and xdp_drops

[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 <lorenzo@kernel.org>
---
 drivers/net/veth.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index bad8fd432067..2307696d4897 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -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);
 
-- 
2.25.1

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

[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 <lorenzo@kernel.org>
---
 drivers/net/veth.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 2307696d4897..093b55acedb1 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -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 | \
-- 
2.25.1

[PATCH net-next 5/5] veth: remove atomic64_add from veth_xdp_xmit hotpath

[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 <lorenzo@kernel.org>
---
 drivers/net/veth.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 093b55acedb1..b6505a6c7102 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -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;
 }
-- 
2.25.1

Re: [PATCH net-next 0/5] add more xdp stats to veth driver

[David Miller]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Thu, 19 Mar 2020 17:41:24 +0100

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

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

[Toshiaki Makita]

On 2020/03/20 1:41, Lorenzo Bianconi wrote:
> 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!

> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
...
> @@ -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.

> +	u64_stats_update_begin(&rq->stats.syncp);

So this can cuase NULL pointer dereference.

Toshiaki Makita

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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3257 bytes --]

> On 2020/03/20 1:41, Lorenzo Bianconi wrote:
> > 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!
> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> ...
> > @@ -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.
> 
> > +	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

> 
> Toshiaki Makita
> 
> > +	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 | \
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Toshiaki Makita]

On 2020/03/20 22:37, Lorenzo Bianconi wrote:
>> On 2020/03/20 1:41, Lorenzo Bianconi wrote:
>>> 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!
>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>> ---
>> ...
>>> @@ -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.
>>
>>> +	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?

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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3093 bytes --]

> On 2020/03/20 22:37, Lorenzo Bianconi wrote:
> > > On 2020/03/20 1:41, Lorenzo Bianconi wrote:
> > > > 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!
> > > 
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > ...
> > > > @@ -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.
> > > 
> > > > +	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?
> 
> 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.

> 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

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

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Toshiaki Makita]

On 2020/03/21 23:30, Lorenzo Bianconi wrote:
>> On 2020/03/20 22:37, Lorenzo Bianconi wrote:
>>>> On 2020/03/20 1:41, Lorenzo Bianconi wrote:
>>>>> 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!
>>>>
>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>>>> ---
>>>> ...
>>>>> @@ -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.
>>>>
>>>>> +	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?
>>
>> 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.
> 
>> 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.

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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 6696 bytes --]

> On 2020/03/21 23:30, Lorenzo Bianconi wrote:
> > > On 2020/03/20 22:37, Lorenzo Bianconi wrote:
> > > > > On 2020/03/20 1:41, Lorenzo Bianconi wrote:

[...]

> 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
> }

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

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index b6505a6c7102..f2acd2ee6287 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -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;

> 
> Thoughts?
> 
> Toshiaki Makita

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Toshiaki Makita]

On 2020/03/24 2:31, Lorenzo Bianconi wrote:
>> On 2020/03/21 23:30, Lorenzo Bianconi wrote:
>>>> On 2020/03/20 22:37, Lorenzo Bianconi wrote:
>>>>>> On 2020/03/20 1:41, Lorenzo Bianconi wrote:
> 
> [...]
> 
>> 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
>> }
> 
> 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.

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index b6505a6c7102..f2acd2ee6287 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -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) },

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.

> +	{ "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;

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

> +		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;
> 
>>
>> Thoughts?
>>
>> Toshiaki Makita

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 7241 bytes --]


[...]

> > > }
> > 
> > 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.
> 
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index b6505a6c7102..f2acd2ee6287 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> > @@ -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) },
> 
> 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

> 
> > +	{ "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;
> 
> 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


> 
> Toshiaki Makita
> 
> > +		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;
> > 
> > > 
> > > Thoughts?
> > > 
> > > Toshiaki Makita

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Toshiaki Makita]

On 2020/03/24 23:36, Lorenzo Bianconi wrote:
> 
> [...]
> 
>>>> }
>>>
>>> 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.
>>
>>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>>> index b6505a6c7102..f2acd2ee6287 100644
>>> --- a/drivers/net/veth.c
>>> +++ b/drivers/net/veth.c
>>> @@ -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) },
>>
>> 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)

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

> 2- to be symmetric  with ndo_xdp_xmit

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

Toshiaki Makita

> 
>>
>>> +	{ "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;
>>
>> 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
> 
> 
>>
>> Toshiaki Makita
>>
>>> +		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;
>>>
>>>>
>>>> Thoughts?
>>>>
>>>> Toshiaki Makita

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 6461 bytes --]

> On 2020/03/24 23:36, Lorenzo Bianconi wrote:

[...]

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

> > 2- to be symmetric  with ndo_xdp_xmit
> 
> I thought consistency between drivers is more important. What about other drivers?
> 
> Toshiaki Makita
> 
> > 
> > > 
> > > > +	{ "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;
> > > 
> > > 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
> > 
> > 
> > > 
> > > Toshiaki Makita
> > > 
> > > > +		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;
> > > > 
> > > > > 
> > > > > Thoughts?
> > > > > 
> > > > > Toshiaki Makita

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]