Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Il 02/06/2014 15:06, Ming Lei ha scritto: > > If you're running SMP under an emulator where exits are expensive, then > this wins. Under KVM it's marginal at best. Both my tests on arm64 and x86 are under KVM, and looks the patch can improve performance a lot. IMO, even though under KVM, virtio-blk performance still depends how well hypervisor( qemu, ...) emulates the device, and basically speaking, it is expensive to switch from guest to host and let host handle the notification. The difference is that virtio-pci supports ioeventfd and virtio-mmio doesn't. With ioeventfd you can tell KVM "I don't care about the value that is written to a memory location, only that it is accessed". Then when the write happens, KVM doesn't do an expensive userspace exit; it just writes 1 to an eventfd. It then returns to the guest, userspace picks up the eventfd via its poll() loop and services the device. This is already useful for throughput on UP, and the small latency cost (because of the cost of the event loop in the I/O thread, and possibly the cost of waking up the thread) is usually offset by the benefit. But on SMP you get double benefit. Obviously, the kernel doesn't have to spin while userspace does its stuff. On top of this, there is also a latency improvement from ioeventfd, because QEMU processes virtqueue_notify under its "big QEMU lock". With ioeventfd, serialized virtqueue processing can be a bottleneck, but it doesn't affect latency. Without ioeventfd it affects the VCPUs' latency and negates a lot of the benefit of Ming Lei's patch. You can try disabling ioeventfd with "-global virtio-blk-pci.ioeventfd=off" on the QEMU command line. Performance will plummet. :) Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Il 02/06/2014 15:06, Ming Lei ha scritto: If you're running SMP under an emulator where exits are expensive, then this wins. Under KVM it's marginal at best. Both my tests on arm64 and x86 are under KVM, and looks the patch can improve performance a lot. IMO, even though under KVM, virtio-blk performance still depends how well hypervisor( qemu, ...) emulates the device, and basically speaking, it is expensive to switch from guest to host and let host handle the notification. The difference is that virtio-pci supports ioeventfd and virtio-mmio doesn't. With ioeventfd you can tell KVM I don't care about the value that is written to a memory location, only that it is accessed. Then when the write happens, KVM doesn't do an expensive userspace exit; it just writes 1 to an eventfd. It then returns to the guest, userspace picks up the eventfd via its poll() loop and services the device. This is already useful for throughput on UP, and the small latency cost (because of the cost of the event loop in the I/O thread, and possibly the cost of waking up the thread) is usually offset by the benefit. But on SMP you get double benefit. Obviously, the kernel doesn't have to spin while userspace does its stuff. On top of this, there is also a latency improvement from ioeventfd, because QEMU processes virtqueue_notify under its big QEMU lock. With ioeventfd, serialized virtqueue processing can be a bottleneck, but it doesn't affect latency. Without ioeventfd it affects the VCPUs' latency and negates a lot of the benefit of Ming Lei's patch. You can try disabling ioeventfd with -global virtio-blk-pci.ioeventfd=off on the QEMU command line. Performance will plummet. :) Paolo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-06-01 19:23, Rusty Russell wrote: Jens Axboe writes: On 2014-05-30 00:10, Rusty Russell wrote: Jens Axboe writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? I agree that there's sometimes an unfortunate trend there. I didn't check, but my assumption was that this is a regression after the blk-mq conversion, in which case I do think it belongs in stable. No, it's always been that way. In the original driver the entire "issue requests" function was under the lock. > > It was your mq conversion which made this optimization possible, and > also made it an optimization: now other the queues can continue while > this one is going. Ah, I stand corrected, you are right. I had this recollection that the prepare and kick where separate before as well, but apparently just bad memory. But in any case, I think the patch is obviously correct and the wins are sufficiently large to warrant a stable inclusion even if it isn't a regression. If you're running SMP under an emulator where exits are expensive, then this wins. Under KVM it's marginal at best. Locking changes which are "obviously correct" make me nervous, too :) I tend to agree. But I think this one is simple enough to warrant doing it, when the performance increase is as large as it is. But IIRC last KS the argument is that not *enough* is going into stable, not that stable isn't stable enough. So maybe it's a non-problem? In principle, pushing the patch to stable definitely isn't an issue with the stable crew. And yes, they apparently do want more stuff. If you look at it from the distro side, having a stable(r) repository is a no brainer. And they'd want to pick this patch anyway, so... -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Mon, Jun 2, 2014 at 9:23 AM, Rusty Russell wrote: > Jens Axboe writes: >> On 2014-05-30 00:10, Rusty Russell wrote: >>> Jens Axboe writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. >>> >>> Really stable? It improves performance, which is nice. But every patch >>> which goes into the kernel fixes a bug, improves clarity, improves >>> performance or adds a feature. I've now seen all four cases get CC'd >>> into stable. >>> >>> Including some of mine explicitly not marked stable which get swept up >>> by enthusiastic stable maintainers :( >>> >>> Is now there *any* patch short of a major rewrite which shouldn't get >>> cc: stable? >> >> I agree that there's sometimes an unfortunate trend there. I didn't >> check, but my assumption was that this is a regression after the blk-mq >> conversion, in which case I do think it belongs in stable. > > No, it's always been that way. In the original driver the entire "issue > requests" function was under the lock. > > It was your mq conversion which made this optimization possible, and > also made it an optimization: now other the queues can continue while > this one is going. > >> But in any case, I think the patch is obviously correct and the wins are >> sufficiently large to warrant a stable inclusion even if it isn't a >> regression. > > If you're running SMP under an emulator where exits are expensive, then > this wins. Under KVM it's marginal at best. Both my tests on arm64 and x86 are under KVM, and looks the patch can improve performance a lot. IMO, even though under KVM, virtio-blk performance still depends how well hypervisor( qemu, ...) emulates the device, and basically speaking, it is expensive to switch from guest to host and let host handle the notification. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Mon, Jun 2, 2014 at 9:23 AM, Rusty Russell ru...@rustcorp.com.au wrote: Jens Axboe ax...@kernel.dk writes: On 2014-05-30 00:10, Rusty Russell wrote: Jens Axboe ax...@kernel.dk writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? I agree that there's sometimes an unfortunate trend there. I didn't check, but my assumption was that this is a regression after the blk-mq conversion, in which case I do think it belongs in stable. No, it's always been that way. In the original driver the entire issue requests function was under the lock. It was your mq conversion which made this optimization possible, and also made it an optimization: now other the queues can continue while this one is going. But in any case, I think the patch is obviously correct and the wins are sufficiently large to warrant a stable inclusion even if it isn't a regression. If you're running SMP under an emulator where exits are expensive, then this wins. Under KVM it's marginal at best. Both my tests on arm64 and x86 are under KVM, and looks the patch can improve performance a lot. IMO, even though under KVM, virtio-blk performance still depends how well hypervisor( qemu, ...) emulates the device, and basically speaking, it is expensive to switch from guest to host and let host handle the notification. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-06-01 19:23, Rusty Russell wrote: Jens Axboe ax...@kernel.dk writes: On 2014-05-30 00:10, Rusty Russell wrote: Jens Axboe ax...@kernel.dk writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? I agree that there's sometimes an unfortunate trend there. I didn't check, but my assumption was that this is a regression after the blk-mq conversion, in which case I do think it belongs in stable. No, it's always been that way. In the original driver the entire issue requests function was under the lock. It was your mq conversion which made this optimization possible, and also made it an optimization: now other the queues can continue while this one is going. Ah, I stand corrected, you are right. I had this recollection that the prepare and kick where separate before as well, but apparently just bad memory. But in any case, I think the patch is obviously correct and the wins are sufficiently large to warrant a stable inclusion even if it isn't a regression. If you're running SMP under an emulator where exits are expensive, then this wins. Under KVM it's marginal at best. Locking changes which are obviously correct make me nervous, too :) I tend to agree. But I think this one is simple enough to warrant doing it, when the performance increase is as large as it is. But IIRC last KS the argument is that not *enough* is going into stable, not that stable isn't stable enough. So maybe it's a non-problem? In principle, pushing the patch to stable definitely isn't an issue with the stable crew. And yes, they apparently do want more stuff. If you look at it from the distro side, having a stable(r) repository is a no brainer. And they'd want to pick this patch anyway, so... -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Jens Axboe writes: > On 2014-05-30 00:10, Rusty Russell wrote: >> Jens Axboe writes: >>> If Rusty agrees, I'd like to add it for 3.16 with a stable marker. >> >> Really stable? It improves performance, which is nice. But every patch >> which goes into the kernel fixes a bug, improves clarity, improves >> performance or adds a feature. I've now seen all four cases get CC'd >> into stable. >> >> Including some of mine explicitly not marked stable which get swept up >> by enthusiastic stable maintainers :( >> >> Is now there *any* patch short of a major rewrite which shouldn't get >> cc: stable? > > I agree that there's sometimes an unfortunate trend there. I didn't > check, but my assumption was that this is a regression after the blk-mq > conversion, in which case I do think it belongs in stable. No, it's always been that way. In the original driver the entire "issue requests" function was under the lock. It was your mq conversion which made this optimization possible, and also made it an optimization: now other the queues can continue while this one is going. > But in any case, I think the patch is obviously correct and the wins are > sufficiently large to warrant a stable inclusion even if it isn't a > regression. If you're running SMP under an emulator where exits are expensive, then this wins. Under KVM it's marginal at best. Locking changes which are "obviously correct" make me nervous, too :) But IIRC last KS the argument is that not *enough* is going into stable, not that stable isn't stable enough. So maybe it's a non-problem? Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Jens Axboe ax...@kernel.dk writes: On 2014-05-30 00:10, Rusty Russell wrote: Jens Axboe ax...@kernel.dk writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? I agree that there's sometimes an unfortunate trend there. I didn't check, but my assumption was that this is a regression after the blk-mq conversion, in which case I do think it belongs in stable. No, it's always been that way. In the original driver the entire issue requests function was under the lock. It was your mq conversion which made this optimization possible, and also made it an optimization: now other the queues can continue while this one is going. But in any case, I think the patch is obviously correct and the wins are sufficiently large to warrant a stable inclusion even if it isn't a regression. If you're running SMP under an emulator where exits are expensive, then this wins. Under KVM it's marginal at best. Locking changes which are obviously correct make me nervous, too :) But IIRC last KS the argument is that not *enough* is going into stable, not that stable isn't stable enough. So maybe it's a non-problem? Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Fri, May 30, 2014 at 10:49:29AM +0800, Ming Lei wrote: > Firstly, it isn't necessary to hold lock of vblk->vq_lock > when notifying hypervisor about queued I/O. > > Secondly, virtqueue_notify() will cause world switch and > it may take long time on some hypervisors(such as, qemu-arm), > so it isn't good to hold the lock and block other vCPUs. > > On arm64 quad core VM(qemu-kvm), the patch can increase I/O > performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: > - without the patch: 14K IOPS > - with the patch: 34K IOPS > > fio script: > [global] > direct=1 > bsrange=4k-4k > timeout=10 > numjobs=4 > ioengine=libaio > iodepth=64 > > filename=/dev/vdc > group_reporting=1 > > [f1] > rw=randread > > Cc: Rusty Russell > Cc: "Michael S. Tsirkin" > Cc: virtualizat...@lists.linux-foundation.org > Signed-off-by: Ming Lei Acked-by: Michael S. Tsirkin > --- > drivers/block/virtio_blk.c |9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 9f340fa..a6f5424 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -162,6 +162,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, > struct request *req) > unsigned int num; > const bool last = (req->cmd_flags & REQ_END) != 0; > int err; > + bool notify = false; > > BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems); > > @@ -214,10 +215,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, > struct request *req) > return BLK_MQ_RQ_QUEUE_ERROR; > } > > - if (last) > - virtqueue_kick(vblk->vq); > - > + if (last && virtqueue_kick_prepare(vblk->vq)) > + notify = true; > spin_unlock_irqrestore(>vq_lock, flags); > + > + if (notify) > + virtqueue_notify(vblk->vq); > return BLK_MQ_RQ_QUEUE_OK; > } > > -- > 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-05-30 00:10, Rusty Russell wrote: Jens Axboe writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? I agree that there's sometimes an unfortunate trend there. I didn't check, but my assumption was that this is a regression after the blk-mq conversion, in which case I do think it belongs in stable. But in any case, I think the patch is obviously correct and the wins are sufficiently large to warrant a stable inclusion even if it isn't a regression. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Ming Lei writes: > Firstly, it isn't necessary to hold lock of vblk->vq_lock > when notifying hypervisor about queued I/O. > > Secondly, virtqueue_notify() will cause world switch and > it may take long time on some hypervisors(such as, qemu-arm), > so it isn't good to hold the lock and block other vCPUs. > > On arm64 quad core VM(qemu-kvm), the patch can increase I/O > performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: > - without the patch: 14K IOPS > - with the patch: 34K IOPS > > fio script: > [global] > direct=1 > bsrange=4k-4k > timeout=10 > numjobs=4 > ioengine=libaio > iodepth=64 > > filename=/dev/vdc > group_reporting=1 > > [f1] > rw=randread > > Cc: Rusty Russell > Cc: "Michael S. Tsirkin" > Cc: virtualizat...@lists.linux-foundation.org > Signed-off-by: Ming Lei Acked-by: Rusty Russell Thanks! Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Jens Axboe writes: > If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Jens Axboe ax...@kernel.dk writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
Ming Lei ming@canonical.com writes: Firstly, it isn't necessary to hold lock of vblk-vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS fio script: [global] direct=1 bsrange=4k-4k timeout=10 numjobs=4 ioengine=libaio iodepth=64 filename=/dev/vdc group_reporting=1 [f1] rw=randread Cc: Rusty Russell ru...@rustcorp.com.au Cc: Michael S. Tsirkin m...@redhat.com Cc: virtualizat...@lists.linux-foundation.org Signed-off-by: Ming Lei ming@canonical.com Acked-by: Rusty Russell ru...@rustcorp.com.au Thanks! Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-05-30 00:10, Rusty Russell wrote: Jens Axboe ax...@kernel.dk writes: If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Really stable? It improves performance, which is nice. But every patch which goes into the kernel fixes a bug, improves clarity, improves performance or adds a feature. I've now seen all four cases get CC'd into stable. Including some of mine explicitly not marked stable which get swept up by enthusiastic stable maintainers :( Is now there *any* patch short of a major rewrite which shouldn't get cc: stable? I agree that there's sometimes an unfortunate trend there. I didn't check, but my assumption was that this is a regression after the blk-mq conversion, in which case I do think it belongs in stable. But in any case, I think the patch is obviously correct and the wins are sufficiently large to warrant a stable inclusion even if it isn't a regression. -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Fri, May 30, 2014 at 10:49:29AM +0800, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk-vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS fio script: [global] direct=1 bsrange=4k-4k timeout=10 numjobs=4 ioengine=libaio iodepth=64 filename=/dev/vdc group_reporting=1 [f1] rw=randread Cc: Rusty Russell ru...@rustcorp.com.au Cc: Michael S. Tsirkin m...@redhat.com Cc: virtualizat...@lists.linux-foundation.org Signed-off-by: Ming Lei ming@canonical.com Acked-by: Michael S. Tsirkin m...@redhat.com --- drivers/block/virtio_blk.c |9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 9f340fa..a6f5424 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -162,6 +162,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req) unsigned int num; const bool last = (req-cmd_flags REQ_END) != 0; int err; + bool notify = false; BUG_ON(req-nr_phys_segments + 2 vblk-sg_elems); @@ -214,10 +215,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req) return BLK_MQ_RQ_QUEUE_ERROR; } - if (last) - virtqueue_kick(vblk-vq); - + if (last virtqueue_kick_prepare(vblk-vq)) + notify = true; spin_unlock_irqrestore(vblk-vq_lock, flags); + + if (notify) + virtqueue_notify(vblk-vq); return BLK_MQ_RQ_QUEUE_OK; } -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Fri, May 30, 2014 at 11:35 AM, Jens Axboe wrote: > On 2014-05-29 21:34, Ming Lei wrote: >> >> On Fri, May 30, 2014 at 11:19 AM, Jens Axboe wrote: >>> >>> On 2014-05-29 20:49, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk->vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS >>> >>> >>> >>> Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it >>> definitely makes sense and I can see it hurting in other places. >> >> >> It isn't easy to observe the improvement on x86 VM, especially >> with few vCPUs, because qemu-system-x86_64 only takes >> several microseconds to handle the notification, but on arm64, it >> may take hundreds of microseconds, so the improvement is >> obvious on arm VM. >> >> I hope this patch can be merged, at least arm VM can benefit >> from it. > > > If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Interesting, even on x86, I still can observe the improvement when the numjobs is set as 2 in the fio script(see commit log), but when numjobs is set as 4, 8, 12, the difference isn't obvious between patched kernel and non-patched kernel. 1, environment - host: 2sockets, each CPU(4cores, 2 threads), total 16 logical cores - guest: 16cores, 8GB ram - guest kernel: 3.15-rc7-next with patch[1] - fio: the script in commit log with numjobs set as 2 2, result - without the patch: ~104K IOPS - with the patch: ~140K IOPS Rusty, considered the same trick has been applied in virt-scsi, do you agree to take the same approach in virt-blk too? [1], http://marc.info/?l=linux-kernel=140135041423441=2 Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Fri, May 30, 2014 at 11:19 AM, Jens Axboe wrote: > On 2014-05-29 20:49, Ming Lei wrote: >> >> Firstly, it isn't necessary to hold lock of vblk->vq_lock >> when notifying hypervisor about queued I/O. >> >> Secondly, virtqueue_notify() will cause world switch and >> it may take long time on some hypervisors(such as, qemu-arm), >> so it isn't good to hold the lock and block other vCPUs. >> >> On arm64 quad core VM(qemu-kvm), the patch can increase I/O >> performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: >> - without the patch: 14K IOPS >> - with the patch: 34K IOPS > > > Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it > definitely makes sense and I can see it hurting in other places. It isn't easy to observe the improvement on x86 VM, especially with few vCPUs, because qemu-system-x86_64 only takes several microseconds to handle the notification, but on arm64, it may take hundreds of microseconds, so the improvement is obvious on arm VM. I hope this patch can be merged, at least arm VM can benefit from it. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-05-29 21:34, Ming Lei wrote: On Fri, May 30, 2014 at 11:19 AM, Jens Axboe wrote: On 2014-05-29 20:49, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk->vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it definitely makes sense and I can see it hurting in other places. It isn't easy to observe the improvement on x86 VM, especially with few vCPUs, because qemu-system-x86_64 only takes several microseconds to handle the notification, but on arm64, it may take hundreds of microseconds, so the improvement is obvious on arm VM. I hope this patch can be merged, at least arm VM can benefit from it. If Rusty agrees, I'd like to add it for 3.16 with a stable marker. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-05-29 20:49, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk->vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it definitely makes sense and I can see it hurting in other places. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-05-29 20:49, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk-vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it definitely makes sense and I can see it hurting in other places. -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Fri, May 30, 2014 at 11:19 AM, Jens Axboe ax...@kernel.dk wrote: On 2014-05-29 20:49, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk-vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it definitely makes sense and I can see it hurting in other places. It isn't easy to observe the improvement on x86 VM, especially with few vCPUs, because qemu-system-x86_64 only takes several microseconds to handle the notification, but on arm64, it may take hundreds of microseconds, so the improvement is obvious on arm VM. I hope this patch can be merged, at least arm VM can benefit from it. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On 2014-05-29 21:34, Ming Lei wrote: On Fri, May 30, 2014 at 11:19 AM, Jens Axboe ax...@kernel.dk wrote: On 2014-05-29 20:49, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk-vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it definitely makes sense and I can see it hurting in other places. It isn't easy to observe the improvement on x86 VM, especially with few vCPUs, because qemu-system-x86_64 only takes several microseconds to handle the notification, but on arm64, it may take hundreds of microseconds, so the improvement is obvious on arm VM. I hope this patch can be merged, at least arm VM can benefit from it. If Rusty agrees, I'd like to add it for 3.16 with a stable marker. -- Jens Axboe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] block: virtio_blk: don't hold spin lock during world switch
On Fri, May 30, 2014 at 11:35 AM, Jens Axboe ax...@kernel.dk wrote: On 2014-05-29 21:34, Ming Lei wrote: On Fri, May 30, 2014 at 11:19 AM, Jens Axboe ax...@kernel.dk wrote: On 2014-05-29 20:49, Ming Lei wrote: Firstly, it isn't necessary to hold lock of vblk-vq_lock when notifying hypervisor about queued I/O. Secondly, virtqueue_notify() will cause world switch and it may take long time on some hypervisors(such as, qemu-arm), so it isn't good to hold the lock and block other vCPUs. On arm64 quad core VM(qemu-kvm), the patch can increase I/O performance a lot with VIRTIO_RING_F_EVENT_IDX enabled: - without the patch: 14K IOPS - with the patch: 34K IOPS Patch looks good to me. I don't see a hit on my qemu-kvm testing, but it definitely makes sense and I can see it hurting in other places. It isn't easy to observe the improvement on x86 VM, especially with few vCPUs, because qemu-system-x86_64 only takes several microseconds to handle the notification, but on arm64, it may take hundreds of microseconds, so the improvement is obvious on arm VM. I hope this patch can be merged, at least arm VM can benefit from it. If Rusty agrees, I'd like to add it for 3.16 with a stable marker. Interesting, even on x86, I still can observe the improvement when the numjobs is set as 2 in the fio script(see commit log), but when numjobs is set as 4, 8, 12, the difference isn't obvious between patched kernel and non-patched kernel. 1, environment - host: 2sockets, each CPU(4cores, 2 threads), total 16 logical cores - guest: 16cores, 8GB ram - guest kernel: 3.15-rc7-next with patch[1] - fio: the script in commit log with numjobs set as 2 2, result - without the patch: ~104K IOPS - with the patch: ~140K IOPS Rusty, considered the same trick has been applied in virt-scsi, do you agree to take the same approach in virt-blk too? [1], http://marc.info/?l=linux-kernelm=140135041423441w=2 Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
