* [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters @ 2018-07-31 9:43 Jason Wang 2018-07-31 9:43 ` [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats Jason Wang ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Jason Wang @ 2018-07-31 9:43 UTC (permalink / raw) To: mst, jasowang; +Cc: virtualization, netdev, linux-kernel, Toshiaki Makita 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats 2018-07-31 9:43 [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Jason Wang @ 2018-07-31 9:43 ` Jason Wang 2018-07-31 10:02 ` Toshiaki Makita ` (2 more replies) 2018-07-31 9:57 ` [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Toshiaki Makita ` (2 subsequent siblings) 3 siblings, 3 replies; 12+ messages in thread From: Jason Wang @ 2018-07-31 9:43 UTC (permalink / raw) To: mst, jasowang; +Cc: virtualization, netdev, linux-kernel, Toshiaki Makita 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats 2018-07-31 9:43 ` [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats Jason Wang @ 2018-07-31 10:02 ` Toshiaki Makita 2018-08-01 1:39 ` Jason Wang 2018-07-31 11:22 ` Michael S. Tsirkin 2018-07-31 17:03 ` David Miller 2 siblings, 1 reply; 12+ messages in thread From: Toshiaki Makita @ 2018-07-31 10:02 UTC (permalink / raw) To: Jason Wang; +Cc: mst, virtualization, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats 2018-07-31 10:02 ` Toshiaki Makita @ 2018-08-01 1:39 ` Jason Wang 2018-08-01 1:46 ` Toshiaki Makita 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2018-08-01 1:39 UTC (permalink / raw) To: Toshiaki Makita; +Cc: mst, virtualization, netdev, linux-kernel 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; >> -}; >> - ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats 2018-08-01 1:39 ` Jason Wang @ 2018-08-01 1:46 ` Toshiaki Makita 0 siblings, 0 replies; 12+ messages in thread From: Toshiaki Makita @ 2018-08-01 1:46 UTC (permalink / raw) To: Jason Wang; +Cc: mst, virtualization, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats 2018-07-31 9:43 ` [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats Jason Wang 2018-07-31 10:02 ` Toshiaki Makita @ 2018-07-31 11:22 ` Michael S. Tsirkin 2018-07-31 17:03 ` David Miller 2 siblings, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2018-07-31 11:22 UTC (permalink / raw) To: Jason Wang; +Cc: virtualization, netdev, linux-kernel, Toshiaki Makita 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats 2018-07-31 9:43 ` [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats Jason Wang 2018-07-31 10:02 ` Toshiaki Makita 2018-07-31 11:22 ` Michael S. Tsirkin @ 2018-07-31 17:03 ` David Miller 2 siblings, 0 replies; 12+ messages in thread From: David Miller @ 2018-07-31 17:03 UTC (permalink / raw) To: jasowang; +Cc: mst, virtualization, netdev, linux-kernel, makita.toshiaki 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters 2018-07-31 9:43 [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Jason Wang 2018-07-31 9:43 ` [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats Jason Wang @ 2018-07-31 9:57 ` Toshiaki Makita 2018-08-01 1:31 ` Jason Wang 2018-07-31 11:22 ` Michael S. Tsirkin 2018-07-31 17:03 ` David Miller 3 siblings, 1 reply; 12+ messages in thread From: Toshiaki Makita @ 2018-07-31 9:57 UTC (permalink / raw) To: Jason Wang; +Cc: mst, virtualization, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters 2018-07-31 9:57 ` [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Toshiaki Makita @ 2018-08-01 1:31 ` Jason Wang 2018-08-01 1:42 ` Toshiaki Makita 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2018-08-01 1:31 UTC (permalink / raw) To: Toshiaki Makita; +Cc: mst, virtualization, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters 2018-08-01 1:31 ` Jason Wang @ 2018-08-01 1:42 ` Toshiaki Makita 0 siblings, 0 replies; 12+ messages in thread From: Toshiaki Makita @ 2018-08-01 1:42 UTC (permalink / raw) To: Jason Wang; +Cc: mst, virtualization, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters 2018-07-31 9:43 [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Jason Wang 2018-07-31 9:43 ` [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats Jason Wang 2018-07-31 9:57 ` [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Toshiaki Makita @ 2018-07-31 11:22 ` Michael S. Tsirkin 2018-07-31 17:03 ` David Miller 3 siblings, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2018-07-31 11:22 UTC (permalink / raw) To: Jason Wang; +Cc: virtualization, netdev, linux-kernel, Toshiaki Makita 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters 2018-07-31 9:43 [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Jason Wang ` (2 preceding siblings ...) 2018-07-31 11:22 ` Michael S. Tsirkin @ 2018-07-31 17:03 ` David Miller 3 siblings, 0 replies; 12+ messages in thread From: David Miller @ 2018-07-31 17:03 UTC (permalink / raw) To: jasowang; +Cc: mst, virtualization, netdev, linux-kernel, makita.toshiaki 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-08-01 1:47 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-31 9:43 [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Jason Wang 2018-07-31 9:43 ` [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats Jason Wang 2018-07-31 10:02 ` Toshiaki Makita 2018-08-01 1:39 ` Jason Wang 2018-08-01 1:46 ` Toshiaki Makita 2018-07-31 11:22 ` Michael S. Tsirkin 2018-07-31 17:03 ` David Miller 2018-07-31 9:57 ` [PATCH net-next 1/2] virtio-net: correctly update XDP_TX counters Toshiaki Makita 2018-08-01 1:31 ` Jason Wang 2018-08-01 1:42 ` Toshiaki Makita 2018-07-31 11:22 ` Michael S. Tsirkin 2018-07-31 17:03 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).