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

[Maxime Leroy]

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.

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 ?

>> >
>>
[..]
>
> 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