Re: [Qemu-devel] [PATCH] libvhost-user: Start VQs on SET_VRING_CALL

[Paolo Bonzini]

On 17/01/2017 20:00, Michael S. Tsirkin wrote:
>>> vhost user is not supposed to use maskfd at all.
>>>
>>> We have this code:
>>>        if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
>>>            dev->use_guest_notifier_mask = false;
>>>        }
>>>
>>> isn't it effective?
>>
>> I'm observing this problem when using vhost-user-scsi, not -net. So the code 
>> above is not in effect. Anyway, I'd expect the race I described to also 
>> happen on vhost-scsi.
>
> maskfd is just racy for vhost-user ATM.  I'm guessing vhost-scsi should
> just set use_guest_notifier_mask, that will fix it.  Alternatively,
> rework masking to support sync with the backend - but I doubt it's
> useful.

I agree with Michael that this is a QEMU bug, not a libvhost-user bug.

Thanks,

Paolo

>>>
>>>
>>>
>>>> Qemu sends the new callfd to the application
>>>>
>>>> It's not hard to repro. How could this situation be avoided?
>>>>
>>>> Cheers,
>>>> Felipe
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> Perhaps it's best for now to delay the callfd notification with a 
>>>>>>>>> flag until it is received?
>>>>>>>>
>>>>>>>> The other idea is to always kick when we receive the callfd. I 
>>>>>>>> remember discussing that alternative with you before libvhost-user 
>>>>>>>> went in. The protocol says both the driver and the backend must handle 
>>>>>>>> spurious kicks. This approach also fixes the bug.
>>>>>>>>
>>>>>>>> I'm happy with whatever alternative you want, as long it makes 
>>>>>>>> libvhost-user usable for storage devices.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Felipe
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Felipe Franciosi <address@hidden>
>>>>>>>>>> ---
>>>>>>>>>> contrib/libvhost-user/libvhost-user.c | 26 +++++++++++++-------------
>>>>>>>>>> 1 file changed, 13 insertions(+), 13 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/contrib/libvhost-user/libvhost-user.c
>>>>>>>>>> b/contrib/libvhost-user/libvhost-user.c
>>>>>>>>>> index af4faad..a46ef90 100644
>>>>>>>>>> --- a/contrib/libvhost-user/libvhost-user.c
>>>>>>>>>> +++ b/contrib/libvhost-user/libvhost-user.c
>>>>>>>>>> @@ -607,19 +607,6 @@ vu_set_vring_kick_exec(VuDev *dev, VhostUserMsg 
>>>>>>>>>> *vmsg)
>>>>>>>>>>      DPRINT("Got kick_fd: %d for vq: %d\n", vmsg->fds[0], index);
>>>>>>>>>>  }
>>>>>>>>>>
>>>>>>>>>> -    dev->vq[index].started = true;
>>>>>>>>>> -    if (dev->iface->queue_set_started) {
>>>>>>>>>> -        dev->iface->queue_set_started(dev, index, true);
>>>>>>>>>> -    }
>>>>>>>>>> -
>>>>>>>>>> -    if (dev->vq[index].kick_fd != -1 && dev->vq[index].handler) {
>>>>>>>>>> -        dev->set_watch(dev, dev->vq[index].kick_fd, VU_WATCH_IN,
>>>>>>>>>> -                       vu_kick_cb, (void *)(long)index);
>>>>>>>>>> -
>>>>>>>>>> -        DPRINT("Waiting for kicks on fd: %d for vq: %d\n",
>>>>>>>>>> -               dev->vq[index].kick_fd, index);
>>>>>>>>>> -    }
>>>>>>>>>> -
>>>>>>>>>>  return false;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> @@ -661,6 +648,19 @@ vu_set_vring_call_exec(VuDev *dev, VhostUserMsg 
>>>>>>>>>> *vmsg)
>>>>>>>>>>
>>>>>>>>>>  DPRINT("Got call_fd: %d for vq: %d\n", vmsg->fds[0], index);
>>>>>>>>>>
>>>>>>>>>> +    dev->vq[index].started = true;
>>>>>>>>>> +    if (dev->iface->queue_set_started) {
>>>>>>>>>> +        dev->iface->queue_set_started(dev, index, true);
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    if (dev->vq[index].kick_fd != -1 && dev->vq[index].handler) {
>>>>>>>>>> +        dev->set_watch(dev, dev->vq[index].kick_fd, VU_WATCH_IN,
>>>>>>>>>> +                       vu_kick_cb, (void *)(long)index);
>>>>>>>>>> +
>>>>>>>>>> +        DPRINT("Waiting for kicks on fd: %d for vq: %d\n",
>>>>>>>>>> +               dev->vq[index].kick_fd, index);
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>>  return false;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> 1.9.4
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
> 
>