diff mbox series

[v2] block: Use enum for blk-mq tagset flags

Message ID 20250102144426.24241-1-john.g.garry@oracle.com (mailing list archive)
State New
Headers show
Series [v2] block: Use enum for blk-mq tagset flags | expand

Commit Message

John Garry Jan. 2, 2025, 2:44 p.m. UTC 
Use an enum for tagset flags, so that they are automatically
renumbered when modified and we don't potentially leave
unused gaps. Some may find this neater.

This also catches when a new flag is added but a corresponding debugfs
name array member is not added.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
Differences to v1:
- Stop using ilog2 in HCTX_FLAG_NAME
- Reword commit message

Note this outstanding comment on v1:
https://lore.kernel.org/linux-block/20241223192727.GA21363@lst.de/

Comments

Christoph Hellwig Jan. 3, 2025, 6:44 a.m. UTC | #1 
On Thu, Jan 02, 2025 at 02:44:26PM +0000, John Garry wrote:
> Use an enum for tagset flags, so that they are automatically
> renumbered when modified and we don't potentially leave
> unused gaps. Some may find this neater.

Just as last time around I think this is a bad idea that just creates
more boilerplate.  I actually wrote a series before my vacation to
drop another unused flag, remove the weirdo policy indiretion and
add better max flag checking while removing code.  Let me rebase
that, finish writing commit log and send it out.
John Garry Jan. 3, 2025, 8:23 a.m. UTC | #2 
On 03/01/2025 06:44, Christoph Hellwig wrote:
> On Thu, Jan 02, 2025 at 02:44:26PM +0000, John Garry wrote:
>> Use an enum for tagset flags, so that they are automatically
>> renumbered when modified and we don't potentially leave
>> unused gaps. Some may find this neater.
> 
> Just as last time around I think this is a bad idea that just creates
> more boilerplate. 

Pros:
- better to not have unused gaps
- catch missing blk-mq debugfs array names
	- this has been a problem in the past

Cons:
- boilerplate

A compromise could be to use some macro to evaluate the flags, like:
#define BLK_MQ_F(flag) (1 << BLK_MQ_B_##flag)

And have, for example:

.flags = BLK_MQ_F(STACKING) | BLK_MQ_F(BLOCKING);

But that will lead to churn in swapping the flag throughout the code and 
maybe also obfuscate, so I highly doubt whether it is even better.

Anyway, I've made my pitch.

> I actually wrote a series before my vacation to
> drop another unused flag, remove the weirdo policy indiretion and
> add better max flag checking while removing code.  Let me rebase
> that, finish writing commit log and send it out.
>
Christoph Hellwig Jan. 3, 2025, 8:27 a.m. UTC | #3 
On Fri, Jan 03, 2025 at 08:23:02AM +0000, John Garry wrote:
> - better to not have unused gaps

Who cares?

> - catch missing blk-mq debugfs array names
> 	- this has been a problem in the past

Again who really cares?

>
> Cons:
> - boilerplate
>
> A compromise could be to use some macro to evaluate the flags, like:
> #define BLK_MQ_F(flag) (1 << BLK_MQ_B_##flag)

Hell no.  The debugfs mess is already annoying as f**k because of
all that macro magic that makes it impossible to grep for.  Don't
add more of that crap that just makes life hell.
John Garry Jan. 3, 2025, 8:38 a.m. UTC | #4 
On 03/01/2025 08:27, Christoph Hellwig wrote:
> On Fri, Jan 03, 2025 at 08:23:02AM +0000, John Garry wrote:
>> - better to not have unused gaps
> Who cares?

it's not that important, as long as it the flags are properly ordered 
and it's obvious where the gaps are

 > >> - catch missing blk-mq debugfs array names
>> 	- this has been a problem in the past
> Again who really cares?

People who use blk-mq debugfs

And maybe the maintainer when we have the tedious routine of a new flag 
being added but the debugfs entry being missed and then the follow on 
patch (to add the entry).

> 
>> Cons:
>> - boilerplate

Anyway, I feel too strongly about all of this.
Jens Axboe Jan. 3, 2025, 3:13 p.m. UTC | #5 
On 1/3/25 1:27 AM, Christoph Hellwig wrote:
>> - catch missing blk-mq debugfs array names
>> 	- this has been a problem in the past
> 
> Again who really cares?

I do care about that one, it's pretty fragile and I can remember at
least 2 or 3 times when an issue like that has been discovered and it
was introduced several releases ago. Yeah not a huge issue as it's just
debugging, but it's annoying that it's so fragile and easy to get out of
sync.
diff mbox series

