Re: [Qemu-devel] [PATCH] hw/virtio: fix vhost user fails to startup when MQ

[Yang, Zhiyong]

Hi,  Maxime, Marc-André Lureau:

Thank you a lot for your suggestions and I’m agree with you. I will send V2 
later according to
Maxime’s suggested change.

Thanks
Zhiyong

From: Marc-André Lureau [mailto:address@hidden
Sent: Wednesday, May 3, 2017 8:37 PM
To: Yang, Zhiyong <address@hidden>; Maxime Coquelin <address@hidden>; 
address@hidden
Cc: Liu, Yuanhan <address@hidden>; address@hidden
Subject: Re: [Qemu-devel] [PATCH] hw/virtio: fix vhost user fails to startup 
when MQ

Hi

On Wed, May 3, 2017 at 7:09 AM Yang, Zhiyong 
<address@hidden<mailto:address@hidden>> wrote:
Hi，Maxime:

> -----Original Message-----
> From: Maxime Coquelin [mailto:address@hidden<mailto:address@hidden>]
> Sent: Tuesday, May 2, 2017 8:23 PM
> To: Yang, Zhiyong <address@hidden<mailto:address@hidden>>; 
> address@hidden<mailto:address@hidden>
> Cc: address@hidden<mailto:address@hidden>
> Subject: Re: [Qemu-devel] [PATCH] hw/virtio: fix vhost user fails to startup 
> when
> MQ
>
>
>
> On 04/28/2017 09:09 AM, Zhiyong Yang wrote:
> >     Qemu2.7~2.9 and vhost user for dpdk 17.02 release work together to
> > cause failures of new connection when negotiating to set MQ.
> > (one queue pair works well).
> >     Because there exist some bugs in qemu code when introducing
> > VHOST_USER_PROTOCOL_F_REPLY_ACK to qemu. When
> vhost_user_set_mem_table
> > is invoked to deal with the vhost message VHOST_USER_SET_MEM_TABLE for
> > the second time, qemu indeed doesn't send the messge (The message
> > needs to be sent only once)but still will be waiting for dpdk's reply
> > ack, then, qemu is always freezing, while DPDK is always waiting for
> > next vhost message from qemu.
> >    The patch aims to fix the bug, MQ can work well.
> >    The same bug is found in function vhost_user_net_set_mtu, it is
> > fixed at the same time.
> >    DPDK related patch is as following:
> >    http://www.dpdk.org/dev/patchwork/patch/23955/
> >
> > Signed-off-by: Zhiyong Yang <address@hidden<mailto:address@hidden>>
> > ---
> >   hw/virtio/vhost-user.c | 18 +++++++++++-------
> >   1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index
> > 9334a8a..c2c54ce 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -205,10 +205,11 @@ static int vhost_user_write(struct vhost_dev *dev,
> VhostUserMsg *msg,
> >       /*
> >        * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
> >        * we just need send it once in the first time. For later such
> > -     * request, we just ignore it.
> > +     * request, we just ignore it. In this case, return value is 1 which is
> > +     * different from 0 that stands for message written successfully.
> >        */
> >       if (vhost_user_one_time_request(msg->request) && dev->vq_index != 0) {
> > -        return 0;
> > +        return 1;
>
> I personally prefer the fix I suggested in the DPDK mail thread, as returning 
> a
> random positive value does look like a workaround:

I think that for vhost_user_write(), it's behaving in a different way for some 
specific vhost messages.
So, it should not return the same returen value 0 which stands for success.

But you need to do the special handling for every caller.

>
> "
> I think the problem must be fixed generally and not per request.
> Maybe in vhost_user_write() if one-time request, just clear the
> VHOST_USER_NEED_REPLY flag. Then, in process_message_reply(), return early
> if this flag isn't set.
> "
It's another choise. Either this one nor that one,  not a big deal. :)
Fixing these  existing bugs is the most important.

While the suggestion from Maxime would work transparently, similar to one-time 
request are transparent to caller today. I also prefer that solution.
thanks
--
Marc-André Lureau