Re: [PATCH V2 for-4.21 2/2] blk-mq: alloc q->queue_ctx as normal array

On Sat, Nov 17, 2018 at 11:03:42AM +0100, Greg Kroah-Hartman wrote:

On Sat, Nov 17, 2018 at 10:34:18AM +0800, Ming Lei wrote:

quoted

On Fri, Nov 16, 2018 at 06:06:23AM -0800, Greg Kroah-Hartman wrote:

quoted

On Fri, Nov 16, 2018 at 07:23:11PM +0800, Ming Lei wrote:

quoted

Now q->queue_ctx is just one read-mostly table for query the
'blk_mq_ctx' instance from one cpu index, it isn't necessary
to allocate it as percpu variable. One simple array may be
more efficient.

"may be", have you run benchmarks to be sure?  If so, can you add the
results of them to this changelog?  If there is no measurable
difference, then why make this change at all?

__blk_mq_get_ctx() is used in fast path, what do you think about which
one is more efficient?

- *per_cpu_ptr(q->queue_ctx, cpu);

- q->queue_ctx[cpu]

You need to actually test to see which one is faster, you might be
surprised :)

In other words, do not just guess.

No performance difference is observed wrt. this patchset when I
run the following fio test on null_blk(modprobe null_blk) in my VM:

fio --direct=1 --size=128G --bsrange=4k-4k --runtime=40 --numjobs=32 \
  --ioengine=libaio --iodepth=64 --group_reporting=1 --filename=/dev/nullb0 \
  --name=null_blk-ttest-randread --rw=randread

Running test is important, but IMO it is more important to understand
the idea behind is correct, or the approach can be proved as correct.

Given the count of test cases can be increased exponentially when the related
factors or settings are covered, obviously we can't run all the test cases.

quoted

At least the latter isn't worse than the former.

How do you know?

As I explained, q->queue_ctx is basically read-only loop-up table after
queue is initialized, and there isn't any benefit to use percpu
allocator here.

quoted

Especially q->queue_ctx is just a read-only look-up table, it doesn't
make sense to make it percpu any more.

Not mention q->queue_ctx[cpu] is more clean/readable.

Again, please test to verify this.

IMO, 'test' can't be enough to verify if one approach is correct,
or patch is correct given we can't cover all cases by test, and it should be
served as a supplement for proof or patch analysis, seems it is usually done
in patch commit log, or review. 


Thanks,
Ming