[PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Minwoo Im]

set->mq_map is now currently cleared if something goes wrong when
establishing a queue map in blk-mq-pci.c.  It's also cleared before
updating a queue map in blk_mq_update_queue_map().

This patch provides an API to clear set->mq_map to make it clear.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 block/blk-mq-pci.c     |  3 +--
 block/blk-mq.c         | 13 ++++++++++---
 include/linux/blk-mq.h |  1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq-pci.c b/block/blk-mq-pci.c
index e233996..ccb9d7e 100644
--- a/block/blk-mq-pci.c
+++ b/block/blk-mq-pci.c
@@ -48,8 +48,7 @@ int blk_mq_pci_map_queues(struct blk_mq_tag_set *set, struct pci_dev *pdev,
 
 fallback:
 	WARN_ON_ONCE(set->nr_hw_queues > 1);
-	for_each_possible_cpu(cpu)
-		set->mq_map[cpu] = 0;
+	blk_mq_clear_mq_map(set);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(blk_mq_pci_map_queues);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index b429d51..c5f9ee1 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2672,10 +2672,18 @@ static int blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
 	return 0;
 }
 
+void blk_mq_clear_mq_map(struct blk_mq_tag_set *set)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		set->mq_map[cpu] = 0;
+}
+EXPORT_SYMBOL(blk_mq_clear_mq_map);
+
 static int blk_mq_update_queue_map(struct blk_mq_tag_set *set)
 {
 	if (set->ops->map_queues) {
-		int cpu;
 		/*
 		 * transport .map_queues is usually done in the following
 		 * way:
@@ -2690,8 +2698,7 @@ static int blk_mq_update_queue_map(struct blk_mq_tag_set *set)
 		 * killing stale mapping since one CPU may not be mapped
 		 * to any hw queue.
 		 */
-		for_each_possible_cpu(cpu)
-			set->mq_map[cpu] = 0;
+		blk_mq_clear_mq_map(set);
 
 		return set->ops->map_queues(set);
 	} else
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index e3147eb..18390dc 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -204,6 +204,7 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 int blk_mq_register_dev(struct device *, struct request_queue *);
 void blk_mq_unregister_dev(struct device *, struct request_queue *);
 
+void blk_mq_clear_mq_map(struct blk_mq_tag_set *set);
 int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set);
 void blk_mq_free_tag_set(struct blk_mq_tag_set *set);
 
-- 
2.7.4

Re: [PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Johannes Thumshirn]

On Sun, Jul 01, 2018 at 01:21:39AM +0900, Minwoo Im wrote:
> set->mq_map is now currently cleared if something goes wrong when
> establishing a queue map in blk-mq-pci.c.  It's also cleared before
> updating a queue map in blk_mq_update_queue_map().
> 
> This patch provides an API to clear set->mq_map to make it clear.

Is there a follow up patch to this which justifies the change?

With no 2nd consumer of the function I fear this will be disregarded
as useless code churn.

Byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Minwoo Im]

Hi Johannes,

Thanks for you kindly comment.

On Mon, 2018-07-02 at 10:15 +0200, Johannes Thumshirn wrote:
> On Sun, Jul 01, 2018 at 01:21:39AM +0900, Minwoo Im wrote:
> > 
> > set->mq_map is now currently cleared if something goes wrong when
> > establishing a queue map in blk-mq-pci.c.  It's also cleared before
> > updating a queue map in blk_mq_update_queue_map().
> > 
> > This patch provides an API to clear set->mq_map to make it clear.
> Is there a follow up patch to this which justifies the change?
> 
> With no 2nd consumer of the function I fear this will be disregarded
> as useless code churn.

No 2nd follow-up patch will be there.  I thought these two parts are
using a same unit-function to clear the set->mq-map.  Also thought it
would be great for the future use when it needs to be cleared.
However, as you mentioned, I totally agree with your point.  It seems
just churns for churns' sake with no more usage for now.

Thanks for your comment, again.

	Minwoo Im

> 
> Byte,
> 	Johannes

Re: [PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Johannes Thumshirn]

On Mon, Jul 02, 2018 at 09:55:57PM +0900, Minwoo Im wrote:
> No 2nd follow-up patch will be there. �I thought these two parts are
> using a same unit-function to clear the set->mq-map. �Also thought it
> would be great for the future use when it needs to be cleared.
> However, as you mentioned, I totally agree with your point. �It seems
> just churns for churns' sake with no more usage for now.

I actually mis-read your patch, I'm sorry.

You're consolidating two callers of this so I guess this is not so
problematic. I somehow ignored the first hunk in the patch.

You could still evaluate if it isn't worth to make it:
+static void blk_mq_clear_mq_map(struct blk_mq_tag_set *set)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		set->mq_map[cpu] = 0;
+}

and put it into 'include/linux/blk-mq.h'.

    Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Minwoo Im]

Hi Johannes,

I appreciate your kindly response.

