On Thu, Jul 09, 2015 at 02:24:51PM +0200, Maxime Leroy wrote: > Hi Martin, Michael, > > On Thu, Jul 9, 2015 at 2:00 PM, Martin Kletzander <mklet...@redhat.com> wrote: > > On Thu, Jul 09, 2015 at 10:06:35AM +0300, Michael S. Tsirkin wrote: > >> > >> On Thu, Jul 09, 2015 at 12:00:59AM +0200, Maxime Leroy wrote: > >>> > >>> Hi Michael, > >>> > >>> On Wed, Jul 8, 2015 at 4:29 PM, Michael S. Tsirkin <m...@redhat.com> > >>> wrote: > >>> > On Thu, May 28, 2015 at 09:23:06AM +0800, Ouyang Changchun wrote: > >>> >> Based on patch by Nikolay Nikolaev: > >>> >> Vhost-user will implement the multi queue support in a similar way > >>> >> to what vhost already has - a separate thread for each queue. > >>> >> To enable the multi queue functionality - a new command line parameter > >>> >> "queues" is introduced for the vhost-user netdev. > >>> >> > >>> >> Signed-off-by: Nikolay Nikolaev <n.nikol...@virtualopensystems.com> > >>> >> Signed-off-by: Changchun Ouyang <changchun.ouy...@intel.com> > >>> > > >>> > So testing turned up a significant issue with the protocol extension in > >>> > this > >>> > one. Specifically, remote has no idea how many queues guest actually > >>> > wants to use (it's dynamic, guest changes this at any time). > >>> > We need support for enabling and disabling queues dynamically. > >>> > > >>> > Given we are past hard freeze, and given no one uses this yet > >>> > (dpdk upstream did not merge supporting protocol), > >>> > I think the best thing to do is to disable this functionality for 2.4. > >>> > I will send a patch to do this shortly. > >>> > >>> You are making a wrong statement, we already use multiqueue for > >>> vhost-user and we expected to have this support officially integrated > >>> in qemu 2.4. > >>> > >>> Libvirt 1.2.17 has been released with multiqueue support for > >>> vhost-user. > >>> (http://libvirt.org/git/?p=libvirt.git;a=commit;h=366c22f2bcf1ddb8253c123f93fd18d1ba9eacd6) > >>> It checks against the version of qemu (i.e. 2.4) to know if > >>> multiqueue is supported or not by qemu. > >>> > >>> (http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=7971723b985b9adc27122a3503e7ab38ced2b57f;hp=e7f5510ef2d28ca0ae0ed5751b1fd3018130d6c1) > >> > >> > >> Ouch. Just another example showing how version checks are evil. > >> We don't want to break libvirt, I agree. > >> > > > > Yes, exactly. Unfortunately if QEMU doesn't expose it in any way we > > don't have many more options. > > > >> I think what we can do is accept the command line created > >> by libvirt, just ignore it and use a single queue only. > >> > > > > Anyway, I think it would be pretty OK to disable it *if and only if* > > you error out with a sensible error message (e.g. "multiple queues are > > not supported yet"). > > I consider that accepting the queue parameter for vhost-user but only > creates a single queue is a bug. > Unfortunately I don't think we have many solution here for libvirt 1.2.17.0 > > Agree with Martin, at least, we should display an error message. > > > > > And from 2.5 and next libvirt release we can fix this properly > > (QEMU - exposing the capability and libvirt - checking for it). > > is it possible to backport this fix in the branch 1.2.17 of libvirt ? > > > > > > >>> Dynamically enabling/disabling queue between host/guest is a nice > >>> feature to have. > >>> But it's not mandatory. > >> > >> Same number of queues on host and guest can work normally, I have > >> validated it with dpdk. > > > > Try to use upstream dpdk without your patches and upstream qemu (with > > patches integrated) and you will see some of the issues I am > > talking about: crashes in guest and in dpdk. > > I test with my own vhost-user implementation, and didn't observe any > crashes on host or guest. > Traffic was just not process by the guess, when the guess was > configured with less ring that the host. > > Anyway, I have to do some modification in my vhost-user implementation > because the vhost-user protocol has changed. > > The VHOST_USER_RESET_OWNER payload has changed in this patch. Now we > are setting the vq_index.
As I explained I plan to revert this anyway. > > But the documentation has not been updated: > http://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/vhost-user.txt;h=2c8e9347ccee80d11b5e740b717093c1e536af02;hb=HEAD#l165 > I am wondering if the VHOST_USER_VERSION should have been increased > with this patch ? Unfortunately everyone seems to ignore the version. I think using feature bits works though. > >> > > >> > [..] > > > > Two main things are broken on the QEMU side: > > - MQ feature requires CTRL vq. QEMU does not pass CTRL vq info to DPDK > > so of course it just lies to guest that it's supported: > > all info is buffered in QEMU but stays unused. > > > > - MQ feature requires that max number of queues is reported > > to guest. QEMU does not request this info from DPDK so > > of course it has no way to know this number. > > Instead it just assumes DPDK can handle whatever's thrown at it, > > but naturally this can't work in practice so of course it doesn't. > > For my understanding, vhost-user just translates ioctl for > /dev/vhost-net into vhost user request over a socket. > > Activation/deactivation of queues for virtio-net, it's not done with > ioctl on /dev/vhost-net. > But by detach or attach tun queues to virtio ring with ioctl > TUNSETQUEUE with flags IFF_ATTACH/DETACH_QUEUE to /dev/net/tun. > > So, how do you plan to extend vhost-user protocol to notify > activation/deactivation of queue ? > > > > >> And have an enhancement patch set to implement dynamically > >> enabling/disabling in 2.5 or 2.4.x > >> After extending the vhost-user spec. > >> > >> Thanks > >> Changchun > > > > 2.5 development is just around the corner. With enough effort we can > > have the patches upstream first thing in 2.5 cycle. > > I'll be happy to help you testing your new patches for multiqueue > support against my vhost-user implementation. > Could you please add me in CC when you send these patches on the mailing list > ? > > > > > Once there, the whole feature can be backported if enough people > > are prepared to dedicate resources to maintaining 2.4.x. > > To me this sounds like a much better deal for everyone > > than releasing a known-broken device and trying to fix it up. > > > > It would be really nice to backport this feature in qemu 2.4.x. > When will it be available into a 2.4.x maintenance? > > Regards, > > Maxime
