On 06/30/2017 10:59 PM, Jens Axboe wrote:

On 06/30/2017 10:17 PM, Jens Axboe wrote:

quoted

On 06/30/2017 08:08 AM, Jens Axboe wrote:

quoted

On 06/30/2017 07:05 AM, Brian King wrote:

quoted

On 06/29/2017 09:17 PM, Jens Axboe wrote:

quoted

On 06/29/2017 07:20 PM, Ming Lei wrote:

quoted

On Fri, Jun 30, 2017 at 2:42 AM, Jens Axboe [off-list ref] wrote:

quoted

On 06/29/2017 10:00 AM, Jens Axboe wrote:

quoted

On 06/29/2017 09:58 AM, Jens Axboe wrote:

quoted

On 06/29/2017 02:40 AM, Ming Lei wrote:

quoted

On Thu, Jun 29, 2017 at 5:49 AM, Jens Axboe [off-list ref] wrote:

quoted

On 06/28/2017 03:12 PM, Brian King wrote:

quoted

This patch converts the in_flight counter in struct hd_struct from a
pair of atomics to a pair of percpu counters. This eliminates a couple
of atomics from the hot path. When running this on a Power system, to
a single null_blk device with 80 submission queues, irq mode 0, with
80 fio jobs, I saw IOPs go from 1.5M IO/s to 11.4 IO/s.

This has been done before, but I've never really liked it. The reason is
that it means that reading the part stat inflight count now has to
iterate over every possible CPU. Did you use partitions in your testing?
How many CPUs were configured? When I last tested this a few years ago
on even a quad core nehalem (which is notoriously shitty for cross-node
latencies), it was a net loss.

One year ago, I saw null_blk's IOPS can be decreased to 10%
of non-RQF_IO_STAT on a dual socket ARM64(each CPU has
96 cores, and dual numa nodes) too, the performance can be
recovered basically if per numa-node counter is introduced and
used in this case, but the patch was never posted out.
If anyone is interested in that, I can rebase the patch on current
block tree and post out. I guess the performance issue might be
related with system cache coherency implementation more or less.
This issue on ARM64 can be observed with the following userspace
atomic counting test too:

       http://kernel.ubuntu.com/~ming/test/cache/

How well did the per-node thing work? Doesn't seem to me like it would
go far enough. And per CPU is too much. One potential improvement would
be to change the part_stat_read() to just loop online CPUs, instead of
all possible CPUs. When CPUs go on/offline, use that as the slow path to
ensure the stats are sane. Often there's a huge difference between
NR_CPUS configured and what the system has. As Brian states, RH ships
with 2048, while I doubt a lot of customers actually run that...

Outside of coming up with a more clever data structure that is fully
CPU topology aware, one thing that could work is just having X cache
line separated read/write inflight counters per node, where X is some
suitable value (like 4). That prevents us from having cross node
traffic, and it also keeps the cross cpu traffic fairly low. That should
provide a nice balance between cost of incrementing the inflight
counting, and the cost of looping for reading it.

And that brings me to the next part...

quoted

quoted

I do agree that we should do something about it, and it's one of those
items I've highlighted in talks about blk-mq on pending issues to fix
up. It's just not great as it currently stands, but I don't think per
CPU counters is the right way to fix it, at least not for the inflight
counter.

Yeah, it won't be a issue for non-mq path, and for blk-mq path, maybe
we can use some blk-mq knowledge(tagset?) to figure out the
'in_flight' counter. I thought about it before, but never got a
perfect solution, and looks it is a bit hard, :-)

The tags are already a bit spread out, so it's worth a shot. That would
remove the need to do anything in the inc/dec path, as the tags already
do that. The inlight count could be easily retrieved with
sbitmap_weight(). The only issue here is that we need separate read and
write counters, and the weight would obviously only get us the total
count. But we can have a slower path for that, just iterate the tags and
count them. The fast path only cares about total count.

Let me try that out real quick.

Well, that only works for whole disk stats, of course... There's no way
around iterating the tags and checking for this to truly work.

Totally untested proof of concept for using the tags for this. I based
this on top of Brian's patch, so it includes his patch plus the
_double() stuff I did which is no longer really needed.

diff --git a/block/bio.c b/block/bio.c
index 9cf98b29588a..ec99d9ba0f33 100644
--- a/block/bio.c
+++ b/block/bio.c

@@ -1737,7 +1737,7 @@ void generic_start_io_acct(int rw, unsigned long sectors,
        part_round_stats(cpu, part);
        part_stat_inc(cpu, part, ios[rw]);
        part_stat_add(cpu, part, sectors[rw], sectors);
-       part_inc_in_flight(part, rw);
+       part_inc_in_flight(cpu, part, rw);

        part_stat_unlock();
 }

@@ -1751,7 +1751,7 @@ void generic_end_io_acct(int rw, struct hd_struct *part,

        part_stat_add(cpu, part, ticks[rw], duration);
        part_round_stats(cpu, part);
-       part_dec_in_flight(part, rw);
+       part_dec_in_flight(cpu, part, rw);

        part_stat_unlock();
 }

