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

[Felipe Franciosi]

Hi Michael,

> On 16 Jan 2017, at 10:22, Michael S. Tsirkin <address@hidden> wrote:
> 
> On Thu, Jan 12, 2017 at 05:14:07PM -0800, Felipe Franciosi wrote:
>> Currently, VQs are started as soon as a SET_VRING_KICK is received. That
>> is too early in the VQ setup process, as the backend might not yet have
>> a callfd to notify in case it received a kick and fully processed the
>> request/command. This patch only starts a VQ when a SET_VRING_CALL is
>> received.
>> 
>> Signed-off-by: Felipe Franciosi <address@hidden>
> 
> Doesn't look right to me.
> It should happen when the fd supplies becomes readable.

Apologies, but I'm confused now. What should happen when which fd becomes 
readable? I thought we were discussing this further down in the thread. My last 
comments are here:

http://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg02755.html

I pointed out what appears to be a race and am looking forward to your 
comments. There is definitely a problem with the current code, so it's a matter 
of finding out exactly what's happening and what's the correct (and 
spec-compliant) way to fix it.

Felipe

> 
>> ---
>> 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