Jason Wang
2018-Jul-31 09:43 UTC
[PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters
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 at syzkaller.appspotmail.com Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") Cc: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> Signed-off-by: Jason Wang <jasowang at 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
Jason Wang
2018-Jul-31 09:43 UTC
[PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats
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 at lab.ntt.co.jp> Signed-off-by: Jason Wang <jasowang at 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
Toshiaki Makita
2018-Jul-31 09:57 UTC
[PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters
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
Toshiaki Makita
2018-Jul-31 10:02 UTC
[PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats
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 at lab.ntt.co.jp> > Signed-off-by: Jason Wang <jasowang at 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
Michael S. Tsirkin
2018-Jul-31 11:22 UTC
[PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters
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 at syzkaller.appspotmail.com > Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") > Cc: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> > Signed-off-by: Jason Wang <jasowang at redhat.com>Acked-by: Michael S. Tsirkin <mst at 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
Michael S. Tsirkin
2018-Jul-31 11:22 UTC
[PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats
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 at lab.ntt.co.jp> > Signed-off-by: Jason Wang <jasowang at redhat.com>Acked-by: Michael S. Tsirkin <mst at 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
David Miller
2018-Jul-31 17:03 UTC
[PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters
From: Jason Wang <jasowang at 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 at syzkaller.appspotmail.com > Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") > Cc: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> > Signed-off-by: Jason Wang <jasowang at redhat.com>Applied.
David Miller
2018-Jul-31 17:03 UTC
[PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats
From: Jason Wang <jasowang at 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 at lab.ntt.co.jp> > Signed-off-by: Jason Wang <jasowang at redhat.com>Applied.
Jason Wang
2018-Aug-01 01:31 UTC
[PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters
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
Maybe Matching Threads
- [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters
- [PATCH net-next 0/8] virtio_net: refactor xdp codes
- [PATCH net-next 0/8] virtio_net: refactor xdp codes
- [RFC net-next 0/8] virtio_net: refactor xdp codes
- [PATCH net-next 3/8] virtio_net: introduce virtnet_xdp_handler() to seprate the logic of run xdp