writeback: show writeback reason with __print_symbolic

Names of the writeback reasons are used in both the main kernel as well as for parsing the tracepoint format file. Instead of duplicating the names in two locations making it likely that they may become out of sync, use some macro magic to make sure all the names stay in sync. Any update only needs to happen in one spot for it to take place in all locations.

Note, this is an RFC patch, and it probably needs much better comments (well, it currently has no comments), and the C() macro probably should have a different name too.

Signed-off-by Steven Rostedt <[email protected]>

Index: linux-trace.git/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +3 -8

@@ -47,15 +47,10 @@ struct wb_writeback_work {
 	struct completion *done;	/* set if the caller waits */
 };
 
+#undef C
+#define C(a, b) [a] = b
 const char *wb_reason_name[] = {
-	[WB_REASON_BACKGROUND]		= "background",
-	[WB_REASON_TRY_TO_FREE_PAGES]	= "try_to_free_pages",
-	[WB_REASON_SYNC]		= "sync",
-	[WB_REASON_PERIODIC]		= "periodic",
-	[WB_REASON_LAPTOP_TIMER]	= "laptop_timer",
-	[WB_REASON_FREE_MORE_MEM]	= "free_more_memory",
-	[WB_REASON_FS_FREE_SPACE]	= "fs_free_space",
-	[WB_REASON_FORKER_THREAD]	= "forker_thread"
+	WB_ENUM_REASONS
 };
 
 /*

Index: linux-trace.git/include/linux/writeback.h
===================================================================

include/linux/writeback.h +13 -11

@@ -38,21 +38,23 @@ enum writeback_sync_modes {
 	WB_SYNC_ALL,	/* Wait on every mapping */
 };
 
+#define WB_ENUM_REASONS							\
+		C(WB_REASON_BACKGROUND,		"background"),		\
+		C(WB_REASON_TRY_TO_FREE_PAGES,	"try_to_free_pages"),	\
+		C(WB_REASON_SYNC,		"sync"),		\
+		C(WB_REASON_PERIODIC,		"periodic"),		\
+		C(WB_REASON_LAPTOP_TIMER,	"laptop_timer"),	\
+		C(WB_REASON_FREE_MORE_MEM,	"free_more_memory"),	\
+		C(WB_REASON_FS_FREE_SPACE,	"fs_free_space"),	\
+		C(WB_REASON_FORKER_THREAD,	"forker_thread")
+
 /*
  * why some writeback work was initiated
  */
