* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-02 15:20 ` [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker Bui Quang Minh
@ 2026-01-03 0:16 ` Michael S. Tsirkin
2026-01-03 16:57 ` Michael S. Tsirkin
2026-01-04 6:09 ` Jason Wang
2 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-03 0:16 UTC (permalink / raw)
To: Bui Quang Minh
Cc: netdev, Jason Wang, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On Fri, Jan 02, 2026 at 10:20:21PM +0700, Bui Quang Minh wrote:
> When we fail to refill the receive buffers, we schedule a delayed worker
> to retry later. However, this worker creates some concurrency issues
> such as races and deadlocks.
include at least one example here, pls.
> To simplify the logic and avoid further
> problems, we will instead retry refilling in the next NAPI poll.
>
> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> Cc: stable@vger.kernel.org
> Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
> drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1bb3aeca66c6..ac514c9383ae 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> }
>
> static int virtnet_receive(struct receive_queue *rq, int budget,
> - unsigned int *xdp_xmit)
> + unsigned int *xdp_xmit, bool *retry_refill)
> {
> struct virtnet_info *vi = rq->vq->vdev->priv;
> struct virtnet_rq_stats stats = {};
> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>
> if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> - spin_lock(&vi->refill_lock);
> - if (vi->refill_enabled)
> - schedule_delayed_work(&vi->refill, 0);
> - spin_unlock(&vi->refill_lock);
> - }
> + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> + *retry_refill = true;
> }
>
> u64_stats_set(&stats.packets, packets);
So this function sets retry_refill to true but assumes caller
will set it to false? seems unnecessarily complex.
just have to always set retry_refill correctly
and not rely on the caller.
> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> struct send_queue *sq;
> unsigned int received;
> unsigned int xdp_xmit = 0;
> - bool napi_complete;
> + bool napi_complete, retry_refill = false;
>
> virtnet_poll_cleantx(rq, budget);
>
> - received = virtnet_receive(rq, budget, &xdp_xmit);
> + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> rq->packets_in_napi += received;
>
> if (xdp_xmit & VIRTIO_XDP_REDIR)
> xdp_do_flush();
>
> /* Out of packets? */
> - if (received < budget) {
> + if (received < budget && !retry_refill) {
> napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> /* Intentionally not taking dim_lock here. This may result in a
> * spurious net_dim call. But if that happens virtnet_rx_dim_work
> @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> virtnet_xdp_put_sq(vi, sq);
> }
>
> - return received;
> + return retry_refill ? budget : received;
a comment can't hurt here, to document what is going on.
> }
>
> static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> if (i < vi->curr_queue_pairs)
> - /* Make sure we have some buffers: if oom use wq. */
> - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> - schedule_delayed_work(&vi->refill, 0);
> + /* If this fails, we will retry later in
> + * NAPI poll, which is scheduled in the below
> + * virtnet_enable_queue_pair
> + */
> + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>
> err = virtnet_enable_queue_pair(vi, i);
> if (err < 0)
> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> bool refill)
> {
> bool running = netif_running(vi->dev);
> - bool schedule_refill = false;
>
> - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> - schedule_refill = true;
> + if (refill)
> + /* If this fails, we will retry later in NAPI poll, which is
> + * scheduled in the below virtnet_napi_enable
> + */
> + try_fill_recv(vi, rq, GFP_KERNEL);
> +
> if (running)
> virtnet_napi_enable(rq);
> -
> - if (schedule_refill)
> - schedule_delayed_work(&vi->refill, 0);
> }
>
> static void virtnet_rx_resume_all(struct virtnet_info *vi)
> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> struct virtio_net_rss_config_trailer old_rss_trailer;
> struct net_device *dev = vi->dev;
> struct scatterlist sg;
> + int i;
>
> if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> return 0;
> @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> }
> succ:
> vi->curr_queue_pairs = queue_pairs;
> - /* virtnet_open() will refill when device is going to up. */
> - spin_lock_bh(&vi->refill_lock);
> - if (dev->flags & IFF_UP && vi->refill_enabled)
> - schedule_delayed_work(&vi->refill, 0);
> - spin_unlock_bh(&vi->refill_lock);
> + if (dev->flags & IFF_UP) {
> + /* Let the NAPI poll refill the receive buffer for us. We can't
> + * safely call try_fill_recv() here because the NAPI might be
> + * enabled already.
> + */
> + local_bh_disable();
> + for (i = 0; i < vi->curr_queue_pairs; i++)
you cam declare i here in the for loop.
and ++i is a bit clearer.
> + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> +
> + local_bh_enable();
> + }
>
> return 0;
> }
> --
> 2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-02 15:20 ` [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker Bui Quang Minh
2026-01-03 0:16 ` Michael S. Tsirkin
@ 2026-01-03 16:57 ` Michael S. Tsirkin
2026-01-03 17:34 ` Michael S. Tsirkin
2026-01-04 4:33 ` Bui Quang Minh
2026-01-04 6:09 ` Jason Wang
2 siblings, 2 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-03 16:57 UTC (permalink / raw)
To: Bui Quang Minh
Cc: netdev, Jason Wang, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On Fri, Jan 02, 2026 at 10:20:21PM +0700, Bui Quang Minh wrote:
> When we fail to refill the receive buffers, we schedule a delayed worker
> to retry later. However, this worker creates some concurrency issues
> such as races and deadlocks. To simplify the logic and avoid further
> problems, we will instead retry refilling in the next NAPI poll.
>
> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> Cc: stable@vger.kernel.org
> Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
> drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1bb3aeca66c6..ac514c9383ae 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> }
>
> static int virtnet_receive(struct receive_queue *rq, int budget,
> - unsigned int *xdp_xmit)
> + unsigned int *xdp_xmit, bool *retry_refill)
> {
> struct virtnet_info *vi = rq->vq->vdev->priv;
> struct virtnet_rq_stats stats = {};
> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>
> if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> - spin_lock(&vi->refill_lock);
> - if (vi->refill_enabled)
> - schedule_delayed_work(&vi->refill, 0);
> - spin_unlock(&vi->refill_lock);
> - }
> + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> + *retry_refill = true;
> }
>
> u64_stats_set(&stats.packets, packets);
> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> struct send_queue *sq;
> unsigned int received;
> unsigned int xdp_xmit = 0;
> - bool napi_complete;
> + bool napi_complete, retry_refill = false;
>
> virtnet_poll_cleantx(rq, budget);
>
> - received = virtnet_receive(rq, budget, &xdp_xmit);
> + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> rq->packets_in_napi += received;
>
> if (xdp_xmit & VIRTIO_XDP_REDIR)
> xdp_do_flush();
>
> /* Out of packets? */
> - if (received < budget) {
> + if (received < budget && !retry_refill) {
> napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> /* Intentionally not taking dim_lock here. This may result in a
> * spurious net_dim call. But if that happens virtnet_rx_dim_work
> @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> virtnet_xdp_put_sq(vi, sq);
> }
>
> - return received;
> + return retry_refill ? budget : received;
> }
>
> static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> if (i < vi->curr_queue_pairs)
> - /* Make sure we have some buffers: if oom use wq. */
> - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> - schedule_delayed_work(&vi->refill, 0);
> + /* If this fails, we will retry later in
> + * NAPI poll, which is scheduled in the below
> + * virtnet_enable_queue_pair
hmm do we even need this, then?
> + */
> + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>
> err = virtnet_enable_queue_pair(vi, i);
> if (err < 0)
> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> bool refill)
> {
> bool running = netif_running(vi->dev);
> - bool schedule_refill = false;
>
> - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> - schedule_refill = true;
> + if (refill)
> + /* If this fails, we will retry later in NAPI poll, which is
> + * scheduled in the below virtnet_napi_enable
> + */
> + try_fill_recv(vi, rq, GFP_KERNEL);
hmm do we even need this, then?
> +
> if (running)
> virtnet_napi_enable(rq);
> -
> - if (schedule_refill)
> - schedule_delayed_work(&vi->refill, 0);
> }
>
> static void virtnet_rx_resume_all(struct virtnet_info *vi)
> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> struct virtio_net_rss_config_trailer old_rss_trailer;
> struct net_device *dev = vi->dev;
> struct scatterlist sg;
> + int i;
>
> if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> return 0;
> @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> }
> succ:
> vi->curr_queue_pairs = queue_pairs;
> - /* virtnet_open() will refill when device is going to up. */
> - spin_lock_bh(&vi->refill_lock);
> - if (dev->flags & IFF_UP && vi->refill_enabled)
> - schedule_delayed_work(&vi->refill, 0);
> - spin_unlock_bh(&vi->refill_lock);
> + if (dev->flags & IFF_UP) {
> + /* Let the NAPI poll refill the receive buffer for us. We can't
> + * safely call try_fill_recv() here because the NAPI might be
> + * enabled already.
> + */
I'd drop this comment, it does not clarify much.
> + local_bh_disable();
> + for (i = 0; i < vi->curr_queue_pairs; i++)
> + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> +
> + local_bh_enable();
> + }
>
> return 0;
> }
> --
> 2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-03 16:57 ` Michael S. Tsirkin
@ 2026-01-03 17:34 ` Michael S. Tsirkin
2026-01-04 4:33 ` Bui Quang Minh
1 sibling, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-03 17:34 UTC (permalink / raw)
To: Bui Quang Minh
Cc: netdev, Jason Wang, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On Sat, Jan 03, 2026 at 11:57:43AM -0500, Michael S. Tsirkin wrote:
> On Fri, Jan 02, 2026 at 10:20:21PM +0700, Bui Quang Minh wrote:
> > When we fail to refill the receive buffers, we schedule a delayed worker
> > to retry later. However, this worker creates some concurrency issues
> > such as races and deadlocks. To simplify the logic and avoid further
> > problems, we will instead retry refilling in the next NAPI poll.
> >
> > Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> > Reported-by: Paolo Abeni <pabeni@redhat.com>
> > Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> > Cc: stable@vger.kernel.org
> > Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > ---
> > drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> > 1 file changed, 30 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 1bb3aeca66c6..ac514c9383ae 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> > }
> >
> > static int virtnet_receive(struct receive_queue *rq, int budget,
> > - unsigned int *xdp_xmit)
> > + unsigned int *xdp_xmit, bool *retry_refill)
> > {
> > struct virtnet_info *vi = rq->vq->vdev->priv;
> > struct virtnet_rq_stats stats = {};
> > @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> > packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
> >
> > if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> > - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> > - spin_lock(&vi->refill_lock);
> > - if (vi->refill_enabled)
> > - schedule_delayed_work(&vi->refill, 0);
> > - spin_unlock(&vi->refill_lock);
> > - }
> > + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> > + *retry_refill = true;
> > }
> >
> > u64_stats_set(&stats.packets, packets);
> > @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > struct send_queue *sq;
> > unsigned int received;
> > unsigned int xdp_xmit = 0;
> > - bool napi_complete;
> > + bool napi_complete, retry_refill = false;
> >
> > virtnet_poll_cleantx(rq, budget);
> >
> > - received = virtnet_receive(rq, budget, &xdp_xmit);
> > + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> > rq->packets_in_napi += received;
> >
> > if (xdp_xmit & VIRTIO_XDP_REDIR)
> > xdp_do_flush();
> >
> > /* Out of packets? */
> > - if (received < budget) {
> > + if (received < budget && !retry_refill) {
> > napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> > /* Intentionally not taking dim_lock here. This may result in a
> > * spurious net_dim call. But if that happens virtnet_rx_dim_work
> > @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > virtnet_xdp_put_sq(vi, sq);
> > }
> >
> > - return received;
> > + return retry_refill ? budget : received;
> > }
> >
> > static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
> >
> > for (i = 0; i < vi->max_queue_pairs; i++) {
> > if (i < vi->curr_queue_pairs)
> > - /* Make sure we have some buffers: if oom use wq. */
> > - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> > - schedule_delayed_work(&vi->refill, 0);
> > + /* If this fails, we will retry later in
> > + * NAPI poll, which is scheduled in the below
> > + * virtnet_enable_queue_pair
>
> hmm do we even need this, then?
Oh this one is GFP_KERNEL. So it makes it more likely the ring will
be filled.
> > + */
> > + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
> >
> > err = virtnet_enable_queue_pair(vi, i);
> > if (err < 0)
> > @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> > bool refill)
> > {
> > bool running = netif_running(vi->dev);
> > - bool schedule_refill = false;
> >
> > - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> > - schedule_refill = true;
> > + if (refill)
> > + /* If this fails, we will retry later in NAPI poll, which is
> > + * scheduled in the below virtnet_napi_enable
> > + */
> > + try_fill_recv(vi, rq, GFP_KERNEL);
>
>
> hmm do we even need this, then?
>
> > +
> > if (running)
> > virtnet_napi_enable(rq);
> > -
> > - if (schedule_refill)
> > - schedule_delayed_work(&vi->refill, 0);
> > }
> >
> > static void virtnet_rx_resume_all(struct virtnet_info *vi)
> > @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > struct virtio_net_rss_config_trailer old_rss_trailer;
> > struct net_device *dev = vi->dev;
> > struct scatterlist sg;
> > + int i;
> >
> > if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> > return 0;
> > @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > }
> > succ:
> > vi->curr_queue_pairs = queue_pairs;
> > - /* virtnet_open() will refill when device is going to up. */
> > - spin_lock_bh(&vi->refill_lock);
> > - if (dev->flags & IFF_UP && vi->refill_enabled)
> > - schedule_delayed_work(&vi->refill, 0);
> > - spin_unlock_bh(&vi->refill_lock);
> > + if (dev->flags & IFF_UP) {
> > + /* Let the NAPI poll refill the receive buffer for us. We can't
> > + * safely call try_fill_recv() here because the NAPI might be
> > + * enabled already.
> > + */
>
> I'd drop this comment, it does not clarify much.
>
> > + local_bh_disable();
> > + for (i = 0; i < vi->curr_queue_pairs; i++)
> > + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> > +
> > + local_bh_enable();
> > + }
> >
> > return 0;
> > }
> > --
> > 2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-03 16:57 ` Michael S. Tsirkin
2026-01-03 17:34 ` Michael S. Tsirkin
@ 2026-01-04 4:33 ` Bui Quang Minh
2026-01-04 13:24 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Bui Quang Minh @ 2026-01-04 4:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Jason Wang, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On 1/3/26 23:57, Michael S. Tsirkin wrote:
> On Fri, Jan 02, 2026 at 10:20:21PM +0700, Bui Quang Minh wrote:
>> When we fail to refill the receive buffers, we schedule a delayed worker
>> to retry later. However, this worker creates some concurrency issues
>> such as races and deadlocks. To simplify the logic and avoid further
>> problems, we will instead retry refilling in the next NAPI poll.
>>
>> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
>> Cc: stable@vger.kernel.org
>> Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>> drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
>> 1 file changed, 30 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 1bb3aeca66c6..ac514c9383ae 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
>> }
>>
>> static int virtnet_receive(struct receive_queue *rq, int budget,
>> - unsigned int *xdp_xmit)
>> + unsigned int *xdp_xmit, bool *retry_refill)
>> {
>> struct virtnet_info *vi = rq->vq->vdev->priv;
>> struct virtnet_rq_stats stats = {};
>> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>> packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>>
>> if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
>> - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
>> - spin_lock(&vi->refill_lock);
>> - if (vi->refill_enabled)
>> - schedule_delayed_work(&vi->refill, 0);
>> - spin_unlock(&vi->refill_lock);
>> - }
>> + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
>> + *retry_refill = true;
>> }
>>
>> u64_stats_set(&stats.packets, packets);
>> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>> struct send_queue *sq;
>> unsigned int received;
>> unsigned int xdp_xmit = 0;
>> - bool napi_complete;
>> + bool napi_complete, retry_refill = false;
>>
>> virtnet_poll_cleantx(rq, budget);
>>
>> - received = virtnet_receive(rq, budget, &xdp_xmit);
>> + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
>> rq->packets_in_napi += received;
>>
>> if (xdp_xmit & VIRTIO_XDP_REDIR)
>> xdp_do_flush();
>>
>> /* Out of packets? */
>> - if (received < budget) {
>> + if (received < budget && !retry_refill) {
>> napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
>> /* Intentionally not taking dim_lock here. This may result in a
>> * spurious net_dim call. But if that happens virtnet_rx_dim_work
>> @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>> virtnet_xdp_put_sq(vi, sq);
>> }
>>
>> - return received;
>> + return retry_refill ? budget : received;
>> }
>>
>> static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
>> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>>
>> for (i = 0; i < vi->max_queue_pairs; i++) {
>> if (i < vi->curr_queue_pairs)
>> - /* Make sure we have some buffers: if oom use wq. */
>> - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
>> - schedule_delayed_work(&vi->refill, 0);
>> + /* If this fails, we will retry later in
>> + * NAPI poll, which is scheduled in the below
>> + * virtnet_enable_queue_pair
> hmm do we even need this, then?
>
>> + */
>> + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>>
>> err = virtnet_enable_queue_pair(vi, i);
>> if (err < 0)
>> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
>> bool refill)
>> {
>> bool running = netif_running(vi->dev);
>> - bool schedule_refill = false;
>>
>> - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
>> - schedule_refill = true;
>> + if (refill)
>> + /* If this fails, we will retry later in NAPI poll, which is
>> + * scheduled in the below virtnet_napi_enable
>> + */
>> + try_fill_recv(vi, rq, GFP_KERNEL);
>
> hmm do we even need this, then?
>
>> +
>> if (running)
>> virtnet_napi_enable(rq);
>> -
>> - if (schedule_refill)
>> - schedule_delayed_work(&vi->refill, 0);
>> }
>>
>> static void virtnet_rx_resume_all(struct virtnet_info *vi)
>> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> struct virtio_net_rss_config_trailer old_rss_trailer;
>> struct net_device *dev = vi->dev;
>> struct scatterlist sg;
>> + int i;
>>
>> if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>> return 0;
>> @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> }
>> succ:
>> vi->curr_queue_pairs = queue_pairs;
>> - /* virtnet_open() will refill when device is going to up. */
>> - spin_lock_bh(&vi->refill_lock);
>> - if (dev->flags & IFF_UP && vi->refill_enabled)
>> - schedule_delayed_work(&vi->refill, 0);
>> - spin_unlock_bh(&vi->refill_lock);
>> + if (dev->flags & IFF_UP) {
>> + /* Let the NAPI poll refill the receive buffer for us. We can't
>> + * safely call try_fill_recv() here because the NAPI might be
>> + * enabled already.
>> + */
> I'd drop this comment, it does not clarify much.
Here I want to note on why we can't use try_fill_recv like in other places.
>
>> + local_bh_disable();
>> + for (i = 0; i < vi->curr_queue_pairs; i++)
>> + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
>> +
>> + local_bh_enable();
>> + }
>>
>> return 0;
>> }
>> --
>> 2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-04 4:33 ` Bui Quang Minh
@ 2026-01-04 13:24 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-04 13:24 UTC (permalink / raw)
To: Bui Quang Minh
Cc: netdev, Jason Wang, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On Sun, Jan 04, 2026 at 11:33:20AM +0700, Bui Quang Minh wrote:
> On 1/3/26 23:57, Michael S. Tsirkin wrote:
> > On Fri, Jan 02, 2026 at 10:20:21PM +0700, Bui Quang Minh wrote:
> > > When we fail to refill the receive buffers, we schedule a delayed worker
> > > to retry later. However, this worker creates some concurrency issues
> > > such as races and deadlocks. To simplify the logic and avoid further
> > > problems, we will instead retry refilling in the next NAPI poll.
> > >
> > > Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> > > Reported-by: Paolo Abeni <pabeni@redhat.com>
> > > Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> > > Cc: stable@vger.kernel.org
> > > Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > > ---
> > > drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> > > 1 file changed, 30 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 1bb3aeca66c6..ac514c9383ae 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> > > }
> > > static int virtnet_receive(struct receive_queue *rq, int budget,
> > > - unsigned int *xdp_xmit)
> > > + unsigned int *xdp_xmit, bool *retry_refill)
> > > {
> > > struct virtnet_info *vi = rq->vq->vdev->priv;
> > > struct virtnet_rq_stats stats = {};
> > > @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> > > packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
> > > if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> > > - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> > > - spin_lock(&vi->refill_lock);
> > > - if (vi->refill_enabled)
> > > - schedule_delayed_work(&vi->refill, 0);
> > > - spin_unlock(&vi->refill_lock);
> > > - }
> > > + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> > > + *retry_refill = true;
> > > }
> > > u64_stats_set(&stats.packets, packets);
> > > @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > struct send_queue *sq;
> > > unsigned int received;
> > > unsigned int xdp_xmit = 0;
> > > - bool napi_complete;
> > > + bool napi_complete, retry_refill = false;
> > > virtnet_poll_cleantx(rq, budget);
> > > - received = virtnet_receive(rq, budget, &xdp_xmit);
> > > + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> > > rq->packets_in_napi += received;
> > > if (xdp_xmit & VIRTIO_XDP_REDIR)
> > > xdp_do_flush();
> > > /* Out of packets? */
> > > - if (received < budget) {
> > > + if (received < budget && !retry_refill) {
> > > napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> > > /* Intentionally not taking dim_lock here. This may result in a
> > > * spurious net_dim call. But if that happens virtnet_rx_dim_work
> > > @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > virtnet_xdp_put_sq(vi, sq);
> > > }
> > > - return received;
> > > + return retry_refill ? budget : received;
> > > }
> > > static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
> > > for (i = 0; i < vi->max_queue_pairs; i++) {
> > > if (i < vi->curr_queue_pairs)
> > > - /* Make sure we have some buffers: if oom use wq. */
> > > - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> > > - schedule_delayed_work(&vi->refill, 0);
> > > + /* If this fails, we will retry later in
> > > + * NAPI poll, which is scheduled in the below
> > > + * virtnet_enable_queue_pair
> > hmm do we even need this, then?
> >
> > > + */
> > > + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
> > > err = virtnet_enable_queue_pair(vi, i);
> > > if (err < 0)
> > > @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> > > bool refill)
> > > {
> > > bool running = netif_running(vi->dev);
> > > - bool schedule_refill = false;
> > > - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> > > - schedule_refill = true;
> > > + if (refill)
> > > + /* If this fails, we will retry later in NAPI poll, which is
> > > + * scheduled in the below virtnet_napi_enable
> > > + */
> > > + try_fill_recv(vi, rq, GFP_KERNEL);
> >
> > hmm do we even need this, then?
> >
> > > +
> > > if (running)
> > > virtnet_napi_enable(rq);
> > > -
> > > - if (schedule_refill)
> > > - schedule_delayed_work(&vi->refill, 0);
> > > }
> > > static void virtnet_rx_resume_all(struct virtnet_info *vi)
> > > @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > struct virtio_net_rss_config_trailer old_rss_trailer;
> > > struct net_device *dev = vi->dev;
> > > struct scatterlist sg;
> > > + int i;
> > > if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> > > return 0;
> > > @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > }
> > > succ:
> > > vi->curr_queue_pairs = queue_pairs;
> > > - /* virtnet_open() will refill when device is going to up. */
> > > - spin_lock_bh(&vi->refill_lock);
> > > - if (dev->flags & IFF_UP && vi->refill_enabled)
> > > - schedule_delayed_work(&vi->refill, 0);
> > > - spin_unlock_bh(&vi->refill_lock);
> > > + if (dev->flags & IFF_UP) {
> > > + /* Let the NAPI poll refill the receive buffer for us. We can't
> > > + * safely call try_fill_recv() here because the NAPI might be
> > > + * enabled already.
> > > + */
> > I'd drop this comment, it does not clarify much.
>
> Here I want to note on why we can't use try_fill_recv like in other places.
It does not really answer the question why, you could just disable napi
if you wanted.
> >
> > > + local_bh_disable();
> > > + for (i = 0; i < vi->curr_queue_pairs; i++)
> > > + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> > > +
> > > + local_bh_enable();
> > > + }
> > > return 0;
> > > }
> > > --
> > > 2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-02 15:20 ` [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker Bui Quang Minh
2026-01-03 0:16 ` Michael S. Tsirkin
2026-01-03 16:57 ` Michael S. Tsirkin
@ 2026-01-04 6:09 ` Jason Wang
2026-01-04 8:34 ` Bui Quang Minh
2026-01-04 13:25 ` Michael S. Tsirkin
2 siblings, 2 replies; 14+ messages in thread
From: Jason Wang @ 2026-01-04 6:09 UTC (permalink / raw)
To: Bui Quang Minh
Cc: netdev, Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev,
virtualization, linux-kernel, bpf, stable
On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>
> When we fail to refill the receive buffers, we schedule a delayed worker
> to retry later. However, this worker creates some concurrency issues
> such as races and deadlocks. To simplify the logic and avoid further
> problems, we will instead retry refilling in the next NAPI poll.
>
> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> Cc: stable@vger.kernel.org
> Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
> drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1bb3aeca66c6..ac514c9383ae 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> }
>
> static int virtnet_receive(struct receive_queue *rq, int budget,
> - unsigned int *xdp_xmit)
> + unsigned int *xdp_xmit, bool *retry_refill)
> {
> struct virtnet_info *vi = rq->vq->vdev->priv;
> struct virtnet_rq_stats stats = {};
> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>
> if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> - spin_lock(&vi->refill_lock);
> - if (vi->refill_enabled)
> - schedule_delayed_work(&vi->refill, 0);
> - spin_unlock(&vi->refill_lock);
> - }
> + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> + *retry_refill = true;
> }
>
> u64_stats_set(&stats.packets, packets);
> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> struct send_queue *sq;
> unsigned int received;
> unsigned int xdp_xmit = 0;
> - bool napi_complete;
> + bool napi_complete, retry_refill = false;
>
> virtnet_poll_cleantx(rq, budget);
>
> - received = virtnet_receive(rq, budget, &xdp_xmit);
> + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
I think we can simply let virtnet_receive() to return the budget when
reill fails.
> rq->packets_in_napi += received;
>
> if (xdp_xmit & VIRTIO_XDP_REDIR)
> xdp_do_flush();
>
> /* Out of packets? */
> - if (received < budget) {
> + if (received < budget && !retry_refill) {
> napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> /* Intentionally not taking dim_lock here. This may result in a
> * spurious net_dim call. But if that happens virtnet_rx_dim_work
> @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> virtnet_xdp_put_sq(vi, sq);
> }
>
> - return received;
> + return retry_refill ? budget : received;
> }
>
> static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> if (i < vi->curr_queue_pairs)
> - /* Make sure we have some buffers: if oom use wq. */
> - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> - schedule_delayed_work(&vi->refill, 0);
> + /* If this fails, we will retry later in
> + * NAPI poll, which is scheduled in the below
> + * virtnet_enable_queue_pair
> + */
> + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
Consider NAPI will be eventually scheduled, I wonder if it's still
worth to refill here.
>
> err = virtnet_enable_queue_pair(vi, i);
> if (err < 0)
> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> bool refill)
> {
> bool running = netif_running(vi->dev);
> - bool schedule_refill = false;
>
> - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> - schedule_refill = true;
> + if (refill)
> + /* If this fails, we will retry later in NAPI poll, which is
> + * scheduled in the below virtnet_napi_enable
> + */
> + try_fill_recv(vi, rq, GFP_KERNEL);
and here.
> +
> if (running)
> virtnet_napi_enable(rq);
> -
> - if (schedule_refill)
> - schedule_delayed_work(&vi->refill, 0);
> }
>
> static void virtnet_rx_resume_all(struct virtnet_info *vi)
> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> struct virtio_net_rss_config_trailer old_rss_trailer;
> struct net_device *dev = vi->dev;
> struct scatterlist sg;
> + int i;
>
> if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> return 0;
> @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> }
> succ:
> vi->curr_queue_pairs = queue_pairs;
> - /* virtnet_open() will refill when device is going to up. */
> - spin_lock_bh(&vi->refill_lock);
> - if (dev->flags & IFF_UP && vi->refill_enabled)
> - schedule_delayed_work(&vi->refill, 0);
> - spin_unlock_bh(&vi->refill_lock);
> + if (dev->flags & IFF_UP) {
> + /* Let the NAPI poll refill the receive buffer for us. We can't
> + * safely call try_fill_recv() here because the NAPI might be
> + * enabled already.
> + */
> + local_bh_disable();
> + for (i = 0; i < vi->curr_queue_pairs; i++)
> + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> +
> + local_bh_enable();
> + }
>
> return 0;
> }
> --
> 2.43.0
>
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-04 6:09 ` Jason Wang
@ 2026-01-04 8:34 ` Bui Quang Minh
2026-01-04 14:03 ` Michael S. Tsirkin
2026-01-04 13:25 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Bui Quang Minh @ 2026-01-04 8:34 UTC (permalink / raw)
To: Jason Wang
Cc: netdev, Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev,
virtualization, linux-kernel, bpf, stable
On 1/4/26 13:09, Jason Wang wrote:
> On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>> When we fail to refill the receive buffers, we schedule a delayed worker
>> to retry later. However, this worker creates some concurrency issues
>> such as races and deadlocks. To simplify the logic and avoid further
>> problems, we will instead retry refilling in the next NAPI poll.
>>
>> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
>> Cc: stable@vger.kernel.org
>> Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>> drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
>> 1 file changed, 30 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 1bb3aeca66c6..ac514c9383ae 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
>> }
>>
>> static int virtnet_receive(struct receive_queue *rq, int budget,
>> - unsigned int *xdp_xmit)
>> + unsigned int *xdp_xmit, bool *retry_refill)
>> {
>> struct virtnet_info *vi = rq->vq->vdev->priv;
>> struct virtnet_rq_stats stats = {};
>> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>> packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>>
>> if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
>> - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
>> - spin_lock(&vi->refill_lock);
>> - if (vi->refill_enabled)
>> - schedule_delayed_work(&vi->refill, 0);
>> - spin_unlock(&vi->refill_lock);
>> - }
>> + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
>> + *retry_refill = true;
>> }
>>
>> u64_stats_set(&stats.packets, packets);
>> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>> struct send_queue *sq;
>> unsigned int received;
>> unsigned int xdp_xmit = 0;
>> - bool napi_complete;
>> + bool napi_complete, retry_refill = false;
>>
>> virtnet_poll_cleantx(rq, budget);
>>
>> - received = virtnet_receive(rq, budget, &xdp_xmit);
>> + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> I think we can simply let virtnet_receive() to return the budget when
> reill fails.
That makes sense, I'll change it.
>
>> rq->packets_in_napi += received;
>>
>> if (xdp_xmit & VIRTIO_XDP_REDIR)
>> xdp_do_flush();
>>
>> /* Out of packets? */
>> - if (received < budget) {
>> + if (received < budget && !retry_refill) {
>> napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
>> /* Intentionally not taking dim_lock here. This may result in a
>> * spurious net_dim call. But if that happens virtnet_rx_dim_work
>> @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>> virtnet_xdp_put_sq(vi, sq);
>> }
>>
>> - return received;
>> + return retry_refill ? budget : received;
>> }
>>
>> static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
>> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>>
>> for (i = 0; i < vi->max_queue_pairs; i++) {
>> if (i < vi->curr_queue_pairs)
>> - /* Make sure we have some buffers: if oom use wq. */
>> - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
>> - schedule_delayed_work(&vi->refill, 0);
>> + /* If this fails, we will retry later in
>> + * NAPI poll, which is scheduled in the below
>> + * virtnet_enable_queue_pair
>> + */
>> + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
> Consider NAPI will be eventually scheduled, I wonder if it's still
> worth to refill here.
With GFP_KERNEL here, I think it's more likely to succeed than
GFP_ATOMIC in NAPI poll. Another small benefit is that the actual packet
can happen earlier. In case the receive buffer is empty and we don't
refill here, the #1 NAPI poll refill the buffer and the #2 NAPI poll can
receive packets. The #2 NAPI poll is scheduled in the interrupt handler
because the #1 NAPI poll will deschedule the NAPI and enable the device
interrupt. In case we successfully refill here, the #1 NAPI poll can
receive packets right away.
>
>> err = virtnet_enable_queue_pair(vi, i);
>> if (err < 0)
>> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
>> bool refill)
>> {
>> bool running = netif_running(vi->dev);
>> - bool schedule_refill = false;
>>
>> - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
>> - schedule_refill = true;
>> + if (refill)
>> + /* If this fails, we will retry later in NAPI poll, which is
>> + * scheduled in the below virtnet_napi_enable
>> + */
>> + try_fill_recv(vi, rq, GFP_KERNEL);
> and here.
>
>> +
>> if (running)
>> virtnet_napi_enable(rq);
>> -
>> - if (schedule_refill)
>> - schedule_delayed_work(&vi->refill, 0);
>> }
>>
>> static void virtnet_rx_resume_all(struct virtnet_info *vi)
>> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> struct virtio_net_rss_config_trailer old_rss_trailer;
>> struct net_device *dev = vi->dev;
>> struct scatterlist sg;
>> + int i;
>>
>> if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>> return 0;
>> @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> }
>> succ:
>> vi->curr_queue_pairs = queue_pairs;
>> - /* virtnet_open() will refill when device is going to up. */
>> - spin_lock_bh(&vi->refill_lock);
>> - if (dev->flags & IFF_UP && vi->refill_enabled)
>> - schedule_delayed_work(&vi->refill, 0);
>> - spin_unlock_bh(&vi->refill_lock);
>> + if (dev->flags & IFF_UP) {
>> + /* Let the NAPI poll refill the receive buffer for us. We can't
>> + * safely call try_fill_recv() here because the NAPI might be
>> + * enabled already.
>> + */
>> + local_bh_disable();
>> + for (i = 0; i < vi->curr_queue_pairs; i++)
>> + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
>> +
>> + local_bh_enable();
>> + }
>>
>> return 0;
>> }
>> --
>> 2.43.0
>>
> Thanks
>
Thanks,
Quang Minh.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-04 8:34 ` Bui Quang Minh
@ 2026-01-04 14:03 ` Michael S. Tsirkin
2026-01-04 14:54 ` Bui Quang Minh
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-04 14:03 UTC (permalink / raw)
To: Bui Quang Minh
Cc: Jason Wang, netdev, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On Sun, Jan 04, 2026 at 03:34:52PM +0700, Bui Quang Minh wrote:
> On 1/4/26 13:09, Jason Wang wrote:
> > On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> > > When we fail to refill the receive buffers, we schedule a delayed worker
> > > to retry later. However, this worker creates some concurrency issues
> > > such as races and deadlocks. To simplify the logic and avoid further
> > > problems, we will instead retry refilling in the next NAPI poll.
> > >
> > > Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> > > Reported-by: Paolo Abeni <pabeni@redhat.com>
> > > Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> > > Cc: stable@vger.kernel.org
> > > Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > > ---
> > > drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> > > 1 file changed, 30 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 1bb3aeca66c6..ac514c9383ae 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> > > }
> > >
> > > static int virtnet_receive(struct receive_queue *rq, int budget,
> > > - unsigned int *xdp_xmit)
> > > + unsigned int *xdp_xmit, bool *retry_refill)
> > > {
> > > struct virtnet_info *vi = rq->vq->vdev->priv;
> > > struct virtnet_rq_stats stats = {};
> > > @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> > > packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
> > >
> > > if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> > > - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> > > - spin_lock(&vi->refill_lock);
> > > - if (vi->refill_enabled)
> > > - schedule_delayed_work(&vi->refill, 0);
> > > - spin_unlock(&vi->refill_lock);
> > > - }
> > > + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> > > + *retry_refill = true;
> > > }
> > >
> > > u64_stats_set(&stats.packets, packets);
> > > @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > struct send_queue *sq;
> > > unsigned int received;
> > > unsigned int xdp_xmit = 0;
> > > - bool napi_complete;
> > > + bool napi_complete, retry_refill = false;
> > >
> > > virtnet_poll_cleantx(rq, budget);
> > >
> > > - received = virtnet_receive(rq, budget, &xdp_xmit);
> > > + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> > I think we can simply let virtnet_receive() to return the budget when
> > reill fails.
>
> That makes sense, I'll change it.
>
> >
> > > rq->packets_in_napi += received;
> > >
> > > if (xdp_xmit & VIRTIO_XDP_REDIR)
> > > xdp_do_flush();
> > >
> > > /* Out of packets? */
> > > - if (received < budget) {
> > > + if (received < budget && !retry_refill) {
> > > napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> > > /* Intentionally not taking dim_lock here. This may result in a
> > > * spurious net_dim call. But if that happens virtnet_rx_dim_work
> > > @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > virtnet_xdp_put_sq(vi, sq);
> > > }
> > >
> > > - return received;
> > > + return retry_refill ? budget : received;
> > > }
> > >
> > > static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
> > >
> > > for (i = 0; i < vi->max_queue_pairs; i++) {
> > > if (i < vi->curr_queue_pairs)
> > > - /* Make sure we have some buffers: if oom use wq. */
> > > - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> > > - schedule_delayed_work(&vi->refill, 0);
> > > + /* If this fails, we will retry later in
> > > + * NAPI poll, which is scheduled in the below
> > > + * virtnet_enable_queue_pair
> > > + */
> > > + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
> > Consider NAPI will be eventually scheduled, I wonder if it's still
> > worth to refill here.
>
> With GFP_KERNEL here, I think it's more likely to succeed than GFP_ATOMIC in
> NAPI poll. Another small benefit is that the actual packet can happen
> earlier. In case the receive buffer is empty and we don't refill here, the
> #1 NAPI poll refill the buffer and the #2 NAPI poll can receive packets. The
> #2 NAPI poll is scheduled in the interrupt handler because the #1 NAPI poll
> will deschedule the NAPI and enable the device interrupt. In case we
> successfully refill here, the #1 NAPI poll can receive packets right away.
>
Right. But I think this is a part that needs elucidating, not
error handling.
/* Pre-fill rq agressively, to make sure we are ready to get packets
* immediately.
* */
> >
> > > err = virtnet_enable_queue_pair(vi, i);
> > > if (err < 0)
> > > @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> > > bool refill)
> > > {
> > > bool running = netif_running(vi->dev);
> > > - bool schedule_refill = false;
> > >
> > > - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> > > - schedule_refill = true;
> > > + if (refill)
> > > + /* If this fails, we will retry later in NAPI poll, which is
> > > + * scheduled in the below virtnet_napi_enable
> > > + */
> > > + try_fill_recv(vi, rq, GFP_KERNEL);
> > and here.
> >
> > > +
> > > if (running)
> > > virtnet_napi_enable(rq);
here the part that isn't clear is why are we refilling if !running
and what handles failures in that case.
> > > -
> > > - if (schedule_refill)
> > > - schedule_delayed_work(&vi->refill, 0);
> > > }
> > >
> > > static void virtnet_rx_resume_all(struct virtnet_info *vi)
> > > @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > struct virtio_net_rss_config_trailer old_rss_trailer;
> > > struct net_device *dev = vi->dev;
> > > struct scatterlist sg;
> > > + int i;
> > >
> > > if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> > > return 0;
> > > @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > }
> > > succ:
> > > vi->curr_queue_pairs = queue_pairs;
> > > - /* virtnet_open() will refill when device is going to up. */
> > > - spin_lock_bh(&vi->refill_lock);
> > > - if (dev->flags & IFF_UP && vi->refill_enabled)
> > > - schedule_delayed_work(&vi->refill, 0);
> > > - spin_unlock_bh(&vi->refill_lock);
> > > + if (dev->flags & IFF_UP) {
> > > + /* Let the NAPI poll refill the receive buffer for us. We can't
> > > + * safely call try_fill_recv() here because the NAPI might be
> > > + * enabled already.
> > > + */
> > > + local_bh_disable();
> > > + for (i = 0; i < vi->curr_queue_pairs; i++)
> > > + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> > > +
> > > + local_bh_enable();
> > > + }
> > >
> > > return 0;
> > > }
> > > --
> > > 2.43.0
> > >
> > Thanks
> >
>
> Thanks,
> Quang Minh.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-04 14:03 ` Michael S. Tsirkin
@ 2026-01-04 14:54 ` Bui Quang Minh
2026-01-04 15:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Bui Quang Minh @ 2026-01-04 14:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, netdev, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On 1/4/26 21:03, Michael S. Tsirkin wrote:
> On Sun, Jan 04, 2026 at 03:34:52PM +0700, Bui Quang Minh wrote:
>> On 1/4/26 13:09, Jason Wang wrote:
>>> On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>>>> When we fail to refill the receive buffers, we schedule a delayed worker
>>>> to retry later. However, this worker creates some concurrency issues
>>>> such as races and deadlocks. To simplify the logic and avoid further
>>>> problems, we will instead retry refilling in the next NAPI poll.
>>>>
>>>> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
>>>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>>>> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
>>>> Cc: stable@vger.kernel.org
>>>> Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>>>> ---
>>>> drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
>>>> 1 file changed, 30 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 1bb3aeca66c6..ac514c9383ae 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
>>>> }
>>>>
>>>> static int virtnet_receive(struct receive_queue *rq, int budget,
>>>> - unsigned int *xdp_xmit)
>>>> + unsigned int *xdp_xmit, bool *retry_refill)
>>>> {
>>>> struct virtnet_info *vi = rq->vq->vdev->priv;
>>>> struct virtnet_rq_stats stats = {};
>>>> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>>>> packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>>>>
>>>> if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
>>>> - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
>>>> - spin_lock(&vi->refill_lock);
>>>> - if (vi->refill_enabled)
>>>> - schedule_delayed_work(&vi->refill, 0);
>>>> - spin_unlock(&vi->refill_lock);
>>>> - }
>>>> + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
>>>> + *retry_refill = true;
>>>> }
>>>>
>>>> u64_stats_set(&stats.packets, packets);
>>>> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>>>> struct send_queue *sq;
>>>> unsigned int received;
>>>> unsigned int xdp_xmit = 0;
>>>> - bool napi_complete;
>>>> + bool napi_complete, retry_refill = false;
>>>>
>>>> virtnet_poll_cleantx(rq, budget);
>>>>
>>>> - received = virtnet_receive(rq, budget, &xdp_xmit);
>>>> + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
>>> I think we can simply let virtnet_receive() to return the budget when
>>> reill fails.
>> That makes sense, I'll change it.
>>
>>>> rq->packets_in_napi += received;
>>>>
>>>> if (xdp_xmit & VIRTIO_XDP_REDIR)
>>>> xdp_do_flush();
>>>>
>>>> /* Out of packets? */
>>>> - if (received < budget) {
>>>> + if (received < budget && !retry_refill) {
>>>> napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
>>>> /* Intentionally not taking dim_lock here. This may result in a
>>>> * spurious net_dim call. But if that happens virtnet_rx_dim_work
>>>> @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>>>> virtnet_xdp_put_sq(vi, sq);
>>>> }
>>>>
>>>> - return received;
>>>> + return retry_refill ? budget : received;
>>>> }
>>>>
>>>> static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
>>>> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>>>>
>>>> for (i = 0; i < vi->max_queue_pairs; i++) {
>>>> if (i < vi->curr_queue_pairs)
>>>> - /* Make sure we have some buffers: if oom use wq. */
>>>> - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
>>>> - schedule_delayed_work(&vi->refill, 0);
>>>> + /* If this fails, we will retry later in
>>>> + * NAPI poll, which is scheduled in the below
>>>> + * virtnet_enable_queue_pair
>>>> + */
>>>> + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>>> Consider NAPI will be eventually scheduled, I wonder if it's still
>>> worth to refill here.
>> With GFP_KERNEL here, I think it's more likely to succeed than GFP_ATOMIC in
>> NAPI poll. Another small benefit is that the actual packet can happen
>> earlier. In case the receive buffer is empty and we don't refill here, the
>> #1 NAPI poll refill the buffer and the #2 NAPI poll can receive packets. The
>> #2 NAPI poll is scheduled in the interrupt handler because the #1 NAPI poll
>> will deschedule the NAPI and enable the device interrupt. In case we
>> successfully refill here, the #1 NAPI poll can receive packets right away.
>>
> Right. But I think this is a part that needs elucidating, not
> error handling.
>
> /* Pre-fill rq agressively, to make sure we are ready to get packets
> * immediately.
> * */
>
>>>> err = virtnet_enable_queue_pair(vi, i);
>>>> if (err < 0)
>>>> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
>>>> bool refill)
>>>> {
>>>> bool running = netif_running(vi->dev);
>>>> - bool schedule_refill = false;
>>>>
>>>> - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
>>>> - schedule_refill = true;
>>>> + if (refill)
>>>> + /* If this fails, we will retry later in NAPI poll, which is
>>>> + * scheduled in the below virtnet_napi_enable
>>>> + */
>>>> + try_fill_recv(vi, rq, GFP_KERNEL);
>>> and here.
>>>
>>>> +
>>>> if (running)
>>>> virtnet_napi_enable(rq);
> here the part that isn't clear is why are we refilling if !running
> and what handles failures in that case.
You are right, we should not refill when !running. I'll move the if
(refill) inside the if (running).
>
>>>> -
>>>> - if (schedule_refill)
>>>> - schedule_delayed_work(&vi->refill, 0);
>>>> }
>>>>
>>>> static void virtnet_rx_resume_all(struct virtnet_info *vi)
>>>> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>> struct virtio_net_rss_config_trailer old_rss_trailer;
>>>> struct net_device *dev = vi->dev;
>>>> struct scatterlist sg;
>>>> + int i;
>>>>
>>>> if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>>>> return 0;
>>>> @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>> }
>>>> succ:
>>>> vi->curr_queue_pairs = queue_pairs;
>>>> - /* virtnet_open() will refill when device is going to up. */
>>>> - spin_lock_bh(&vi->refill_lock);
>>>> - if (dev->flags & IFF_UP && vi->refill_enabled)
>>>> - schedule_delayed_work(&vi->refill, 0);
>>>> - spin_unlock_bh(&vi->refill_lock);
>>>> + if (dev->flags & IFF_UP) {
>>>> + /* Let the NAPI poll refill the receive buffer for us. We can't
>>>> + * safely call try_fill_recv() here because the NAPI might be
>>>> + * enabled already.
>>>> + */
>>>> + local_bh_disable();
>>>> + for (i = 0; i < vi->curr_queue_pairs; i++)
>>>> + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
>>>> +
>>>> + local_bh_enable();
>>>> + }
>>>>
>>>> return 0;
>>>> }
>>>> --
>>>> 2.43.0
>>>>
>>> Thanks
>>>
>> Thanks,
>> Quang Minh.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-04 14:54 ` Bui Quang Minh
@ 2026-01-04 15:12 ` Michael S. Tsirkin
2026-01-05 15:03 ` Bui Quang Minh
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-04 15:12 UTC (permalink / raw)
To: Bui Quang Minh
Cc: Jason Wang, netdev, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On Sun, Jan 04, 2026 at 09:54:30PM +0700, Bui Quang Minh wrote:
> On 1/4/26 21:03, Michael S. Tsirkin wrote:
> > On Sun, Jan 04, 2026 at 03:34:52PM +0700, Bui Quang Minh wrote:
> > > On 1/4/26 13:09, Jason Wang wrote:
> > > > On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> > > > > When we fail to refill the receive buffers, we schedule a delayed worker
> > > > > to retry later. However, this worker creates some concurrency issues
> > > > > such as races and deadlocks. To simplify the logic and avoid further
> > > > > problems, we will instead retry refilling in the next NAPI poll.
> > > > >
> > > > > Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> > > > > Reported-by: Paolo Abeni <pabeni@redhat.com>
> > > > > Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> > > > > Cc: stable@vger.kernel.org
> > > > > Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > > > > ---
> > > > > drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> > > > > 1 file changed, 30 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 1bb3aeca66c6..ac514c9383ae 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> > > > > }
> > > > >
> > > > > static int virtnet_receive(struct receive_queue *rq, int budget,
> > > > > - unsigned int *xdp_xmit)
> > > > > + unsigned int *xdp_xmit, bool *retry_refill)
> > > > > {
> > > > > struct virtnet_info *vi = rq->vq->vdev->priv;
> > > > > struct virtnet_rq_stats stats = {};
> > > > > @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> > > > > packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
> > > > >
> > > > > if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> > > > > - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> > > > > - spin_lock(&vi->refill_lock);
> > > > > - if (vi->refill_enabled)
> > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > - spin_unlock(&vi->refill_lock);
> > > > > - }
> > > > > + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> > > > > + *retry_refill = true;
> > > > > }
> > > > >
> > > > > u64_stats_set(&stats.packets, packets);
> > > > > @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > > > struct send_queue *sq;
> > > > > unsigned int received;
> > > > > unsigned int xdp_xmit = 0;
> > > > > - bool napi_complete;
> > > > > + bool napi_complete, retry_refill = false;
> > > > >
> > > > > virtnet_poll_cleantx(rq, budget);
> > > > >
> > > > > - received = virtnet_receive(rq, budget, &xdp_xmit);
> > > > > + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> > > > I think we can simply let virtnet_receive() to return the budget when
> > > > reill fails.
> > > That makes sense, I'll change it.
> > >
> > > > > rq->packets_in_napi += received;
> > > > >
> > > > > if (xdp_xmit & VIRTIO_XDP_REDIR)
> > > > > xdp_do_flush();
> > > > >
> > > > > /* Out of packets? */
> > > > > - if (received < budget) {
> > > > > + if (received < budget && !retry_refill) {
> > > > > napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> > > > > /* Intentionally not taking dim_lock here. This may result in a
> > > > > * spurious net_dim call. But if that happens virtnet_rx_dim_work
> > > > > @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > > > virtnet_xdp_put_sq(vi, sq);
> > > > > }
> > > > >
> > > > > - return received;
> > > > > + return retry_refill ? budget : received;
> > > > > }
> > > > >
> > > > > static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > > > @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
> > > > >
> > > > > for (i = 0; i < vi->max_queue_pairs; i++) {
> > > > > if (i < vi->curr_queue_pairs)
> > > > > - /* Make sure we have some buffers: if oom use wq. */
> > > > > - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > + /* If this fails, we will retry later in
> > > > > + * NAPI poll, which is scheduled in the below
> > > > > + * virtnet_enable_queue_pair
> > > > > + */
> > > > > + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
> > > > Consider NAPI will be eventually scheduled, I wonder if it's still
> > > > worth to refill here.
> > > With GFP_KERNEL here, I think it's more likely to succeed than GFP_ATOMIC in
> > > NAPI poll. Another small benefit is that the actual packet can happen
> > > earlier. In case the receive buffer is empty and we don't refill here, the
> > > #1 NAPI poll refill the buffer and the #2 NAPI poll can receive packets. The
> > > #2 NAPI poll is scheduled in the interrupt handler because the #1 NAPI poll
> > > will deschedule the NAPI and enable the device interrupt. In case we
> > > successfully refill here, the #1 NAPI poll can receive packets right away.
> > >
> > Right. But I think this is a part that needs elucidating, not
> > error handling.
> >
> > /* Pre-fill rq agressively, to make sure we are ready to get packets
> > * immediately.
> > * */
> >
> > > > > err = virtnet_enable_queue_pair(vi, i);
> > > > > if (err < 0)
> > > > > @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> > > > > bool refill)
> > > > > {
> > > > > bool running = netif_running(vi->dev);
> > > > > - bool schedule_refill = false;
> > > > >
> > > > > - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> > > > > - schedule_refill = true;
> > > > > + if (refill)
> > > > > + /* If this fails, we will retry later in NAPI poll, which is
> > > > > + * scheduled in the below virtnet_napi_enable
> > > > > + */
> > > > > + try_fill_recv(vi, rq, GFP_KERNEL);
> > > > and here.
> > > >
> > > > > +
> > > > > if (running)
> > > > > virtnet_napi_enable(rq);
> > here the part that isn't clear is why are we refilling if !running
> > and what handles failures in that case.
>
> You are right, we should not refill when !running. I'll move the if (refill)
> inside the if (running).
Sounds like a helper that does refill+virtnet_napi_enable
would be in order then? fill_recv_aggressively(vi, rq) ?
> >
> > > > > -
> > > > > - if (schedule_refill)
> > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > }
> > > > >
> > > > > static void virtnet_rx_resume_all(struct virtnet_info *vi)
> > > > > @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > > > struct virtio_net_rss_config_trailer old_rss_trailer;
> > > > > struct net_device *dev = vi->dev;
> > > > > struct scatterlist sg;
> > > > > + int i;
> > > > >
> > > > > if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> > > > > return 0;
> > > > > @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > > > }
> > > > > succ:
> > > > > vi->curr_queue_pairs = queue_pairs;
> > > > > - /* virtnet_open() will refill when device is going to up. */
> > > > > - spin_lock_bh(&vi->refill_lock);
> > > > > - if (dev->flags & IFF_UP && vi->refill_enabled)
> > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > - spin_unlock_bh(&vi->refill_lock);
> > > > > + if (dev->flags & IFF_UP) {
> > > > > + /* Let the NAPI poll refill the receive buffer for us. We can't
> > > > > + * safely call try_fill_recv() here because the NAPI might be
> > > > > + * enabled already.
> > > > > + */
> > > > > + local_bh_disable();
> > > > > + for (i = 0; i < vi->curr_queue_pairs; i++)
> > > > > + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> > > > > +
> > > > > + local_bh_enable();
> > > > > + }
> > > > >
> > > > > return 0;
> > > > > }
> > > > > --
> > > > > 2.43.0
> > > > >
> > > > Thanks
> > > >
> > > Thanks,
> > > Quang Minh.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-04 15:12 ` Michael S. Tsirkin
@ 2026-01-05 15:03 ` Bui Quang Minh
2026-01-05 20:35 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Bui Quang Minh @ 2026-01-05 15:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, netdev, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On 1/4/26 22:12, Michael S. Tsirkin wrote:
> On Sun, Jan 04, 2026 at 09:54:30PM +0700, Bui Quang Minh wrote:
>> On 1/4/26 21:03, Michael S. Tsirkin wrote:
>>> On Sun, Jan 04, 2026 at 03:34:52PM +0700, Bui Quang Minh wrote:
>>>> On 1/4/26 13:09, Jason Wang wrote:
>>>>> On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>>>>>> When we fail to refill the receive buffers, we schedule a delayed worker
>>>>>> to retry later. However, this worker creates some concurrency issues
>>>>>> such as races and deadlocks. To simplify the logic and avoid further
>>>>>> problems, we will instead retry refilling in the next NAPI poll.
>>>>>>
>>>>>> Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
>>>>>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>>>>>> Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>>>>>> ---
>>>>>> drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
>>>>>> 1 file changed, 30 insertions(+), 25 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>> index 1bb3aeca66c6..ac514c9383ae 100644
>>>>>> --- a/drivers/net/virtio_net.c
>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>> @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
>>>>>> }
>>>>>>
>>>>>> static int virtnet_receive(struct receive_queue *rq, int budget,
>>>>>> - unsigned int *xdp_xmit)
>>>>>> + unsigned int *xdp_xmit, bool *retry_refill)
>>>>>> {
>>>>>> struct virtnet_info *vi = rq->vq->vdev->priv;
>>>>>> struct virtnet_rq_stats stats = {};
>>>>>> @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>>>>>> packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
>>>>>>
>>>>>> if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
>>>>>> - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
>>>>>> - spin_lock(&vi->refill_lock);
>>>>>> - if (vi->refill_enabled)
>>>>>> - schedule_delayed_work(&vi->refill, 0);
>>>>>> - spin_unlock(&vi->refill_lock);
>>>>>> - }
>>>>>> + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
>>>>>> + *retry_refill = true;
>>>>>> }
>>>>>>
>>>>>> u64_stats_set(&stats.packets, packets);
>>>>>> @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>>>>>> struct send_queue *sq;
>>>>>> unsigned int received;
>>>>>> unsigned int xdp_xmit = 0;
>>>>>> - bool napi_complete;
>>>>>> + bool napi_complete, retry_refill = false;
>>>>>>
>>>>>> virtnet_poll_cleantx(rq, budget);
>>>>>>
>>>>>> - received = virtnet_receive(rq, budget, &xdp_xmit);
>>>>>> + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
>>>>> I think we can simply let virtnet_receive() to return the budget when
>>>>> reill fails.
>>>> That makes sense, I'll change it.
>>>>
>>>>>> rq->packets_in_napi += received;
>>>>>>
>>>>>> if (xdp_xmit & VIRTIO_XDP_REDIR)
>>>>>> xdp_do_flush();
>>>>>>
>>>>>> /* Out of packets? */
>>>>>> - if (received < budget) {
>>>>>> + if (received < budget && !retry_refill) {
>>>>>> napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
>>>>>> /* Intentionally not taking dim_lock here. This may result in a
>>>>>> * spurious net_dim call. But if that happens virtnet_rx_dim_work
>>>>>> @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>>>>>> virtnet_xdp_put_sq(vi, sq);
>>>>>> }
>>>>>>
>>>>>> - return received;
>>>>>> + return retry_refill ? budget : received;
>>>>>> }
>>>>>>
>>>>>> static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
>>>>>> @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
>>>>>>
>>>>>> for (i = 0; i < vi->max_queue_pairs; i++) {
>>>>>> if (i < vi->curr_queue_pairs)
>>>>>> - /* Make sure we have some buffers: if oom use wq. */
>>>>>> - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
>>>>>> - schedule_delayed_work(&vi->refill, 0);
>>>>>> + /* If this fails, we will retry later in
>>>>>> + * NAPI poll, which is scheduled in the below
>>>>>> + * virtnet_enable_queue_pair
>>>>>> + */
>>>>>> + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>>>>> Consider NAPI will be eventually scheduled, I wonder if it's still
>>>>> worth to refill here.
>>>> With GFP_KERNEL here, I think it's more likely to succeed than GFP_ATOMIC in
>>>> NAPI poll. Another small benefit is that the actual packet can happen
>>>> earlier. In case the receive buffer is empty and we don't refill here, the
>>>> #1 NAPI poll refill the buffer and the #2 NAPI poll can receive packets. The
>>>> #2 NAPI poll is scheduled in the interrupt handler because the #1 NAPI poll
>>>> will deschedule the NAPI and enable the device interrupt. In case we
>>>> successfully refill here, the #1 NAPI poll can receive packets right away.
>>>>
>>> Right. But I think this is a part that needs elucidating, not
>>> error handling.
>>>
>>> /* Pre-fill rq agressively, to make sure we are ready to get packets
>>> * immediately.
>>> * */
>>>
>>>>>> err = virtnet_enable_queue_pair(vi, i);
>>>>>> if (err < 0)
>>>>>> @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
>>>>>> bool refill)
>>>>>> {
>>>>>> bool running = netif_running(vi->dev);
>>>>>> - bool schedule_refill = false;
>>>>>>
>>>>>> - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
>>>>>> - schedule_refill = true;
>>>>>> + if (refill)
>>>>>> + /* If this fails, we will retry later in NAPI poll, which is
>>>>>> + * scheduled in the below virtnet_napi_enable
>>>>>> + */
>>>>>> + try_fill_recv(vi, rq, GFP_KERNEL);
>>>>> and here.
>>>>>
>>>>>> +
>>>>>> if (running)
>>>>>> virtnet_napi_enable(rq);
>>> here the part that isn't clear is why are we refilling if !running
>>> and what handles failures in that case.
>> You are right, we should not refill when !running. I'll move the if (refill)
>> inside the if (running).
> Sounds like a helper that does refill+virtnet_napi_enable
> would be in order then? fill_recv_aggressively(vi, rq) ?
I think the helper can make the code a little more complicated. In
virtnet_open(), the RX NAPI is enabled in virtnet_enable_queue_pair() so
we need to add a flag like enable_rx. Then change the virtnet_open() to
for (i = 0; i < vi->max_queue_pairs; i++) {
if (i < vi->curr_queue_pairs) {
fill_recv_aggressively(vi, rq);
err = virtnet_enable_queue_pair(..., enable_rx = false);
if (err < 0)
goto err_enable_qp;
} else {
err = virtnet_enable_queue_pair(..., enable_rx = true);
if (err < 0)
goto err_enable_qp;
}
}
>
>>>>>> -
>>>>>> - if (schedule_refill)
>>>>>> - schedule_delayed_work(&vi->refill, 0);
>>>>>> }
>>>>>>
>>>>>> static void virtnet_rx_resume_all(struct virtnet_info *vi)
>>>>>> @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>>>> struct virtio_net_rss_config_trailer old_rss_trailer;
>>>>>> struct net_device *dev = vi->dev;
>>>>>> struct scatterlist sg;
>>>>>> + int i;
>>>>>>
>>>>>> if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>>>>>> return 0;
>>>>>> @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>>>> }
>>>>>> succ:
>>>>>> vi->curr_queue_pairs = queue_pairs;
>>>>>> - /* virtnet_open() will refill when device is going to up. */
>>>>>> - spin_lock_bh(&vi->refill_lock);
>>>>>> - if (dev->flags & IFF_UP && vi->refill_enabled)
>>>>>> - schedule_delayed_work(&vi->refill, 0);
>>>>>> - spin_unlock_bh(&vi->refill_lock);
>>>>>> + if (dev->flags & IFF_UP) {
>>>>>> + /* Let the NAPI poll refill the receive buffer for us. We can't
>>>>>> + * safely call try_fill_recv() here because the NAPI might be
>>>>>> + * enabled already.
>>>>>> + */
>>>>>> + local_bh_disable();
>>>>>> + for (i = 0; i < vi->curr_queue_pairs; i++)
>>>>>> + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
>>>>>> +
>>>>>> + local_bh_enable();
>>>>>> + }
>>>>>>
>>>>>> return 0;
>>>>>> }
>>>>>> --
>>>>>> 2.43.0
>>>>>>
>>>>> Thanks
>>>>>
>>>> Thanks,
>>>> Quang Minh.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-05 15:03 ` Bui Quang Minh
@ 2026-01-05 20:35 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-05 20:35 UTC (permalink / raw)
To: Bui Quang Minh
Cc: Jason Wang, netdev, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, virtualization, linux-kernel,
bpf, stable
On Mon, Jan 05, 2026 at 10:03:22PM +0700, Bui Quang Minh wrote:
> On 1/4/26 22:12, Michael S. Tsirkin wrote:
> > On Sun, Jan 04, 2026 at 09:54:30PM +0700, Bui Quang Minh wrote:
> > > On 1/4/26 21:03, Michael S. Tsirkin wrote:
> > > > On Sun, Jan 04, 2026 at 03:34:52PM +0700, Bui Quang Minh wrote:
> > > > > On 1/4/26 13:09, Jason Wang wrote:
> > > > > > On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> > > > > > > When we fail to refill the receive buffers, we schedule a delayed worker
> > > > > > > to retry later. However, this worker creates some concurrency issues
> > > > > > > such as races and deadlocks. To simplify the logic and avoid further
> > > > > > > problems, we will instead retry refilling in the next NAPI poll.
> > > > > > >
> > > > > > > Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> > > > > > > Reported-by: Paolo Abeni <pabeni@redhat.com>
> > > > > > > Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > > > > > > ---
> > > > > > > drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> > > > > > > 1 file changed, 30 insertions(+), 25 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > index 1bb3aeca66c6..ac514c9383ae 100644
> > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> > > > > > > }
> > > > > > >
> > > > > > > static int virtnet_receive(struct receive_queue *rq, int budget,
> > > > > > > - unsigned int *xdp_xmit)
> > > > > > > + unsigned int *xdp_xmit, bool *retry_refill)
> > > > > > > {
> > > > > > > struct virtnet_info *vi = rq->vq->vdev->priv;
> > > > > > > struct virtnet_rq_stats stats = {};
> > > > > > > @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> > > > > > > packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
> > > > > > >
> > > > > > > if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> > > > > > > - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> > > > > > > - spin_lock(&vi->refill_lock);
> > > > > > > - if (vi->refill_enabled)
> > > > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > > > - spin_unlock(&vi->refill_lock);
> > > > > > > - }
> > > > > > > + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> > > > > > > + *retry_refill = true;
> > > > > > > }
> > > > > > >
> > > > > > > u64_stats_set(&stats.packets, packets);
> > > > > > > @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > > > > > struct send_queue *sq;
> > > > > > > unsigned int received;
> > > > > > > unsigned int xdp_xmit = 0;
> > > > > > > - bool napi_complete;
> > > > > > > + bool napi_complete, retry_refill = false;
> > > > > > >
> > > > > > > virtnet_poll_cleantx(rq, budget);
> > > > > > >
> > > > > > > - received = virtnet_receive(rq, budget, &xdp_xmit);
> > > > > > > + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
> > > > > > I think we can simply let virtnet_receive() to return the budget when
> > > > > > reill fails.
> > > > > That makes sense, I'll change it.
> > > > >
> > > > > > > rq->packets_in_napi += received;
> > > > > > >
> > > > > > > if (xdp_xmit & VIRTIO_XDP_REDIR)
> > > > > > > xdp_do_flush();
> > > > > > >
> > > > > > > /* Out of packets? */
> > > > > > > - if (received < budget) {
> > > > > > > + if (received < budget && !retry_refill) {
> > > > > > > napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> > > > > > > /* Intentionally not taking dim_lock here. This may result in a
> > > > > > > * spurious net_dim call. But if that happens virtnet_rx_dim_work
> > > > > > > @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > > > > > > virtnet_xdp_put_sq(vi, sq);
> > > > > > > }
> > > > > > >
> > > > > > > - return received;
> > > > > > > + return retry_refill ? budget : received;
> > > > > > > }
> > > > > > >
> > > > > > > static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > > > > > @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
> > > > > > >
> > > > > > > for (i = 0; i < vi->max_queue_pairs; i++) {
> > > > > > > if (i < vi->curr_queue_pairs)
> > > > > > > - /* Make sure we have some buffers: if oom use wq. */
> > > > > > > - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> > > > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > > > + /* If this fails, we will retry later in
> > > > > > > + * NAPI poll, which is scheduled in the below
> > > > > > > + * virtnet_enable_queue_pair
> > > > > > > + */
> > > > > > > + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
> > > > > > Consider NAPI will be eventually scheduled, I wonder if it's still
> > > > > > worth to refill here.
> > > > > With GFP_KERNEL here, I think it's more likely to succeed than GFP_ATOMIC in
> > > > > NAPI poll. Another small benefit is that the actual packet can happen
> > > > > earlier. In case the receive buffer is empty and we don't refill here, the
> > > > > #1 NAPI poll refill the buffer and the #2 NAPI poll can receive packets. The
> > > > > #2 NAPI poll is scheduled in the interrupt handler because the #1 NAPI poll
> > > > > will deschedule the NAPI and enable the device interrupt. In case we
> > > > > successfully refill here, the #1 NAPI poll can receive packets right away.
> > > > >
> > > > Right. But I think this is a part that needs elucidating, not
> > > > error handling.
> > > >
> > > > /* Pre-fill rq agressively, to make sure we are ready to get packets
> > > > * immediately.
> > > > * */
> > > >
> > > > > > > err = virtnet_enable_queue_pair(vi, i);
> > > > > > > if (err < 0)
> > > > > > > @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> > > > > > > bool refill)
> > > > > > > {
> > > > > > > bool running = netif_running(vi->dev);
> > > > > > > - bool schedule_refill = false;
> > > > > > >
> > > > > > > - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> > > > > > > - schedule_refill = true;
> > > > > > > + if (refill)
> > > > > > > + /* If this fails, we will retry later in NAPI poll, which is
> > > > > > > + * scheduled in the below virtnet_napi_enable
> > > > > > > + */
> > > > > > > + try_fill_recv(vi, rq, GFP_KERNEL);
> > > > > > and here.
> > > > > >
> > > > > > > +
> > > > > > > if (running)
> > > > > > > virtnet_napi_enable(rq);
> > > > here the part that isn't clear is why are we refilling if !running
> > > > and what handles failures in that case.
> > > You are right, we should not refill when !running. I'll move the if (refill)
> > > inside the if (running).
> > Sounds like a helper that does refill+virtnet_napi_enable
> > would be in order then? fill_recv_aggressively(vi, rq) ?
>
> I think the helper can make the code a little more complicated. In
> virtnet_open(), the RX NAPI is enabled in virtnet_enable_queue_pair() so we
> need to add a flag like enable_rx. Then change the virtnet_open() to
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> if (i < vi->curr_queue_pairs) {
> fill_recv_aggressively(vi, rq);
> err = virtnet_enable_queue_pair(..., enable_rx = false);
> if (err < 0)
> goto err_enable_qp;
> } else {
> err = virtnet_enable_queue_pair(..., enable_rx = true);
> if (err < 0)
> goto err_enable_qp;
> }
>
> }
ok then, feel free to ignore this suggestion.
> >
> > > > > > > -
> > > > > > > - if (schedule_refill)
> > > > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > > > }
> > > > > > >
> > > > > > > static void virtnet_rx_resume_all(struct virtnet_info *vi)
> > > > > > > @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > > > > > struct virtio_net_rss_config_trailer old_rss_trailer;
> > > > > > > struct net_device *dev = vi->dev;
> > > > > > > struct scatterlist sg;
> > > > > > > + int i;
> > > > > > >
> > > > > > > if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> > > > > > > return 0;
> > > > > > > @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > > > > > > }
> > > > > > > succ:
> > > > > > > vi->curr_queue_pairs = queue_pairs;
> > > > > > > - /* virtnet_open() will refill when device is going to up. */
> > > > > > > - spin_lock_bh(&vi->refill_lock);
> > > > > > > - if (dev->flags & IFF_UP && vi->refill_enabled)
> > > > > > > - schedule_delayed_work(&vi->refill, 0);
> > > > > > > - spin_unlock_bh(&vi->refill_lock);
> > > > > > > + if (dev->flags & IFF_UP) {
> > > > > > > + /* Let the NAPI poll refill the receive buffer for us. We can't
> > > > > > > + * safely call try_fill_recv() here because the NAPI might be
> > > > > > > + * enabled already.
> > > > > > > + */
> > > > > > > + local_bh_disable();
> > > > > > > + for (i = 0; i < vi->curr_queue_pairs; i++)
> > > > > > > + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> > > > > > > +
> > > > > > > + local_bh_enable();
> > > > > > > + }
> > > > > > >
> > > > > > > return 0;
> > > > > > > }
> > > > > > > --
> > > > > > > 2.43.0
> > > > > > >
> > > > > > Thanks
> > > > > >
> > > > > Thanks,
> > > > > Quang Minh.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net v2 1/3] virtio-net: don't schedule delayed refill worker
2026-01-04 6:09 ` Jason Wang
2026-01-04 8:34 ` Bui Quang Minh
@ 2026-01-04 13:25 ` Michael S. Tsirkin
1 sibling, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2026-01-04 13:25 UTC (permalink / raw)
To: Jason Wang
Cc: Bui Quang Minh, netdev, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev,
virtualization, linux-kernel, bpf, stable
On Sun, Jan 04, 2026 at 02:09:06PM +0800, Jason Wang wrote:
> On Fri, Jan 2, 2026 at 11:20 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> >
> > When we fail to refill the receive buffers, we schedule a delayed worker
> > to retry later. However, this worker creates some concurrency issues
> > such as races and deadlocks. To simplify the logic and avoid further
> > problems, we will instead retry refilling in the next NAPI poll.
> >
> > Fixes: 4bc12818b363 ("virtio-net: disable delayed refill when pausing rx")
> > Reported-by: Paolo Abeni <pabeni@redhat.com>
> > Closes: https://netdev-ctrl.bots.linux.dev/logs/vmksft/drv-hw-dbg/results/400961/3-xdp-py/stderr
> > Cc: stable@vger.kernel.org
> > Suggested-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > ---
> > drivers/net/virtio_net.c | 55 ++++++++++++++++++++++------------------
> > 1 file changed, 30 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 1bb3aeca66c6..ac514c9383ae 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -3035,7 +3035,7 @@ static int virtnet_receive_packets(struct virtnet_info *vi,
> > }
> >
> > static int virtnet_receive(struct receive_queue *rq, int budget,
> > - unsigned int *xdp_xmit)
> > + unsigned int *xdp_xmit, bool *retry_refill)
> > {
> > struct virtnet_info *vi = rq->vq->vdev->priv;
> > struct virtnet_rq_stats stats = {};
> > @@ -3047,12 +3047,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> > packets = virtnet_receive_packets(vi, rq, budget, xdp_xmit, &stats);
> >
> > if (rq->vq->num_free > min((unsigned int)budget, virtqueue_get_vring_size(rq->vq)) / 2) {
> > - if (!try_fill_recv(vi, rq, GFP_ATOMIC)) {
> > - spin_lock(&vi->refill_lock);
> > - if (vi->refill_enabled)
> > - schedule_delayed_work(&vi->refill, 0);
> > - spin_unlock(&vi->refill_lock);
> > - }
> > + if (!try_fill_recv(vi, rq, GFP_ATOMIC))
> > + *retry_refill = true;
> > }
> >
> > u64_stats_set(&stats.packets, packets);
> > @@ -3129,18 +3125,18 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > struct send_queue *sq;
> > unsigned int received;
> > unsigned int xdp_xmit = 0;
> > - bool napi_complete;
> > + bool napi_complete, retry_refill = false;
> >
> > virtnet_poll_cleantx(rq, budget);
> >
> > - received = virtnet_receive(rq, budget, &xdp_xmit);
> > + received = virtnet_receive(rq, budget, &xdp_xmit, &retry_refill);
>
> I think we can simply let virtnet_receive() to return the budget when
> reill fails.
Great idea.
> > rq->packets_in_napi += received;
> >
> > if (xdp_xmit & VIRTIO_XDP_REDIR)
> > xdp_do_flush();
> >
> > /* Out of packets? */
> > - if (received < budget) {
> > + if (received < budget && !retry_refill) {
> > napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> > /* Intentionally not taking dim_lock here. This may result in a
> > * spurious net_dim call. But if that happens virtnet_rx_dim_work
> > @@ -3160,7 +3156,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > virtnet_xdp_put_sq(vi, sq);
> > }
> >
> > - return received;
> > + return retry_refill ? budget : received;
> > }
> >
> > static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > @@ -3230,9 +3226,11 @@ static int virtnet_open(struct net_device *dev)
> >
> > for (i = 0; i < vi->max_queue_pairs; i++) {
> > if (i < vi->curr_queue_pairs)
> > - /* Make sure we have some buffers: if oom use wq. */
> > - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> > - schedule_delayed_work(&vi->refill, 0);
> > + /* If this fails, we will retry later in
> > + * NAPI poll, which is scheduled in the below
> > + * virtnet_enable_queue_pair
> > + */
> > + try_fill_recv(vi, &vi->rq[i], GFP_KERNEL);
>
> Consider NAPI will be eventually scheduled, I wonder if it's still
> worth to refill here.
>
> >
> > err = virtnet_enable_queue_pair(vi, i);
> > if (err < 0)
> > @@ -3473,15 +3471,15 @@ static void __virtnet_rx_resume(struct virtnet_info *vi,
> > bool refill)
> > {
> > bool running = netif_running(vi->dev);
> > - bool schedule_refill = false;
> >
> > - if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
> > - schedule_refill = true;
> > + if (refill)
> > + /* If this fails, we will retry later in NAPI poll, which is
> > + * scheduled in the below virtnet_napi_enable
> > + */
> > + try_fill_recv(vi, rq, GFP_KERNEL);
>
> and here.
>
> > +
> > if (running)
> > virtnet_napi_enable(rq);
> > -
> > - if (schedule_refill)
> > - schedule_delayed_work(&vi->refill, 0);
> > }
> >
> > static void virtnet_rx_resume_all(struct virtnet_info *vi)
> > @@ -3777,6 +3775,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > struct virtio_net_rss_config_trailer old_rss_trailer;
> > struct net_device *dev = vi->dev;
> > struct scatterlist sg;
> > + int i;
> >
> > if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
> > return 0;
> > @@ -3829,11 +3828,17 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> > }
> > succ:
> > vi->curr_queue_pairs = queue_pairs;
> > - /* virtnet_open() will refill when device is going to up. */
> > - spin_lock_bh(&vi->refill_lock);
> > - if (dev->flags & IFF_UP && vi->refill_enabled)
> > - schedule_delayed_work(&vi->refill, 0);
> > - spin_unlock_bh(&vi->refill_lock);
> > + if (dev->flags & IFF_UP) {
> > + /* Let the NAPI poll refill the receive buffer for us. We can't
> > + * safely call try_fill_recv() here because the NAPI might be
> > + * enabled already.
> > + */
> > + local_bh_disable();
> > + for (i = 0; i < vi->curr_queue_pairs; i++)
> > + virtqueue_napi_schedule(&vi->rq[i].napi, vi->rq[i].vq);
> > +
> > + local_bh_enable();
> > + }
> >
> > return 0;
> > }
> > --
> > 2.43.0
> >
>
> Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread