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

[Laurent Vivier <lvivier@redhat.com>]

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
>>>
>