[PATCH 0/2] virtio: Fixes for TX ring sizing and resize error reporting

[PATCH 0/2] virtio: Fixes for TX ring sizing and resize error reporting

[Laurent Vivier]

This patch series contains two fixes for the virtio subsystem.

The first patch fixes an error reporting bug in virtio_ring's
virtqueue_resize() function. Previously, errors from internal resize
helpers could be masked if the subsequent re-enabling of the virtqueue
succeeded. This patch restores the correct error propagation, ensuring that
callers of virtqueue_resize() are properly informed of underlying resize
failures.

The second patch addresses a reliability issue in virtio_net where the TX
ring size could be configured too small, potentially leading to
persistently stopped queues and degraded performance. It enforces a
minimum TX ring size to ensure there's always enough space for at least one
maximally-fragmented packet plus an additional slot.

Laurent Vivier (2):
  virtio_ring: Fix error reporting in virtqueue_resize
  virtio_net: Enforce minimum TX ring size for reliability

 drivers/net/virtio_net.c     | 6 ++++++
 drivers/virtio/virtio_ring.c | 8 ++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.49.0

[PATCH 1/2] virtio_ring: Fix error reporting in virtqueue_resize

[Laurent Vivier]

The virtqueue_resize() function was not correctly propagating error codes
from its internal resize helper functions, specifically
virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
returned an error, but the subsequent call to virtqueue_enable_after_reset()
succeeded, the original error from the resize operation would be masked.
Consequently, virtqueue_resize() could incorrectly report success to its
caller despite an underlying resize failure.

This change restores the original code behavior:

       if (vdev->config->enable_vq_after_reset(_vq))
               return -EBUSY;

       return err;

Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
Cc: xuanzhuo@linux.alibaba.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/virtio/virtio_ring.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b784aab66867..4397392bfef0 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 		     void (*recycle_done)(struct virtqueue *vq))
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
-	int err;
+	int err, err_reset;
 
 	if (num > vq->vq.num_max)
 		return -E2BIG;
@@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 	else
 		err = virtqueue_resize_split(_vq, num);
 
-	return virtqueue_enable_after_reset(_vq);
+	err_reset = virtqueue_enable_after_reset(_vq);
+	if (err_reset)
+		return err_reset;
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(virtqueue_resize);
 
-- 
2.49.0

Re: [PATCH 1/2] virtio_ring: Fix error reporting in virtqueue_resize

[Jason Wang]

On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> The virtqueue_resize() function was not correctly propagating error codes
> from its internal resize helper functions, specifically
> virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
> returned an error, but the subsequent call to virtqueue_enable_after_reset()
> succeeded, the original error from the resize operation would be masked.
> Consequently, virtqueue_resize() could incorrectly report success to its
> caller despite an underlying resize failure.
>
> This change restores the original code behavior:
>
>        if (vdev->config->enable_vq_after_reset(_vq))
>                return -EBUSY;
>
>        return err;
>
> Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
> Cc: xuanzhuo@linux.alibaba.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b784aab66867..4397392bfef0 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>                      void (*recycle_done)(struct virtqueue *vq))
>  {
>         struct vring_virtqueue *vq = to_vvq(_vq);
> -       int err;
> +       int err, err_reset;
>
>         if (num > vq->vq.num_max)
>                 return -E2BIG;
> @@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>         else
>                 err = virtqueue_resize_split(_vq, num);
>
> -       return virtqueue_enable_after_reset(_vq);
> +       err_reset = virtqueue_enable_after_reset(_vq);

I wonder if we should call virtqueue_enable_after_reset() when
virtqueue_resize_xxx() fail.

Thanks

> +       if (err_reset)
> +               return err_reset;
> +
> +       return err;
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_resize);
>
> --
> 2.49.0
>

Re: [PATCH 1/2] virtio_ring: Fix error reporting in virtqueue_resize

[Laurent Vivier]

