[Qemu-devel] [PATCH] eventfd: making it thread safe

[Qemu-devel] [PATCH] eventfd: making it thread safe

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

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 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)
and enables/disabled MSI/MSIX which creates eventfd handles.
As the main loop should be waiting on the newly created eventfds,
it has to be restarted.

The patch adds the qemu_notify_event() call to interrupt select()
to make main_loop() 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(+)

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

Re: [Qemu-devel] [PATCH] eventfd: making it thread safe

[Avi Kivity]

On 08/01/2012 07:05 AM, David Gibson wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> 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 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)
> and enables/disabled MSI/MSIX which creates eventfd handles.
> As the main loop should be waiting on the newly created eventfds,
> it has to be restarted.
> 
> The patch adds the qemu_notify_event() call to interrupt select()
> to make main_loop() 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(+)
> 
> 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;
>  }

Perhaps it's better to do this unconditionally (on the delete path too)
so that removals are processed without delay and we don't have closed
fds hanging around in select().

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] eventfd: making it thread safe

[David Gibson]

On Mon, Aug 06, 2012 at 05:05:57PM +0300, Avi Kivity wrote:
> On 08/01/2012 07:05 AM, David Gibson wrote:
> > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> > 
> > 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 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)
> > and enables/disabled MSI/MSIX which creates eventfd handles.
> > As the main loop should be waiting on the newly created eventfds,
> > it has to be restarted.
> > 
> > The patch adds the qemu_notify_event() call to interrupt select()
> > to make main_loop() 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(+)
> > 
> > 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;
> >  }
> 
> Perhaps it's better to do this unconditionally (on the delete path too)
> so that removals are processed without delay and we don't have closed
> fds hanging around in select().

Well, I understand that Alexey discussed the patch with Paolo and
Michael Tsirkin, and this was the preferred approach for now.  Since
obviously no events will happen on deleted fds, removing them from the
select() is not really urgent.

This is a very straightforward fix for a real problem, can we please
just merge the damn thing.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH] eventfd: making it thread safe

[Paolo Bonzini]

Il 07/08/2012 06:02, David Gibson ha scritto:
>> Perhaps it's better to do this unconditionally (on the delete path too)
>> so that removals are processed without delay and we don't have closed
>> fds hanging around in select().
> 
> Well, I understand that Alexey discussed the patch with Paolo and
> Michael Tsirkin, and this was the preferred approach for now.  Since
> obviously no events will happen on deleted fds, removing them from the
> select() is not really urgent.

Avi is not speaking about deleted fds, but about existing fds whose
handlers are temporarily removed.  I don't see it as a blocker for
merging the patch because we've never observed it (and it's unlikely,
because temporary removal of handlers typically occurs from within a
handler, not from another thread).

Paolo

[Qemu-devel] [PATCH] eventfd: making it thread safe

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

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 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)
and enables/disabled MSI/MSIX which creates eventfd handles.
As the main loop should be waiting on the newly created eventfds,
it has to be restarted.

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

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

---
 iohandler.c |    1 +
 1 file changed, 1 insertion(+)

Anthony, this bugfix for eventfds has been sent before, but seems to
have fallen through the cracks.  Please apply.

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

[Qemu-devel] [PATCH] eventfd: making it thread safe

[Alexey Kardashevskiy]

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 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)
and enables/disabled MSI/MSIX which creates eventfd handles.
As the main loop should be waiting on the newly created eventfds,
it has to be restarted.

The patch adds the qemu_notify_event() call to interrupt select()
to make main_loop() 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(+)

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

[Qemu-devel] [PATCH] eventfd: making it rhread safe

[Alexey Kardashevskiy]

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 sometime we need to update the handlers list from another thread. For that the main loop's select() needs to be restarted with the updated list.

The patch adds the qemu_notify_event() call to interrupt select() and make wrapping code 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(+)

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

[Qemu-devel] [PATCH] eventfd: making it thread safe

[Alexey Kardashevskiy]

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(+)

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

Re: [Qemu-devel] [PATCH] eventfd: making it thread safe

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] eventfd: making it thread safe

[Alexey Kardashevskiy]

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


-- 
Alexey

[Qemu-devel] [PATCH] eventfd: making it thread safe

[Alexey Kardashevskiy]

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 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)
and enables/disabled MSI/MSIX which closes/creates eventfd handles.
As the main loop should be waiting on the newly created eventfds,
it has to be restarted.

The patch adds the qemu_notify_event() call to interrupt select()
to make main_loop() 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(+)

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