| Message ID | 1547724045-2726-3-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | virtio_net: Fix problems around XDP tx and napi_tx | expand |
On 2019/1/17 下午7:20, Toshiaki Makita wrote: > When napi_tx is enabled, virtnet_poll_cleantx() called > free_old_xmit_skbs() even for xdp send queue. > This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled > device tx bytes counters because skb->len is meaningless value, and even > triggered oops due to general protection fault on freeing them. > > Since xdp send queues do not aquire locks, old xdp_frames should be > freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for > xdp send queues. > > Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI > handler is called even without calling start_xmit() because cb for tx is > by default enabled. Once the handler is called, it enabled the cb again, > and then the handler would be called again. We don't need this handler > for XDP, so don't enable cb as well as not calling free_old_xmit_skbs(). > > Also, we need to disable tx NAPI when disabling XDP, so > virtnet_poll_tx() can safely access curr_queue_pairs and > xdp_queue_pairs, which are not atomically updated while disabling XDP. I suggest to split this into another patch or squash this part to patch 1. Other looks good. Thanks > > Fixes: b92f1e6751a6 ("virtio-net: transmit napi") > Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi") > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > --- > drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++---------------- > 1 file changed, 33 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index a08da9e..7d35e6d 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq) > u64_stats_update_end(&sq->stats.syncp); > } > > +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q) > +{ > + if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs)) > + return false; > + else if (q < vi->curr_queue_pairs) > + return true; > + else > + return false; > +} > + > static void virtnet_poll_cleantx(struct receive_queue *rq) > { > struct virtnet_info *vi = rq->vq->vdev->priv; > @@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq) > struct send_queue *sq = &vi->sq[index]; > struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index); > > - if (!sq->napi.weight) > + if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index)) > return; > > if (__netif_tx_trylock(txq)) { > @@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget) > { > struct send_queue *sq = container_of(napi, struct send_queue, napi); > struct virtnet_info *vi = sq->vq->vdev->priv; > - struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq)); > + unsigned int index = vq2txq(sq->vq); > + struct netdev_queue *txq; > > + if (unlikely(is_xdp_raw_buffer_queue(vi, index))) { > + /* We don't need to enable cb for XDP */ > + napi_complete_done(napi, 0); > + return 0; > + } > + > + txq = netdev_get_tx_queue(vi->dev, index); > __netif_tx_lock(txq, raw_smp_processor_id()); > free_old_xmit_skbs(sq); > __netif_tx_unlock(txq); > @@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, > } > > /* Make sure NAPI is not using any XDP TX queues for RX. */ > - if (netif_running(dev)) > - for (i = 0; i < vi->max_queue_pairs; i++) > + if (netif_running(dev)) { > + for (i = 0; i < vi->max_queue_pairs; i++) { > napi_disable(&vi->rq[i].napi); > + virtnet_napi_tx_disable(&vi->sq[i].napi); > + } > + } > > netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); > err = _virtnet_set_queues(vi, curr_qp + xdp_qp); > @@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, > } > if (old_prog) > bpf_prog_put(old_prog); > - if (netif_running(dev)) > + if (netif_running(dev)) { > virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); > + virtnet_napi_tx_enable(vi, vi->sq[i].vq, > + &vi->sq[i].napi); > + } > } > > return 0; > > err: > if (netif_running(dev)) { > - for (i = 0; i < vi->max_queue_pairs; i++) > + for (i = 0; i < vi->max_queue_pairs; i++) { > virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); > + virtnet_napi_tx_enable(vi, vi->sq[i].vq, > + &vi->sq[i].napi); > + } > } > if (prog) > bpf_prog_sub(prog, vi->max_queue_pairs - 1); > @@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi) > put_page(vi->rq[i].alloc_frag.page); > } > > -static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q) > -{ > - if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs)) > - return false; > - else if (q < vi->curr_queue_pairs) > - return true; > - else > - return false; > -} > - > static void free_unused_bufs(struct virtnet_info *vi) > { > void *buf;
On 2019/01/17 21:39, Jason Wang wrote: > On 2019/1/17 下午7:20, Toshiaki Makita wrote: >> When napi_tx is enabled, virtnet_poll_cleantx() called >> free_old_xmit_skbs() even for xdp send queue. >> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled >> device tx bytes counters because skb->len is meaningless value, and even >> triggered oops due to general protection fault on freeing them. >> >> Since xdp send queues do not aquire locks, old xdp_frames should be >> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for >> xdp send queues. >> >> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI >> handler is called even without calling start_xmit() because cb for tx is >> by default enabled. Once the handler is called, it enabled the cb again, >> and then the handler would be called again. We don't need this handler >> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs(). >> >> Also, we need to disable tx NAPI when disabling XDP, so >> virtnet_poll_tx() can safely access curr_queue_pairs and >> xdp_queue_pairs, which are not atomically updated while disabling XDP. > > > I suggest to split this into another patch or squash this part to patch 1. This part is for invocation of is_xdp_raw_buffer_queue() from virtnet_poll_tx(), which is added in this patch, so I'm thinking it's more natural to keep this hunk in this patch.
On 2019/1/18 上午9:44, Toshiaki Makita wrote: > On 2019/01/17 21:39, Jason Wang wrote: >> On 2019/1/17 下午7:20, Toshiaki Makita wrote: >>> When napi_tx is enabled, virtnet_poll_cleantx() called >>> free_old_xmit_skbs() even for xdp send queue. >>> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled >>> device tx bytes counters because skb->len is meaningless value, and even >>> triggered oops due to general protection fault on freeing them. >>> >>> Since xdp send queues do not aquire locks, old xdp_frames should be >>> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for >>> xdp send queues. >>> >>> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI >>> handler is called even without calling start_xmit() because cb for tx is >>> by default enabled. Once the handler is called, it enabled the cb again, >>> and then the handler would be called again. We don't need this handler >>> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs(). >>> >>> Also, we need to disable tx NAPI when disabling XDP, so >>> virtnet_poll_tx() can safely access curr_queue_pairs and >>> xdp_queue_pairs, which are not atomically updated while disabling XDP. >> >> I suggest to split this into another patch or squash this part to patch 1. > This part is for invocation of is_xdp_raw_buffer_queue() from > virtnet_poll_tx(), which is added in this patch, so I'm thinking it's > more natural to keep this hunk in this patch. I see. Acked-by: Jason Wang <jasowang@redhat.com> Thanks
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index a08da9e..7d35e6d 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq) u64_stats_update_end(&sq->stats.syncp); } +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q) +{ + if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs)) + return false; + else if (q < vi->curr_queue_pairs) + return true; + else + return false; +} + static void virtnet_poll_cleantx(struct receive_queue *rq) { struct virtnet_info *vi = rq->vq->vdev->priv; @@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq) struct send_queue *sq = &vi->sq[index]; struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index); - if (!sq->napi.weight) + if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index)) return; if (__netif_tx_trylock(txq)) { @@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget) { struct send_queue *sq = container_of(napi, struct send_queue, napi); struct virtnet_info *vi = sq->vq->vdev->priv; - struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq)); + unsigned int index = vq2txq(sq->vq); + struct netdev_queue *txq; + if (unlikely(is_xdp_raw_buffer_queue(vi, index))) { + /* We don't need to enable cb for XDP */ + napi_complete_done(napi, 0); + return 0; + } + + txq = netdev_get_tx_queue(vi->dev, index); __netif_tx_lock(txq, raw_smp_processor_id()); free_old_xmit_skbs(sq); __netif_tx_unlock(txq); @@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, } /* Make sure NAPI is not using any XDP TX queues for RX. */ - if (netif_running(dev)) - for (i = 0; i < vi->max_queue_pairs; i++) + if (netif_running(dev)) { + for (i = 0; i < vi->max_queue_pairs; i++) { napi_disable(&vi->rq[i].napi); + virtnet_napi_tx_disable(&vi->sq[i].napi); + } + } netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); err = _virtnet_set_queues(vi, curr_qp + xdp_qp); @@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, } if (old_prog) bpf_prog_put(old_prog); - if (netif_running(dev)) + if (netif_running(dev)) { virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); + virtnet_napi_tx_enable(vi, vi->sq[i].vq, + &vi->sq[i].napi); + } } return 0; err: if (netif_running(dev)) { - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_queue_pairs; i++) { virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); + virtnet_napi_tx_enable(vi, vi->sq[i].vq, + &vi->sq[i].napi); + } } if (prog) bpf_prog_sub(prog, vi->max_queue_pairs - 1); @@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi) put_page(vi->rq[i].alloc_frag.page); } -static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q) -{ - if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs)) - return false; - else if (q < vi->curr_queue_pairs) - return true; - else - return false; -} - static void free_unused_bufs(struct virtnet_info *vi) { void *buf;
When napi_tx is enabled, virtnet_poll_cleantx() called free_old_xmit_skbs() even for xdp send queue. This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled device tx bytes counters because skb->len is meaningless value, and even triggered oops due to general protection fault on freeing them. Since xdp send queues do not aquire locks, old xdp_frames should be freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for xdp send queues. Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI handler is called even without calling start_xmit() because cb for tx is by default enabled. Once the handler is called, it enabled the cb again, and then the handler would be called again. We don't need this handler for XDP, so don't enable cb as well as not calling free_old_xmit_skbs(). Also, we need to disable tx NAPI when disabling XDP, so virtnet_poll_tx() can safely access curr_queue_pairs and xdp_queue_pairs, which are not atomically updated while disabling XDP. Fixes: b92f1e6751a6 ("virtio-net: transmit napi") Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi") Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> --- drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 16 deletions(-)