[Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed

[Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed

[Kevin Wolf]

linux-aio uses a BH in order to make sure that the remaining completions
are processed even in nested event loops of completion callbacks in
order to avoid deadlocks.

There is no need, however, to have the BH overhead for the first call
into qemu_laio_completion_bh() or after all pending completions have
already been processed. Therefore, this patch calls directly into
qemu_laio_completion_bh() in qemu_laio_completion_cb() and cancels
the BH after qemu_laio_completion_bh() has processed all pending
completions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/linux-aio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/linux-aio.c b/block/linux-aio.c
index fe7cece..e468960 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -149,6 +149,8 @@ static void qemu_laio_completion_bh(void *opaque)
     if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
         ioq_submit(s);
     }
+
+    qemu_bh_cancel(s->completion_bh);
 }
 
 static void qemu_laio_completion_cb(EventNotifier *e)
@@ -156,7 +158,7 @@ static void qemu_laio_completion_cb(EventNotifier *e)
     LinuxAioState *s = container_of(e, LinuxAioState, e);
 
     if (event_notifier_test_and_clear(&s->e)) {
-        qemu_bh_schedule(s->completion_bh);
+        qemu_laio_completion_bh(s);
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed

[Paolo Bonzini]

On 15/06/2016 13:16, Kevin Wolf wrote:
> linux-aio uses a BH in order to make sure that the remaining completions
> are processed even in nested event loops of completion callbacks in
> order to avoid deadlocks.
> 
> There is no need, however, to have the BH overhead for the first call
> into qemu_laio_completion_bh() or after all pending completions have
> already been processed. Therefore, this patch calls directly into
> qemu_laio_completion_bh() in qemu_laio_completion_cb() and cancels
> the BH after qemu_laio_completion_bh() has processed all pending
> completions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/linux-aio.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index fe7cece..e468960 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -149,6 +149,8 @@ static void qemu_laio_completion_bh(void *opaque)
>      if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
>          ioq_submit(s);
>      }
> +
> +    qemu_bh_cancel(s->completion_bh);
>  }
>  
>  static void qemu_laio_completion_cb(EventNotifier *e)
> @@ -156,7 +158,7 @@ static void qemu_laio_completion_cb(EventNotifier *e)
>      LinuxAioState *s = container_of(e, LinuxAioState, e);
>  
>      if (event_notifier_test_and_clear(&s->e)) {
> -        qemu_bh_schedule(s->completion_bh);
> +        qemu_laio_completion_bh(s);
>      }
>  }
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Wed, Jun 15, 2016 at 01:16:42PM +0200, Kevin Wolf wrote:
> linux-aio uses a BH in order to make sure that the remaining completions
> are processed even in nested event loops of completion callbacks in
> order to avoid deadlocks.
> 
> There is no need, however, to have the BH overhead for the first call
> into qemu_laio_completion_bh() or after all pending completions have
> already been processed. Therefore, this patch calls directly into
> qemu_laio_completion_bh() in qemu_laio_completion_cb() and cancels
> the BH after qemu_laio_completion_bh() has processed all pending
> completions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/linux-aio.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] linux-aio: Cancel BH if not needed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

On Wed, Jun 15, 2016 at 01:16:42PM +0200, Kevin Wolf wrote:
> linux-aio uses a BH in order to make sure that the remaining completions
> are processed even in nested event loops of completion callbacks in
> order to avoid deadlocks.
> 
> There is no need, however, to have the BH overhead for the first call
> into qemu_laio_completion_bh() or after all pending completions have
> already been processed. Therefore, this patch calls directly into
> qemu_laio_completion_bh() in qemu_laio_completion_cb() and cancels
> the BH after qemu_laio_completion_bh() has processed all pending
> completions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/linux-aio.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I ran 4 x iodepth=16 random 4KB read I/O benchmarks.  There might be an
improvement but it's within the error margin.  My benchmarking setup can
be noisy...

Anyway, this patch doesn't hurt performance.  Guest and host are RHEL 7.2.

$ ./analyze.py runs/
Name                                          IOPS   Error
linux-aio-bh-optimizations-ccb9dc1      12942616.0 ± 16.83%
linux-aio-bh-optimizations-ccb9dc1-2    13833110.4 ± 4.74%
linux-aio-bh-optimizations-off-23b0d9f  13303981.4 ± 2.21%

