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

[Stefano Garzarella]

On Wed, Jun 30, 2021 at 03:44:17PM -0700, Jiang Wang . wrote:
> On Thu, Jun 24, 2021 at 7:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > On Wed, Jun 23, 2021 at 11:50:33PM -0700, Jiang Wang . wrote:
> > >Hi Stefano,
> > >
> > >I checked virtio_net_set_multiqueue(), which will help with following
> > >changes in my patch:
> > >
> > >#ifdef CONFIG_VHOST_VSOCK_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);
> > >#endif
> > >
> > >But I think there is still an issue with the following lines, right?
> >
> > Yep, I think so.
> >
> > >
> > >#ifdef CONFIG_VHOST_VSOCK_DGRAM
> > >struct vhost_virtqueue vhost_vqs[4];
> > >#else
> > >struct vhost_virtqueue vhost_vqs[2];
> > >#endif
> > >
> > >I think the problem with feature bits is that they are set and get after
> > >vhost_vsock_common_realize() and after vhost_dev_init() in 
> > drivers/vhost/vsock.c
> > >But those virtqueues need to be set up correctly beforehand.
> >
> > I think we can follow net and scsi vhost devices, so we can set a
> > VHOST_VSOCK_VQ_MAX(5), allocates all the queues in any case and then use
> > only the queues acked by the guest.
> Thanks for the advice. I checked both net and scsi and scsi is more helpful.

Yeah :-)

> > >
> > >I tried to test with the host kernel allocating 4 vqs, but qemu only
> > >allocated 2 vqs, and
> > >guest kernel will not be able to send even the vsock stream packets. I
> > >think the host
> > >kernel and the qemu have to agree on the number of vhost_vqs. Do you agree?
> > >Did I miss something?
> >
> > Mmm, I need to check, but for example vhost-net calls vhost_dev_init()
> > with VHOST_NET_VQ_MAX, but then the guest can decide to use only one
> > couple of TX and RX queues.
> >
> > I'm not sure about qemu point of view, but I expected that QEMU can set
> > less queues then queues allocated by the kernel. `vhost_dev.nvqs` should
> > be set with the amount of queue that QEMU can handle.
> I checked that vhost_dev.nvqs is still the maximum number of queues (4 queues).
> But I found a way to workaround it. More details in the following text.
>
> > >
> > >Another idea to make the setting in runtime instead of compiling time
> > >is to use
> > >qemu cmd-line options, then qemu can allocate 2 or 4 queues depending
> > >on
> > >the cmd line. This will solve the issue when the host kernel is an old
> > >one( no dgram
> > >support) and the qemu is a new one.
> >
> > I don't think this is a good idea, at most we can add an ioctl that qemu
> > can use to query the kernel about allocated queues, but I still need to
> > understand better if we really we need this.
>
> Hmm. Both net and scsi use the qemu cmd line option to configure
> number of queues. Qemu cmdline is a runtime setting and flexible.
> I think qemu cmdline is better than ioctl. I also make the qemu cmd
> line option default to only allocate two queues to be compatible with
> old versions.

Can we avoid both and allocate the maximum number of queue that QEMU can 
handle?

I'm not sure that adding a parameter to QEMU is a good idea. If possible 
it should be automatic.

> > >
> > >But there is still an issue when the host kernel is a new one, while
> > >the qemu
> > >is an old one.  I am not sure how to make the virtqueues numbers to
> > >change in run-time
> > >for the host kernel. In another email thread, you mentioned removing 
> > >kconfig
> > >in the linux kernel, I believe that is related to this qemu patch,
> > >right?
> >
> > It was related to both, I don't think we should build QEMU and Linux
> > with or without dgram support.
> >
> > > If so,
> > >any ideas that I can make the host kernel change the number of vqs 
> > >in
> > >the run-time
> > >or when starting up vsock? The only way I can think of is to use a
> > >kernel module parameter
> > >for the vsock_vhost module. Any other ideas? Thanks.
> >
> > I need to check better, but we should be able to do all at run time
> > looking at the features field. As I said, both QEMU and kernel can
> > allocate the maximum number of queues that they can handle, then 
> > enable
> > only the queues allocated by the guest (e.g. during
> > vhost_vsock_common_start()).
>
> Yes. I checked the code and found there is an implementation bug ( or
> limitation) in drivers/vhost/vsock.c. In vhost_vsock_start(), if a 
> queue
> failed to init, the code will clean up all previous successfully
> allocated queues. That is why V1 code does not work when
> host kernel is new,  but qemu and guest kernel is old. I made a change
> there and it works now. I will clean up the patch a little bit and
> send V2 soon.

Great! I'll review the new version!

Thanks,
Stefano