Re: [PATCH v4] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella <sgarzare@redhat.com>]

On Sun, Sep 05, 2021 at 11:08:34AM -0700, Jiang Wang . wrote:
>On Mon, Aug 9, 2021 at 3:58 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> On Thu, Aug 05, 2021 at 12:07:02PM -0700, Jiang Wang . wrote:
>> >On Wed, Aug 4, 2021 at 1:13 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> On Tue, Aug 03, 2021 at 11:41:32PM +0000, Jiang Wang wrote:

[...]

>> >> >+
>> >> >+    if (nvqs == MAX_VQS_WITH_DGRAM) {
>> >> >+        vvc->dgram_recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>> >> >+                                              vhost_vsock_common_handle_output);
>> >> >+        vvc->dgram_trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>> >> >+
>> >> >vhost_vsock_common_handle_output);
>> >> >+    }
>> >> >+
>> >>
>> >> I'm still concerned about compatibility with guests that don't
>> >> support
>> >> dgram, as I mentioned in the previous email.
>> >>
>> >> I need to do some testing, but my guess is it won't work if the host
>> >> (QEMU and vhost-vsock) supports it and the guest doesn't.
>> >>
>> >> I still think that we should allocate an array of queues and then decide
>> >> at runtime which one to use for events (third or fifth) after the guest
>> >> has acked the features.
>> >>
>> >Agree. I will check where the guest ack the feature. If you have any
>>
>> I'm not sure we should delete them, I think we can allocate 5 queues and
>> then use queue 3 or 5 for events in vhost_vsock_common_start(), when the
>> guest already acked the features.
>>
>
>I think I just solved most compatibility issues during migration. The
>previous error I saw was due to a bug in vhost-vsock kernel module.

Please post the fix upstream.

>After fixing that, I did not change anything for qemu ( i.e, still the same
>version 4, btw I will fix fd issue in v5) and did a few migration tests.
>Most of them are good.
>
>There are two test cases that migration failed with "Features 0x130000002
>unsupported"error, which is due to
>SEQPACKET qemu patch (my dgram patch
>is based on seqpacket patch). Not sure if we need to
>fix it or not.  I think the compatibility is good as of now. Let me

I'm a bit lost. Do you mean because one of the QEMU doesn't support 
SEQPACKET?


>know if you have other concerns or more test cases to test.
>Otherwise, I will submit V5 soon.
>
>Test results:
>Left three columns are the source set-up,  right are destination set up.
>Host and Guest refers to the host and guest kernel respectively. These
>tests are not complete, and I make the src and dest kernel mostly the
>same version. But I did test one case where source kernel has dgram
>support while dest kernel does not and it is good. Btw, if the src kernel
>and dest kernel have a big difference( like 5.14 vs 4.19), then QEMU
>will show some msr errors which I guess is kind of expected.
>
>Host        QEMU        Guest            --> Host        QEMU            result
>dgram       no-dgram    no-dgram        dgram       no-dgram        Good
>dgram       no-dgram    dgram           dgram       no-dgram        Good
>dgram       dgram       no-dgram        dgram       dgram           Good
>dgram       dgram       no-dgram        dgram       no-dgram        Good
>dgram       dgram       dgram           dgram       no-dgram
>load feature error *1
>
>no-dgram    no-dgram    dgram           no-dgram    no-dgram        Good
>no-dgram    dgram       dgram           no-dgram    dgram             Good
>no-dgram    dgram       no-dgram        no-dgram    dgram           Good
>no-dgram    dgram       no-dgram        no-dgram    no-dgram        Good
>no-dgram    dgram       dgram           no-dgram    no-dgram
>load feature error *1
>
>dgram       dgram       no-dgram        no-dgram    no-dgram        Good
>
>*1 Qemu shows following error messages:
>qemu-system-x86_64: Features 0x130000002 unsupported. Allowed
>features: 0x179000000
>qemu-system-x86_64: Failed to load virtio-vhost_vsock:virtio
>qemu-system-x86_64: error while loading state for instance 0x0 of
>device '0000:00:05.0/virtio-vhost_vsock'
>qemu-system-x86_64: load of migration failed: Operation not permitted
>
>This is due to SEQPACKET feature bit.

Can you test with both (source and destination) that support SEQPACKET 
(or not)?

I mean, it is better if the only variable is the dgram support.

Anyway the tests seem ok to me :-)

>
>
>Step back and rethink about whether the event vq number should be 3 or or 5,
>now I think it does not matter. The tx and rx queues (whether 2 or 4 queues)
>belong to vhost, but event queue belongs to QEMU. The qemu code
>allocates an array  for vhost_dev.vqs only for tx and rx queues. So
>event queue is never in that array. That means we don't need to
>worry about even queue number is 3 or 5. And my tests confirmed that.
>I think for the virtio spec, we need to put event queue somewhere and
>it looks like having a relative position to tx rx queues. But for vhost kernel
>implementation, the event queue is a special case and not related to
>tx or rx queues.

Yep, the important thing is that QEMU uses the right queue depending on 
whether DGRAM support has been negotiated or not.

Thanks,
Stefano