Re: [Qemu-devel] [PATCH v5] vhost-user: add multi queue support

[Michael S. Tsirkin]

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 <address@hidden> 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 <address@hidden>
> >>> 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 <address@hidden>
> >>> >> Signed-off-by: Changchun Ouyang <address@hidden>
> >>> >
> >>> > 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