Re: [PATCH] virtio-net: Add queues for RSS during migration

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2025/05/21 9:51, Jason Wang wrote:
> On Fri, May 16, 2025 at 11:29 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/05/16 10:44, Jason Wang wrote:
>>> On Wed, May 14, 2025 at 2:58 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/05/14 14:05, 'Jason Wang' via devel wrote:
>>>>> On Sat, May 10, 2025 at 3:24 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> virtio_net_pre_load_queues() inspects vdev->guest_features to tell if
>>>>>> VIRTIO_NET_F_RSS or VIRTIO_NET_F_MQ is enabled to infer the required
>>>>>> number of queues. This works for VIRTIO_NET_F_MQ but it doesn't for
>>>>>> VIRTIO_NET_F_RSS because only the lowest 32 bits of vdev->guest_features
>>>>>> is set at the point and VIRTIO_NET_F_RSS uses bit 60 while
>>>>>> VIRTIO_NET_F_MQ uses bit 22.
>>>>>>
>>>>>> Instead of inferring the required number of queues from
>>>>>> vdev->guest_features, use the number loaded from the vm state.
>>>>>>
>>>>>> Fixes: 8c49756825da ("virtio-net: Add only one queue pair when realizing")
>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>> ---
>>>>>>     include/hw/virtio/virtio.h |  2 +-
>>>>>>     hw/net/virtio-net.c        | 11 ++++-------
>>>>>>     hw/virtio/virtio.c         | 14 +++++++-------
>>>>>>     3 files changed, 12 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>>>> index 638691028050..af52580c1e63 100644
>>>>>> --- a/include/hw/virtio/virtio.h
>>>>>> +++ b/include/hw/virtio/virtio.h
>>>>>> @@ -211,7 +211,7 @@ struct VirtioDeviceClass {
>>>>>>         int (*start_ioeventfd)(VirtIODevice *vdev);
>>>>>>         void (*stop_ioeventfd)(VirtIODevice *vdev);
>>>>>>         /* Called before loading queues. Useful to add queues before loading. */
>>>>>> -    int (*pre_load_queues)(VirtIODevice *vdev);
>>>>>> +    int (*pre_load_queues)(VirtIODevice *vdev, uint32_t n);
>>>>>
>>>>> This turns out to be tricky as it has a lot of assumptions as
>>>>> described in the changelog (e.g only lower 32bit of guest_features is
>>>>> correct etc when calling this function).
>>>>
>>>> The problem is that I missed the following comment in
>>>> hw/virtio/virtio.c:
>>>>        /*
>>>>         * Temporarily set guest_features low bits - needed by
>>>>         * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>>         * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ.
>>>>         *
>>>>         * Note: devices should always test host features in future - don't
>>>> create
>>>>         * new dependencies like this.
>>>>         */
>>>>        vdev->guest_features = features;
>>>>
>>>> This problem is specific to guest_features so avoiding it should give us
>>>> a reliable solution.
>>>
>>> I meant not all the states were fully loaded for pre_load_queues(),
>>> this seems another trick besides the above one. We should seek a way
>>> to do it in post_load() or at least document the assumptions.
>>
>> The name of the function already clarifies the state is not fully
>> loaded. An implementation of the function can make no assumption on the
>> state except that you can add queues here, which is already documented.
> 
> Where? I can only see this:
> 
>      /* Called before loading queues. Useful to add queues before loading. */

I meant it is documented that it can add queues. There is nothing else 
to document as an implementation of the function can make no assumption 
else.

> 
>>
>>>
>>>>
>>>>>
>>>>> Looking at the commit that introduces this which is 9379ea9db3c that says:
>>>>>
>>>>> """
>>>>> Otherwise the loaded queues will not have handlers and elements
>>>>> in them will not be processed.
>>>>> """
>>>>>
>>>>> I fail to remember what it means or what happens if we don't do 9379ea9db3c.
>>>>
>>>> The packets and control commands in the queues except the first queue
>>>> will not be processed because they do not have handle_output set.
>>>
>>> I don't understand here, the VM is not resumed in this case. Or what
>>> issue did you see here?
>>
>> Below is the order of relevant events that happen when loading and
>> resuming a VM for migration:
>>
>> 1) vdc->realize() gets called. virtio-net adds one RX queue, one TX
>> queue, and one control queue.
>> 2) vdc->pre_load_queues() gets called. virtio-net adds more queues if
>> the "n" parameter requires that.
>> 3) The state of queues are loaded.
>> 4) The VM gets resumed.
>>
>> If we skip 2), 3) will load states of queues that are not added yet,
>> which breaks these queues and packets and leave control packets on them
>> unprocessed.
> 
> Ok, let's document this and
> 
> 1) explain why only virtio-net requires the pre_load_queues (due to
> the fact that the index of ctrl vq could be changed according to
> #queue_paris)

We would need this logic even if the index of ctrl vq didn't change. We 
need it because virtio-net have varying number of queues, which needs to 
be added before loading.

> 2) the commit also fixes the issue that changing the TAP queue pairs twice

Commit 9379ea9db3c makes it change the TAP queue pairs once more. This 
patch reverts it, but that doesn't matter because the operation is 
idempotent.

More concretely, before commit 9379ea9db3c, we had two points that 
updates the TAP queue pairs when loading a VM for migration:
1) virtio_net_set_features()
2) virtio_net_post_load_device()

Commit 9379ea9db3c added a third point: virtio_net_pre_load_queues().
This patch removes the third point by avoid calling 
virtio_net_set_queue_pairs(), which is a nice side effect.

Regards,
Akihiko Odaki