| Message ID | 1628519378-211232-9-git-send-email-john.garry@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | blk-mq: Reduce static requests memory footprint for shared sbitmap | expand |
On Mon, Aug 09, 2021 at 10:29:35PM +0800, John Garry wrote: > Add a variant of the init_request function which does not pass a hctx_idx > arg. > > This is important for shared sbitmap support, as it needs to be ensured for > introducing shared static rqs that the LLDD cannot think that requests > are associated with a specific HW queue. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > block/blk-mq.c | 15 ++++++++++----- > include/linux/blk-mq.h | 7 +++++++ > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index f14cc2705f9b..4d6723cfa582 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -2427,13 +2427,15 @@ struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, > static int blk_mq_init_request(struct blk_mq_tag_set *set, struct request *rq, > unsigned int hctx_idx, int node) > { > - int ret; > + int ret = 0; > > - if (set->ops->init_request) { > + if (set->ops->init_request) > ret = set->ops->init_request(set, rq, hctx_idx, node); > - if (ret) > - return ret; > - } > + else if (set->ops->init_request_no_hctx) > + ret = set->ops->init_request_no_hctx(set, rq, node); The only shared sbitmap user of SCSI does not use passed hctx_idx, not sure we need such new callback. If you really want to do this, just wondering why not pass '-1' as hctx_idx in case of shared sbitmap? Thanks, Ming
On 18/08/2021 08:38, Ming Lei wrote: > On Mon, Aug 09, 2021 at 10:29:35PM +0800, John Garry wrote: >> Add a variant of the init_request function which does not pass a hctx_idx >> arg. >> >> This is important for shared sbitmap support, as it needs to be ensured for >> introducing shared static rqs that the LLDD cannot think that requests >> are associated with a specific HW queue. >> >> Signed-off-by: John Garry<john.garry@huawei.com> >> --- >> block/blk-mq.c | 15 ++++++++++----- >> include/linux/blk-mq.h | 7 +++++++ >> 2 files changed, 17 insertions(+), 5 deletions(-) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index f14cc2705f9b..4d6723cfa582 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -2427,13 +2427,15 @@ struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, >> static int blk_mq_init_request(struct blk_mq_tag_set *set, struct request *rq, >> unsigned int hctx_idx, int node) >> { >> - int ret; >> + int ret = 0; >> >> - if (set->ops->init_request) { >> + if (set->ops->init_request) >> ret = set->ops->init_request(set, rq, hctx_idx, node); >> - if (ret) >> - return ret; >> - } >> + else if (set->ops->init_request_no_hctx) >> + ret = set->ops->init_request_no_hctx(set, rq, node); Hi Ming, > The only shared sbitmap user of SCSI does not use passed hctx_idx, not > sure we need such new callback. Sure, actually most versions of init_request callback don't use hctx_idx. Or numa_node arg. > If you really want to do this, just wondering why not pass '-1' as > hctx_idx in case of shared sbitmap? Yeah, I did consider that. hctx_idx is an unsigned, and I generally don't like -1U - but that's no big deal. But I also didn't like how it relies on the driver init_request callback to check the value, which changes the semantics. Obviously we don't add new versions of init_request for new block drivers which use shared sbitmap everyday, and any new ones would get it right. I suppose I can go the way you suggest - I just thought that this method was neat as well. Thanks, John
diff --git a/block/blk-mq.c b/block/blk-mq.c index f14cc2705f9b..4d6723cfa582 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2427,13 +2427,15 @@ struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, static int blk_mq_init_request(struct blk_mq_tag_set *set, struct request *rq, unsigned int hctx_idx, int node) { - int ret; + int ret = 0; - if (set->ops->init_request) { + if (set->ops->init_request) ret = set->ops->init_request(set, rq, hctx_idx, node); - if (ret) - return ret; - } + else if (set->ops->init_request_no_hctx) + ret = set->ops->init_request_no_hctx(set, rq, node); + + if (ret) + return ret; WRITE_ONCE(rq->state, MQ_RQ_IDLE); return 0; @@ -3487,6 +3489,9 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set) if (!set->ops->get_budget ^ !set->ops->put_budget) return -EINVAL; + if (set->ops->init_request && set->ops->init_request_no_hctx) + return -EINVAL; + if (set->queue_depth > BLK_MQ_MAX_DEPTH) { pr_info("blk-mq: reduced tag depth to %u\n", BLK_MQ_MAX_DEPTH); diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index 22215db36122..c838b24944c2 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -357,6 +357,13 @@ struct blk_mq_ops { */ int (*init_request)(struct blk_mq_tag_set *set, struct request *, unsigned int, unsigned int); + + /** + * @init_request: Same as init_request, except no hw queue index is passed + */ + int (*init_request_no_hctx)(struct blk_mq_tag_set *set, struct request *, + unsigned int); + /** * @exit_request: Ditto for exit/teardown. */
Add a variant of the init_request function which does not pass a hctx_idx arg. This is important for shared sbitmap support, as it needs to be ensured for introducing shared static rqs that the LLDD cannot think that requests are associated with a specific HW queue. Signed-off-by: John Garry <john.garry@huawei.com> --- block/blk-mq.c | 15 ++++++++++----- include/linux/blk-mq.h | 7 +++++++ 2 files changed, 17 insertions(+), 5 deletions(-) -- 2.26.2