[Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.

remy.noel@blade-group.com posted 2 patches 6 years, 10 months ago
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181220152030.28035-1-remy.noel@blade-group.com
util/aio-posix.c | 90 +++++++++++++++++++++++++++++-------------------
util/aio-win32.c | 67 ++++++++++++++++-------------------
2 files changed, 84 insertions(+), 73 deletions(-)
[Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by remy.noel@blade-group.com 6 years, 10 months ago
From: Remy Noel <remy.noel@blade-group.com>

It is possible for an io_poll/read/write callback to be concurrently executed along
with an aio_set_fd_handlers. This can cause all sorts of problems, like
a NULL callback or a bad opaque pointer.

V2:
    * Do not use RCU anymore as it inccurs a performance loss
V3:
    * Don't drop revents when a handler is modified [Stefan]
V4:
    * Unregister fd from ctx epoll when removing fd_handler [Paolo]

Remy Noel (2):
  aio-posix: Unregister fd from ctx epoll when removing fd_handler.
  aio-posix: Fix concurrent aio_poll/set_fd_handler.

 util/aio-posix.c | 90 +++++++++++++++++++++++++++++-------------------
 util/aio-win32.c | 67 ++++++++++++++++-------------------
 2 files changed, 84 insertions(+), 73 deletions(-)

-- 
2.19.2


Re: [Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by Stefan Hajnoczi 6 years, 10 months ago
On Thu, Dec 20, 2018 at 04:20:28PM +0100, remy.noel@blade-group.com wrote:
> From: Remy Noel <remy.noel@blade-group.com>
> 
> It is possible for an io_poll/read/write callback to be concurrently executed along
> with an aio_set_fd_handlers. This can cause all sorts of problems, like
> a NULL callback or a bad opaque pointer.
> 
> V2:
>     * Do not use RCU anymore as it inccurs a performance loss
> V3:
>     * Don't drop revents when a handler is modified [Stefan]
> V4:
>     * Unregister fd from ctx epoll when removing fd_handler [Paolo]
> 
> Remy Noel (2):
>   aio-posix: Unregister fd from ctx epoll when removing fd_handler.
>   aio-posix: Fix concurrent aio_poll/set_fd_handler.
> 
>  util/aio-posix.c | 90 +++++++++++++++++++++++++++++-------------------
>  util/aio-win32.c | 67 ++++++++++++++++-------------------
>  2 files changed, 84 insertions(+), 73 deletions(-)
> 
> -- 
> 2.19.2
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by Paolo Bonzini 6 years, 10 months ago
On 20/12/18 17:37, Stefan Hajnoczi wrote:
> On Thu, Dec 20, 2018 at 04:20:28PM +0100, remy.noel@blade-group.com wrote:
>> From: Remy Noel <remy.noel@blade-group.com>
>>
>> It is possible for an io_poll/read/write callback to be concurrently executed along
>> with an aio_set_fd_handlers. This can cause all sorts of problems, like
>> a NULL callback or a bad opaque pointer.
>>
>> V2:
>>     * Do not use RCU anymore as it inccurs a performance loss
>> V3:
>>     * Don't drop revents when a handler is modified [Stefan]
>> V4:
>>     * Unregister fd from ctx epoll when removing fd_handler [Paolo]
>>
>> Remy Noel (2):
>>   aio-posix: Unregister fd from ctx epoll when removing fd_handler.
>>   aio-posix: Fix concurrent aio_poll/set_fd_handler.
>>
>>  util/aio-posix.c | 90 +++++++++++++++++++++++++++++-------------------
>>  util/aio-win32.c | 67 ++++++++++++++++-------------------
>>  2 files changed, 84 insertions(+), 73 deletions(-)
>>
>> -- 
>> 2.19.2
>>
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

FWIW, I had missed the early version that used RCU, but lockcnt is
already very RCU-like, so not using RCU is the right thing to do.  The
difference between lockcnt and RCU is that cleanup is done by the reader
instead of a separate thread.  Because we know that reader/writer
concurrency is very rare for AioContext handlers, the tradeoffs favor
lockcnt over RCU.

Paolo

Re: [Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by Remy NOEL 6 years, 10 months ago
On 12/21/18 12:34 PM, Paolo Bonzini wrote:

> FWIW, I had missed the early version that used RCU, but lockcnt is
> already very RCU-like, so not using RCU is the right thing to do.  The
> difference between lockcnt and RCU is that cleanup is done by the reader
> instead of a separate thread.  Because we know that reader/writer
> concurrency is very rare for AioContext handlers, the tradeoffs favor
> lockcnt over RCU.

Indeed, i forgot to CC you in the first batch (get_maintainer.pl was not 
finding you).

Thanks for the RCU hints, i though the performance hit was due to the 
RCU being global to qemu.


Remy


Re: [Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by remy.noel@blade-group.com 6 years, 10 months ago
On Thu, Dec 20, 2018 at 04:20:28PM +0100, Remy Noel wrote:
>From: Remy Noel <remy.noel@blade-group.com>
>
>It is possible for an io_poll/read/write callback to be concurrently executed along
>with an aio_set_fd_handlers. This can cause all sorts of problems, like
>a NULL callback or a bad opaque pointer.
>
>V2:
>    * Do not use RCU anymore as it inccurs a performance loss
>V3:
>    * Don't drop revents when a handler is modified [Stefan]
>V4:
>    * Unregister fd from ctx epoll when removing fd_handler [Paolo]
>
>Remy Noel (2):
>  aio-posix: Unregister fd from ctx epoll when removing fd_handler.
>  aio-posix: Fix concurrent aio_poll/set_fd_handler.
>
> util/aio-posix.c | 90 +++++++++++++++++++++++++++++-------------------
> util/aio-win32.c | 67 ++++++++++++++++-------------------
> 2 files changed, 84 insertions(+), 73 deletions(-)
>
>-- 
>2.19.2
>
ping.

Does it needs anything for getting queued ?

Thanks.

Remy.

Re: [Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by Stefan Hajnoczi 6 years, 10 months ago
On Thu, Dec 20, 2018 at 04:20:28PM +0100, remy.noel@blade-group.com wrote:
> From: Remy Noel <remy.noel@blade-group.com>
> 
> It is possible for an io_poll/read/write callback to be concurrently executed along
> with an aio_set_fd_handlers. This can cause all sorts of problems, like
> a NULL callback or a bad opaque pointer.
> 
> V2:
>     * Do not use RCU anymore as it inccurs a performance loss
> V3:
>     * Don't drop revents when a handler is modified [Stefan]
> V4:
>     * Unregister fd from ctx epoll when removing fd_handler [Paolo]
> 
> Remy Noel (2):
>   aio-posix: Unregister fd from ctx epoll when removing fd_handler.
>   aio-posix: Fix concurrent aio_poll/set_fd_handler.
> 
>  util/aio-posix.c | 90 +++++++++++++++++++++++++++++-------------------
>  util/aio-win32.c | 67 ++++++++++++++++-------------------
>  2 files changed, 84 insertions(+), 73 deletions(-)
> 
> -- 
> 2.19.2
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan
Re: [Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by Stefan Hajnoczi 6 years, 9 months ago
On Thu, Dec 20, 2018 at 04:20:28PM +0100, remy.noel@blade-group.com wrote:
> From: Remy Noel <remy.noel@blade-group.com>
> 
> It is possible for an io_poll/read/write callback to be concurrently executed along
> with an aio_set_fd_handlers. This can cause all sorts of problems, like
> a NULL callback or a bad opaque pointer.
> 
> V2:
>     * Do not use RCU anymore as it inccurs a performance loss
> V3:
>     * Don't drop revents when a handler is modified [Stefan]
> V4:
>     * Unregister fd from ctx epoll when removing fd_handler [Paolo]
> 
> Remy Noel (2):
>   aio-posix: Unregister fd from ctx epoll when removing fd_handler.
>   aio-posix: Fix concurrent aio_poll/set_fd_handler.
> 
>  util/aio-posix.c | 90 +++++++++++++++++++++++++++++-------------------
>  util/aio-win32.c | 67 ++++++++++++++++-------------------
>  2 files changed, 84 insertions(+), 73 deletions(-)
> 
> -- 
> 2.19.2
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan
Re: [Qemu-devel] [QEMU-devel][PATCH v4 0/2] Fix concurrent aio_poll/set_fd_handler.
Posted by remy.noel@blade-group.com 6 years, 9 months ago
On Sat, Jan 12, 2019 at 08:30:08AM +0000, Stefan Hajnoczi wrote:
>Thanks, applied to my block tree:
>https://github.com/stefanha/qemu/commits/block
>
Thanks !

Remy