| Message ID | 1542103016-21037-4-git-send-email-jianchao.w.wang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: refactor and fix on issue request directly | expand |
On 11/13/18 2:56 AM, Jianchao Wang wrote: > When issue request directly and the task is migrated out of the > original cpu where it allocates request, hctx could be ran on > the cpu where it is not mapped. > To fix this, > - insert the request forcibly if BLK_MQ_F_BLOCKING is set. > - check whether the current is mapped to the hctx, if not, insert > forcibly. > - invoke __blk_mq_issue_directly under preemption disabled. I'm not too crazy about this one, adding a get/put_cpu() in the hot path, and a cpumask test. The fact is that most/no drivers care about strict placement. We always try to do so, if convenient, since it's faster, but this seems to be doing the opposite. I'd be more inclined to have a driver flag if it needs guaranteed placement, using one an ops BLK_MQ_F_STRICT_CPU flag or similar. What do you think?
Hi Jens Thanks for your kindly response. On 11/13/18 9:44 PM, Jens Axboe wrote: > On 11/13/18 2:56 AM, Jianchao Wang wrote: >> When issue request directly and the task is migrated out of the >> original cpu where it allocates request, hctx could be ran on >> the cpu where it is not mapped. >> To fix this, >> - insert the request forcibly if BLK_MQ_F_BLOCKING is set. >> - check whether the current is mapped to the hctx, if not, insert >> forcibly. >> - invoke __blk_mq_issue_directly under preemption disabled. > > I'm not too crazy about this one, adding a get/put_cpu() in the hot > path, and a cpumask test. The fact is that most/no drivers care > about strict placement. We always try to do so, if convenient, > since it's faster, but this seems to be doing the opposite. > > I'd be more inclined to have a driver flag if it needs guaranteed > placement, using one an ops BLK_MQ_F_STRICT_CPU flag or similar. > > What do you think? > I'd inclined blk-mq should comply with a unified rule, no matter the issuing directly path or inserting one. Then blk-mq would have a simpler model. And also this guarantee could be a little good for drivers, especially the case where cpu and hw queue mapping is 1:1. Regarding with hot path, do you concern about the nvme device ? If so, how about split a standalone path for it ? Thanks Jianchao
On Wed, Nov 14, 2018 at 10:15 AM jianchao.wang <jianchao.w.wang@oracle.com> wrote: > > Hi Jens > > Thanks for your kindly response. > > On 11/13/18 9:44 PM, Jens Axboe wrote: > > On 11/13/18 2:56 AM, Jianchao Wang wrote: > >> When issue request directly and the task is migrated out of the > >> original cpu where it allocates request, hctx could be ran on > >> the cpu where it is not mapped. > >> To fix this, > >> - insert the request forcibly if BLK_MQ_F_BLOCKING is set. > >> - check whether the current is mapped to the hctx, if not, insert > >> forcibly. > >> - invoke __blk_mq_issue_directly under preemption disabled. > > > > I'm not too crazy about this one, adding a get/put_cpu() in the hot > > path, and a cpumask test. The fact is that most/no drivers care > > about strict placement. We always try to do so, if convenient, > > since it's faster, but this seems to be doing the opposite. > > > > I'd be more inclined to have a driver flag if it needs guaranteed > > placement, using one an ops BLK_MQ_F_STRICT_CPU flag or similar. > > > > What do you think? > > > > I'd inclined blk-mq should comply with a unified rule, no matter the > issuing directly path or inserting one. Then blk-mq would have a simpler > model. And also this guarantee could be a little good for drivers, > especially the case where cpu and hw queue mapping is 1:1. I guess it is quite hard to respect this rule 100%, such as in case of CPU hotplug. Thanks, Ming Lei
On 11/14/18 11:02 AM, Ming Lei wrote: > On Wed, Nov 14, 2018 at 10:15 AM jianchao.wang > <jianchao.w.wang@oracle.com> wrote: >> >> Hi Jens >> >> Thanks for your kindly response. >> >> On 11/13/18 9:44 PM, Jens Axboe wrote: >>> On 11/13/18 2:56 AM, Jianchao Wang wrote: >>>> When issue request directly and the task is migrated out of the >>>> original cpu where it allocates request, hctx could be ran on >>>> the cpu where it is not mapped. >>>> To fix this, >>>> - insert the request forcibly if BLK_MQ_F_BLOCKING is set. >>>> - check whether the current is mapped to the hctx, if not, insert >>>> forcibly. >>>> - invoke __blk_mq_issue_directly under preemption disabled. >>> >>> I'm not too crazy about this one, adding a get/put_cpu() in the hot >>> path, and a cpumask test. The fact is that most/no drivers care >>> about strict placement. We always try to do so, if convenient, >>> since it's faster, but this seems to be doing the opposite. >>> >>> I'd be more inclined to have a driver flag if it needs guaranteed >>> placement, using one an ops BLK_MQ_F_STRICT_CPU flag or similar. >>> >>> What do you think? >>> >> >> I'd inclined blk-mq should comply with a unified rule, no matter the >> issuing directly path or inserting one. Then blk-mq would have a simpler >> model. And also this guarantee could be a little good for drivers, >> especially the case where cpu and hw queue mapping is 1:1. > > I guess it is quite hard to respect this rule 100%, such as in case of > CPU hotplug. > Yes, it is indeed the case. Looks like this patch is contentious. I will drop this one and post later as a standalone one if necessary. Thanks Jianchao
diff --git a/block/blk-mq.c b/block/blk-mq.c index 11c52bb..58f15cc 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1776,6 +1776,17 @@ static blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, blk_status_t ret = BLK_STS_RESOURCE; int srcu_idx; + if (hctx->flags & BLK_MQ_F_BLOCKING) { + force = true; + goto out; + } + + if (unlikely(!cpumask_test_cpu(get_cpu(), hctx->cpumask))) { + put_cpu(); + force = true; + goto out; + } + hctx_lock(hctx, &srcu_idx); /* * hctx_lock is needed before checking quiesced flag. @@ -1808,7 +1819,8 @@ static blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, out_unlock: hctx_unlock(hctx, srcu_idx); - + put_cpu(); +out: switch (ret) { case BLK_STS_OK: break;
When issue request directly and the task is migrated out of the original cpu where it allocates request, hctx could be ran on the cpu where it is not mapped. To fix this, - insert the request forcibly if BLK_MQ_F_BLOCKING is set. - check whether the current is mapped to the hctx, if not, insert forcibly. - invoke __blk_mq_issue_directly under preemption disabled. Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> --- block/blk-mq.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)