All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed
@ 2016-06-15 11:16 Kevin Wolf
  2016-06-15 12:39 ` Paolo Bonzini
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Kevin Wolf @ 2016-06-15 11:16 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, stefanha, qemu-devel

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed
  2016-06-15 11:16 [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed Kevin Wolf
@ 2016-06-15 12:39 ` Paolo Bonzini
  2016-06-16 16:08 ` Stefan Hajnoczi
  2016-06-17 10:13 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
  2 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2016-06-15 12:39 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: stefanha, qemu-devel



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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed
  2016-06-15 11:16 [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed Kevin Wolf
  2016-06-15 12:39 ` Paolo Bonzini
@ 2016-06-16 16:08 ` Stefan Hajnoczi
  2016-06-17 10:13 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
  2 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2016-06-16 16:08 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-block, pbonzini, qemu-devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] linux-aio: Cancel BH if not needed
  2016-06-15 11:16 [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed Kevin Wolf
  2016-06-15 12:39 ` Paolo Bonzini
  2016-06-16 16:08 ` Stefan Hajnoczi
@ 2016-06-17 10:13 ` Stefan Hajnoczi
  2016-06-17 10:24   ` Kevin Wolf
  2 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2016-06-17 10:13 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-block, pbonzini, qemu-devel, stefanha

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] linux-aio: Cancel BH if not needed
  2016-06-17 10:13 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2016-06-17 10:24   ` Kevin Wolf
  2016-06-20 10:34     ` Stefan Hajnoczi
  0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2016-06-17 10:24 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: qemu-block, pbonzini, qemu-devel, stefanha

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] linux-aio: Cancel BH if not needed
  2016-06-17 10:24   ` Kevin Wolf
@ 2016-06-20 10:34     ` Stefan Hajnoczi
  0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2016-06-20 10:34 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-block, pbonzini, qemu-devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-06-20 10:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-15 11:16 [Qemu-devel] [PATCH] linux-aio: Cancel BH if not needed Kevin Wolf
2016-06-15 12:39 ` Paolo Bonzini
2016-06-16 16:08 ` Stefan Hajnoczi
2016-06-17 10:13 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-06-17 10:24   ` Kevin Wolf
2016-06-20 10:34     ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.