diff --git a/block/blk-core.c b/block/blk-core.c
index af393d5a9680..6ab2efbe940b 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -2434,8 +2434,13 @@ void blk_account_io_done(struct request *req)

                part_stat_inc(cpu, part, ios[rw]);
                part_stat_add(cpu, part, ticks[rw], duration);
-               part_round_stats(cpu, part);
-               part_dec_in_flight(part, rw);
+
+               if (req->q->mq_ops)
+                       part_round_stats_mq(req->q, cpu, part);
+               else {
+                       part_round_stats(cpu, part);
+                       part_dec_in_flight(cpu, part, rw);
+               }

                hd_struct_put(part);
                part_stat_unlock();

@@ -2492,8 +2497,12 @@ void blk_account_io_start(struct request *rq, bool new_io)
                        part = &rq->rq_disk->part0;
                        hd_struct_get(part);
                }
-               part_round_stats(cpu, part);
-               part_inc_in_flight(part, rw);
+               if (rq->q->mq_ops)
+                       part_round_stats_mq(rq->q, cpu, part);
+               else {
+                       part_round_stats(cpu, part);
+                       part_inc_in_flight(cpu, part, rw);
+               }
                rq->part = part;
        }

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 99038830fb42..3b5eb2d4b964 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -634,7 +634,7 @@ static void blk_account_io_merge(struct request *req)
                part = req->part;

                part_round_stats(cpu, part);
-               part_dec_in_flight(part, rq_data_dir(req));
+               part_dec_in_flight(cpu, part, rq_data_dir(req));

                hd_struct_put(part);
                part_stat_unlock();

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index d0be72ccb091..a7b897740c47 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c

@@ -214,7 +214,7 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
                bitnr += tags->nr_reserved_tags;
        rq = tags->rqs[bitnr];

-       if (rq->q == hctx->queue)
+       if (rq && rq->q == hctx->queue)
                iter_data->fn(hctx, rq, iter_data->data, reserved);
        return true;
 }

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 05dfa3f270ae..cad4d2c26285 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c

@@ -43,6 +43,58 @@ static LIST_HEAD(all_q_list);
 static void blk_mq_poll_stats_start(struct request_queue *q);
 static void blk_mq_poll_stats_fn(struct blk_stat_callback *cb);

+struct mq_inflight {
+       struct hd_struct *part;
+       unsigned int inflight;
+};
+
+static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
+                                 struct request *rq, void *priv,
+                                 bool reserved)
+{
+       struct mq_inflight *mi = priv;
+
+       if (rq->part == mi->part &&
+           test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
+               mi->inflight++;
+}
+
+unsigned long part_in_flight_mq(struct request_queue *q,
+                               struct hd_struct *part)
+{
+       struct mq_inflight mi = { .part = part, .inflight = 0 };
+
+       blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi);
+       return mi.inflight;
+}

Compared with the totally percpu approach, this way might help 1:M or
N:M mapping, but won't help 1:1 map(NVMe), when hctx is mapped to
each CPU(especially there are huge hw queues on a big system), :-(

Not disagreeing with that, without having some mechanism to only
loop queues that have pending requests. That would be similar to the
ctx_map for sw to hw queues. But I don't think that would be worthwhile
doing, I like your pnode approach better. However, I'm still not fully
convinced that one per node is enough to get the scalability we need.

Would be great if Brian could re-test with your updated patch, so we
know how it works for him at least.

I'll try running with both approaches today and see how they compare.

Focus on Ming's, a variant of that is the most likely path forward,
imho. It'd be great to do a quick run on mine as well, just to establish
how it compares to mainline, though.

Here's a cleaned up series:

http://git.kernel.dk/cgit/linux-block/log/?h=mq-inflight

(it's against mainline for now, I will update it to be against
for-4.13/block in a rebase).

One optimization on top of this I want to do is to only iterate once,
even for a partition - pass in both parts, and increment two different
counts. If we collapse the two part time stamps, then that's doable, and
it means we only have to iterate once.

Essentially this series makes the inc/dec a noop, since we don't have to
do anything. The reading is basically no worse than a cpu online
iteration, since we never have more queues than online CPUs. That's an
improvement over per-cpu for-each-possible loops. For a lot of cases,
it's much less, since we have fewer queues than CPUs. I'll need an hour
or two to hone this a bit more, but then it would be great if you can
retest. I'll send out an email when that's done, it'll be some time over
this weekend.

Did the double-read with one iteration change, it was pretty trivial:

http://git.kernel.dk/cgit/linux-block/commit/?h=mq-inflight&id=b841804f826df072f706ae86d0eb533342f0297a

Now:

http://git.kernel.dk/cgit/linux-block/commit/?h=mq-inflight&id=87f73ef2b9edb6834001df8f7cb48c7a116e8cd3

And updated the branch here:

http://git.kernel.dk/cgit/linux-block/log/?h=mq-inflight

to include that, and be based on top of for-4.13/block. If you prefer just
pulling a branch, pull:

git://git.kernel.dk/linux-block mq-inflight

I've tested it, and made a few adjustments and fixes. The branches are
the same, just rebased. Works fine for me, even with partitions now.

Let me know how it works for you.

-- 
Jens Axboe