On Mon, 2018-07-02 at 15:02 +0200, Johannes Thumshirn wrote:
> On Mon, Jul 02, 2018 at 09:55:57PM +0900, Minwoo Im wrote:
> > 
> > No 2nd follow-up patch will be there.  I thought these two parts are
> > using a same unit-function to clear the set->mq-map.  Also thought it
> > would be great for the future use when it needs to be cleared.
> > However, as you mentioned, I totally agree with your point.  It seems
> > just churns for churns' sake with no more usage for now.
> I actually mis-read your patch, I'm sorry.
> 
> You're consolidating two callers of this so I guess this is not so
> problematic. I somehow ignored the first hunk in the patch.
> 
> You could still evaluate if it isn't worth to make it:
> +static void blk_mq_clear_mq_map(struct blk_mq_tag_set *set)
> +{
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu)
> +		set->mq_map[cpu] = 0;
> +}
> 
> and put it into 'include/linux/blk-mq.h'.

Do you mean this whole bunch of function should be moved into blk-mq.h with
'inline' format?

I have already added this function prototype to blk-mq.h without 'static' to
make blk-mq-pci.c to use it.

If you don't mind, can you please elaborate a little bit more?

Thanks,

	Minwoo Im

> 
>     Johannes

Re: [PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Johannes Thumshirn]

On Mon, Jul 02, 2018 at 10:20:03PM +0900, Minwoo Im wrote:
[...]
> > You could still evaluate if it isn't worth to make it:
> > +static void blk_mq_clear_mq_map(struct blk_mq_tag_set *set)
> > +{
> > +	int cpu;
> > +
> > +	for_each_possible_cpu(cpu)
> > +		set->mq_map[cpu] = 0;
> > +}
> > 
> > and put it into 'include/linux/blk-mq.h'.
> 
> Do you mean this whole bunch of function should be moved into blk-mq.h with
> 'inline' format?

yes

> I have already added this function prototype to blk-mq.h without 'static' to
> make blk-mq-pci.c to use it.

The function itself is pretty small and you use it in two different
files, so I'd make it an inline function in blk-mq.h, like this:

diff --git a/block/blk-mq-pci.c b/block/blk-mq-pci.c
index e233996bb76f..ccb9d7e9adbd 100644
--- a/block/blk-mq-pci.c
+++ b/block/blk-mq-pci.c
@@ -48,8 +48,7 @@ int blk_mq_pci_map_queues(struct blk_mq_tag_set *set, struct pci_dev *pdev,
 
 fallback:
 	WARN_ON_ONCE(set->nr_hw_queues > 1);
-	for_each_possible_cpu(cpu)
-		set->mq_map[cpu] = 0;
+	blk_mq_clear_mq_map(set);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(blk_mq_pci_map_queues);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 95919268564b..664c3817545b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2687,7 +2687,6 @@ static int blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
 static int blk_mq_update_queue_map(struct blk_mq_tag_set *set)
 {
 	if (set->ops->map_queues) {
-		int cpu;
 		/*
 		 * transport .map_queues is usually done in the following
 		 * way:
@@ -2702,8 +2701,7 @@ static int blk_mq_update_queue_map(struct blk_mq_tag_set *set)
 		 * killing stale mapping since one CPU may not be mapped
 		 * to any hw queue.
 		 */
-		for_each_possible_cpu(cpu)
-			set->mq_map[cpu] = 0;
+		blk_mq_clear_mq_map(set);
 
 		return set->ops->map_queues(set);
 	} else
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index e3147eb74222..a8ac78faa2fc 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -237,6 +237,14 @@ enum {
 
 u32 blk_mq_unique_tag(struct request *rq);
 
+static inline void blk_mq_clear_mq_map(struct blk_mq_tag_set *set)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		set->mq_map[cpu] = 0;
+}
+
 static inline u16 blk_mq_unique_tag_to_hwq(u32 unique_tag)
 {
 	return unique_tag >> BLK_MQ_UNIQUE_TAG_BITS;


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Minwoo Im]

On Mon, 2018-07-02 at 15:27 +0200, Johannes Thumshirn wrote:
> On Mon, Jul 02, 2018 at 10:20:03PM +0900, Minwoo Im wrote:
> [...]
> > 
> > > 
> > > You could still evaluate if it isn't worth to make it:
> > > +static void blk_mq_clear_mq_map(struct blk_mq_tag_set *set)
> > > +{
> > > +	int cpu;
> > > +
> > > +	for_each_possible_cpu(cpu)
> > > +		set->mq_map[cpu] = 0;
> > > +}
> > > 
> > > and put it into 'include/linux/blk-mq.h'.
> > Do you mean this whole bunch of function should be moved into blk-mq.h with
> > 'inline' format?
> yes

Johannes,

I was wondering you did mean 'inline' or not because the function in the
previous mail only with 'static' without 'inline' keyword :)
That's why I asked it one more time to figure out what you really mean.

I really appreciate your kindly directives.

I'll make it up for v2 patch.

Thanks, really.

	Minwoo Im

Re: [PATCH] blk-mq: code clean-up by adding an API to clear set->mq_map

[Johannes Thumshirn]

On Mon, Jul 02, 2018 at 10:34:19PM +0900, Minwoo Im wrote:
> I was wondering you did mean 'inline' or not because the function in the
> previous mail only with 'static' without 'inline' keyword :)
> That's why I asked it one more time to figure out what you really mean.

Ah sorry. I'm juggling too much things in parallel currently. Sorry
for that.

   Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850