On 21/05/2025 03:00, Jason Wang wrote:
> On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> The virtqueue_resize() function was not correctly propagating error codes
>> from its internal resize helper functions, specifically
>> virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
>> returned an error, but the subsequent call to virtqueue_enable_after_reset()
>> succeeded, the original error from the resize operation would be masked.
>> Consequently, virtqueue_resize() could incorrectly report success to its
>> caller despite an underlying resize failure.
>>
>> This change restores the original code behavior:
>>
>>         if (vdev->config->enable_vq_after_reset(_vq))
>>                 return -EBUSY;
>>
>>         return err;
>>
>> Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
>> Cc: xuanzhuo@linux.alibaba.com
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   drivers/virtio/virtio_ring.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index b784aab66867..4397392bfef0 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>                       void (*recycle_done)(struct virtqueue *vq))
>>   {
>>          struct vring_virtqueue *vq = to_vvq(_vq);
>> -       int err;
>> +       int err, err_reset;
>>
>>          if (num > vq->vq.num_max)
>>                  return -E2BIG;
>> @@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>          else
>>                  err = virtqueue_resize_split(_vq, num);
>>
>> -       return virtqueue_enable_after_reset(_vq);
>> +       err_reset = virtqueue_enable_after_reset(_vq);
> 
> I wonder if we should call virtqueue_enable_after_reset() when
> virtqueue_resize_xxx() fail.

Original code modified by ad48d53b5b3f did the reset. And the commit removes it without 
explanation.

And as we did a virtqueue_disable_and_recycle(), I think we need the 
virtqueue_enable_after_reset() to restart the queue.

In virtnet_tx_resize(), we have virtnet_tx_resume() unconditionnaly, even in case of error 
of virtqueue_resize(). virtnet_tx_resize() is called by virtnet_set_ringparam(), that is 
the function called by 'ethtool -G' and I think a failure of ethtool should not break the 
virtqueue.

Thanks,
Laurent


> 
> Thanks
> 
>> +       if (err_reset)
>> +               return err_reset;
>> +
>> +       return err;
>>   }
>>   EXPORT_SYMBOL_GPL(virtqueue_resize);
>>
>> --
>> 2.49.0
>>
> 

Re: [PATCH 1/2] virtio_ring: Fix error reporting in virtqueue_resize

[Xuan Zhuo]

We should provide feedback to the caller indicating the queue's current status
whether it is still valid and whether its size has been successfully
modified. Here I selected the first. The caller can know the second by
virtqueue_get_vring_size().

Thanks.


On Tue, 20 May 2025 13:05:25 +0200, Laurent Vivier <lvivier@redhat.com> wrote:
> The virtqueue_resize() function was not correctly propagating error codes
> from its internal resize helper functions, specifically
> virtqueue_resize_packet() and virtqueue_resize_split(). If these helpers
> returned an error, but the subsequent call to virtqueue_enable_after_reset()
> succeeded, the original error from the resize operation would be masked.
> Consequently, virtqueue_resize() could incorrectly report success to its
> caller despite an underlying resize failure.
>
> This change restores the original code behavior:
>
>        if (vdev->config->enable_vq_after_reset(_vq))
>                return -EBUSY;
>
>        return err;
>
> Fix: commit ad48d53b5b3f ("virtio_ring: separate the logic of reset/enable from virtqueue_resize")
> Cc: xuanzhuo@linux.alibaba.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b784aab66867..4397392bfef0 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2797,7 +2797,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>  		     void (*recycle_done)(struct virtqueue *vq))
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> -	int err;
> +	int err, err_reset;
>
>  	if (num > vq->vq.num_max)
>  		return -E2BIG;
> @@ -2819,7 +2819,11 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>  	else
>  		err = virtqueue_resize_split(_vq, num);
>
> -	return virtqueue_enable_after_reset(_vq);
> +	err_reset = virtqueue_enable_after_reset(_vq);
> +	if (err_reset)
> +		return err_reset;
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_resize);
>
> --
> 2.49.0
>

[PATCH 2/2] virtio_net: Enforce minimum TX ring size for reliability

[Laurent Vivier]

The `tx_may_stop()` logic stops TX queues if free descriptors
(`sq->vq->num_free`) fall below the threshold of (2 + `MAX_SKB_FRAGS`).
If the total ring size (`ring_num`) is not strictly greater than this
value, queues can become persistently stopped or stop after minimal
use, severely degrading performance.

A single sk_buff transmission typically requires descriptors for:
- The virtio_net_hdr (1 descriptor)
- The sk_buff's linear data (head) (1 descriptor)
- Paged fragments (up to MAX_SKB_FRAGS descriptors)

This patch enforces that the TX ring size ('ring_num') must be strictly
greater than (2 + MAX_SKB_FRAGS). This ensures that the ring is
always large enough to hold at least one maximally-fragmented packet
plus at least one additional slot.

