| Message ID | 1465989402-18890-1-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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>
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
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
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).
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); } }
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(-)