eventfd: making it thread safe

QEMU uses IO handlers to run select() in the main loop.
The handlers list is managed by qemu_set_fd_handler() helper
which works fine when called from the main thread as it is
called not when select() is waiting.

However IO handlers list can be changed in the thread other than
the main one doing os_host_main_loop_wait(), for example, as a result
of a hypercall which changes PCI config space (VFIO on POWER is the case)
and enables/disabled MSI/MSIX which closes/creates eventfd handles.
If the main loop is waiting on such eventfd, it has to be restarted.

The patch adds the qemu_notify_event() call to interrupt select()
and make main_loop() to restart select() with the updated IO
handlers list.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 iohandler.c |    1 +
 1 file changed, 1 insertion(+)

On Wed, Jul 18, 2012 at 10:08:53PM +1000, Alexey Kardashevskiy wrote:
> QEMU uses IO handlers to run select() in the main loop.
> The handlers list is managed by qemu_set_fd_handler() helper
> which works fine when called from the main thread as it is
> called not when select() is waiting.

when select() is not waiting?

> 
> However IO handlers list can be changed in the thread other than
> the main one doing os_host_main_loop_wait(), for example, as a result
> of a hypercall which changes PCI config space (VFIO on POWER is the case)

So the problem is only with VFIO? Can it affect vhost-net?

> and enables/disabled MSI/MSIX which closes/creates eventfd handles.

There doesn't seem to be a notification in case an fd is
deleted. It's probably not at all urgent to remove
an fd from select - why do you mention closing handles?

> If the main loop is waiting on such eventfd, it has to be restarted.

Do you really mean 'should be waiting on the newly created
eventfd'?

> The patch adds the qemu_notify_event() call to interrupt select()
> and make main_loop() to restart select()

s/and make main_loop() to restart/to make main_loop() restart/?

> with the updated IO
> handlers list.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  iohandler.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/iohandler.c b/iohandler.c
> index 3c74de6..dea4355 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -77,6 +77,7 @@ int qemu_set_fd_handler2(int fd,
>          ioh->fd_write = fd_write;
>          ioh->opaque = opaque;
>          ioh->deleted = 0;
> +        qemu_notify_event();
>      }
>      return 0;
>  }
> -- 
> 1.7.10.4

On 18/07/12 22:22, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2012 at 10:08:53PM +1000, Alexey Kardashevskiy wrote:
>> QEMU uses IO handlers to run select() in the main loop.
>> The handlers list is managed by qemu_set_fd_handler() helper
>> which works fine when called from the main thread as it is
>> called not when select() is waiting.
> 
> when select() is not waiting?
> 
>>
>> However IO handlers list can be changed in the thread other than
>> the main one doing os_host_main_loop_wait(), for example, as a result
>> of a hypercall which changes PCI config space (VFIO on POWER is the case)
> 
> So the problem is only with VFIO? Can it affect vhost-net?

Honestly I have no idea about vhost-net as I never tried it.


>> and enables/disabled MSI/MSIX which closes/creates eventfd handles.
> 
> There doesn't seem to be a notification in case an fd is
> deleted. It's probably not at all urgent to remove
> an fd from select - why do you mention closing handles?

Agrhh. I missed this comment in the patch I just reposted.
Mentioned because the file* is still open when there is no need in it.
It has no effect for eventfd but may have for somebody else so we probably
want to add a notification on deletion. Dunno.


>> If the main loop is waiting on such eventfd, it has to be restarted.
> 
> Do you really mean 'should be waiting on the newly created
> eventfd'?
> 
>> The patch adds the qemu_notify_event() call to interrupt select()
>> and make main_loop() to restart select()
> 
> s/and make main_loop() to restart/to make main_loop() restart/?

Thanks for the comments. David used to polish my english but he is vacation
now :)


>> with the updated IO
>> handlers list.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  iohandler.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/iohandler.c b/iohandler.c
>> index 3c74de6..dea4355 100644
>> --- a/iohandler.c
>> +++ b/iohandler.c
>> @@ -77,6 +77,7 @@ int qemu_set_fd_handler2(int fd,
>>          ioh->fd_write = fd_write;
>>          ioh->opaque = opaque;
>>          ioh->deleted = 0;
>> +        qemu_notify_event();
>>      }
>>      return 0;
>>  }
>> -- 
>> 1.7.10.4