Reported-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/net/virtio_net.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e53ba600605a..866961f368a2 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3481,6 +3481,12 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
 {
 	int qindex, err;
 
+	if (ring_num <= 2+MAX_SKB_FRAGS) {
+		netdev_err(vi->dev, "tx size (%d) cannot be smaller than %d\n",
+			   ring_num, 2+MAX_SKB_FRAGS);
+		return -EINVAL;
+	}
+
 	qindex = sq - vi->sq;
 
 	virtnet_tx_pause(vi, sq);
-- 
2.49.0

Re: [PATCH 2/2] virtio_net: Enforce minimum TX ring size for reliability

[Jason Wang]

On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> The `tx_may_stop()` logic stops TX queues if free descriptors
> (`sq->vq->num_free`) fall below the threshold of (2 + `MAX_SKB_FRAGS`).
> If the total ring size (`ring_num`) is not strictly greater than this
> value, queues can become persistently stopped or stop after minimal
> use, severely degrading performance.
>
> A single sk_buff transmission typically requires descriptors for:
> - The virtio_net_hdr (1 descriptor)
> - The sk_buff's linear data (head) (1 descriptor)
> - Paged fragments (up to MAX_SKB_FRAGS descriptors)
>
> This patch enforces that the TX ring size ('ring_num') must be strictly
> greater than (2 + MAX_SKB_FRAGS). This ensures that the ring is
> always large enough to hold at least one maximally-fragmented packet
> plus at least one additional slot.
>
> Reported-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/net/virtio_net.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e53ba600605a..866961f368a2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3481,6 +3481,12 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
>  {
>         int qindex, err;
>
> +       if (ring_num <= 2+MAX_SKB_FRAGS) {

Nit: space is probably needed around "+"

> +               netdev_err(vi->dev, "tx size (%d) cannot be smaller than %d\n",
> +                          ring_num, 2+MAX_SKB_FRAGS);

And here.

> +               return -EINVAL;
> +       }
> +
>         qindex = sq - vi->sq;
>
>         virtnet_tx_pause(vi, sq);
> --
> 2.49.0
>

Other than this.

Acked-by: Jason Wang <jasowang@redhat.com>

(Maybe we can proceed on don't stall if we had at least 1 left if
indirect descriptors are supported).

Thanks

Re: [PATCH 2/2] virtio_net: Enforce minimum TX ring size for reliability

[Laurent Vivier]

On 21/05/2025 03:01, Jason Wang wrote:
> On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> The `tx_may_stop()` logic stops TX queues if free descriptors
>> (`sq->vq->num_free`) fall below the threshold of (2 + `MAX_SKB_FRAGS`).
>> If the total ring size (`ring_num`) is not strictly greater than this
>> value, queues can become persistently stopped or stop after minimal
>> use, severely degrading performance.
>>
>> A single sk_buff transmission typically requires descriptors for:
>> - The virtio_net_hdr (1 descriptor)
>> - The sk_buff's linear data (head) (1 descriptor)
>> - Paged fragments (up to MAX_SKB_FRAGS descriptors)
>>
>> This patch enforces that the TX ring size ('ring_num') must be strictly
>> greater than (2 + MAX_SKB_FRAGS). This ensures that the ring is
>> always large enough to hold at least one maximally-fragmented packet
>> plus at least one additional slot.
>>
>> Reported-by: Lei Yang <leiyang@redhat.com>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   drivers/net/virtio_net.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index e53ba600605a..866961f368a2 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -3481,6 +3481,12 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
>>   {
>>          int qindex, err;
>>
>> +       if (ring_num <= 2+MAX_SKB_FRAGS) {
> 
> Nit: space is probably needed around "+"

I agree, but I kept the original syntax used everywhere in the file. It eases the search 
of the value in the file.

> 
>> +               netdev_err(vi->dev, "tx size (%d) cannot be smaller than %d\n",
>> +                          ring_num, 2+MAX_SKB_FRAGS);
> 
> And here.
> 
>> +               return -EINVAL;
>> +       }
>> +
>>          qindex = sq - vi->sq;
>>
>>          virtnet_tx_pause(vi, sq);
>> --
>> 2.49.0
>>
> 
> Other than this.
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> (Maybe we can proceed on don't stall if we had at least 1 left if
> indirect descriptors are supported).

But in this case, how to know when to stall the queue?

Thank,
Laurent
> 
> Thanks
> 

Re: [PATCH 2/2] virtio_net: Enforce minimum TX ring size for reliability

[Michael S. Tsirkin]

On Wed, May 21, 2025 at 09:45:47AM +0200, Laurent Vivier wrote:
> On 21/05/2025 03:01, Jason Wang wrote:
> > On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
> > > 
> > > The `tx_may_stop()` logic stops TX queues if free descriptors
> > > (`sq->vq->num_free`) fall below the threshold of (2 + `MAX_SKB_FRAGS`).
> > > If the total ring size (`ring_num`) is not strictly greater than this
> > > value, queues can become persistently stopped or stop after minimal
> > > use, severely degrading performance.
> > > 
> > > A single sk_buff transmission typically requires descriptors for:
> > > - The virtio_net_hdr (1 descriptor)
> > > - The sk_buff's linear data (head) (1 descriptor)
> > > - Paged fragments (up to MAX_SKB_FRAGS descriptors)
> > > 
> > > This patch enforces that the TX ring size ('ring_num') must be strictly
> > > greater than (2 + MAX_SKB_FRAGS). This ensures that the ring is
> > > always large enough to hold at least one maximally-fragmented packet
> > > plus at least one additional slot.
> > > 
> > > Reported-by: Lei Yang <leiyang@redhat.com>
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > >   drivers/net/virtio_net.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index e53ba600605a..866961f368a2 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -3481,6 +3481,12 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
> > >   {
> > >          int qindex, err;
> > > 
> > > +       if (ring_num <= 2+MAX_SKB_FRAGS) {
> > 
> > Nit: space is probably needed around "+"
> 
> I agree, but I kept the original syntax used everywhere in the file. It
> eases the search of the value in the file.


it's a mixed bag:

drivers/net/virtio_net.c:       struct scatterlist sg[MAX_SKB_FRAGS + 2];
drivers/net/virtio_net.c:       struct scatterlist sg[MAX_SKB_FRAGS + 2];
drivers/net/virtio_net.c:       if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
drivers/net/virtio_net.c:       if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:                       if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:       if (*num_buf > MAX_SKB_FRAGS + 1)
drivers/net/virtio_net.c:       if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
drivers/net/virtio_net.c:               if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:       if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:               vi->big_packets_num_skbfrags = guest_gso ? MAX_SKB_FRAGS : DIV_ROUND_UP(mtu, PAGE_SIZE);


we should fix it all. I think MAX_SKB_FRAGS + 2 is also cleaner than the
weird 2 + syntax.



> > 
> > > +               netdev_err(vi->dev, "tx size (%d) cannot be smaller than %d\n",
> > > +                          ring_num, 2+MAX_SKB_FRAGS);
> > 
> > And here.
> > 
> > > +               return -EINVAL;
> > > +       }
> > > +
> > >          qindex = sq - vi->sq;
> > > 
> > >          virtnet_tx_pause(vi, sq);
> > > --
> > > 2.49.0
> > > 
> > 
> > Other than this.
> > 
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > (Maybe we can proceed on don't stall if we had at least 1 left if
> > indirect descriptors are supported).
> 
> But in this case, how to know when to stall the queue?
> 
> Thank,
> Laurent
> > 
> > Thanks
> > 

Re: [PATCH 2/2] virtio_net: Enforce minimum TX ring size for reliability

[Laurent Vivier]

On 21/05/2025 10:39, Michael S. Tsirkin wrote:
> On Wed, May 21, 2025 at 09:45:47AM +0200, Laurent Vivier wrote:
>> On 21/05/2025 03:01, Jason Wang wrote:
>>> On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
>>>>
>>>> The `tx_may_stop()` logic stops TX queues if free descriptors
>>>> (`sq->vq->num_free`) fall below the threshold of (2 + `MAX_SKB_FRAGS`).
>>>> If the total ring size (`ring_num`) is not strictly greater than this
>>>> value, queues can become persistently stopped or stop after minimal
>>>> use, severely degrading performance.
>>>>
>>>> A single sk_buff transmission typically requires descriptors for:
>>>> - The virtio_net_hdr (1 descriptor)
>>>> - The sk_buff's linear data (head) (1 descriptor)
>>>> - Paged fragments (up to MAX_SKB_FRAGS descriptors)
>>>>
>>>> This patch enforces that the TX ring size ('ring_num') must be strictly
>>>> greater than (2 + MAX_SKB_FRAGS). This ensures that the ring is
>>>> always large enough to hold at least one maximally-fragmented packet
>>>> plus at least one additional slot.
>>>>
>>>> Reported-by: Lei Yang <leiyang@redhat.com>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>>    drivers/net/virtio_net.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index e53ba600605a..866961f368a2 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -3481,6 +3481,12 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
>>>>    {
>>>>           int qindex, err;
>>>>
>>>> +       if (ring_num <= 2+MAX_SKB_FRAGS) {
>>>
>>> Nit: space is probably needed around "+"
>>
>> I agree, but I kept the original syntax used everywhere in the file. It
>> eases the search of the value in the file.
> 
> 
> it's a mixed bag:
> 
> drivers/net/virtio_net.c:       struct scatterlist sg[MAX_SKB_FRAGS + 2];
> drivers/net/virtio_net.c:       struct scatterlist sg[MAX_SKB_FRAGS + 2];
> drivers/net/virtio_net.c:       if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
> drivers/net/virtio_net.c:       if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> drivers/net/virtio_net.c:                       if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> drivers/net/virtio_net.c:       if (*num_buf > MAX_SKB_FRAGS + 1)
> drivers/net/virtio_net.c:       if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> drivers/net/virtio_net.c:               if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) {
> drivers/net/virtio_net.c:       if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) {
> drivers/net/virtio_net.c:               vi->big_packets_num_skbfrags = guest_gso ? MAX_SKB_FRAGS : DIV_ROUND_UP(mtu, PAGE_SIZE);
> 
> 
> we should fix it all. I think MAX_SKB_FRAGS + 2 is also cleaner than the
> weird 2 + syntax.

OK, I'm going to add a patch that will cleanup all this stuff.

Thanks,
Laurent

> 
> 
> 
>>>
>>>> +               netdev_err(vi->dev, "tx size (%d) cannot be smaller than %d\n",
>>>> +                          ring_num, 2+MAX_SKB_FRAGS);
>>>
>>> And here.
>>>
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>>           qindex = sq - vi->sq;
>>>>
>>>>           virtnet_tx_pause(vi, sq);
>>>> --
>>>> 2.49.0
>>>>
>>>
>>> Other than this.
>>>
>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>>
>>> (Maybe we can proceed on don't stall if we had at least 1 left if
>>> indirect descriptors are supported).
>>
>> But in this case, how to know when to stall the queue?
>>
>> Thank,
>> Laurent
>>>
>>> Thanks
>>>
> 

Re: [PATCH 2/2] virtio_net: Enforce minimum TX ring size for reliability

[Jason Wang]

On Wed, May 21, 2025 at 3:45 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> On 21/05/2025 03:01, Jason Wang wrote:
> > On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@redhat.com> wrote:
> >>
> >> The `tx_may_stop()` logic stops TX queues if free descriptors
> >> (`sq->vq->num_free`) fall below the threshold of (2 + `MAX_SKB_FRAGS`).
> >> If the total ring size (`ring_num`) is not strictly greater than this
> >> value, queues can become persistently stopped or stop after minimal
> >> use, severely degrading performance.
> >>
> >> A single sk_buff transmission typically requires descriptors for:
> >> - The virtio_net_hdr (1 descriptor)
> >> - The sk_buff's linear data (head) (1 descriptor)
> >> - Paged fragments (up to MAX_SKB_FRAGS descriptors)
> >>
> >> This patch enforces that the TX ring size ('ring_num') must be strictly
> >> greater than (2 + MAX_SKB_FRAGS). This ensures that the ring is
> >> always large enough to hold at least one maximally-fragmented packet
> >> plus at least one additional slot.
> >>
> >> Reported-by: Lei Yang <leiyang@redhat.com>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>   drivers/net/virtio_net.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index e53ba600605a..866961f368a2 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -3481,6 +3481,12 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
> >>   {
> >>          int qindex, err;
> >>
> >> +       if (ring_num <= 2+MAX_SKB_FRAGS) {
> >
> > Nit: space is probably needed around "+"
>
> I agree, but I kept the original syntax used everywhere in the file. It eases the search
> of the value in the file.
>
> >
> >> +               netdev_err(vi->dev, "tx size (%d) cannot be smaller than %d\n",
> >> +                          ring_num, 2+MAX_SKB_FRAGS);
> >
> > And here.
> >
> >> +               return -EINVAL;
> >> +       }
> >> +
> >>          qindex = sq - vi->sq;
> >>
> >>          virtnet_tx_pause(vi, sq);
> >> --
> >> 2.49.0
> >>
> >
> > Other than this.
> >
> > Acked-by: Jason Wang <jasowang@redhat.com>
> >
> > (Maybe we can proceed on don't stall if we had at least 1 left if
> > indirect descriptors are supported).
>
> But in this case, how to know when to stall the queue?

I meant something like:

        if (sq->vq->num_free < virito_has_feature(INDIRECT) ? 1 :
2+MAX_SKB_FRAGS) {
        }

This might be useful for the case where MAX_SKB_FRAGS is greater than
17 as well.

(But it's an independent topic anyhow)

Thanks

>
> Thank,
> Laurent
> >
> > Thanks
> >
>