-enum wb_reason {
-	WB_REASON_BACKGROUND,
-	WB_REASON_TRY_TO_FREE_PAGES,
-	WB_REASON_SYNC,
-	WB_REASON_PERIODIC,
-	WB_REASON_LAPTOP_TIMER,
-	WB_REASON_FREE_MORE_MEM,
-	WB_REASON_FS_FREE_SPACE,
-	WB_REASON_FORKER_THREAD,
+#undef C
+#define C(a, b) a
+enum wb_reason { WB_ENUM_REASONS, WB_REASONS_MAX };
 
-	WB_REASON_MAX,
-};
 extern const char *wb_reason_name[];
 
 /*

Index: linux-trace.git/include/trace/events/writeback.h
===================================================================

include/trace/events/writeback.h +3 -8

@@ -21,16 +21,11 @@
 		{I_REFERENCED,		"I_REFERENCED"}		\
 	)
 
+#undef C
+#define C(a, b) {a, b}
 #define show_work_reason(reason)					\
 	__print_symbolic(reason,					\
-		{WB_REASON_BACKGROUND,		"background"},		\
-		{WB_REASON_TRY_TO_FREE_PAGES,	"try_to_free_pages"},	\
-		{WB_REASON_SYNC,		"sync"},		\
-		{WB_REASON_PERIODIC,		"periodic"},		\
-		{WB_REASON_LAPTOP_TIMER,	"laptop_timer"},	\
-		{WB_REASON_FREE_MORE_MEM,	"free_more_memory"},	\
-		{WB_REASON_FS_FREE_SPACE,	"fs_free_space"},	\
-		{WB_REASON_FORKER_THREAD,	"forker_thread"}	\
+			 WB_ENUM_REASONS				\
 	)
 
 struct wb_writeback_work;

Hi Steven,

Quoted 6 lines · Steven Rostedt

Names of the writeback reasons are used in both the main kernel as well as for parsing the tracepoint format file. Instead of duplicating the names in two locations making it likely that they may become out of sync, use some macro magic to make sure all the names stay in sync. Any update only needs to happen in one spot for it to take place in all locations.

It looks a bit hacky,

but does the nice job of de-duplicating code.
And it compiles. So I like it and would like to take it with the below rename :-)

Quoted 3 lines

Note, this is an RFC patch, and it probably needs much better comments (well, it currently has no comments), and the C() macro probably should have a different name too.

C => WB_ENUM_REASONS_ITEM? It may look unpleasantly long, however is unique enough to make the many #define/#undef safe.

Names of the writeback reasons are used in both the main kernel as well as for parsing the tracepoint format file. Instead of duplicating the names in two locations making it likely that they may become out of sync, use some macro magic to make sure all the names stay in sync. Any update only needs to happen in one spot for it to take place in all locations.

Note, this is an RFC patch, and it probably needs much better comments (well, it currently has no comments), and the C() macro probably should have a different name too.

Quoted 7 lines · Steven Rostedt

Names of the writeback reasons are used in both the main kernel as well as for parsing the tracepoint format file. Instead of duplicating the names in two locations making it likely that they may become out of sync, use some macro magic to make sure all the names stay in sync. Any update only needs to happen in one spot for it to take place in all locations.

Quoted 3 lines

Note, this is an RFC patch, and it probably needs much better comments (well, it currently has no comments), and the C() macro probably should have a different name too.

I'm not sure this is a pattern we want to repeat all over the place - print_symbolic() is quite widely used and adding macro redefinitions all over the place doesn't fill me with joy.

AFAICT this code doesn't need a declared array to work so you can

just use a preprocessor construct like this (as used in XFS):

#define value_1 1
#define value_2 2
.....

or

enum {
	value_1	= 1,
	value_2 = 2,
	.....
}

followed by:

#define VALUES	\
	{ value_1,	"Value 1" }, \
	{ value_2,	"Value 2" }, \
	.....

And it just uses print_symbolic(__entry->value, VALUES); to print them out.

If this construct does everything requiredi, then I think it is a much better pattern to use because it's easy to maintain, doesn't require an array to be declared in a C file and doesn't require macro tricks to do it's job....

Quoted 8 lines · Dave Chinner

Quoted 7 lines · Steven Rostedt

Names of the writeback reasons are used in both the main kernel as well as for parsing the tracepoint format file. Instead of duplicating the names in two locations making it likely that they may become out of sync, use some macro magic to make sure all the names stay in sync. Any update only needs to happen in one spot for it to take place in all locations.

Quoted 7 lines

Quoted 3 lines

Note, this is an RFC patch, and it probably needs much better comments (well, it currently has no comments), and the C() macro probably should have a different name too.

I'm not sure this is a pattern we want to repeat all over the place - print_symbolic() is quite widely used and adding macro redefinitions all over the place doesn't fill me with joy.

Yeah, unfortunately...

Quoted 1 line

AFAICT this code doesn't need a declared array to work so you can

You mean the string array wb_reason_name[]? Ah it's actually not used for now -- until there comes the (planned) writeback stats patch to show the reason names in some debugfs/sysfs interface.

Quoted 9 lines

followed by:

#define VALUES	\
	{ value_1,	"Value 1" }, \
	{ value_2,	"Value 2" }, \
	.....

And it just uses print_symbolic(__entry->value, VALUES); to print them out.

If using the above macros, wb_reason_name[] can be defined as

        static const struct trace_print_flags wb_reason_name[] = { VALUES };

and reference it in this way

        wb_reason_name[reason][1]

The first element is redundant info and will be ignored, because

        wb_reason_name[reason][0] == reason

Quoted 5 lines

If this construct does everything requiredi, then I think it is a much better pattern to use because it's easy to maintain, doesn't require an array to be declared in a C file and doesn't require macro tricks to do it's job....

Hmm, I looked through XFS tracing code and find no use case like the wb_reason_name[]. That is, the XFS symbolic names are only used for tracing output and there is no sharing with debugfs/sysfs outputs.