Re: [PATCH V8 07/11] blk-mq: stop to handle IO and drain IO before hctx becomes inactive

[Ming Lei <ming.lei@redhat.com>]

On Wed, May 06, 2020 at 10:56:10AM +0100, Will Deacon wrote:
> On Wed, May 06, 2020 at 04:07:27PM +0800, Ming Lei wrote:
> > On Wed, May 06, 2020 at 08:28:03AM +0100, Will Deacon wrote:
> > > On Wed, May 06, 2020 at 09:24:25AM +0800, Ming Lei wrote:
> > > > On Tue, May 05, 2020 at 05:46:18PM +0200, Christoph Hellwig wrote:
> > > > > On Thu, Apr 30, 2020 at 10:02:54PM +0800, Ming Lei wrote:
> > > > > > BLK_MQ_S_INACTIVE is only set when the last cpu of this hctx is becoming
> > > > > > offline, and blk_mq_hctx_notify_offline() is called from cpu hotplug
> > > > > > handler. So if there is any request of this hctx submitted from somewhere,
> > > > > > it has to this last cpu. That is done by blk-mq's queue mapping.
> > > > > > 
> > > > > > In case of direct issue, basically blk_mq_get_driver_tag() is run after
> > > > > > the request is allocated, that is why I mentioned the chance of
> > > > > > migration is very small.
> > > > > 
> > > > > "very small" does not cut it, it has to be zero.  And it seems the
> > > > > new version still has this hack.
> > > > 
> > > > But smp_mb() is used for ordering the WRITE and READ, so it is correct.
> > > > 
> > > > barrier() is enough when process migration doesn't happen.
> > > 
> > > Without numbers I would just make the smp_mb() unconditional. Your
> > > questionable optimisation trades that for a load of the CPU ID and a
> > > conditional branch, which isn't obviously faster to me. It's also very
> > 
> > The CPU ID is just percpu READ, and unlikely() has been used for
> > optimizing the conditional branch. And smp_mb() could cause CPU stall, I
> > guess, so it should be much slower than reading CPU ID.
> 
> Percpu accesses aren't uniformly cheap across architectures.

I believe percpu access is cheap enough than smp_mb() in almost
every SMP ARCH, otherwise no one would use percpu variable on that
ARCH.

> 
> > Let's see the attached microbench[1], the result shows that smp_mb() is
> > 10+ times slower than smp_processor_id() with one conditional branch.
> 
> Nobody said anything about smp_mb() in a tight loop, so this is hardly
> surprising. Throughput of barrier instructions will hit a ceiling fairly
> quickly, but they don't have to cause stalls in general use. I would expect
> the numbers to converge if you added some back-off to the loops (e.g.
> ndelay() or something). But I was really hoping for some numbers from the
> block layer itself, since that's what we actually care about.

I believe that the microbench is enough to show smp_mb() is much heavier
and slower than smp_processor_id() with conditional branch.

Cause some aio or io uring workload just takes CPU to submit IO without
any delay, and we don't want to take extra CPU unnecessarily in IO
submission side. And IOPS may reach millions or dozens of millions
level. Storage guys have been working very hard to optimize the whole
IO path.

> 
> > [    1.239951] test_foo: smp_mb 738701907 smp_id 62904315 result 0 overflow 5120
> > 
> > The micronbench is run on simple 8cores KVM guest, and cpu is
> > 'Model name:          Intel(R) Core(TM) i7-8665U CPU @ 1.90GHz'.
> > 
> > Result is pretty stable in my 5 runs of VM boot.
> 
> Honestly, I get the impression that you're not particularly happy with me
> putting in the effort to review your patches, so I'll leave it up to
> Christoph as to whether he wants to predicate the concurrency design on
> a hokey microbenchmark.
> 
> FWIW: I agree that the code should work as you have it in v10, I just think
> it's unnecessarily complicated and fragile.

Yeah, it works and it is correct, and we can document the usage, another point
is that CPU hotplug doesn't happen frequently, so we shouldn't introduce extra
cost for handling cpu hotplug in fast IO path, meantime smp_mb() won't
be something which can be ignored, especially in some big machine with
lots of CPU cores.

Thanks,
Ming