Patch

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 4b6b20ccdb53..dc976a42ecb2 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -179,7 +179,7 @@  static const char *const alloc_policy_name[] = {
 };
 #undef BLK_TAG_ALLOC_NAME
 
-#define HCTX_FLAG_NAME(name) [ilog2(BLK_MQ_F_##name)] = #name
+#define HCTX_FLAG_NAME(name) [BLK_MQ_B_##name] = #name
 static const char *const hctx_flag_name[] = {
 	HCTX_FLAG_NAME(TAG_QUEUE_SHARED),
 	HCTX_FLAG_NAME(STACKING),
@@ -196,7 +196,7 @@  static int hctx_flags_show(void *data, struct seq_file *m)
 	const int alloc_policy = BLK_MQ_FLAG_TO_ALLOC_POLICY(hctx->flags);
 
 	BUILD_BUG_ON(ARRAY_SIZE(hctx_flag_name) !=
-			BLK_MQ_F_ALLOC_POLICY_START_BIT);
+			BLK_MQ_B_ALLOC_POLICY_START_BIT);
 	BUILD_BUG_ON(ARRAY_SIZE(alloc_policy_name) != BLK_TAG_ALLOC_MAX);
 
 	seq_puts(m, "alloc_policy=");
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 7f6c482ebf54..8ef1a2455490 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -666,33 +666,39 @@  struct blk_mq_ops {
 #endif
 };
 
-/* Keep hctx_flag_name[] in sync with the definitions below */
 enum {
-	BLK_MQ_F_TAG_QUEUE_SHARED = 1 << 1,
+	BLK_MQ_B_TAG_QUEUE_SHARED,
 	/*
 	 * Set when this device requires underlying blk-mq device for
 	 * completing IO:
 	 */
-	BLK_MQ_F_STACKING	= 1 << 2,
-	BLK_MQ_F_TAG_HCTX_SHARED = 1 << 3,
-	BLK_MQ_F_BLOCKING	= 1 << 4,
+	BLK_MQ_B_STACKING,
+	BLK_MQ_B_TAG_HCTX_SHARED,
+	BLK_MQ_B_BLOCKING,
 	/* Do not allow an I/O scheduler to be configured. */
-	BLK_MQ_F_NO_SCHED	= 1 << 5,
-
+	BLK_MQ_B_NO_SCHED,
 	/*
 	 * Select 'none' during queue registration in case of a single hwq
 	 * or shared hwqs instead of 'mq-deadline'.
 	 */
-	BLK_MQ_F_NO_SCHED_BY_DEFAULT	= 1 << 6,
-	BLK_MQ_F_ALLOC_POLICY_START_BIT = 7,
-	BLK_MQ_F_ALLOC_POLICY_BITS = 1,
+	BLK_MQ_B_NO_SCHED_BY_DEFAULT,
+	BLK_MQ_B_ALLOC_POLICY_START_BIT,
+	BLK_MQ_B_ALLOC_POLICY_BITS = 1,
 };
+/* Keep hctx_flag_name[] in sync with the definitions below */
+#define BLK_MQ_F_TAG_QUEUE_SHARED	(1 << BLK_MQ_B_TAG_QUEUE_SHARED)
+#define BLK_MQ_F_STACKING		(1 << BLK_MQ_B_STACKING)
+#define BLK_MQ_F_TAG_HCTX_SHARED	(1 << BLK_MQ_B_TAG_HCTX_SHARED)
+#define BLK_MQ_F_BLOCKING		(1 << BLK_MQ_B_BLOCKING)
+#define BLK_MQ_F_NO_SCHED		(1 << BLK_MQ_B_NO_SCHED)
+#define BLK_MQ_F_NO_SCHED_BY_DEFAULT	(1 << BLK_MQ_B_NO_SCHED_BY_DEFAULT)
+
 #define BLK_MQ_FLAG_TO_ALLOC_POLICY(flags) \
-	((flags >> BLK_MQ_F_ALLOC_POLICY_START_BIT) & \
-		((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1))
+	((flags >> BLK_MQ_B_ALLOC_POLICY_START_BIT) & \
+		((1 << BLK_MQ_B_ALLOC_POLICY_BITS) - 1))
 #define BLK_ALLOC_POLICY_TO_MQ_FLAG(policy) \
-	((policy & ((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1)) \
-		<< BLK_MQ_F_ALLOC_POLICY_START_BIT)
+	((policy & ((1 << BLK_MQ_B_ALLOC_POLICY_BITS) - 1)) \
+		<< BLK_MQ_B_ALLOC_POLICY_START_BIT)
 
 #define BLK_MQ_MAX_DEPTH	(10240)
 #define BLK_MQ_NO_HCTX_IDX	(-1U)