[Performance]
1. One thread (numjobs=1) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 208k | 279k | ~34%
2. Three threads (numjobs=3) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 615k | 728k | ~18%
nvme | 728k | 873k | ~20%
The result shows that the performance gain of bio-based polling is
comparable as that of mq polling in the same test case.
3. Three threads (numjobs=3) randread (bs=12k, direct=1) one
**dm-stripe** device, which is built upon 3 nvme devices, with one
polling hw queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 314k | 354k | ~13%
nvme | 728k | 873k | ~20%
4. This patchset shall do no harm to the performance of the original mq
polling. Following is the test results of one thread (numjobs=1)
randread (bs=4k, direct=1) one nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---------------- | --------------- | -------------------- | ----
without patchset | 242k | 332k | ~39%
with patchset | 236k | 332k | ~39%
[Changes since v2]
Patchset v2 caches all hw queues (in polling mode) of underlying mq
devices in dm layer. The polling routine actually iterates through all
these cached hw queues.
However, mq may change the queue mapping at runtime (e.g., NVMe RESET
command), thus the cached hw queues in dm layer may be out-of-date. Thus
patchset v3 falls back to the implementation of the very first RFC
version, in which the mq layer needs to export one interface iterating
all polling hw queues (patch 5), and the bio-based polling routine just
calls this interface to iterate all polling hw queues.
Besides, several new optimization is proposed.
- patch 1,2,7
same as v2, untouched
- patch 3
Considering advice from Christoph Hellwig, while refactoring blk_poll(),
split mq and bio-based polling routine from the very beginning. Now
blk_poll() is just a simple entry. blk_bio_poll() is simply copied from
blk_mq_poll(), while the loop structure is some sort of duplication
though.
- patch 4
This patch is newly added to support turning on/off polling through
'/sys/block/<dev>/queue/io_poll' dynamiclly for bio-based devices.
Patchset v2 implemented this functionality by added one new queue flag,
which is not preferred since the queue flag resource is quite short of
nowadays.
- patch 5
This patch is newly added, preparing for the following bio-based
polling. The following bio-based polling will call this helper function,
accounting on the corresponding hw queue.
- patch 6
It's from the very first RFC version, preparing for the following
bio-based polling.
- patch 8
One fixing patch needed by the following bio-based polling. It's
actually a v2 of [1]. I had sent the v2 singly in-reply-to [1], though
it has not been visible on the mailing list maybe due to the delay.
- patch 9
It's from the very first RFC version.
- patch 10
This patch is newly added. Patchset v2 had ever proposed one
optimization that, skipping the **busy** hw queues during the iteration
phase. Back upon that time, one flag of 'atomic_t' is specifically
maintained in dm layer, representing if the corresponding hw queue is
busy or not. The idea is inherited, while the implementation changes.
Now @nvmeq->cq_poll_lock is used directly here, no need for extra flag
anymore.
This optimization can significantly reduce the competition for one hw
queue between multiple polling instances. Following statistics is the
test result when 3 threads concurrently randread (bs=4k, direct=1) one
dm-linear device, which is built upon 3 nvme devices, with one polling
hw queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
----------- | --------------- | -------------------- | ----
without opt | 318k | 256k | ~-20%
with opt | 314k | 354k | ~13%
- patch 11
This is another newly added optimizatin for bio-based polling.
One intuitive insight is that, when the original bio submitted to dm
device doesn't get split, then the bio gets enqueued into only one hw
queue of one of the underlying mq devices. In this case, we no longer
need to track all split bios, and one cookie (for the only split bio)
is enough. It is implemented by returning the pointer to the
corresponding hw queue in this case.
It should be safe by directly returning the pointer to the hw queue,
since 'struct blk_mq_hw_ctx' won't be freed during the whole lifetime of
'struct request_queue'. Even when the number of hw queues may decrease
when NVMe RESET happens, the 'struct request_queue' structure of decreased
hw queues won't be freed, instead it's buffered into
&q->unused_hctx_list list.
Though this optimization seems quite intuitive, the performance test
shows that it does no benefit nor harm to the performance, while 3
threads concurrently randreading (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw queue
per nvme device.
I'm not sure why it doesn't work, maybe because the number of devices,
or the depth of the devcice stack is to low in my test case?
[Remained Issue]
It has been mentioned in patch 4 that, users could change the state of
the underlying devices through '/sys/block/<dev>/io_poll', bypassing
the dm device above. Thus it can cause a situation where QUEUE_FLAG_POLL
is still set for the request_queue of dm device, while one of the
underlying mq device may has cleared this flag.
In this case, it will pass the 'test_bit(QUEUE_FLAG_POLL, &q->queue_flags)'
check in blk_poll(), while the input cookie may actually points to a hw
queue in IRQ mode since patch 11. Thus for this hw queue (in IRQ mode),
the bio-based polling routine will handle this hw queue acquiring
'spin_lock(&nvmeq->cq_poll_lock)' (refer
drivers/nvme/host/pci.c:nvme_poll), which is not adequate since this hw
queue may also be accessed in IRQ context. In other words,
spin_lock_irq() should be used here.
I have not come up one simple way to fix it. I don't want to do sanity
check (e.g., the type of the hw queue is HCTX_TYPE_POLL or not) in the
IO path (submit_bio()/blk_poll()), i.e., fast path.
We'd better fix it in the control path, i.e., dm could be aware of the
change when attribute (e.g., support io_poll or not) of one of the
underlying devices changed at runtime.
[1] https://listman.redhat.com/archives/dm-devel/2021-February/msg00028.html
changes since v1:
- patch 1,2,4 is the same as v1 and have already been reviewed
- patch 3 is refactored a bit on the basis of suggestions from
Mike Snitzer.
- patch 5 is newly added and introduces one new queue flag
representing if the queue is capable of IO polling. This mainly
simplifies the logic in queue_poll_store().
- patch 6 implements the core mechanism supporting IO polling.
The sanity check checking if the dm device supports IO polling is
also folded into this patch, and the queue flag will be cleared if
it doesn't support, in case of table reloading.
Jeffle Xu (11):
block: move definition of blk_qc_t to types.h
block: add queue_to_disk() to get gendisk from request_queue
block: add poll method to support bio-based IO polling
block: add poll_capable method to support bio-based IO polling
block/mq: extract one helper function polling hw queue
block/mq: add iterator for polling hw queues
dm: always return BLK_QC_T_NONE for bio-based device
dm: fix iterate_device sanity check
dm: support IO polling for bio-based dm device
nvme/pci: don't wait for locked polling queue
dm: fastpath of bio-based polling
block/blk-core.c | 89 +++++++++++++-
block/blk-mq.c | 27 +----
block/blk-sysfs.c | 14 ++-
drivers/md/dm-table.c | 215 ++++++++++++++++++----------------
drivers/md/dm.c | 90 +++++++++++---
drivers/md/dm.h | 2 +-
drivers/nvme/host/pci.c | 4 +-
include/linux/blk-mq.h | 21 ++++
include/linux/blk_types.h | 10 +-
include/linux/blkdev.h | 4 +
include/linux/device-mapper.h | 1 +
include/linux/fs.h | 2 +-
include/linux/types.h | 3 +
include/trace/events/kyber.h | 6 +-
14 files changed, 333 insertions(+), 155 deletions(-)
--
2.27.0
So that kiocb.ki_cookie can be defined as blk_qc_t, which will enforce
the encapsulation.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Mike Snitzer <redacted>
---
include/linux/blk_types.h | 2 +-
include/linux/fs.h | 2 +-
include/linux/types.h | 3 +++
3 files changed, 5 insertions(+), 2 deletions(-)
This method can be used to check if bio-based device supports IO polling
or not. For mq devices, checking for hw queue in polling mode is
adequate, while the sanity check shall be implementation specific for
bio-based devices. For example, dm device needs to check if all
underlying devices are capable of IO polling.
Though bio-based device may have done the sanity check during the
device initialization phase, cacheing the result of this sanity check
(such as by cacheing in the queue_flags) may not work. Because for dm
devices, users could change the state of the underlying devices through
'/sys/block/<dev>/io_poll', bypassing the dm device above. In this case,
the cached result of the very beginning sanity check could be
out-of-date. Thus the sanity check needs to be done every time 'io_poll'
is to be modified.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
block/blk-sysfs.c | 14 +++++++++++---
include/linux/blkdev.h | 1 +
2 files changed, 12 insertions(+), 3 deletions(-)
->poll_fn was introduced in commit ea435e1b9392 ("block: add a poll_fn
callback to struct request_queue") to support bio-based queues such as
nvme multipath, but was later removed in commit 529262d56dbe ("block:
remove ->poll_fn").
Given commit c62b37d96b6e ("block: move ->make_request_fn to struct
block_device_operations") restore the possibility of bio-based IO
polling support by adding an ->poll method to gendisk->fops.
Make blk_mq_poll() specific to blk-mq, while blk_bio_poll() is
originally a copy from blk_mq_poll(), and is specific to bio-based
polling.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
block/blk-core.c | 58 ++++++++++++++++++++++++++++++++++++++++++
block/blk-mq.c | 22 +---------------
include/linux/blk-mq.h | 1 +
include/linux/blkdev.h | 1 +
4 files changed, 61 insertions(+), 21 deletions(-)
Sometimes we need to get the corresponding gendisk from request_queue.
It is preferred that block drivers store private data in
gendisk->private_data rather than request_queue->queuedata, e.g. see:
commit c4a59c4e5db3 ("dm: stop using ->queuedata").
So if only request_queue is given, we need to get its corresponding
gendisk to get the private data stored in that gendisk.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
Reviewed-by: Mike Snitzer <redacted>
---
include/linux/blkdev.h | 2 ++
include/trace/events/kyber.h | 6 +++---
2 files changed, 5 insertions(+), 3 deletions(-)
DM will iterate and poll all polling hardware queues of all target mq
devices when polling IO for dm device. To mitigate the race introduced
by iterating all target hw queues, a per-hw-queue flag is maintained
to indicate whether this polling hw queue currently being polled on or
not. Every polling hw queue is exclusive to one polling instance, i.e.,
the polling instance will skip this polling hw queue if this hw queue
currently is being polled by another polling instance, and start
polling on the next hw queue.
IO polling is enabled when all underlying target devices are capable
of IO polling. The sanity check supports the stacked device model, in
which one dm device may be build upon another dm device. In this case,
the mapped device will check if the underlying dm target device
supports IO polling.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
drivers/md/dm-table.c | 26 ++++++++++++++
drivers/md/dm.c | 64 +++++++++++++++++++++++++++++++++++
include/linux/device-mapper.h | 1 +
3 files changed, 91 insertions(+)
Offer one fastpath of bio-based polling when bio submitted to dm device
is not split.
In this case, there will be only one bio submitted to only one polling
hw queue of one underlying mq device, and thus we don't need to track
all split bios or iterate through all polling hw queues. The pointer to
the polling hw queue the bio submitted to is returned here as the
returned cookie. In this case, the polling routine will call
mq_ops->poll() directly with the hw queue converted from the input
cookie.
If the original bio submitted to dm device is split to multiple bios and
thus submitted to multiple polling hw queues, the bio submission routine
will return BLK_QC_T_BIO_MULTI, while the polling routine will fall
back to iterating all hw queues (in polling mode) of all underlying mq
devices.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
block/blk-core.c | 33 +++++++++++++++++++++++++++++++--
include/linux/blk_types.h | 8 ++++++++
include/linux/types.h | 2 +-
3 files changed, 40 insertions(+), 3 deletions(-)
@@ -956,11 +956,19 @@ static blk_qc_t __submit_bio(struct bio *bio)*bio_list_on_stack[0]containsbiossubmittedbythecurrent->submit_bio.*bio_list_on_stack[1]containsbiosthatweresubmittedbeforethecurrent*->submit_bio_bio,butthathaven'tbeenprocessedyet.+*+*Return:+*-BLK_QC_T_NONE,noneedforIOpolling.+*-BLK_QC_T_BIO_MULTI,@biogetssplitandenqueuedintomultihwqueues.+*-Otherwise,@bioisnotsplit,returningthepointertothecorresponding+*hwqueuethatthebioenqueuedintoasthereturnedcookie.*/staticblk_qc_t__submit_bio_noacct(structbio*bio){structbio_listbio_list_on_stack[2];blk_qc_tret=BLK_QC_T_NONE;+structrequest_queue*top_q=bio->bi_disk->queue;+boolpoll_on=test_bit(QUEUE_FLAG_POLL,&top_q->queue_flags);BUG_ON(bio->bi_next);
@@ -968,6 +976,7 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)current->bio_list=bio_list_on_stack;do{+blk_qc_tcookie;structrequest_queue*q=bio->bi_disk->queue;structbio_listlower,same;
@@ -980,7 +989,20 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)bio_list_on_stack[1]=bio_list_on_stack[0];bio_list_init(&bio_list_on_stack[0]);-ret=__submit_bio(bio);+cookie=__submit_bio(bio);++if(poll_on&&+blk_qc_t_bio_valid(ret)&&blk_qc_t_valid(cookie)){+unsignedintqueue_num=blk_qc_t_to_queue_num(cookie);+structblk_mq_hw_ctx*hctx=q->queue_hw_ctx[queue_num];++cookie=(blk_qc_t)hctx;++if(!blk_qc_t_valid(ret))/* set initial value */+ret=cookie;+elseif(ret!=cookie)/* bio got split */+ret=BLK_QC_T_BIO_MULTI;+}/**Sortnewbiosintothoseforalowerlevelandthoseforthe
@@ -1003,6 +1025,7 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)}while((bio=bio_list_pop(&bio_list_on_stack[0])));current->bio_list=NULL;+returnret;}
Extract the logic of polling one hw queue and related statistics
handling out as the helper function.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
block/blk-mq.c | 5 +----
include/linux/blk-mq.h | 13 +++++++++++++
2 files changed, 14 insertions(+), 4 deletions(-)
Currently the returned cookie of bio-based device is not used at all.
Cookie of bio-based device will be refactored in the following patch.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
Reviewed-by: Mike Snitzer <redacted>
---
drivers/md/dm.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
@@ -1252,14 +1252,13 @@ void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors)}EXPORT_SYMBOL_GPL(dm_accept_partial_bio);-staticblk_qc_t__map_bio(structdm_target_io*tio)+staticvoid__map_bio(structdm_target_io*tio){intr;sector_tsector;structbio*clone=&tio->clone;structdm_io*io=tio->io;structdm_target*ti=tio->ti;-blk_qc_tret=BLK_QC_T_NONE;clone->bi_end_io=clone_endio;
@@ -1278,7 +1277,7 @@ static blk_qc_t __map_bio(struct dm_target_io *tio)caseDM_MAPIO_REMAPPED:/* the bio has been remapped so dispatch it */trace_block_bio_remap(clone,bio_dev(io->orig_bio),sector);-ret=submit_bio_noacct(clone);+submit_bio_noacct(clone);break;caseDM_MAPIO_KILL:free_tio(tio);
@@ -1621,13 +1617,11 @@ static blk_qc_t __split_and_process_bio(struct mapped_device *md,/* drop the extra reference count */dec_pending(ci.io,errno_to_blk_status(error));-returnret;}staticblk_qc_tdm_submit_bio(structbio*bio){structmapped_device*md=bio->bi_disk->private_data;-blk_qc_tret=BLK_QC_T_NONE;intsrcu_idx;structdm_table*map;
@@ -1657,10 +1651,10 @@ static blk_qc_t dm_submit_bio(struct bio *bio)if(is_abnormal_io(bio))blk_queue_split(&bio);-ret=__split_and_process_bio(md,map,bio);+__split_and_process_bio(md,map,bio);out:dm_put_live_table(md,srcu_idx);-returnret;+returnBLK_QC_T_NONE;}/*-----------------------------------------------------------------
There's no sense waiting for the hw queue when it currently has been
locked by another polling instance. The polling instance currently
occupying the hw queue will help reap the completion events.
It shall be safe to surrender the hw queue, as long as we could reapply
for polling later. For Synchronous polling, blk_poll() will reapply for
polling, since @spin is always True in this case. While For asynchronous
polling, i.e. io_uring itself will reapply for polling when the previous
polling returns 0.
Besides, it shall do no harm to the polling performance of mq devices.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
drivers/nvme/host/pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
According to the definition of dm_iterate_devices_fn:
* This function must iterate through each section of device used by the
* target until it encounters a non-zero return code, which it then returns.
* Returns zero if no callout returned non-zero.
For some target type (e.g., dm-stripe), one call of iterate_devices()
may iterate multiple underlying devices internally, in which case a
non-zero return code returned by iterate_devices_callout_fn will stop
the iteration in advance.
Thus if we want to ensure that _all_ underlying devices support some
kind of attribute, the iteration structure like
dm_table_supports_nowait() should be used, while the input
iterate_devices_callout_fn should handle the 'not support' semantics.
On the opposite, the iteration structure like dm_table_any_dev_attr()
should be used if _any_ underlying device supporting this attibute is
sufficient. In this case, the input iterate_devices_callout_fn should
handle the 'support' semantics.
Besides, remove the unnecessary NULL pointer check for request_queue,
since the request_queue pointer returned from bdev_get_queue() shall
never be NULL according to commit ff9ea323816d ("block, bdi: an active
gendisk always has a request_queue associated with it").
Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
Cc: stable@vger.kernel.org
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
drivers/md/dm-table.c | 195 +++++++++++++++++++-----------------------
drivers/md/dm.c | 2 +-
drivers/md/dm.h | 2 +-
3 files changed, 91 insertions(+), 108 deletions(-)
@@ -925,7 +925,7 @@ static int dm_table_determine_type(struct dm_table *t)verify_bio_based:/* We must use this table as bio-based */t->type=DM_TYPE_BIO_BASED;-if(dm_table_supports_dax(t,device_supports_dax,&page_size)||+if(dm_table_supports_dax(t,device_not_dax_capable,&page_size)||(list_empty(devices)&&live_md_type==DM_TYPE_DAX_BIO_BASED)){t->type=DM_TYPE_DAX_BIO_BASED;}
@@ -1404,7 +1423,7 @@ static int validate_hardware_zoned_model(struct dm_table *table,if(!zone_sectors||!is_power_of_2(zone_sectors))return-EINVAL;-if(!dm_table_matches_zone_sectors(table,zone_sectors)){+if(dm_table_any_dev_attr(table,device_not_matches_zone_sectors,&zone_sectors)){DMERR("%s: zone sectors is not consistent across all devices",dm_device_name(table->md));return-EINVAL;
On Mon, Feb 08, 2021 at 04:52:41PM +0800, Jeffle Xu wrote:
DM will iterate and poll all polling hardware queues of all target mq
devices when polling IO for dm device. To mitigate the race introduced
by iterating all target hw queues, a per-hw-queue flag is maintained
What is the per-hw-queue flag?
to indicate whether this polling hw queue currently being polled on or
not. Every polling hw queue is exclusive to one polling instance, i.e.,
the polling instance will skip this polling hw queue if this hw queue
currently is being polled by another polling instance, and start
polling on the next hw queue.
Not see such skip in dm_poll_one_dev() in which
queue_for_each_poll_hw_ctx() is called directly for polling all POLL
hctxs of the request queue, so can you explain it a bit more about this
skip mechanism?
Even though such skipping is implemented, not sure if good performance
can be reached because hctx poll may be done in ping-pong style
among several CPUs. But blk-mq hctx is supposed to have its cpu affinities.
--
Ming
On Mon, Feb 08, 2021 at 04:52:41PM +0800, Jeffle Xu wrote:
quoted
DM will iterate and poll all polling hardware queues of all target mq
devices when polling IO for dm device. To mitigate the race introduced
by iterating all target hw queues, a per-hw-queue flag is maintained
What is the per-hw-queue flag?
Sorry I forgot to update the commit message as the implementation
changed. Actually this mechanism is implemented by patch 10 of this
patch set.
quoted
to indicate whether this polling hw queue currently being polled on or
not. Every polling hw queue is exclusive to one polling instance, i.e.,
the polling instance will skip this polling hw queue if this hw queue
currently is being polled by another polling instance, and start
polling on the next hw queue.
Not see such skip in dm_poll_one_dev() in which
queue_for_each_poll_hw_ctx() is called directly for polling all POLL
hctxs of the request queue, so can you explain it a bit more about this
skip mechanism?
It is implemented as patch 10 of this patch set. When spin_trylock()
fails, the polling instance will return immediately, instead of busy
waiting.
Even though such skipping is implemented, not sure if good performance
can be reached because hctx poll may be done in ping-pong style
among several CPUs. But blk-mq hctx is supposed to have its cpu affinities.
Yes, the mechanism of iterating all hw queues can make the competition
worse.
If every underlying data device has **only** one polling hw queue, then
this ping-pong style polling still exist, even when we implement split
bio tracking mechanism, i.e., acquiring the specific hw queue the bio
enqueued into. Because multiple polling instance has to compete for the
only polling hw queue.
But if multiple polling hw queues per device are reserved for multiple
polling instances, (e.g., every underlying data device has 3 polling hw
queues when there are 3 polling instances), just as what we practice on
mq polling, then the current implementation of iterating all hw queues
will indeed works in a ping-pong style, while this issue shall not exist
when accurate split bio tracking mechanism could be implemented.
As for the performance, I cite the test results here, as summarized in
the cover-letter
(https://lore.kernel.org/io-uring/20210208085243.82367-1-jefflexu@linux.alibaba.com/)
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
----------- | --------------- | -------------------- | ----
without opt | 318k | 256k | ~-20%
with opt | 314k | 354k | ~13%
The 'opt' refers to the optimization of patch 10, i.e., the skipping
mechanism. There are 3 polling instances (i.e., 3 CPUs) in this test case.
Indeed the current implementation of iterating all hw queues is some
sort of compromise, as I find it really difficult to implement the
accurate split bio mechanism, and to achieve high performance at the
same time. Thus I turn to optimizing the original implementation of
iterating all hw queues, such as optimization of patch 10 and 11.
--
Thanks,
Jeffle
On Mon, Feb 08, 2021 at 04:52:41PM +0800, Jeffle Xu wrote:
quoted
DM will iterate and poll all polling hardware queues of all target mq
devices when polling IO for dm device. To mitigate the race introduced
by iterating all target hw queues, a per-hw-queue flag is maintained
What is the per-hw-queue flag?
Sorry I forgot to update the commit message as the implementation
changed. Actually this mechanism is implemented by patch 10 of this
patch set.
quoted
quoted
to indicate whether this polling hw queue currently being polled on or
not. Every polling hw queue is exclusive to one polling instance, i.e.,
the polling instance will skip this polling hw queue if this hw queue
currently is being polled by another polling instance, and start
polling on the next hw queue.
Not see such skip in dm_poll_one_dev() in which
queue_for_each_poll_hw_ctx() is called directly for polling all POLL
hctxs of the request queue, so can you explain it a bit more about this
skip mechanism?
It is implemented as patch 10 of this patch set. When spin_trylock()
fails, the polling instance will return immediately, instead of busy
waiting.
quoted
Even though such skipping is implemented, not sure if good performance
can be reached because hctx poll may be done in ping-pong style
among several CPUs. But blk-mq hctx is supposed to have its cpu affinities.
Yes, the mechanism of iterating all hw queues can make the competition
worse.
If every underlying data device has **only** one polling hw queue, then
this ping-pong style polling still exist, even when we implement split
bio tracking mechanism, i.e., acquiring the specific hw queue the bio
enqueued into. Because multiple polling instance has to compete for the
only polling hw queue.
But if multiple polling hw queues per device are reserved for multiple
polling instances, (e.g., every underlying data device has 3 polling hw
queues when there are 3 polling instances), just as what we practice on
mq polling, then the current implementation of iterating all hw queues
will indeed works in a ping-pong style, while this issue shall not exist
when accurate split bio tracking mechanism could be implemented.
If not considering process migration, I could somehow avoid iterating
all hw queues, still in the framework of the current implementation. For
example. For example, the CPU number of the IO submitting process could
be stored in the cookie, while the polling routine will only iterate hw
queues to which the stored CPU number maps. Just a temporary insight
though ....
--
Thanks,
Jeffle
On Tue, Feb 09, 2021 at 02:13:38PM +0800, JeffleXu wrote:
On 2/9/21 11:11 AM, Ming Lei wrote:
quoted
On Mon, Feb 08, 2021 at 04:52:41PM +0800, Jeffle Xu wrote:
quoted
DM will iterate and poll all polling hardware queues of all target mq
devices when polling IO for dm device. To mitigate the race introduced
by iterating all target hw queues, a per-hw-queue flag is maintained
What is the per-hw-queue flag?
Sorry I forgot to update the commit message as the implementation
changed. Actually this mechanism is implemented by patch 10 of this
patch set.
It is hard to associate patch 10's spin_trylock() with per-hw-queue
flag. Also scsi's poll implementation is in-progress, and scsi's poll may
not be implemented in this way.
quoted
quoted
to indicate whether this polling hw queue currently being polled on or
not. Every polling hw queue is exclusive to one polling instance, i.e.,
the polling instance will skip this polling hw queue if this hw queue
currently is being polled by another polling instance, and start
polling on the next hw queue.
Not see such skip in dm_poll_one_dev() in which
queue_for_each_poll_hw_ctx() is called directly for polling all POLL
hctxs of the request queue, so can you explain it a bit more about this
skip mechanism?
It is implemented as patch 10 of this patch set. When spin_trylock()
fails, the polling instance will return immediately, instead of busy
waiting.
quoted
Even though such skipping is implemented, not sure if good performance
can be reached because hctx poll may be done in ping-pong style
among several CPUs. But blk-mq hctx is supposed to have its cpu affinities.
Yes, the mechanism of iterating all hw queues can make the competition
worse.
If every underlying data device has **only** one polling hw queue, then
this ping-pong style polling still exist, even when we implement split
bio tracking mechanism, i.e., acquiring the specific hw queue the bio
enqueued into. Because multiple polling instance has to compete for the
only polling hw queue.
But if multiple polling hw queues per device are reserved for multiple
polling instances, (e.g., every underlying data device has 3 polling hw
queues when there are 3 polling instances), just as what we practice on
mq polling, then the current implementation of iterating all hw queues
will indeed works in a ping-pong style, while this issue shall not exist
when accurate split bio tracking mechanism could be implemented.
In reality it could be possible to have one hw queue for each numa node.
And you may re-use blk_mq_map_queue() for getting the proper hw queue for poll.
--
Ming
On Tue, Feb 09, 2021 at 02:13:38PM +0800, JeffleXu wrote:
quoted
On 2/9/21 11:11 AM, Ming Lei wrote:
quoted
On Mon, Feb 08, 2021 at 04:52:41PM +0800, Jeffle Xu wrote:
quoted
DM will iterate and poll all polling hardware queues of all target mq
devices when polling IO for dm device. To mitigate the race introduced
by iterating all target hw queues, a per-hw-queue flag is maintained
What is the per-hw-queue flag?
Sorry I forgot to update the commit message as the implementation
changed. Actually this mechanism is implemented by patch 10 of this
patch set.
It is hard to associate patch 10's spin_trylock() with per-hw-queue
flag.
You're right, the commit message here is totally a mistake. Actually I
had ever implemented a per-hw-queue flag, such as
structblk_mq_hw_ctx{atomic_tbusy;...};
In this case, the skipping mechanism is implemented in block layer.
But later I refactor the code and move the implementation to the device
driver layer as described by patch 10, while forgetting to update the
commit message. The reason why I implement it in device driver layer is
that, the competition actually stems from the underlying device driver
(e.g., nvme driver), as described in the following snippet.
It is @nvmeq->cq_poll_lock, i.e., the implementation of the underlying
device driver that has caused the competition. Thus maybe it is
reasonable to handle the competition issue in the device driver layer?
Also scsi's poll implementation is in-progress, and scsi's poll may
not be implemented in this way.
Yes. The defect of leaving the competition issue to the device driver
layer is that, every device driver supporting polling need to be somehow
optimized individually. Actually I have not taken a close look at the
other two types of nvme driver (drivers/nvme/host/tcp.c and
drivers/nvme/host/rdma.c), which also support polling.
quoted
quoted
quoted
to indicate whether this polling hw queue currently being polled on or
not. Every polling hw queue is exclusive to one polling instance, i.e.,
the polling instance will skip this polling hw queue if this hw queue
currently is being polled by another polling instance, and start
polling on the next hw queue.
Not see such skip in dm_poll_one_dev() in which
queue_for_each_poll_hw_ctx() is called directly for polling all POLL
hctxs of the request queue, so can you explain it a bit more about this
skip mechanism?
It is implemented as patch 10 of this patch set. When spin_trylock()
fails, the polling instance will return immediately, instead of busy
waiting.
quoted
Even though such skipping is implemented, not sure if good performance
can be reached because hctx poll may be done in ping-pong style
among several CPUs. But blk-mq hctx is supposed to have its cpu affinities.
Yes, the mechanism of iterating all hw queues can make the competition
worse.
If every underlying data device has **only** one polling hw queue, then
this ping-pong style polling still exist, even when we implement split
bio tracking mechanism, i.e., acquiring the specific hw queue the bio
enqueued into. Because multiple polling instance has to compete for the
only polling hw queue.
But if multiple polling hw queues per device are reserved for multiple
polling instances, (e.g., every underlying data device has 3 polling hw
queues when there are 3 polling instances), just as what we practice on
mq polling, then the current implementation of iterating all hw queues
will indeed works in a ping-pong style, while this issue shall not exist
when accurate split bio tracking mechanism could be implemented.
In reality it could be possible to have one hw queue for each numa node.
And you may re-use blk_mq_map_queue() for getting the proper hw queue for poll.
Hi,
Any comment on this series?
Thanks,
Jeffle
On 2/8/21 4:52 PM, Jeffle Xu wrote:
[Performance]
1. One thread (numjobs=1) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 208k | 279k | ~34%
2. Three threads (numjobs=3) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 615k | 728k | ~18%
nvme | 728k | 873k | ~20%
The result shows that the performance gain of bio-based polling is
comparable as that of mq polling in the same test case.
3. Three threads (numjobs=3) randread (bs=12k, direct=1) one
**dm-stripe** device, which is built upon 3 nvme devices, with one
polling hw queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 314k | 354k | ~13%
nvme | 728k | 873k | ~20%
4. This patchset shall do no harm to the performance of the original mq
polling. Following is the test results of one thread (numjobs=1)
randread (bs=4k, direct=1) one nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---------------- | --------------- | -------------------- | ----
without patchset | 242k | 332k | ~39%
with patchset | 236k | 332k | ~39%
[Changes since v2]
Patchset v2 caches all hw queues (in polling mode) of underlying mq
devices in dm layer. The polling routine actually iterates through all
these cached hw queues.
However, mq may change the queue mapping at runtime (e.g., NVMe RESET
command), thus the cached hw queues in dm layer may be out-of-date. Thus
patchset v3 falls back to the implementation of the very first RFC
version, in which the mq layer needs to export one interface iterating
all polling hw queues (patch 5), and the bio-based polling routine just
calls this interface to iterate all polling hw queues.
Besides, several new optimization is proposed.
- patch 1,2,7
same as v2, untouched
- patch 3
Considering advice from Christoph Hellwig, while refactoring blk_poll(),
split mq and bio-based polling routine from the very beginning. Now
blk_poll() is just a simple entry. blk_bio_poll() is simply copied from
blk_mq_poll(), while the loop structure is some sort of duplication
though.
- patch 4
This patch is newly added to support turning on/off polling through
'/sys/block/<dev>/queue/io_poll' dynamiclly for bio-based devices.
Patchset v2 implemented this functionality by added one new queue flag,
which is not preferred since the queue flag resource is quite short of
nowadays.
- patch 5
This patch is newly added, preparing for the following bio-based
polling. The following bio-based polling will call this helper function,
accounting on the corresponding hw queue.
- patch 6
It's from the very first RFC version, preparing for the following
bio-based polling.
- patch 8
One fixing patch needed by the following bio-based polling. It's
actually a v2 of [1]. I had sent the v2 singly in-reply-to [1], though
it has not been visible on the mailing list maybe due to the delay.
- patch 9
It's from the very first RFC version.
- patch 10
This patch is newly added. Patchset v2 had ever proposed one
optimization that, skipping the **busy** hw queues during the iteration
phase. Back upon that time, one flag of 'atomic_t' is specifically
maintained in dm layer, representing if the corresponding hw queue is
busy or not. The idea is inherited, while the implementation changes.
Now @nvmeq->cq_poll_lock is used directly here, no need for extra flag
anymore.
This optimization can significantly reduce the competition for one hw
queue between multiple polling instances. Following statistics is the
test result when 3 threads concurrently randread (bs=4k, direct=1) one
dm-linear device, which is built upon 3 nvme devices, with one polling
hw queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
----------- | --------------- | -------------------- | ----
without opt | 318k | 256k | ~-20%
with opt | 314k | 354k | ~13%
- patch 11
This is another newly added optimizatin for bio-based polling.
One intuitive insight is that, when the original bio submitted to dm
device doesn't get split, then the bio gets enqueued into only one hw
queue of one of the underlying mq devices. In this case, we no longer
need to track all split bios, and one cookie (for the only split bio)
is enough. It is implemented by returning the pointer to the
corresponding hw queue in this case.
It should be safe by directly returning the pointer to the hw queue,
since 'struct blk_mq_hw_ctx' won't be freed during the whole lifetime of
'struct request_queue'. Even when the number of hw queues may decrease
when NVMe RESET happens, the 'struct request_queue' structure of decreased
hw queues won't be freed, instead it's buffered into
&q->unused_hctx_list list.
Though this optimization seems quite intuitive, the performance test
shows that it does no benefit nor harm to the performance, while 3
threads concurrently randreading (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw queue
per nvme device.
I'm not sure why it doesn't work, maybe because the number of devices,
or the depth of the devcice stack is to low in my test case?
[Remained Issue]
It has been mentioned in patch 4 that, users could change the state of
the underlying devices through '/sys/block/<dev>/io_poll', bypassing
the dm device above. Thus it can cause a situation where QUEUE_FLAG_POLL
is still set for the request_queue of dm device, while one of the
underlying mq device may has cleared this flag.
In this case, it will pass the 'test_bit(QUEUE_FLAG_POLL, &q->queue_flags)'
check in blk_poll(), while the input cookie may actually points to a hw
queue in IRQ mode since patch 11. Thus for this hw queue (in IRQ mode),
the bio-based polling routine will handle this hw queue acquiring
'spin_lock(&nvmeq->cq_poll_lock)' (refer
drivers/nvme/host/pci.c:nvme_poll), which is not adequate since this hw
queue may also be accessed in IRQ context. In other words,
spin_lock_irq() should be used here.
I have not come up one simple way to fix it. I don't want to do sanity
check (e.g., the type of the hw queue is HCTX_TYPE_POLL or not) in the
IO path (submit_bio()/blk_poll()), i.e., fast path.
We'd better fix it in the control path, i.e., dm could be aware of the
change when attribute (e.g., support io_poll or not) of one of the
underlying devices changed at runtime.
[1] https://listman.redhat.com/archives/dm-devel/2021-February/msg00028.html
changes since v1:
- patch 1,2,4 is the same as v1 and have already been reviewed
- patch 3 is refactored a bit on the basis of suggestions from
Mike Snitzer.
- patch 5 is newly added and introduces one new queue flag
representing if the queue is capable of IO polling. This mainly
simplifies the logic in queue_poll_store().
- patch 6 implements the core mechanism supporting IO polling.
The sanity check checking if the dm device supports IO polling is
also folded into this patch, and the queue flag will be cleared if
it doesn't support, in case of table reloading.
Jeffle Xu (11):
block: move definition of blk_qc_t to types.h
block: add queue_to_disk() to get gendisk from request_queue
block: add poll method to support bio-based IO polling
block: add poll_capable method to support bio-based IO polling
block/mq: extract one helper function polling hw queue
block/mq: add iterator for polling hw queues
dm: always return BLK_QC_T_NONE for bio-based device
dm: fix iterate_device sanity check
dm: support IO polling for bio-based dm device
nvme/pci: don't wait for locked polling queue
dm: fastpath of bio-based polling
block/blk-core.c | 89 +++++++++++++-
block/blk-mq.c | 27 +----
block/blk-sysfs.c | 14 ++-
drivers/md/dm-table.c | 215 ++++++++++++++++++----------------
drivers/md/dm.c | 90 +++++++++++---
drivers/md/dm.h | 2 +-
drivers/nvme/host/pci.c | 4 +-
include/linux/blk-mq.h | 21 ++++
include/linux/blk_types.h | 10 +-
include/linux/blkdev.h | 4 +
include/linux/device-mapper.h | 1 +
include/linux/fs.h | 2 +-
include/linux/types.h | 3 +
include/trace/events/kyber.h | 6 +-
14 files changed, 333 insertions(+), 155 deletions(-)
@@ -1657,6 +1657,68 @@ static blk_qc_t dm_submit_bio(struct bio *bio)returnBLK_QC_T_NONE;}+staticintdm_poll_one_md(structmapped_device*md);++staticintdm_poll_one_dev(structdm_target*ti,structdm_dev*dev,+sector_tstart,sector_tlen,void*data)+{+inti,*count=data;+structrequest_queue*q=bdev_get_queue(dev->bdev);+structblk_mq_hw_ctx*hctx;++if(queue_is_mq(q)){+if(!percpu_ref_tryget(&q->q_usage_counter))+return0;++queue_for_each_poll_hw_ctx(q,hctx,i)+*count+=blk_mq_poll_hctx(q,hctx);++percpu_ref_put(&q->q_usage_counter);+}else+*count+=dm_poll_one_md(dev->bdev->bd_disk->private_data);
This is fragile, because in the future there may be other bio-based
drivers that support polling. You should check that "q" is really a device
mapper device before calling dm_poll_one_md on it.
Mikulas
Offer one fastpath of bio-based polling when bio submitted to dm device
is not split.
In this case, there will be only one bio submitted to only one polling
hw queue of one underlying mq device, and thus we don't need to track
all split bios or iterate through all polling hw queues. The pointer to
the polling hw queue the bio submitted to is returned here as the
returned cookie.
This doesn't seem safe - note that between submit_bio() and blk_poll(), no
locks are held - so the device mapper device may be reconfigured
arbitrarily. When you call blk_poll() with a pointer returned by
submit_bio(), the pointer may point to a stale address.
Mikulas
quoted hunk
In this case, the polling routine will call
mq_ops->poll() directly with the hw queue converted from the input
cookie.
If the original bio submitted to dm device is split to multiple bios and
thus submitted to multiple polling hw queues, the bio submission routine
will return BLK_QC_T_BIO_MULTI, while the polling routine will fall
back to iterating all hw queues (in polling mode) of all underlying mq
devices.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
block/blk-core.c | 33 +++++++++++++++++++++++++++++++--
include/linux/blk_types.h | 8 ++++++++
include/linux/types.h | 2 +-
3 files changed, 40 insertions(+), 3 deletions(-)
@@ -956,11 +956,19 @@ static blk_qc_t __submit_bio(struct bio *bio)*bio_list_on_stack[0]containsbiossubmittedbythecurrent->submit_bio.*bio_list_on_stack[1]containsbiosthatweresubmittedbeforethecurrent*->submit_bio_bio,butthathaven'tbeenprocessedyet.+*+*Return:+*-BLK_QC_T_NONE,noneedforIOpolling.+*-BLK_QC_T_BIO_MULTI,@biogetssplitandenqueuedintomultihwqueues.+*-Otherwise,@bioisnotsplit,returningthepointertothecorresponding+*hwqueuethatthebioenqueuedintoasthereturnedcookie.*/staticblk_qc_t__submit_bio_noacct(structbio*bio){structbio_listbio_list_on_stack[2];blk_qc_tret=BLK_QC_T_NONE;+structrequest_queue*top_q=bio->bi_disk->queue;+boolpoll_on=test_bit(QUEUE_FLAG_POLL,&top_q->queue_flags);BUG_ON(bio->bi_next);
@@ -968,6 +976,7 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)current->bio_list=bio_list_on_stack;do{+blk_qc_tcookie;structrequest_queue*q=bio->bi_disk->queue;structbio_listlower,same;
@@ -980,7 +989,20 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)bio_list_on_stack[1]=bio_list_on_stack[0];bio_list_init(&bio_list_on_stack[0]);-ret=__submit_bio(bio);+cookie=__submit_bio(bio);++if(poll_on&&+blk_qc_t_bio_valid(ret)&&blk_qc_t_valid(cookie)){+unsignedintqueue_num=blk_qc_t_to_queue_num(cookie);+structblk_mq_hw_ctx*hctx=q->queue_hw_ctx[queue_num];++cookie=(blk_qc_t)hctx;++if(!blk_qc_t_valid(ret))/* set initial value */+ret=cookie;+elseif(ret!=cookie)/* bio got split */+ret=BLK_QC_T_BIO_MULTI;+}/**Sortnewbiosintothoseforalowerlevelandthoseforthe
@@ -1003,6 +1025,7 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)}while((bio=bio_list_pop(&bio_list_on_stack[0])));current->bio_list=NULL;+returnret;}
@@ -1657,6 +1657,68 @@ static blk_qc_t dm_submit_bio(struct bio *bio)returnBLK_QC_T_NONE;}+staticintdm_poll_one_md(structmapped_device*md);++staticintdm_poll_one_dev(structdm_target*ti,structdm_dev*dev,+sector_tstart,sector_tlen,void*data)+{+inti,*count=data;+structrequest_queue*q=bdev_get_queue(dev->bdev);+structblk_mq_hw_ctx*hctx;++if(queue_is_mq(q)){+if(!percpu_ref_tryget(&q->q_usage_counter))+return0;++queue_for_each_poll_hw_ctx(q,hctx,i)+*count+=blk_mq_poll_hctx(q,hctx);++percpu_ref_put(&q->q_usage_counter);+}else+*count+=dm_poll_one_md(dev->bdev->bd_disk->private_data);
This is fragile, because in the future there may be other bio-based
drivers that support polling. You should check that "q" is really a device
mapper device before calling dm_poll_one_md on it.
Sorry I missed this reply. Your advice matters, thanks.
--
Thanks,
Jeffle
Offer one fastpath of bio-based polling when bio submitted to dm device
is not split.
In this case, there will be only one bio submitted to only one polling
hw queue of one underlying mq device, and thus we don't need to track
all split bios or iterate through all polling hw queues. The pointer to
the polling hw queue the bio submitted to is returned here as the
returned cookie.
This doesn't seem safe - note that between submit_bio() and blk_poll(), no
locks are held - so the device mapper device may be reconfigured
arbitrarily. When you call blk_poll() with a pointer returned by
submit_bio(), the pointer may point to a stale address.
Thanks for the feedback. Indeed maybe it's not a good idea to directly
return a 'struct blk_mq_hw_ctx *' pointer as the returned cookie.
Currently I have no idea to fix it, orz... The
blk_get_queue()/blk_put_queue() tricks may not work in this case.
Because the returned cookie may not be used at all. Before calling
blk_poll(), the polling routine may find that the corresponding IO has
already completed, and thus won't call blk_poll(), in which case we have
no place to put the refcount.
But I really don't want to drop this optimization, since this
optimization is quite intuitive when dm device maps to a lot of
underlying devices. Though this optimization doesn't actually achieve
reasonable performance gain in my test, maybe because there are at most
seven nvme devices in my test machine.
Any thoughts?
Thanks,
Jeffle
quoted
In this case, the polling routine will call
mq_ops->poll() directly with the hw queue converted from the input
cookie.
If the original bio submitted to dm device is split to multiple bios and
thus submitted to multiple polling hw queues, the bio submission routine
will return BLK_QC_T_BIO_MULTI, while the polling routine will fall
back to iterating all hw queues (in polling mode) of all underlying mq
devices.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
block/blk-core.c | 33 +++++++++++++++++++++++++++++++--
include/linux/blk_types.h | 8 ++++++++
include/linux/types.h | 2 +-
3 files changed, 40 insertions(+), 3 deletions(-)
@@ -956,11 +956,19 @@ static blk_qc_t __submit_bio(struct bio *bio)*bio_list_on_stack[0]containsbiossubmittedbythecurrent->submit_bio.*bio_list_on_stack[1]containsbiosthatweresubmittedbeforethecurrent*->submit_bio_bio,butthathaven'tbeenprocessedyet.+*+*Return:+*-BLK_QC_T_NONE,noneedforIOpolling.+*-BLK_QC_T_BIO_MULTI,@biogetssplitandenqueuedintomultihwqueues.+*-Otherwise,@bioisnotsplit,returningthepointertothecorresponding+*hwqueuethatthebioenqueuedintoasthereturnedcookie.*/staticblk_qc_t__submit_bio_noacct(structbio*bio){structbio_listbio_list_on_stack[2];blk_qc_tret=BLK_QC_T_NONE;+structrequest_queue*top_q=bio->bi_disk->queue;+boolpoll_on=test_bit(QUEUE_FLAG_POLL,&top_q->queue_flags);BUG_ON(bio->bi_next);
@@ -968,6 +976,7 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)current->bio_list=bio_list_on_stack;do{+blk_qc_tcookie;structrequest_queue*q=bio->bi_disk->queue;structbio_listlower,same;
@@ -980,7 +989,20 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)bio_list_on_stack[1]=bio_list_on_stack[0];bio_list_init(&bio_list_on_stack[0]);-ret=__submit_bio(bio);+cookie=__submit_bio(bio);++if(poll_on&&+blk_qc_t_bio_valid(ret)&&blk_qc_t_valid(cookie)){+unsignedintqueue_num=blk_qc_t_to_queue_num(cookie);+structblk_mq_hw_ctx*hctx=q->queue_hw_ctx[queue_num];++cookie=(blk_qc_t)hctx;++if(!blk_qc_t_valid(ret))/* set initial value */+ret=cookie;+elseif(ret!=cookie)/* bio got split */+ret=BLK_QC_T_BIO_MULTI;+}/**Sortnewbiosintothoseforalowerlevelandthoseforthe
@@ -1003,6 +1025,7 @@ static blk_qc_t __submit_bio_noacct(struct bio *bio)}while((bio=bio_list_pop(&bio_list_on_stack[0])));current->bio_list=NULL;+returnret;}
@@ -1657,6 +1657,68 @@ static blk_qc_t dm_submit_bio(struct bio *bio)returnBLK_QC_T_NONE;}+staticintdm_poll_one_md(structmapped_device*md);++staticintdm_poll_one_dev(structdm_target*ti,structdm_dev*dev,+sector_tstart,sector_tlen,void*data)+{+inti,*count=data;+structrequest_queue*q=bdev_get_queue(dev->bdev);+structblk_mq_hw_ctx*hctx;++if(queue_is_mq(q)){+if(!percpu_ref_tryget(&q->q_usage_counter))+return0;++queue_for_each_poll_hw_ctx(q,hctx,i)+*count+=blk_mq_poll_hctx(q,hctx);++percpu_ref_put(&q->q_usage_counter);+}else+*count+=dm_poll_one_md(dev->bdev->bd_disk->private_data);
This is fragile, because in the future there may be other bio-based
drivers that support polling. You should check that "q" is really a device
mapper device before calling dm_poll_one_md on it.
This can be easily fixed by calling disk->fops->poll() recursively, such as
````
if (queue_is_mq(q)) {
...
} else {
//disk = disk of underlying device
pdata->count += disk->fops->poll(q, pdata->cookie);
}
Offer one fastpath of bio-based polling when bio submitted to dm device
is not split.
In this case, there will be only one bio submitted to only one polling
hw queue of one underlying mq device, and thus we don't need to track
all split bios or iterate through all polling hw queues. The pointer to
the polling hw queue the bio submitted to is returned here as the
returned cookie.
This doesn't seem safe - note that between submit_bio() and blk_poll(), no
locks are held - so the device mapper device may be reconfigured
arbitrarily. When you call blk_poll() with a pointer returned by
submit_bio(), the pointer may point to a stale address.
Thanks for the feedback. Indeed maybe it's not a good idea to directly
return a 'struct blk_mq_hw_ctx *' pointer as the returned cookie.
Currently I have no idea to fix it, orz... The
blk_get_queue()/blk_put_queue() tricks may not work in this case.
Because the returned cookie may not be used at all. Before calling
blk_poll(), the polling routine may find that the corresponding IO has
already completed, and thus won't call blk_poll(), in which case we have
no place to put the refcount.
But I really don't want to drop this optimization, since this
optimization is quite intuitive when dm device maps to a lot of
underlying devices. Though this optimization doesn't actually achieve
reasonable performance gain in my test, maybe because there are at most
seven nvme devices in my test machine.
Any thoughts?
Thanks,
Jeffle
Hi
I reworked device mapper polling, so that we poll in the function
__split_and_process_bio. The pointer to a queue and the polling cookie is
passed only inside device mapper code, it never leaves it.
I'll send you my patches - try them and tell me how does it perform
compared to your patchset.
Mikulas
Offer one fastpath of bio-based polling when bio submitted to dm device
is not split.
In this case, there will be only one bio submitted to only one polling
hw queue of one underlying mq device, and thus we don't need to track
all split bios or iterate through all polling hw queues. The pointer to
the polling hw queue the bio submitted to is returned here as the
returned cookie.
This doesn't seem safe - note that between submit_bio() and blk_poll(), no
locks are held - so the device mapper device may be reconfigured
arbitrarily. When you call blk_poll() with a pointer returned by
submit_bio(), the pointer may point to a stale address.
Thanks for the feedback. Indeed maybe it's not a good idea to directly
return a 'struct blk_mq_hw_ctx *' pointer as the returned cookie.
Currently I have no idea to fix it, orz... The
blk_get_queue()/blk_put_queue() tricks may not work in this case.
Because the returned cookie may not be used at all. Before calling
blk_poll(), the polling routine may find that the corresponding IO has
already completed, and thus won't call blk_poll(), in which case we have
no place to put the refcount.
But I really don't want to drop this optimization, since this
optimization is quite intuitive when dm device maps to a lot of
underlying devices. Though this optimization doesn't actually achieve
reasonable performance gain in my test, maybe because there are at most
seven nvme devices in my test machine.
Any thoughts?
Thanks,
Jeffle
Hi
I reworked device mapper polling, so that we poll in the function
__split_and_process_bio. The pointer to a queue and the polling cookie is
passed only inside device mapper code, it never leaves it.
I'll send you my patches - try them and tell me how does it perform
compared to your patchset.
Thanks. Be glad to hear that you're also working on this. I'm glad to
give some comments on your patch set.
--
Thanks,
Jeffle
From: Mike Snitzer <hidden> Date: 2021-03-10 20:02:23
On Mon, Feb 08 2021 at 3:52am -0500,
Jeffle Xu [off-list ref] wrote:
[Performance]
1. One thread (numjobs=1) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 208k | 279k | ~34%
2. Three threads (numjobs=3) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 615k | 728k | ~18%
nvme | 728k | 873k | ~20%
The result shows that the performance gain of bio-based polling is
comparable as that of mq polling in the same test case.
3. Three threads (numjobs=3) randread (bs=12k, direct=1) one
**dm-stripe** device, which is built upon 3 nvme devices, with one
polling hw queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 314k | 354k | ~13%
nvme | 728k | 873k | ~20%
So this "3." case is meant to illustrate effects of polling when bio is
split to 3 different underlying blk-mq devices. (think it odd that
dm-stripe across 3 nvme devices is performing so much worse than a
single nvme device)
Would be nice to see comparison of this same workload but _with_ CPU
affinity to show relative benefit of your patch 11 in this series (where
you try to leverage CPU pinning).
In general, I don't think patch 11 a worthwhile effort. It is a
half-measure that is trying to paper over the fact that this bio-based
IO polling patchset is quite coarse grained about how it copes with the
need to poll multiple devices.
Patch 10 is a proper special case that should be accounted for (when a
bio isn't split and only gets submitted to a single blk-mq
device/hctx). But even patch 10's approach is fragile (we we've
discussed in private and I'll touch on in reply to patch 10).
But I think patch 11 should be dropped and we defer optimizing bio
splitting at a later time with follow-on work.
Just so you're aware, I'm now thinking the proper way forward is to
update DM core, at the time of DM table load, to assemble an array of
underlying _data_ devices in that table (as iterated with
.iterate_devices) -- this would allow each underlying data device to be
assigned an immutable index for the lifetime of a DM table. It'd be
hooked off the 'struct dm_table' and would share that object's
lifetime.
With that bit of infrastructure in place, we could then take steps to
make DM's cookie more dense in its description of underlying devices
that need polling. This is where I'll get a bit handwavvy.. but I
raised this idea with Joe Thornber and he is going to have a think about
it too.
But this is all to say: optimizing the complex case of bio-splitting
that is such an integral part of bio-based IO processing needs more than
what you've attempted to do (noble effort on your part but again, really
just a half-measure).
SO I think it best to keep the initial implementation of bio-based
polling relatively simple by laying foundation for follow-on work. And
it is also important to _not_ encode in block core some meaning to what
_should_ be a largely opaque cookie (blk_qc_t) that is for the
underlying driver to make sense of.
4. This patchset shall do no harm to the performance of the original mq
polling. Following is the test results of one thread (numjobs=1)
randread (bs=4k, direct=1) one nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---------------- | --------------- | -------------------- | ----
without patchset | 242k | 332k | ~39%
with patchset | 236k | 332k | ~39%
OK, good, this needs to be the case.
[Changes since v2]
Patchset v2 caches all hw queues (in polling mode) of underlying mq
devices in dm layer. The polling routine actually iterates through all
these cached hw queues.
However, mq may change the queue mapping at runtime (e.g., NVMe RESET
command), thus the cached hw queues in dm layer may be out-of-date. Thus
patchset v3 falls back to the implementation of the very first RFC
version, in which the mq layer needs to export one interface iterating
all polling hw queues (patch 5), and the bio-based polling routine just
calls this interface to iterate all polling hw queues.
Besides, several new optimization is proposed.
- patch 1,2,7
same as v2, untouched
- patch 3
Considering advice from Christoph Hellwig, while refactoring blk_poll(),
split mq and bio-based polling routine from the very beginning. Now
blk_poll() is just a simple entry. blk_bio_poll() is simply copied from
blk_mq_poll(), while the loop structure is some sort of duplication
though.
- patch 4
This patch is newly added to support turning on/off polling through
'/sys/block/<dev>/queue/io_poll' dynamiclly for bio-based devices.
Patchset v2 implemented this functionality by added one new queue flag,
which is not preferred since the queue flag resource is quite short of
nowadays.
- patch 5
This patch is newly added, preparing for the following bio-based
polling. The following bio-based polling will call this helper function,
accounting on the corresponding hw queue.
- patch 6
It's from the very first RFC version, preparing for the following
bio-based polling.
- patch 8
One fixing patch needed by the following bio-based polling. It's
actually a v2 of [1]. I had sent the v2 singly in-reply-to [1], though
it has not been visible on the mailing list maybe due to the delay.
- patch 9
It's from the very first RFC version.
- patch 10
This patch is newly added. Patchset v2 had ever proposed one
optimization that, skipping the **busy** hw queues during the iteration
phase. Back upon that time, one flag of 'atomic_t' is specifically
maintained in dm layer, representing if the corresponding hw queue is
busy or not. The idea is inherited, while the implementation changes.
Now @nvmeq->cq_poll_lock is used directly here, no need for extra flag
anymore.
This optimization can significantly reduce the competition for one hw
queue between multiple polling instances. Following statistics is the
test result when 3 threads concurrently randread (bs=4k, direct=1) one
dm-linear device, which is built upon 3 nvme devices, with one polling
hw queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
----------- | --------------- | -------------------- | ----
without opt | 318k | 256k | ~-20%
with opt | 314k | 354k | ~13%
- patch 11
This is another newly added optimizatin for bio-based polling.
One intuitive insight is that, when the original bio submitted to dm
device doesn't get split, then the bio gets enqueued into only one hw
queue of one of the underlying mq devices. In this case, we no longer
need to track all split bios, and one cookie (for the only split bio)
is enough. It is implemented by returning the pointer to the
corresponding hw queue in this case.
It should be safe by directly returning the pointer to the hw queue,
since 'struct blk_mq_hw_ctx' won't be freed during the whole lifetime of
'struct request_queue'. Even when the number of hw queues may decrease
when NVMe RESET happens, the 'struct request_queue' structure of decreased
hw queues won't be freed, instead it's buffered into
&q->unused_hctx_list list.
Though this optimization seems quite intuitive, the performance test
shows that it does no benefit nor harm to the performance, while 3
threads concurrently randreading (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw queue
per nvme device.
I'm not sure why it doesn't work, maybe because the number of devices,
or the depth of the devcice stack is to low in my test case?
Looks like these patch references are stale (was relative to v3 I
think).. e.g: "patch 11" from v3 is really "patch 10" in v5.. but its
implementation has changed because Mikulas pointed out that the
implementation was unsafe.. IIRC?
Anyway, I'll just focus on reviewing each patch in this v5 now.
Thanks,
Mike
On Mon, Feb 08 2021 at 3:52am -0500,
Jeffle Xu [off-list ref] wrote:
quoted
[Performance]
1. One thread (numjobs=1) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 208k | 279k | ~34%
2. Three threads (numjobs=3) randread (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw
queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 615k | 728k | ~18%
nvme | 728k | 873k | ~20%
The result shows that the performance gain of bio-based polling is
comparable as that of mq polling in the same test case.
3. Three threads (numjobs=3) randread (bs=12k, direct=1) one
**dm-stripe** device, which is built upon 3 nvme devices, with one
polling hw queue per nvme device.
It's compared to 3 threads directly randread 3 nvme devices, with one
polling hw queue per nvme device. No CPU affinity set for these 3
threads. Thus every thread can access every nvme device
(filename=/dev/nvme0n1:/dev/nvme1n1:/dev/nvme2n1), i.e., every thread
need to competing for every polling hw queue.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---- | --------------- | -------------------- | ----
dm | 314k | 354k | ~13%
nvme | 728k | 873k | ~20%
So this "3." case is meant to illustrate effects of polling when bio is
split to 3 different underlying blk-mq devices. (think it odd that
dm-stripe across 3 nvme devices is performing so much worse than a
single nvme device)
Would be nice to see comparison of this same workload but _with_ CPU
affinity to show relative benefit of your patch 11 in this series (where
you try to leverage CPU pinning).
Actually I have done the test. You can see that without the optimization
in patch 1,, i.e., to leverage CPU pinning, there's only ~13%
performance gain of polling.
As I stated here [1] (see the clause two "2. sub-fastpath"), with the
optimization in patch 11, the performance gain is ~32%. The optimization
behave well. It mainly decreases the race where multiple CPUs competing
for the same hw queue.
[1]
https://patchwork.kernel.org/project/dm-devel/cover/20210303115740.127001-1-jefflexu@linux.alibaba.com/
In general, I don't think patch 11 a worthwhile effort. It is a
half-measure that is trying to paper over the fact that this bio-based
IO polling patchset is quite coarse grained about how it copes with the
need to poll multiple devices.
Indeed. It is a result of somewhat compromise. If we have better scheme
improving multi-thread performance, it shall certainly go... I can
explain the difficulty of implementing the optimization in other ways in
the following comment.
Patch 10 is a proper special case that should be accounted for (when a
bio isn't split and only gets submitted to a single blk-mq
device/hctx). But even patch 10's approach is fragile (we we've
discussed in private and I'll touch on in reply to patch 10).
But I think patch 11 should be dropped and we defer optimizing bio
splitting at a later time with follow-on work.
Just so you're aware, I'm now thinking the proper way forward is to
update DM core, at the time of DM table load, to assemble an array of
underlying _data_ devices in that table (as iterated with
.iterate_devices) -- this would allow each underlying data device to be
assigned an immutable index for the lifetime of a DM table. It'd be
hooked off the 'struct dm_table' and would share that object's
lifetime.
I have also considered it. There's two way organizing the table, with
each having its own defect though.
1. Each dm table maintains underlying devices at its own level, e.g., if
dm-0 is built upon dm-1, then the dm table of dm-0 maintains all
underlying devices of dm-0, while the dm table of dm-1 maintaining its
own underlying devices.
In this case, there are multiple arrays at each level. If the returned
cookie is still one integer type, it may be difficult to design the
cookie, making it reflecting one slot among all these multiple arrays.
If the returned cookie is actually a group of indexes, with each
indexing one dm table, then where and how to store these indexes...
2. The dm device at the top most level maintains a large array,
aggregating all underlying devices on all levels.
In this case, one unique array is maintained and one cookie of integer
type is enough. However the defect is that, when one underlying dm
device reloads table, how and when to update the array in the top most
level.
With that bit of infrastructure in place, we could then take steps to
make DM's cookie more dense in its description of underlying devices
that need polling. This is where I'll get a bit handwavvy.. but I
raised this idea with Joe Thornber and he is going to have a think about
it too.
But this is all to say: optimizing the complex case of bio-splitting
that is such an integral part of bio-based IO processing needs more than
what you've attempted to do (noble effort on your part but again, really
just a half-measure).
SO I think it best to keep the initial implementation of bio-based
polling relatively simple by laying foundation for follow-on work. And
it is also important to _not_ encode in block core some meaning to what
_should_ be a largely opaque cookie (blk_qc_t) that is for the
underlying driver to make sense of.
Agreed. The difficulty of the design pattern is explained in the
corresponding reply for v5.
quoted
4. This patchset shall do no harm to the performance of the original mq
polling. Following is the test results of one thread (numjobs=1)
randread (bs=4k, direct=1) one nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
---------------- | --------------- | -------------------- | ----
without patchset | 242k | 332k | ~39%
with patchset | 236k | 332k | ~39%
OK, good, this needs to be the case.
quoted
[Changes since v2]
Patchset v2 caches all hw queues (in polling mode) of underlying mq
devices in dm layer. The polling routine actually iterates through all
these cached hw queues.
However, mq may change the queue mapping at runtime (e.g., NVMe RESET
command), thus the cached hw queues in dm layer may be out-of-date. Thus
patchset v3 falls back to the implementation of the very first RFC
version, in which the mq layer needs to export one interface iterating
all polling hw queues (patch 5), and the bio-based polling routine just
calls this interface to iterate all polling hw queues.
Besides, several new optimization is proposed.
- patch 1,2,7
same as v2, untouched
- patch 3
Considering advice from Christoph Hellwig, while refactoring blk_poll(),
split mq and bio-based polling routine from the very beginning. Now
blk_poll() is just a simple entry. blk_bio_poll() is simply copied from
blk_mq_poll(), while the loop structure is some sort of duplication
though.
- patch 4
This patch is newly added to support turning on/off polling through
'/sys/block/<dev>/queue/io_poll' dynamiclly for bio-based devices.
Patchset v2 implemented this functionality by added one new queue flag,
which is not preferred since the queue flag resource is quite short of
nowadays.
- patch 5
This patch is newly added, preparing for the following bio-based
polling. The following bio-based polling will call this helper function,
accounting on the corresponding hw queue.
- patch 6
It's from the very first RFC version, preparing for the following
bio-based polling.
- patch 8
One fixing patch needed by the following bio-based polling. It's
actually a v2 of [1]. I had sent the v2 singly in-reply-to [1], though
it has not been visible on the mailing list maybe due to the delay.
- patch 9
It's from the very first RFC version.
- patch 10
This patch is newly added. Patchset v2 had ever proposed one
optimization that, skipping the **busy** hw queues during the iteration
phase. Back upon that time, one flag of 'atomic_t' is specifically
maintained in dm layer, representing if the corresponding hw queue is
busy or not. The idea is inherited, while the implementation changes.
Now @nvmeq->cq_poll_lock is used directly here, no need for extra flag
anymore.
This optimization can significantly reduce the competition for one hw
queue between multiple polling instances. Following statistics is the
test result when 3 threads concurrently randread (bs=4k, direct=1) one
dm-linear device, which is built upon 3 nvme devices, with one polling
hw queue per nvme device.
| IOPS (IRQ mode) | IOPS (iopoll=1 mode) | diff
----------- | --------------- | -------------------- | ----
without opt | 318k | 256k | ~-20%
with opt | 314k | 354k | ~13%
- patch 11
This is another newly added optimizatin for bio-based polling.
One intuitive insight is that, when the original bio submitted to dm
device doesn't get split, then the bio gets enqueued into only one hw
queue of one of the underlying mq devices. In this case, we no longer
need to track all split bios, and one cookie (for the only split bio)
is enough. It is implemented by returning the pointer to the
corresponding hw queue in this case.
It should be safe by directly returning the pointer to the hw queue,
since 'struct blk_mq_hw_ctx' won't be freed during the whole lifetime of
'struct request_queue'. Even when the number of hw queues may decrease
when NVMe RESET happens, the 'struct request_queue' structure of decreased
hw queues won't be freed, instead it's buffered into
&q->unused_hctx_list list.
Though this optimization seems quite intuitive, the performance test
shows that it does no benefit nor harm to the performance, while 3
threads concurrently randreading (bs=4k, direct=1) one dm-linear
device, which is built upon 3 nvme devices, with one polling hw queue
per nvme device.
I'm not sure why it doesn't work, maybe because the number of devices,
or the depth of the devcice stack is to low in my test case?
Looks like these patch references are stale (was relative to v3 I
think).. e.g: "patch 11" from v3 is really "patch 10" in v5.. but its
implementation has changed because Mikulas pointed out that the
implementation was unsafe.. IIRC?
Anyway, I'll just focus on reviewing each patch in this v5 now.
Thanks,
Mike
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel