| Message ID | 1628519378-211232-7-git-send-email-john.garry@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | blk-mq: Reduce static requests memory footprint for shared sbitmap | expand |
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on block/for-next]
[also build test WARNING on v5.14-rc5 next-20210809]
[cannot apply to mkp-scsi/for-next ceph-client/for-linus scsi/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Garry/blk-mq-Reduce-static-requests-memory-footprint-for-shared-sbitmap/20210809-223943
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/535293cab26a196d29b64d9ce8a7274bfd1806d8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Garry/blk-mq-Reduce-static-requests-memory-footprint-for-shared-sbitmap/20210809-223943
git checkout 535293cab26a196d29b64d9ce8a7274bfd1806d8
# save the attached .config to linux build tree
make W=1 ARCH=um SUBARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> block/blk-mq.c:2313:6: warning: no previous prototype for 'blk_mq_clear_rq_mapping' [-Wmissing-prototypes]
2313 | void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags,
| ^~~~~~~~~~~~~~~~~~~~~~~
vim +/blk_mq_clear_rq_mapping +2313 block/blk-mq.c
2311
2312 /* called before freeing request pool in @tags */
> 2313 void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags,
2314 struct blk_mq_tags *tags)
2315 {
2316 struct page *page;
2317 unsigned long flags;
2318
2319 list_for_each_entry(page, &tags->page_list, lru) {
2320 unsigned long start = (unsigned long)page_address(page);
2321 unsigned long end = start + order_to_size(page->private);
2322 int i;
2323
2324 for (i = 0; i < drv_tags->nr_tags; i++) {
2325 struct request *rq = drv_tags->rqs[i];
2326 unsigned long rq_addr = (unsigned long)rq;
2327
2328 if (rq_addr >= start && rq_addr < end) {
2329 WARN_ON_ONCE(refcount_read(&rq->ref) != 0);
2330 cmpxchg(&drv_tags->rqs[i], rq, NULL);
2331 }
2332 }
2333 }
2334
2335 /*
2336 * Wait until all pending iteration is done.
2337 *
2338 * Request reference is cleared and it is guaranteed to be observed
2339 * after the ->lock is released.
2340 */
2341 spin_lock_irqsave(&drv_tags->lock, flags);
2342 spin_unlock_irqrestore(&drv_tags->lock, flags);
2343 }
2344
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 09/08/2021 18:00, kernel test robot wrote: > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot<lkp@intel.com> > > All warnings (new ones prefixed by >>): > >>> block/blk-mq.c:2313:6: warning: no previous prototype for 'blk_mq_clear_rq_mapping' [-Wmissing-prototypes] > 2313 | void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags, > | ^~~~~~~~~~~~~~~~~~~~~~~ > > > vim +/blk_mq_clear_rq_mapping +2313 block/blk-mq.c > > 2311 > 2312 /* called before freeing request pool in @tags */ >> 2313 void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags, > 2314 struct blk_mq_tags *tags) I will fix in a new version after other review. Thanks
Hi John,
I love your patch! Perhaps something to improve:
[auto build test WARNING on block/for-next]
[also build test WARNING on v5.14-rc5 next-20210809]
[cannot apply to mkp-scsi/for-next ceph-client/for-linus scsi/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/John-Garry/blk-mq-Reduce-static-requests-memory-footprint-for-shared-sbitmap/20210809-223943
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: hexagon-randconfig-r036-20210809 (attached as .config)
compiler: clang version 12.0.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/535293cab26a196d29b64d9ce8a7274bfd1806d8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review John-Garry/blk-mq-Reduce-static-requests-memory-footprint-for-shared-sbitmap/20210809-223943
git checkout 535293cab26a196d29b64d9ce8a7274bfd1806d8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=hexagon
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> block/blk-mq.c:2313:6: warning: no previous prototype for function 'blk_mq_clear_rq_mapping' [-Wmissing-prototypes]
void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags,
^
block/blk-mq.c:2313:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags,
^
static
1 warning generated.
vim +/blk_mq_clear_rq_mapping +2313 block/blk-mq.c
2311
2312 /* called before freeing request pool in @tags */
> 2313 void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags,
2314 struct blk_mq_tags *tags)
2315 {
2316 struct page *page;
2317 unsigned long flags;
2318
2319 list_for_each_entry(page, &tags->page_list, lru) {
2320 unsigned long start = (unsigned long)page_address(page);
2321 unsigned long end = start + order_to_size(page->private);
2322 int i;
2323
2324 for (i = 0; i < drv_tags->nr_tags; i++) {
2325 struct request *rq = drv_tags->rqs[i];
2326 unsigned long rq_addr = (unsigned long)rq;
2327
2328 if (rq_addr >= start && rq_addr < end) {
2329 WARN_ON_ONCE(refcount_read(&rq->ref) != 0);
2330 cmpxchg(&drv_tags->rqs[i], rq, NULL);
2331 }
2332 }
2333 }
2334
2335 /*
2336 * Wait until all pending iteration is done.
2337 *
2338 * Request reference is cleared and it is guaranteed to be observed
2339 * after the ->lock is released.
2340 */
2341 spin_lock_irqsave(&drv_tags->lock, flags);
2342 spin_unlock_irqrestore(&drv_tags->lock, flags);
2343 }
2344
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Aug 09, 2021 at 10:29:33PM +0800, John Garry wrote: > Function blk_mq_clear_rq_mapping() will be used for shared sbitmap tags > in future, so pass a driver tags pointer instead of the tagset container > and HW queue index. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > block/blk-mq.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 42c4b8d1a570..0bb596f4d061 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -2310,10 +2310,9 @@ static size_t order_to_size(unsigned int order) > } > > /* called before freeing request pool in @tags */ > -static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, > - struct blk_mq_tags *tags, unsigned int hctx_idx) > +void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags, > + struct blk_mq_tags *tags) > { > - struct blk_mq_tags *drv_tags = set->tags[hctx_idx]; > struct page *page; > unsigned long flags; > > @@ -2322,7 +2321,7 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, > unsigned long end = start + order_to_size(page->private); > int i; > > - for (i = 0; i < set->queue_depth; i++) { > + for (i = 0; i < drv_tags->nr_tags; i++) { > struct request *rq = drv_tags->rqs[i]; > unsigned long rq_addr = (unsigned long)rq; > > @@ -2346,8 +2345,11 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, > void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, > unsigned int hctx_idx) > { > + struct blk_mq_tags *drv_tags; > struct page *page; > > + drv_tags = set->tags[hctx_idx]; Indent. > + > if (tags->static_rqs && set->ops->exit_request) { > int i; > > @@ -2361,7 +2363,7 @@ void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, > } > } > > - blk_mq_clear_rq_mapping(set, tags, hctx_idx); > + blk_mq_clear_rq_mapping(drv_tags, tags); Maybe you can pass set->tags[hctx_idx] directly since there is only one reference on it. -- Ming
>> >> @@ -2346,8 +2345,11 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, >> void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, >> unsigned int hctx_idx) >> { >> + struct blk_mq_tags *drv_tags; >> struct page *page; >> >> + drv_tags = set->tags[hctx_idx]; > Hi Ming, > Indent. That's intentional, as we have from later patch: void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, unsigned int hctx_idx) { struct blk_mq_tags *drv_tags; struct page *page; + if (blk_mq_is_sbitmap_shared(set->flags)) + drv_tags = set->shared_sbitmap_tags; + else drv_tags = set->tags[hctx_idx]; ... blk_mq_clear_rq_mapping(drv_tags, tags); } And it's just nice to not re-indent later. > >> + >> if (tags->static_rqs && set->ops->exit_request) { >> int i; >> >> @@ -2361,7 +2363,7 @@ void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, >> } >> } >> >> - blk_mq_clear_rq_mapping(set, tags, hctx_idx); >> + blk_mq_clear_rq_mapping(drv_tags, tags); > > Maybe you can pass set->tags[hctx_idx] directly since there is only one > reference on it. > Again, intentional for similar reason, as above. Thanks, John
On Wed, Aug 18, 2021 at 01:00:13PM +0100, John Garry wrote: > > > @@ -2346,8 +2345,11 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, > > > void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, > > > unsigned int hctx_idx) > > > { > > > + struct blk_mq_tags *drv_tags; > > > struct page *page; > > > + drv_tags = set->tags[hctx_idx]; > > > > Hi Ming, > > > Indent. > > That's intentional, as we have from later patch: > > void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, > unsigned int hctx_idx) > { > struct blk_mq_tags *drv_tags; > struct page *page; > > + if (blk_mq_is_sbitmap_shared(set->flags)) > + drv_tags = set->shared_sbitmap_tags; > + else > drv_tags = set->tags[hctx_idx]; > > ... > > blk_mq_clear_rq_mapping(drv_tags, tags); > > } > > And it's just nice to not re-indent later. But this way is weird, and I don't think checkpatch.pl is happy with it. -- Ming
On 19/08/2021 01:39, Ming Lei wrote: >> That's intentional, as we have from later patch: >> >> void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, >> unsigned int hctx_idx) >> { >> struct blk_mq_tags *drv_tags; >> struct page *page; >> >> + if (blk_mq_is_sbitmap_shared(set->flags)) >> + drv_tags = set->shared_sbitmap_tags; >> + else >> drv_tags = set->tags[hctx_idx]; >> >> ... >> >> blk_mq_clear_rq_mapping(drv_tags, tags); >> >> } >> >> And it's just nice to not re-indent later. > But this way is weird, and I don't think checkpatch.pl is happy with > it. There is the idea to try to not remove/change code earlier in a series - I am taking it to an extreme! I can stop. On another related topic, how about this change also: ---8<--- void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags, struct blk_mq_tags *tags) { + /* There is no need to clear a driver tags own mapping */ + if (drv_tags == tags) + return; --->8--- Thanks, John
On Thu, Aug 19, 2021 at 08:32:20AM +0100, John Garry wrote: > On 19/08/2021 01:39, Ming Lei wrote: > > > That's intentional, as we have from later patch: > > > > > > void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, > > > unsigned int hctx_idx) > > > { > > > struct blk_mq_tags *drv_tags; > > > struct page *page; > > > > > > + if (blk_mq_is_sbitmap_shared(set->flags)) > > > + drv_tags = set->shared_sbitmap_tags; > > > + else > > > drv_tags = set->tags[hctx_idx]; > > > > > > ... > > > > > > blk_mq_clear_rq_mapping(drv_tags, tags); > > > > > > } > > > > > > And it's just nice to not re-indent later. > > But this way is weird, and I don't think checkpatch.pl is happy with > > it. > > There is the idea to try to not remove/change code earlier in a series - I > am taking it to an extreme! I can stop. > > On another related topic, how about this change also: > > ---8<--- > void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags, > struct blk_mq_tags *tags) > { > > + /* There is no need to clear a driver tags own mapping */ > + if (drv_tags == tags) > + return; > --->8--- The change itself is correct, and no need to clear driver tags ->rq[] since all request queues have been cleaned up when freeing tagset. Thanks, Ming
diff --git a/block/blk-mq.c b/block/blk-mq.c index 42c4b8d1a570..0bb596f4d061 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2310,10 +2310,9 @@ static size_t order_to_size(unsigned int order) } /* called before freeing request pool in @tags */ -static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, - struct blk_mq_tags *tags, unsigned int hctx_idx) +void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags, + struct blk_mq_tags *tags) { - struct blk_mq_tags *drv_tags = set->tags[hctx_idx]; struct page *page; unsigned long flags; @@ -2322,7 +2321,7 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, unsigned long end = start + order_to_size(page->private); int i; - for (i = 0; i < set->queue_depth; i++) { + for (i = 0; i < drv_tags->nr_tags; i++) { struct request *rq = drv_tags->rqs[i]; unsigned long rq_addr = (unsigned long)rq; @@ -2346,8 +2345,11 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, unsigned int hctx_idx) { + struct blk_mq_tags *drv_tags; struct page *page; + drv_tags = set->tags[hctx_idx]; + if (tags->static_rqs && set->ops->exit_request) { int i; @@ -2361,7 +2363,7 @@ void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, } } - blk_mq_clear_rq_mapping(set, tags, hctx_idx); + blk_mq_clear_rq_mapping(drv_tags, tags); while (!list_empty(&tags->page_list)) { page = list_first_entry(&tags->page_list, struct page, lru);
Function blk_mq_clear_rq_mapping() will be used for shared sbitmap tags in future, so pass a driver tags pointer instead of the tagset container and HW queue index. Signed-off-by: John Garry <john.garry@huawei.com> --- block/blk-mq.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) -- 2.26.2