Jens Axboe <[email protected]> writes:
> On 2014-05-30 00:10, Rusty Russell wrote:
>> Jens Axboe <[email protected]> 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.
On Mon, Jun 2, 2014 at 9:23 AM, Rusty Russell <[email protected]> wrote:
> Jens Axboe <[email protected]> writes:
>> On 2014-05-30 00:10, Rusty Russell wrote:
>>> Jens Axboe <[email protected]> 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
On 2014-06-01 19:23, Rusty Russell wrote:
> Jens Axboe <[email protected]> writes:
>> On 2014-05-30 00:10, Rusty Russell wrote:
>>> Jens Axboe <[email protected]> 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
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