diff mbox series

[v2,06/11] blk-mq: Pass driver tags to blk_mq_clear_rq_mapping()

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

Commit Message

John Garry Aug. 9, 2021, 2:29 p.m. UTC
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

Comments

kernel test robot Aug. 9, 2021, 5 p.m. UTC | #1
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
John Garry Aug. 9, 2021, 5:10 p.m. UTC | #2
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
kernel test robot Aug. 9, 2021, 7:55 p.m. UTC | #3
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
Ming Lei Aug. 18, 2021, 4:02 a.m. UTC | #4
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
John Garry Aug. 18, 2021, noon UTC | #5
>>   

>> @@ -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
Ming Lei Aug. 19, 2021, 12:39 a.m. UTC | #6
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
John Garry Aug. 19, 2021, 7:32 a.m. UTC | #7
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
Ming Lei Aug. 23, 2021, 2:59 a.m. UTC | #8
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 mbox series

Patch

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);