[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

[PATCH 0/9] readahead stats/tracing, backwards prefetching and more (v2)

[Wu Fengguang]

Andrew,

This is what the bit fields look like :)

Changes since v1:
- use bit fields: pattern, for_mmap, for_metadata, lseek
- comment the various readahead patterns
- drop boot options "readahead=" and "readahead_stats="
- add for_metadata
- add snapping to EOF

 [PATCH 1/9] block: limit default readahead size for small devices
 [PATCH 2/9] readahead: snap readahead request to EOF
 [PATCH 3/9] readahead: record readahead patterns
 [PATCH 4/9] readahead: tag mmap page fault call sites
 [PATCH 5/9] readahead: tag metadata call sites
 [PATCH 6/9] readahead: add /debug/readahead/stats
 [PATCH 7/9] readahead: add vfs/readahead tracing event
 [PATCH 8/9] readahead: basic support for backwards prefetching
 [PATCH 9/9] readahead: dont do start-of-file readahead after lseek()

 block/genhd.c              |   20 ++
 fs/ext3/dir.c              |    1 
 fs/ext4/dir.c              |    1 
 fs/read_write.c            |    3 
 include/linux/fs.h         |   41 +++++
 include/linux/mm.h         |    4 
 include/trace/events/vfs.h |   64 ++++++++
 mm/Kconfig                 |   15 ++
 mm/filemap.c               |    9 -
 mm/readahead.c             |  257 ++++++++++++++++++++++++++++++++++-
 10 files changed, 404 insertions(+), 11 deletions(-)

Thanks,
Fengguang

[PATCH 7/9] readahead: add vfs/readahead tracing event

[Wu Fengguang]

[-- Attachment #1: readahead-tracer.patch --]
[-- Type: text/plain, Size: 3853 bytes --]

This is very useful for verifying whether the readahead algorithms are
working to the expectation.

Example output:

# echo 1 > /debug/tracing/events/vfs/readahead/enable
# cp test-file /dev/null
# cat /debug/tracing/trace  # trimmed output
readahead-initial(dev=0:15, ino=100177, req=0+2, ra=0+4-2, async=0) = 4
readahead-subsequent(dev=0:15, ino=100177, req=2+2, ra=4+8-8, async=1) = 8
readahead-subsequent(dev=0:15, ino=100177, req=4+2, ra=12+16-16, async=1) = 16
readahead-subsequent(dev=0:15, ino=100177, req=12+2, ra=28+32-32, async=1) = 32
readahead-subsequent(dev=0:15, ino=100177, req=28+2, ra=60+60-60, async=1) = 24
readahead-subsequent(dev=0:15, ino=100177, req=60+2, ra=120+60-60, async=1) = 0

CC: Ingo Molnar <mingo@elte.hu>
CC: Jens Axboe <axboe@kernel.dk>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Rik van Riel <riel@redhat.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 include/trace/events/vfs.h |   64 +++++++++++++++++++++++++++++++++++
 mm/readahead.c             |    5 ++
 2 files changed, 69 insertions(+)

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-next/include/trace/events/vfs.h	2011-11-29 20:58:59.000000000 +0800
@@ -0,0 +1,64 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM vfs
+
+#if !defined(_TRACE_VFS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_VFS_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(readahead,
+	TP_PROTO(struct address_space *mapping,
+		 pgoff_t offset,
+		 unsigned long req_size,
+		 enum readahead_pattern pattern,
+		 pgoff_t start,
+		 unsigned long size,
+		 unsigned long async_size,
+		 unsigned int actual),
+
+	TP_ARGS(mapping, offset, req_size, pattern, start, size, async_size,
+		actual),
+
+	TP_STRUCT__entry(
+		__field(	dev_t,		dev		)
+		__field(	ino_t,		ino		)
+		__field(	pgoff_t,	offset		)
+		__field(	unsigned long,	req_size	)
+		__field(	unsigned int,	pattern		)
+		__field(	pgoff_t,	start		)
+		__field(	unsigned int,	size		)
+		__field(	unsigned int,	async_size	)
+		__field(	unsigned int,	actual		)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= mapping->host->i_sb->s_dev;
+		__entry->ino		= mapping->host->i_ino;
+		__entry->offset		= offset;
+		__entry->req_size	= req_size;
+		__entry->pattern	= pattern;
+		__entry->start		= start;
+		__entry->size		= size;
+		__entry->async_size	= async_size;
+		__entry->actual		= actual;
+	),
+
+	TP_printk("readahead-%s(dev=%d:%d, ino=%lu, "
+		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
+			ra_pattern_names[__entry->pattern],
+			MAJOR(__entry->dev),
+			MINOR(__entry->dev),
+			__entry->ino,
+			__entry->offset,
+			__entry->req_size,
+			__entry->start,
+			__entry->size,
+			__entry->async_size,
+			__entry->start > __entry->offset,
+			__entry->actual)
+);
+
+#endif /* _TRACE_VFS_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--- linux-next.orig/mm/readahead.c	2011-11-29 20:58:53.000000000 +0800
+++ linux-next/mm/readahead.c	2011-11-29 20:59:20.000000000 +0800
@@ -29,6 +29,9 @@ static const char * const ra_pattern_nam
 	[RA_PATTERN_ALL]                = "all",
 };
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/vfs.h>
+
 /*
  * Initialise a struct file's readahead state.  Assumes that the caller has
  * memset *ra to zero.
@@ -215,6 +218,8 @@ static inline void readahead_event(struc
 				for_mmap, for_metadata,
 				pattern, start, size, async_size, actual);
 #endif
+	trace_readahead(mapping, offset, req_size,
+			pattern, start, size, async_size, actual);
 }
 
 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 7/9] readahead: add vfs/readahead tracing event

[Christoph Hellwig]

> +	TP_printk("readahead-%s(dev=%d:%d, ino=%lu, "

please don't duplicate the tracepoint name in the output string.
Also don't use braces, as it jsut complicates parsing.

> +		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
> +			ra_pattern_names[__entry->pattern],

Instead of doing a manual array lookup please use __print_symbolic so
that users of the binary interface (like trace-cmd) also get the
right output.

> --- linux-next.orig/mm/readahead.c	2011-11-29 20:58:53.000000000 +0800
> +++ linux-next/mm/readahead.c	2011-11-29 20:59:20.000000000 +0800
> @@ -29,6 +29,9 @@ static const char * const ra_pattern_nam
>  	[RA_PATTERN_ALL]                = "all",
>  };
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/vfs.h>

Maybe we should create a new fs/trace.c just for this instead of stickin
it into the first file that created a tracepoint in the "vfs" namespace.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] writeback: show writeback reason with __print_symbolic

[Wu Fengguang]

> > +		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
> > +			ra_pattern_names[__entry->pattern],
> 
> Instead of doing a manual array lookup please use __print_symbolic so
> that users of the binary interface (like trace-cmd) also get the
> right output.

FYI, here is the related fix on writeback traces.

---
This makes the traces trace-cmd friendly, 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,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>