[PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters

[PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters

[Jason Wang]

Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
count TX XDP stats in virtnet_receive(). This will cause several
issues:

- virtnet_xdp_sq() was called without checking whether or not XDP is
  set. This may cause out of bound access when there's no enough txq
  for XDP.
- Stats were updated even if there's no XDP/XDP_TX.

Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
TX XDP counter itself and remove the unnecessary tx stats embedded in
rx stats.

Reported-by: syzbot+604f8271211546f5b3c7@syzkaller.appspotmail.com
Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 39 ++++-----------------------------------
 1 file changed, 4 insertions(+), 35 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1880c86..72d3f68 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -105,10 +105,6 @@ struct virtnet_rq_stats {
 
 struct virtnet_rx_stats {
 	struct virtnet_rq_stat_items rx;
-	struct {
-		unsigned int xdp_tx;
-		unsigned int xdp_tx_drops;
-	} tx;
 };
 
 #define VIRTNET_SQ_STAT(m)	offsetof(struct virtnet_sq_stats, m)
@@ -485,22 +481,6 @@ static struct send_queue *virtnet_xdp_sq(struct virtnet_info *vi)
 	return &vi->sq[qp];
 }
 
-static int __virtnet_xdp_tx_xmit(struct virtnet_info *vi,
-				   struct xdp_frame *xdpf)
-{
-	struct xdp_frame *xdpf_sent;
-	struct send_queue *sq;
-	unsigned int len;
-
-	sq = virtnet_xdp_sq(vi);
-
-	/* Free up any pending old buffers before queueing new ones. */
-	while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
-		xdp_return_frame(xdpf_sent);
-
-	return __virtnet_xdp_xmit_one(vi, sq, xdpf);
-}
-
 static int virtnet_xdp_xmit(struct net_device *dev,
 			    int n, struct xdp_frame **frames, u32 flags)
 {
@@ -707,10 +687,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
 			xdpf = convert_to_xdp_frame(&xdp);
 			if (unlikely(!xdpf))
 				goto err_xdp;
-			stats->tx.xdp_tx++;
-			err = __virtnet_xdp_tx_xmit(vi, xdpf);
-			if (unlikely(err)) {
-				stats->tx.xdp_tx_drops++;
+			err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
+			if (unlikely(err < 0)) {
 				trace_xdp_exception(vi->dev, xdp_prog, act);
 				goto err_xdp;
 			}
@@ -879,10 +857,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			xdpf = convert_to_xdp_frame(&xdp);
 			if (unlikely(!xdpf))
 				goto err_xdp;
-			stats->tx.xdp_tx++;
-			err = __virtnet_xdp_tx_xmit(vi, xdpf);
-			if (unlikely(err)) {
-				stats->tx.xdp_tx_drops++;
+			err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
+			if (unlikely(err < 0)) {
 				trace_xdp_exception(vi->dev, xdp_prog, act);
 				if (unlikely(xdp_page != page))
 					put_page(xdp_page);
@@ -1315,7 +1291,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 {
 	struct virtnet_info *vi = rq->vq->vdev->priv;
 	struct virtnet_rx_stats stats = {};
-	struct send_queue *sq;
 	unsigned int len;
 	void *buf;
 	int i;
@@ -1351,12 +1326,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 	}
 	u64_stats_update_end(&rq->stats.syncp);
 
-	sq = virtnet_xdp_sq(vi);
-	u64_stats_update_begin(&sq->stats.syncp);
-	sq->stats.xdp_tx += stats.tx.xdp_tx;
-	sq->stats.xdp_tx_drops += stats.tx.xdp_tx_drops;
-	u64_stats_update_end(&sq->stats.syncp);
-
 	return stats.rx.packets;
 }
 
-- 
2.7.4

[PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats

[Jason Wang]

We don't maintain tx counters in rx stats any more. There's no need
for an extra container of rq stats.

Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 44 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 72d3f68..14f661c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -87,7 +87,8 @@ struct virtnet_sq_stats {
 	u64 kicks;
 };
 
-struct virtnet_rq_stat_items {
+struct virtnet_rq_stats {
+	struct u64_stats_sync syncp;
 	u64 packets;
 	u64 bytes;
 	u64 drops;
@@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
 	u64 kicks;
 };
 
-struct virtnet_rq_stats {
-	struct u64_stats_sync syncp;
-	struct virtnet_rq_stat_items items;
-};
-
-struct virtnet_rx_stats {
-	struct virtnet_rq_stat_items rx;
-};
-
 #define VIRTNET_SQ_STAT(m)	offsetof(struct virtnet_sq_stats, m)
-#define VIRTNET_RQ_STAT(m)	offsetof(struct virtnet_rq_stat_items, m)
+#define VIRTNET_RQ_STAT(m)	offsetof(struct virtnet_rq_stats, m)
 
 static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
 	{ "packets",		VIRTNET_SQ_STAT(packets) },
@@ -617,7 +609,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
 				     void *buf, void *ctx,
 				     unsigned int len,
 				     unsigned int *xdp_xmit,
-				     struct virtnet_rx_stats *stats)
+				     struct virtnet_rq_stats *stats)
 {
 	struct sk_buff *skb;
 	struct bpf_prog *xdp_prog;
@@ -632,7 +624,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
 	int err;
 
 	len -= vi->hdr_len;
-	stats->rx.bytes += len;
+	stats->bytes += len;
 
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(rq->xdp_prog);
@@ -674,7 +666,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
 		xdp.rxq = &rq->xdp_rxq;
 		orig_data = xdp.data;
 		act = bpf_prog_run_xdp(xdp_prog, &xdp);
-		stats->rx.xdp_packets++;
+		stats->xdp_packets++;
 
 		switch (act) {
 		case XDP_PASS:
@@ -683,7 +675,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
 			len = xdp.data_end - xdp.data;
 			break;
 		case XDP_TX:
-			stats->rx.xdp_tx++;
+			stats->xdp_tx++;
 			xdpf = convert_to_xdp_frame(&xdp);
 			if (unlikely(!xdpf))
 				goto err_xdp;
@@ -696,7 +688,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
 			rcu_read_unlock();
 			goto xdp_xmit;
 		case XDP_REDIRECT:
-			stats->rx.xdp_redirects++;
+			stats->xdp_redirects++;
 			err = xdp_do_redirect(dev, &xdp, xdp_prog);
 			if (err)
 				goto err_xdp;
@@ -730,8 +722,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
 
 err_xdp:
 	rcu_read_unlock();
-	stats->rx.xdp_drops++;
-	stats->rx.drops++;
+	stats->xdp_drops++;
+	stats->drops++;
 	put_page(page);
 xdp_xmit:
 	return NULL;
@@ -742,19 +734,19 @@ static struct sk_buff *receive_big(struct net_device *dev,
 				   struct receive_queue *rq,
 				   void *buf,
 				   unsigned int len,
-				   struct virtnet_rx_stats *stats)
+				   struct virtnet_rq_stats *stats)
 {
 	struct page *page = buf;
 	struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
 
-	stats->rx.bytes += len - vi->hdr_len;
+	stats->bytes += len - vi->hdr_len;
 	if (unlikely(!skb))
 		goto err;
 
 	return skb;
 
 err:
-	stats->rx.drops++;
+	stats->drops++;
 	give_pages(rq, page);
 	return NULL;
 }
@@ -766,7 +758,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 					 void *ctx,
 					 unsigned int len,
 					 unsigned int *xdp_xmit,
-					 struct virtnet_rx_stats *stats)
+					 struct virtnet_rq_stats *stats)
 {
 	struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
 	u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
@@ -779,7 +771,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	int err;
 
 	head_skb = NULL;
-	stats->rx.bytes += len - vi->hdr_len;
+	stats->bytes += len - vi->hdr_len;
 
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(rq->xdp_prog);
@@ -828,7 +820,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		xdp.rxq = &rq->xdp_rxq;
 
 		act = bpf_prog_run_xdp(xdp_prog, &xdp);
-		stats->rx.xdp_packets++;
+		stats->xdp_packets++;
 
 		switch (act) {
 		case XDP_PASS:
@@ -853,7 +845,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			}
 			break;
 		case XDP_TX:
-			stats->rx.xdp_tx++;
+			stats->xdp_tx++;
 			xdpf = convert_to_xdp_frame(&xdp);
 			if (unlikely(!xdpf))
 				goto err_xdp;
@@ -870,7 +862,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			rcu_read_unlock();
 			goto xdp_xmit;
 		case XDP_REDIRECT:
-			stats->rx.xdp_redirects++;
+			stats->xdp_redirects++;
 			err = xdp_do_redirect(dev, &xdp, xdp_prog);
 			if (err) {
 				if (unlikely(xdp_page != page))
@@ -920,7 +912,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			goto err_buf;
 		}
 
-		stats->rx.bytes += len;
+		stats->bytes += len;
 		page = virt_to_head_page(buf);
 
 		truesize = mergeable_ctx_to_truesize(ctx);
@@ -966,7 +958,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 
 err_xdp:
 	rcu_read_unlock();
-	stats->rx.xdp_drops++;
+	stats->xdp_drops++;
 err_skb:
 	put_page(page);
 	while (num_buf-- > 1) {
@@ -977,12 +969,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 			dev->stats.rx_length_errors++;
 			break;
 		}
-		stats->rx.bytes += len;
+		stats->bytes += len;
 		page = virt_to_head_page(buf);
 		put_page(page);
 	}
 err_buf:
-	stats->rx.drops++;
+	stats->drops++;
 	dev_kfree_skb(head_skb);
 xdp_xmit:
 	return NULL;
@@ -991,7 +983,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 			void *buf, unsigned int len, void **ctx,
 			unsigned int *xdp_xmit,
-			struct virtnet_rx_stats *stats)
+			struct virtnet_rq_stats *stats)
 {
 	struct net_device *dev = vi->dev;
 	struct sk_buff *skb;
@@ -1212,7 +1204,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
 	} while (rq->vq->num_free);
 	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
 		u64_stats_update_begin(&rq->stats.syncp);
-		rq->stats.items.kicks++;
+		rq->stats.kicks++;
 		u64_stats_update_end(&rq->stats.syncp);
 	}
 
@@ -1290,7 +1282,7 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 			   unsigned int *xdp_xmit)
 {
 	struct virtnet_info *vi = rq->vq->vdev->priv;
-	struct virtnet_rx_stats stats = {};
+	struct virtnet_rq_stats stats = {};
 	unsigned int len;
 	void *buf;
 	int i;
@@ -1298,16 +1290,16 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 	if (!vi->big_packets || vi->mergeable_rx_bufs) {
 		void *ctx;
 
-		while (stats.rx.packets < budget &&
+		while (stats.packets < budget &&
 		       (buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
 			receive_buf(vi, rq, buf, len, ctx, xdp_xmit, &stats);
-			stats.rx.packets++;
+			stats.packets++;
 		}
 	} else {
-		while (stats.rx.packets < budget &&
+		while (stats.packets < budget &&
 		       (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
 			receive_buf(vi, rq, buf, len, NULL, xdp_xmit, &stats);
-			stats.rx.packets++;
+			stats.packets++;
 		}
 	}
 
@@ -1321,12 +1313,12 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 		size_t offset = virtnet_rq_stats_desc[i].offset;
 		u64 *item;
 
-		item = (u64 *)((u8 *)&rq->stats.items + offset);
-		*item += *(u64 *)((u8 *)&stats.rx + offset);
+		item = (u64 *)((u8 *)&rq->stats + offset);
+		*item += *(u64 *)((u8 *)&stats + offset);
 	}
 	u64_stats_update_end(&rq->stats.syncp);
 
-	return stats.rx.packets;
+	return stats.packets;
 }
 
 static void free_old_xmit_skbs(struct send_queue *sq)
@@ -1686,9 +1678,9 @@ static void virtnet_stats(struct net_device *dev,
 
 		do {
 			start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
-			rpackets = rq->stats.items.packets;
-			rbytes   = rq->stats.items.bytes;
-			rdrops   = rq->stats.items.drops;
+			rpackets = rq->stats.packets;
+			rbytes   = rq->stats.bytes;
+			rdrops   = rq->stats.drops;
 		} while (u64_stats_fetch_retry_irq(&rq->stats.syncp, start));
 
 		tot->rx_packets += rpackets;
@@ -2078,7 +2070,7 @@ static void virtnet_get_ethtool_stats(struct net_device *dev,
 	for (i = 0; i < vi->curr_queue_pairs; i++) {
 		struct receive_queue *rq = &vi->rq[i];
 
-		stats_base = (u8 *)&rq->stats.items;
+		stats_base = (u8 *)&rq->stats;
 		do {
 			start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
 			for (j = 0; j < VIRTNET_RQ_STATS_LEN; j++) {
-- 
2.7.4

Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters

[Toshiaki Makita]

On 2018/07/31 18:43, Jason Wang wrote:
> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
> count TX XDP stats in virtnet_receive(). This will cause several
> issues:
> 
> - virtnet_xdp_sq() was called without checking whether or not XDP is
>   set. This may cause out of bound access when there's no enough txq
>   for XDP.
> - Stats were updated even if there's no XDP/XDP_TX.>
> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
> TX XDP counter itself and remove the unnecessary tx stats embedded in
> rx stats.

Thanks for fixing this.
I wanted to avoid calling u64_stats_update_begin() (i.e. smp_wmb() in 32
bit systems) for every packet. So I'd like to keep sq stats in
virtnet_rx_stats.

-- 
Toshiaki Makita

Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats

[Toshiaki Makita]

On 2018/07/31 18:43, Jason Wang wrote:
> We don't maintain tx counters in rx stats any more. There's no need
> for an extra container of rq stats.
> 
> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
>  1 file changed, 36 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 72d3f68..14f661c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>  	u64 kicks;
>  };
>  
> -struct virtnet_rq_stat_items {
> +struct virtnet_rq_stats {
> +	struct u64_stats_sync syncp;
>  	u64 packets;
>  	u64 bytes;
>  	u64 drops;
> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>  	u64 kicks;
>  };
>  
> -struct virtnet_rq_stats {
> -	struct u64_stats_sync syncp;
> -	struct virtnet_rq_stat_items items;
> -};

I'm not thinking removing sq stat is needed but even if it is I want to
keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
stack in virtnet_receive. I would just remove virtnet_rx_stats if necessary.

> -
> -struct virtnet_rx_stats {
> -	struct virtnet_rq_stat_items rx;
> -};
> -

-- 
Toshiaki Makita

Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters

[Michael S. Tsirkin]

On Tue, Jul 31, 2018 at 05:43:38PM +0800, Jason Wang wrote:
> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
> count TX XDP stats in virtnet_receive(). This will cause several
> issues:
> 
> - virtnet_xdp_sq() was called without checking whether or not XDP is
>   set. This may cause out of bound access when there's no enough txq
>   for XDP.
> - Stats were updated even if there's no XDP/XDP_TX.
> 
> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
> TX XDP counter itself and remove the unnecessary tx stats embedded in
> rx stats.
> 
> Reported-by: syzbot+604f8271211546f5b3c7@syzkaller.appspotmail.com
> Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c | 39 ++++-----------------------------------
>  1 file changed, 4 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1880c86..72d3f68 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -105,10 +105,6 @@ struct virtnet_rq_stats {
>  
>  struct virtnet_rx_stats {
>  	struct virtnet_rq_stat_items rx;
> -	struct {
> -		unsigned int xdp_tx;
> -		unsigned int xdp_tx_drops;
> -	} tx;
>  };
>  
>  #define VIRTNET_SQ_STAT(m)	offsetof(struct virtnet_sq_stats, m)
> @@ -485,22 +481,6 @@ static struct send_queue *virtnet_xdp_sq(struct virtnet_info *vi)
>  	return &vi->sq[qp];
>  }
>  
> -static int __virtnet_xdp_tx_xmit(struct virtnet_info *vi,
> -				   struct xdp_frame *xdpf)
> -{
> -	struct xdp_frame *xdpf_sent;
> -	struct send_queue *sq;
> -	unsigned int len;
> -
> -	sq = virtnet_xdp_sq(vi);
> -
> -	/* Free up any pending old buffers before queueing new ones. */
> -	while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
> -		xdp_return_frame(xdpf_sent);
> -
> -	return __virtnet_xdp_xmit_one(vi, sq, xdpf);
> -}
> -
>  static int virtnet_xdp_xmit(struct net_device *dev,
>  			    int n, struct xdp_frame **frames, u32 flags)
>  {
> @@ -707,10 +687,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  			xdpf = convert_to_xdp_frame(&xdp);
>  			if (unlikely(!xdpf))
>  				goto err_xdp;
> -			stats->tx.xdp_tx++;
> -			err = __virtnet_xdp_tx_xmit(vi, xdpf);
> -			if (unlikely(err)) {
> -				stats->tx.xdp_tx_drops++;
> +			err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> +			if (unlikely(err < 0)) {
>  				trace_xdp_exception(vi->dev, xdp_prog, act);
>  				goto err_xdp;
>  			}
> @@ -879,10 +857,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  			xdpf = convert_to_xdp_frame(&xdp);
>  			if (unlikely(!xdpf))
>  				goto err_xdp;
> -			stats->tx.xdp_tx++;
> -			err = __virtnet_xdp_tx_xmit(vi, xdpf);
> -			if (unlikely(err)) {
> -				stats->tx.xdp_tx_drops++;
> +			err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> +			if (unlikely(err < 0)) {
>  				trace_xdp_exception(vi->dev, xdp_prog, act);
>  				if (unlikely(xdp_page != page))
>  					put_page(xdp_page);
> @@ -1315,7 +1291,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  {
>  	struct virtnet_info *vi = rq->vq->vdev->priv;
>  	struct virtnet_rx_stats stats = {};
> -	struct send_queue *sq;
>  	unsigned int len;
>  	void *buf;
>  	int i;
> @@ -1351,12 +1326,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  	}
>  	u64_stats_update_end(&rq->stats.syncp);
>  
> -	sq = virtnet_xdp_sq(vi);
> -	u64_stats_update_begin(&sq->stats.syncp);
> -	sq->stats.xdp_tx += stats.tx.xdp_tx;
> -	sq->stats.xdp_tx_drops += stats.tx.xdp_tx_drops;
> -	u64_stats_update_end(&sq->stats.syncp);
> -
>  	return stats.rx.packets;
>  }
>  
> -- 
> 2.7.4

Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats

[Michael S. Tsirkin]

On Tue, Jul 31, 2018 at 05:43:39PM +0800, Jason Wang wrote:
> We don't maintain tx counters in rx stats any more. There's no need
> for an extra container of rq stats.
> 
> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
>  1 file changed, 36 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 72d3f68..14f661c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>  	u64 kicks;
>  };
>  
> -struct virtnet_rq_stat_items {
> +struct virtnet_rq_stats {
> +	struct u64_stats_sync syncp;
>  	u64 packets;
>  	u64 bytes;
>  	u64 drops;
> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>  	u64 kicks;
>  };
>  
> -struct virtnet_rq_stats {
> -	struct u64_stats_sync syncp;
> -	struct virtnet_rq_stat_items items;
> -};
> -
> -struct virtnet_rx_stats {
> -	struct virtnet_rq_stat_items rx;
> -};
> -
>  #define VIRTNET_SQ_STAT(m)	offsetof(struct virtnet_sq_stats, m)
> -#define VIRTNET_RQ_STAT(m)	offsetof(struct virtnet_rq_stat_items, m)
> +#define VIRTNET_RQ_STAT(m)	offsetof(struct virtnet_rq_stats, m)
>  
>  static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
>  	{ "packets",		VIRTNET_SQ_STAT(packets) },
> @@ -617,7 +609,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  				     void *buf, void *ctx,
>  				     unsigned int len,
>  				     unsigned int *xdp_xmit,
> -				     struct virtnet_rx_stats *stats)
> +				     struct virtnet_rq_stats *stats)
>  {
>  	struct sk_buff *skb;
>  	struct bpf_prog *xdp_prog;
> @@ -632,7 +624,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  	int err;
>  
>  	len -= vi->hdr_len;
> -	stats->rx.bytes += len;
> +	stats->bytes += len;
>  
>  	rcu_read_lock();
>  	xdp_prog = rcu_dereference(rq->xdp_prog);
> @@ -674,7 +666,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  		xdp.rxq = &rq->xdp_rxq;
>  		orig_data = xdp.data;
>  		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> -		stats->rx.xdp_packets++;
> +		stats->xdp_packets++;
>  
>  		switch (act) {
>  		case XDP_PASS:
> @@ -683,7 +675,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  			len = xdp.data_end - xdp.data;
>  			break;
>  		case XDP_TX:
> -			stats->rx.xdp_tx++;
> +			stats->xdp_tx++;
>  			xdpf = convert_to_xdp_frame(&xdp);
>  			if (unlikely(!xdpf))
>  				goto err_xdp;
> @@ -696,7 +688,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  			rcu_read_unlock();
>  			goto xdp_xmit;
>  		case XDP_REDIRECT:
> -			stats->rx.xdp_redirects++;
> +			stats->xdp_redirects++;
>  			err = xdp_do_redirect(dev, &xdp, xdp_prog);
>  			if (err)
>  				goto err_xdp;
> @@ -730,8 +722,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  
>  err_xdp:
>  	rcu_read_unlock();
> -	stats->rx.xdp_drops++;
> -	stats->rx.drops++;
> +	stats->xdp_drops++;
> +	stats->drops++;
>  	put_page(page);
>  xdp_xmit:
>  	return NULL;
> @@ -742,19 +734,19 @@ static struct sk_buff *receive_big(struct net_device *dev,
>  				   struct receive_queue *rq,
>  				   void *buf,
>  				   unsigned int len,
> -				   struct virtnet_rx_stats *stats)
> +				   struct virtnet_rq_stats *stats)
>  {
>  	struct page *page = buf;
>  	struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
>  
> -	stats->rx.bytes += len - vi->hdr_len;
> +	stats->bytes += len - vi->hdr_len;
>  	if (unlikely(!skb))
>  		goto err;
>  
>  	return skb;
>  
>  err:
> -	stats->rx.drops++;
> +	stats->drops++;
>  	give_pages(rq, page);
>  	return NULL;
>  }
> @@ -766,7 +758,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  					 void *ctx,
>  					 unsigned int len,
>  					 unsigned int *xdp_xmit,
> -					 struct virtnet_rx_stats *stats)
> +					 struct virtnet_rq_stats *stats)
>  {
>  	struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
>  	u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> @@ -779,7 +771,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  	int err;
>  
>  	head_skb = NULL;
> -	stats->rx.bytes += len - vi->hdr_len;
> +	stats->bytes += len - vi->hdr_len;
>  
>  	rcu_read_lock();
>  	xdp_prog = rcu_dereference(rq->xdp_prog);
> @@ -828,7 +820,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  		xdp.rxq = &rq->xdp_rxq;
>  
>  		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> -		stats->rx.xdp_packets++;
> +		stats->xdp_packets++;
>  
>  		switch (act) {
>  		case XDP_PASS:
> @@ -853,7 +845,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  			}
>  			break;
>  		case XDP_TX:
> -			stats->rx.xdp_tx++;
> +			stats->xdp_tx++;
>  			xdpf = convert_to_xdp_frame(&xdp);
>  			if (unlikely(!xdpf))
>  				goto err_xdp;
> @@ -870,7 +862,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  			rcu_read_unlock();
>  			goto xdp_xmit;
>  		case XDP_REDIRECT:
> -			stats->rx.xdp_redirects++;
> +			stats->xdp_redirects++;
>  			err = xdp_do_redirect(dev, &xdp, xdp_prog);
>  			if (err) {
>  				if (unlikely(xdp_page != page))
> @@ -920,7 +912,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  			goto err_buf;
>  		}
>  
> -		stats->rx.bytes += len;
> +		stats->bytes += len;
>  		page = virt_to_head_page(buf);
>  
>  		truesize = mergeable_ctx_to_truesize(ctx);
> @@ -966,7 +958,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  
>  err_xdp:
>  	rcu_read_unlock();
> -	stats->rx.xdp_drops++;
> +	stats->xdp_drops++;
>  err_skb:
>  	put_page(page);
>  	while (num_buf-- > 1) {
> @@ -977,12 +969,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  			dev->stats.rx_length_errors++;
>  			break;
>  		}
> -		stats->rx.bytes += len;
> +		stats->bytes += len;
>  		page = virt_to_head_page(buf);
>  		put_page(page);
>  	}
>  err_buf:
> -	stats->rx.drops++;
> +	stats->drops++;
>  	dev_kfree_skb(head_skb);
>  xdp_xmit:
>  	return NULL;
> @@ -991,7 +983,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  			void *buf, unsigned int len, void **ctx,
>  			unsigned int *xdp_xmit,
> -			struct virtnet_rx_stats *stats)
> +			struct virtnet_rq_stats *stats)
>  {
>  	struct net_device *dev = vi->dev;
>  	struct sk_buff *skb;
> @@ -1212,7 +1204,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>  	} while (rq->vq->num_free);
>  	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
>  		u64_stats_update_begin(&rq->stats.syncp);
> -		rq->stats.items.kicks++;
> +		rq->stats.kicks++;
>  		u64_stats_update_end(&rq->stats.syncp);
>  	}
>  
> @@ -1290,7 +1282,7 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  			   unsigned int *xdp_xmit)
>  {
>  	struct virtnet_info *vi = rq->vq->vdev->priv;
> -	struct virtnet_rx_stats stats = {};
> +	struct virtnet_rq_stats stats = {};
>  	unsigned int len;
>  	void *buf;
>  	int i;
> @@ -1298,16 +1290,16 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  	if (!vi->big_packets || vi->mergeable_rx_bufs) {
>  		void *ctx;
>  
> -		while (stats.rx.packets < budget &&
> +		while (stats.packets < budget &&
>  		       (buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
>  			receive_buf(vi, rq, buf, len, ctx, xdp_xmit, &stats);
> -			stats.rx.packets++;
> +			stats.packets++;
>  		}
>  	} else {
> -		while (stats.rx.packets < budget &&
> +		while (stats.packets < budget &&
>  		       (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
>  			receive_buf(vi, rq, buf, len, NULL, xdp_xmit, &stats);
> -			stats.rx.packets++;
> +			stats.packets++;
>  		}
>  	}
>  
> @@ -1321,12 +1313,12 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  		size_t offset = virtnet_rq_stats_desc[i].offset;
>  		u64 *item;
>  
> -		item = (u64 *)((u8 *)&rq->stats.items + offset);
> -		*item += *(u64 *)((u8 *)&stats.rx + offset);
> +		item = (u64 *)((u8 *)&rq->stats + offset);
> +		*item += *(u64 *)((u8 *)&stats + offset);
>  	}
>  	u64_stats_update_end(&rq->stats.syncp);
>  
> -	return stats.rx.packets;
> +	return stats.packets;
>  }
>  
>  static void free_old_xmit_skbs(struct send_queue *sq)
> @@ -1686,9 +1678,9 @@ static void virtnet_stats(struct net_device *dev,
>  
>  		do {
>  			start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
> -			rpackets = rq->stats.items.packets;
> -			rbytes   = rq->stats.items.bytes;
> -			rdrops   = rq->stats.items.drops;
> +			rpackets = rq->stats.packets;
> +			rbytes   = rq->stats.bytes;
> +			rdrops   = rq->stats.drops;
>  		} while (u64_stats_fetch_retry_irq(&rq->stats.syncp, start));
>  
>  		tot->rx_packets += rpackets;
> @@ -2078,7 +2070,7 @@ static void virtnet_get_ethtool_stats(struct net_device *dev,
>  	for (i = 0; i < vi->curr_queue_pairs; i++) {
>  		struct receive_queue *rq = &vi->rq[i];
>  
> -		stats_base = (u8 *)&rq->stats.items;
> +		stats_base = (u8 *)&rq->stats;
>  		do {
>  			start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
>  			for (j = 0; j < VIRTNET_RQ_STATS_LEN; j++) {
> -- 
> 2.7.4

Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters

[David Miller]

From: Jason Wang <jasowang@redhat.com>
Date: Tue, 31 Jul 2018 17:43:38 +0800

> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
> count TX XDP stats in virtnet_receive(). This will cause several
> issues:
> 
> - virtnet_xdp_sq() was called without checking whether or not XDP is
>   set. This may cause out of bound access when there's no enough txq
>   for XDP.
> - Stats were updated even if there's no XDP/XDP_TX.
> 
> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
> TX XDP counter itself and remove the unnecessary tx stats embedded in
> rx stats.
> 
> Reported-by: syzbot+604f8271211546f5b3c7@syzkaller.appspotmail.com
> Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Applied.

Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats

[David Miller]

From: Jason Wang <jasowang@redhat.com>
Date: Tue, 31 Jul 2018 17:43:39 +0800

> We don't maintain tx counters in rx stats any more. There's no need
> for an extra container of rq stats.
> 
> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Applied.

Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters

[Jason Wang]

On 2018年07月31日 17:57, Toshiaki Makita wrote:
> On 2018/07/31 18:43, Jason Wang wrote:
>> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
>> count TX XDP stats in virtnet_receive(). This will cause several
>> issues:
>>
>> - virtnet_xdp_sq() was called without checking whether or not XDP is
>>    set. This may cause out of bound access when there's no enough txq
>>    for XDP.
>> - Stats were updated even if there's no XDP/XDP_TX.>
>> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
>> TX XDP counter itself and remove the unnecessary tx stats embedded in
>> rx stats.
> Thanks for fixing this.
> I wanted to avoid calling u64_stats_update_begin() (i.e. smp_wmb() in 32
> bit systems) for every packet. So I'd like to keep sq stats in
> virtnet_rx_stats.
>

We can optimize this by adding batching on top. (virtnet_xdp_xmit() 
accepts an array of xdp frames). If you like, please send a patch for this.

Thanks

Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats

[Jason Wang]

On 2018年07月31日 18:02, Toshiaki Makita wrote:
> On 2018/07/31 18:43, Jason Wang wrote:
>> We don't maintain tx counters in rx stats any more. There's no need
>> for an extra container of rq stats.
>>
>> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
>>   1 file changed, 36 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 72d3f68..14f661c 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>>   	u64 kicks;
>>   };
>>   
>> -struct virtnet_rq_stat_items {
>> +struct virtnet_rq_stats {
>> +	struct u64_stats_sync syncp;
>>   	u64 packets;
>>   	u64 bytes;
>>   	u64 drops;
>> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>>   	u64 kicks;
>>   };
>>   
>> -struct virtnet_rq_stats {
>> -	struct u64_stats_sync syncp;
>> -	struct virtnet_rq_stat_items items;
>> -};
> I'm not thinking removing sq stat is needed but even if it is I want to
> keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
> stack in virtnet_receive. I would just remove virtnet_rx_stats if necessary.

It's a nop on 64bit machines. And an unsigned on 32bit. So it's overhead 
could be ignored I think.

Thanks

>> -
>> -struct virtnet_rx_stats {
>> -	struct virtnet_rq_stat_items rx;
>> -};
>> -

Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters

[Toshiaki Makita]

On 2018/08/01 10:31, Jason Wang wrote:
> On 2018年07月31日 17:57, Toshiaki Makita wrote:
>> On 2018/07/31 18:43, Jason Wang wrote:
>>> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
>>> count TX XDP stats in virtnet_receive(). This will cause several
>>> issues:
>>>
>>> - virtnet_xdp_sq() was called without checking whether or not XDP is
>>>    set. This may cause out of bound access when there's no enough txq
>>>    for XDP.
>>> - Stats were updated even if there's no XDP/XDP_TX.>
>>> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
>>> TX XDP counter itself and remove the unnecessary tx stats embedded in
>>> rx stats.
>> Thanks for fixing this.
>> I wanted to avoid calling u64_stats_update_begin() (i.e. smp_wmb() in 32
>> bit systems) for every packet. So I'd like to keep sq stats in
>> virtnet_rx_stats.
>>
> 
> We can optimize this by adding batching on top. (virtnet_xdp_xmit()
> accepts an array of xdp frames). If you like, please send a patch for this.

Yes, that sounds like a better optimization. will think about it...

Thanks,
Toshiaki Makita

Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats

[Toshiaki Makita]

On 2018/08/01 10:39, Jason Wang wrote:
> On 2018年07月31日 18:02, Toshiaki Makita wrote:
>> On 2018/07/31 18:43, Jason Wang wrote:
>>> We don't maintain tx counters in rx stats any more. There's no need
>>> for an extra container of rq stats.
>>>
>>> Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>   drivers/net/virtio_net.c | 80
>>> ++++++++++++++++++++++--------------------------
>>>   1 file changed, 36 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 72d3f68..14f661c 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>>>       u64 kicks;
>>>   };
>>>   -struct virtnet_rq_stat_items {
>>> +struct virtnet_rq_stats {
>>> +    struct u64_stats_sync syncp;
>>>       u64 packets;
>>>       u64 bytes;
>>>       u64 drops;
>>> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>>>       u64 kicks;
>>>   };
>>>   -struct virtnet_rq_stats {
>>> -    struct u64_stats_sync syncp;
>>> -    struct virtnet_rq_stat_items items;
>>> -};
>> I'm not thinking removing sq stat is needed but even if it is I want to
>> keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
>> stack in virtnet_receive. I would just remove virtnet_rx_stats if
>> necessary.
> 
> It's a nop on 64bit machines. And an unsigned on 32bit. So it's overhead
> could be ignored I think.

It's not a big problem so that's OK. I just felt like you reverted
unnecessarily too much. Anyway it is already applied and I'm not
thinking of changing this any more.

-- 
Toshiaki Makita