[PATCH] writeback: show writeback reason with __print_symbolic

[PATCH] writeback: show writeback reason with __print_symbolic

[Wu Fengguang]

This makes the traces friendly to trace-cmd, at the cost of a bit code duplication.

CC: Curt Wohlgemuth <curtw@google.com>
CC: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 include/trace/events/writeback.h |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

--- linux-next.orig/include/trace/events/writeback.h	2011-12-08 16:44:38.000000000 +0800
+++ linux-next/include/trace/events/writeback.h	2011-12-08 16:53:41.000000000 +0800
@@ -21,6 +21,18 @@
 		{I_REFERENCED,		"I_REFERENCED"}		\
 	)
 
+#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"}	\
+	)
+
 struct wb_writeback_work;
 
 DECLARE_EVENT_CLASS(writeback_work_class,
@@ -55,7 +67,7 @@ DECLARE_EVENT_CLASS(writeback_work_class
 		  __entry->for_kupdate,
 		  __entry->range_cyclic,
 		  __entry->for_background,
-		  wb_reason_name[__entry->reason]
+		  show_work_reason(__entry->reason)
 	)
 );
 #define DEFINE_WRITEBACK_WORK_EVENT(name) \
@@ -184,7 +196,8 @@ TRACE_EVENT(writeback_queue_io,
 		__entry->older,	/* older_than_this in jiffies */
 		__entry->age,	/* older_than_this in relative milliseconds */
 		__entry->moved,
-		wb_reason_name[__entry->reason])
+		show_work_reason(__entry->reason)
+	)
 );
 
 TRACE_EVENT(global_dirty_state,

[RFC][PATCH] writeback: Unduplicate writeback reason

[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.

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 <rostedt@goodmis.org>

Index: linux-trace.git/fs/fs-writeback.c
===================================================================
--- linux-trace.git.orig/fs/fs-writeback.c
+++ linux-trace.git/fs/fs-writeback.c
@@ -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
===================================================================
--- linux-trace.git.orig/include/linux/writeback.h
+++ linux-trace.git/include/linux/writeback.h
@@ -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
===================================================================
--- linux-trace.git.orig/include/trace/events/writeback.h
+++ linux-trace.git/include/trace/events/writeback.h
@@ -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;

Re: [RFC][PATCH] writeback: Unduplicate writeback reason

[Wu Fengguang]

Hi Steven,

On Wed, Dec 14, 2011 at 09:14:00AM +0800, Steven Rostedt wrote:
> 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 :-)

> 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. 

Thanks!
Fengguang

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Index: linux-trace.git/fs/fs-writeback.c
> ===================================================================
> --- linux-trace.git.orig/fs/fs-writeback.c
> +++ linux-trace.git/fs/fs-writeback.c
> @@ -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
> ===================================================================
> --- linux-trace.git.orig/include/linux/writeback.h
> +++ linux-trace.git/include/linux/writeback.h
> @@ -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
> ===================================================================
> --- linux-trace.git.orig/include/trace/events/writeback.h
> +++ linux-trace.git/include/trace/events/writeback.h
> @@ -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;
> 

Re: [RFC][PATCH] writeback: Unduplicate writeback reason

[Steven Rostedt]

On Wed, 2011-12-14 at 09:49 +0800, Wu Fengguang wrote:
> Hi Steven,
> 
> On Wed, Dec 14, 2011 at 09:14:00AM +0800, Steven Rostedt wrote:
> > 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,

One of my professors showed me this trick a long time ago when I was
going for my Masters. It was actually the trick I based the entire
TRACE_EVENT() macro magic on :)

>  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 :-)
> 
> > 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. 

Sure. Acked-by: Steven Rostedt <rostedt@goodmis.org>

Note, there's one buggy issue still. That the enums names still trip up
trace-cmd. But that's fine, because I have a plan to fix that too. But
that will be on the ftrace said.

Thanks!

-- Steve

Re: [RFC][PATCH] writeback: Unduplicate writeback reason

[Dave Chinner]

On Tue, Dec 13, 2011 at 08:14:00PM -0500, Steven Rostedt wrote:
> 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.

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....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC][PATCH] writeback: Unduplicate writeback reason

[Wu Fengguang]

On Wed, Dec 14, 2011 at 11:28:44AM +0800, Dave Chinner wrote:
> On Tue, Dec 13, 2011 at 08:14:00PM -0500, Steven Rostedt wrote:
> > 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.
> 
> 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...

> 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.

So for the upcoming 3.2, wb_reason_name[] can be removed to avoid the
duplication. However the question still remains how exactly are we
going to re-introduce it in future?

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

> 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.
So we may be talking about different situations.

Thanks,
Fengguang

Re: [RFC][PATCH] writeback: Unduplicate writeback reason

[Dave Chinner]

On Wed, Dec 14, 2011 at 09:16:19PM +0800, Wu Fengguang wrote:
> On Wed, Dec 14, 2011 at 11:28:44AM +0800, Dave Chinner wrote:
> > On Tue, Dec 13, 2011 at 08:14:00PM -0500, Steven Rostedt wrote:
> > > 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.
> > 
> > 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...
> 
> > 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.

Ah, ok. I missed that context.

.....

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

Yeah, that would work, and still place the definitions close
together....

> 
> > 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.

Right. I didn't realise the writeback stuff was eventually going to
be shared with sysfs. Still, it looks like it works better than
playing macro redefinition games with you addition above, so perhaps
it was best I didn't realise this in the first place. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com