qemu-system-x86_64 -pidfile qemu.pid -daemonize \
                   -machine accel=kvm -cpu host \
		   -smp 4 -m 1024 \
		   -netdev user,id=netdev0,hostfwd=tcp::2222-:22 \
                   -object iothread,id=iothread0 \
		   -device virtio-net-pci,netdev=netdev0 \
		   -drive if=none,id=drive0,file=/var/lib/libvirt/images/test.img,format=raw,aio=native,cache=none \
                   -device virtio-blk-pci,drive=drive0 \
		   -drive if=none,id=drive1,file=/dev/nullb0,format=raw,aio=native,cache=none \
                   -device virtio-blk-pci,drive=drive1 \
		   -display none

$ cat fio.job
[global]
filename=/dev/vdb
ioengine=libaio
direct=1
runtime=60
ramp_time=5
gtod_reduce=1

[job1]
numjobs=4
iodepth=16
rw=randread
bs=4K

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] linux-aio: Cancel BH if not needed

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]

Am 17.06.2016 um 12:13 hat Stefan Hajnoczi geschrieben:
> On Wed, Jun 15, 2016 at 01:16:42PM +0200, Kevin Wolf wrote:
> > linux-aio uses a BH in order to make sure that the remaining completions
> > are processed even in nested event loops of completion callbacks in
> > order to avoid deadlocks.
> > 
> > There is no need, however, to have the BH overhead for the first call
> > into qemu_laio_completion_bh() or after all pending completions have
> > already been processed. Therefore, this patch calls directly into
> > qemu_laio_completion_bh() in qemu_laio_completion_cb() and cancels
> > the BH after qemu_laio_completion_bh() has processed all pending
> > completions.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/linux-aio.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I ran 4 x iodepth=16 random 4KB read I/O benchmarks.  There might be an
> improvement but it's within the error margin.  My benchmarking setup can
> be noisy...
> 
> Anyway, this patch doesn't hurt performance.  Guest and host are RHEL 7.2.

Thanks for confirming!

> $ ./analyze.py runs/
> Name                                          IOPS   Error
> linux-aio-bh-optimizations-ccb9dc1      12942616.0 ± 16.83%
> linux-aio-bh-optimizations-ccb9dc1-2    13833110.4 ± 4.74%
> linux-aio-bh-optimizations-off-23b0d9f  13303981.4 ± 2.21%

What are these three commits? Is the first one with your virtio-blk
changes and this patch, the second only this patch and the third one
the baseline?

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] linux-aio: Cancel BH if not needed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

On Fri, Jun 17, 2016 at 12:24:11PM +0200, Kevin Wolf wrote:
> Am 17.06.2016 um 12:13 hat Stefan Hajnoczi geschrieben:
> > On Wed, Jun 15, 2016 at 01:16:42PM +0200, Kevin Wolf wrote:
> > > linux-aio uses a BH in order to make sure that the remaining completions
> > > are processed even in nested event loops of completion callbacks in
> > > order to avoid deadlocks.
> > > 
> > > There is no need, however, to have the BH overhead for the first call
> > > into qemu_laio_completion_bh() or after all pending completions have
> > > already been processed. Therefore, this patch calls directly into
> > > qemu_laio_completion_bh() in qemu_laio_completion_cb() and cancels
> > > the BH after qemu_laio_completion_bh() has processed all pending
> > > completions.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/linux-aio.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > I ran 4 x iodepth=16 random 4KB read I/O benchmarks.  There might be an
> > improvement but it's within the error margin.  My benchmarking setup can
> > be noisy...
> > 
> > Anyway, this patch doesn't hurt performance.  Guest and host are RHEL 7.2.
> 
> Thanks for confirming!
> 
> > $ ./analyze.py runs/
> > Name                                          IOPS   Error
> > linux-aio-bh-optimizations-ccb9dc1      12942616.0 ± 16.83%
> > linux-aio-bh-optimizations-ccb9dc1-2    13833110.4 ± 4.74%
> > linux-aio-bh-optimizations-off-23b0d9f  13303981.4 ± 2.21%
> 
> What are these three commits? Is the first one with your virtio-blk
> changes and this patch, the second only this patch and the third one
> the baseline?

The git trees are from qemu.git/master with no out-of-tree patches (my
stuff isn't there).

1. linux-aio-bh-optimizations-ccb9dc1      12942616.0 ± 16.83%

This is your "linux-aio: Cancel BH if not needed" in qemu.git/master.

2. linux-aio-bh-optimizations-ccb9dc1-2    13833110.4 ± 4.74%

I wanted to try again because the 16.83% error margin is very high and
probably due to noise.  This is just a re-run of #1.

3. linux-aio-bh-optimizations-off-23b0d9f  13303981.4 ± 2.21%

This is the commit before your "linux-aio: Cancel BH if not needed"
(ccb9dc1^ == 23b0d9f).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]