Re: [Qemu-devel] [PATCH 1/1] virtio: fallback from irqfd to non-irqfd notify

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 03/01/2017 01:54 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 01, 2017 at 12:50:04PM +0100, Halil Pasic wrote:
>> The commits 03de2f527 "virtio-blk: do not use vring in dataplane"  and
>> 9ffe337c08 "virtio-blk: always use dataplane path if ioeventfd is active"
>> changed how notifications are done for virtio-blk substantially. Due to a
>> race condition interrupts are lost when irqfd is torn down after
>> notify_guest_bh was scheduled but before it actually runs.  Furthermore
>> virtio_notify_irqfd ignores the value returned by event_notifier_set
>> which correctly indicates that notification has failed due to bad file
>> descriptor.
>>
>> Let's fix this by making virtio_notify_irqfd fall back to the non-irqfd
>> notification mechanism if event_notifier_set fails.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>
>> This is probably not the only way to fix this: suggestions welcome. I
>> did not use a fixes tag because I'm not sure yet where exactly things got
>> broken. Maybe guys more familiar with dataplane an coroutines can help
>> (Paolo, Stefan).
>> ---
>>  hw/virtio/virtio.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 23483c7..8e1c1e9 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1581,7 +1581,9 @@ void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
>>       * to an atomic operation.
>>       */
>>      virtio_set_isr(vq->vdev, 0x1);
>> -    event_notifier_set(&vq->guest_notifier);
>> +    if (event_notifier_set(&vq->guest_notifier)) {
>> +        virtio_notify_vector(vdev, vq->vector);
>> +    }
> 
> Does this fail because the underlying fd got closed?

Yes. Its data_plane_stop()->virtio_ccw_set_guest_notifier->event_notifier_cleanup().
The function event_notifier_cleanup() does not set fds to -1 :(.
Seems to me, it would be safer to do so. Should I make a patch?

> Then there's a problem: trying to write to a closed
> fd might corrupt an unrelated fd.
> If you want to use this way we need to set fds to -1 when we close.

Sorry, did not check for that. OTOH Paolo says my approach is
fundamentally flawed because virtio_notify_vector is not thread
safe. I suggest we continue the discussion there.

Regards,
Halil

> 
>>  }
>>  
>>  static void virtio_irq(VirtQueue *vq)
>> -- 
>> 2.8.4
>