On Wed, Jun 30, 2021 at 11:55 PM Stefano Garzarella <sgarz...@redhat.com> wrote: > > On Wed, Jun 30, 2021 at 03:44:17PM -0700, Jiang Wang . wrote: > >On Thu, Jun 24, 2021 at 7:31 AM Stefano Garzarella <sgarz...@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 did not find any way to do that. When the host kernel is new, QEMU can handle 4 queues. When the host kernel is an old one, QEMU can only handle 2 queues.
> I'm not sure that adding a parameter to QEMU is a good idea. If possible > it should be automatic. > OK. I will keep that in mind and dig more to see if there is a way to do that. Thanks. > > > >> > > >> >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 >