Re: [External] Re: [RFC v7] virtio/vsock: add two more queues for datagram types

[Stefano Garzarella]

On Fri, Sep 24, 2021 at 03:27:30PM -0700, Jiang Wang . wrote:
> On Thu, Sep 23, 2021 at 2:18 AM Stefano Garzarella <sgarzare@redhat.com> wrote:

[...]

> > >
> > >To explain it, I think the event queue number does not matter for the
> > >vhost and qemu. The vhost-vsock kernel module does not allocate any
> > >data structure for the event queue.  Qemu also only allocates an array of
> > >size 2 or 4 for the tx, rx queues. The event queue is handled 
> > >separately.
> > >
> > >The event queue number only shows up in the spec, but in real code, I don't
> > >see anywhere that the event queue number is used explicitly or implicitly.
> > >The Event queue looks independent of tx, rx queues.
> >
> > Yep, it is used in the linux driver. Look at
> > virtio_transport_event_work(), it uses VSOCK_VQ_EVENT (2).
> Agree, it is used in virtio driver and QEMU as you mentioned later.
>
> > The main test to do is to migrate a guest with active connections that
> By active connections, do you mean active vsock stream connections?

Yep, e.g using ncat also without exchanging data. After the migration 
the socket should be reset by the driver if the event queue is working.

> The guest should be the server or the client? 

Doesn't matter.

> I have two simple vsock client,
> server tests which only send messages for each direction once. Or I can also
> use something like iperf3.
>
> > doesn't support F_DGRAM on an host that support it and check if, after
> > the migration, the connections are reset and the CID updated.
>
> Not sure about why CID should be updated? Right now, on the destination
> host, I used the same CID as the one on the source host. You mean choose
> another guest CID on the destination host? I will try that.

Yep, because another thing the driver does when it handles the event in 
the event queue is to update the guest CID.
Usually you should use different CIDs if you migrate on the same host.

> > I think is not working now.

[...]

> >
> I see. The example code helps a lot.
>
> > Then use `vvc->event_vq_id` in :
> > - vhost_vsock_common_send_transport_reset()
> > - vhost_vsock_common_post_load() (instead of 2 wired in the code)
> I see now vhost_vsock_common_post_load() has a problem. One way
> to fix it is as you mentioned. Another way is to check the acked feature
> bit here too and change the event queue number to 2 or 4 accordingly.
>
> In your example code:
> vvc->vqs[i] = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>                                         vhost_vsock_common_handle_output);
> We still need to do some assignment as following:
> vvc->recv_vq = vvc->vqs[0];
> vvc->trans_vq = vvc->vqs[1];
> ...(skipped other similar assignments)

Why we need this?
QEMU handles only the event queue.
recv_vq, trans_vq pointers are never used (only to deallocate).

> I think both ways will work.  Your example adds ordering for those recv and
> trans vqs and makes it similar to virtio and vhost code. I will go that route.

Make sense.

> > Maybe in vhost_vsock_common_send_transport_reset() we can skip the
> > message enqueueing if the device is not started
> > (vvc->vhost_dev.started).
> OK. Btw, I can make this a separate change because it looks like a
> standalone bugfix? I.e, not related to dgram?

Yep, sure.

Stefano