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

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

On 2025/05/22 10:50, 'Jason Wang' via devel wrote:
> On Wed, May 21, 2025 at 11:51 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> 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.
> 
> Well, if the ctrl vq index doesn't change we don't need a dynamic
> virtio_add_queue() we can do them all in realize just like other
> multiqueue devices.

The number of virtqueues also affects the behavior visible to the guest 
so we shouldn't add them all in realize anyway. In particular, struct 
virtio_pci_common_cfg contains fields related virtqueus, and most (if 
not all) of them are affected by the number of virtqueues.

Regards,
Akihiko Odaki