[PATCH 21/22] xfs: %pF is only for function pointers

[PATCH 21/22] xfs: %pF is only for function pointers

[Scott Wood]

Use %pS for actual addresses, otherwise you'll get bad output
on arches like ppc64 where %pF expects a function descriptor.

Signed-off-by: Scott Wood <scottwood@freescale.com>
Cc: xfs@oss.sgi.com
---
 fs/xfs/xfs_error.c |  2 +-
 fs/xfs/xfs_trace.h | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 3ee186a..338e50b 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -131,7 +131,7 @@ xfs_error_report(
 {
 	if (level <= xfs_error_level) {
 		xfs_alert_tag(mp, XFS_PTAG_ERROR_REPORT,
-		"Internal error %s at line %d of file %s.  Caller %pF",
+		"Internal error %s at line %d of file %s.  Caller %pS",
 			    tag, linenum, filename, ra);
 
 		xfs_stack_trace();
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 51372e3..b5ac81e 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -115,7 +115,7 @@ DECLARE_EVENT_CLASS(xfs_perag_class,
 		__entry->refcount = refcount;
 		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d agno %u refcount %d caller %pf",
+	TP_printk("dev %d:%d agno %u refcount %d caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->agno,
 		  __entry->refcount,
@@ -239,7 +239,7 @@ TRACE_EVENT(xfs_iext_insert,
 		__entry->caller_ip = caller_ip;
 	),
 	TP_printk("dev %d:%d ino 0x%llx state %s idx %ld "
-		  "offset %lld block %lld count %lld flag %d caller %pf",
+		  "offset %lld block %lld count %lld flag %d caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __print_flags(__entry->bmap_state, "|", XFS_BMAP_EXT_FLAGS),
@@ -283,7 +283,7 @@ DECLARE_EVENT_CLASS(xfs_bmap_class,
 		__entry->caller_ip = caller_ip;
 	),
 	TP_printk("dev %d:%d ino 0x%llx state %s idx %ld "
-		  "offset %lld block %lld count %lld flag %d caller %pf",
+		  "offset %lld block %lld count %lld flag %d caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __print_flags(__entry->bmap_state, "|", XFS_BMAP_EXT_FLAGS),
@@ -329,7 +329,7 @@ DECLARE_EVENT_CLASS(xfs_buf_class,
 		__entry->caller_ip = caller_ip;
 	),
 	TP_printk("dev %d:%d bno 0x%llx nblks 0x%x hold %d pincount %d "
-		  "lock %d flags %s caller %pf",
+		  "lock %d flags %s caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  (unsigned long long)__entry->bno,
 		  __entry->nblks,
@@ -402,7 +402,7 @@ DECLARE_EVENT_CLASS(xfs_buf_flags_class,
 		__entry->caller_ip = caller_ip;
 	),
 	TP_printk("dev %d:%d bno 0x%llx len 0x%zx hold %d pincount %d "
-		  "lock %d flags %s caller %pf",
+		  "lock %d flags %s caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  (unsigned long long)__entry->bno,
 		  __entry->buffer_length,
@@ -447,7 +447,7 @@ TRACE_EVENT(xfs_buf_ioerror,
 		__entry->caller_ip = caller_ip;
 	),
 	TP_printk("dev %d:%d bno 0x%llx len 0x%zx hold %d pincount %d "
-		  "lock %d error %d flags %s caller %pf",
+		  "lock %d error %d flags %s caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  (unsigned long long)__entry->bno,
 		  __entry->buffer_length,
@@ -613,7 +613,7 @@ DECLARE_EVENT_CLASS(xfs_lock_class,
 		__entry->lock_flags = lock_flags;
 		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d ino 0x%llx flags %s caller %pf",
+	TP_printk("dev %d:%d ino 0x%llx flags %s caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __print_flags(__entry->lock_flags, "|", XFS_LOCK_FLAGS),
@@ -702,7 +702,7 @@ DECLARE_EVENT_CLASS(xfs_iref_class,
 		__entry->pincount = atomic_read(&ip->i_pincount);
 		__entry->caller_ip = caller_ip;
 	),
-	TP_printk("dev %d:%d ino 0x%llx count %d pincount %d caller %pf",
+	TP_printk("dev %d:%d ino 0x%llx count %d pincount %d caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __entry->count,
@@ -1333,7 +1333,7 @@ TRACE_EVENT(xfs_bunmap,
 		__entry->flags = flags;
 	),
 	TP_printk("dev %d:%d ino 0x%llx size 0x%llx bno 0x%llx len 0x%llx"
-		  "flags %s caller %pf",
+		  "flags %s caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
 		  __entry->size,
@@ -1466,7 +1466,7 @@ TRACE_EVENT(xfs_agf,
 	),
 	TP_printk("dev %d:%d agno %u flags %s length %u roots b %u c %u "
 		  "levels b %u c %u flfirst %u fllast %u flcount %u "
-		  "freeblks %u longest %u caller %pf",
+		  "freeblks %u longest %u caller %ps",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->agno,
 		  __print_flags(__entry->flags, "|", XFS_AGF_FLAGS),
-- 
2.1.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 21/22] xfs: %pF is only for function pointers

[Dave Chinner]

On Wed, Mar 11, 2015 at 10:13:56PM -0500, Scott Wood wrote:
> Use %pS for actual addresses, otherwise you'll get bad output
> on arches like ppc64 where %pF expects a function descriptor.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Cc: xfs@oss.sgi.com

Scott, I've just found that this change (commit 65dd297 "xfs: %pF is
only for function pointers") breaks the symbolic printing in XFS
trace events on x86_64. eg.

> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 51372e3..b5ac81e 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -115,7 +115,7 @@ DECLARE_EVENT_CLASS(xfs_perag_class,
>  		__entry->refcount = refcount;
>  		__entry->caller_ip = caller_ip;
>  	),
> -	TP_printk("dev %d:%d agno %u refcount %d caller %pf",
> +	TP_printk("dev %d:%d agno %u refcount %d caller %ps",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  __entry->agno,
>  		  __entry->refcount,

This results in output like this:

760.828474: xfs_perag_get:  dev 253:32 agno 13 refcount 10 caller 0xffffffff814eef02s
760.828476: xfs_perag_put:  dev 253:32 agno 13 refcount 9 caller 0xffffffff814eefe8s

When I revert this commit, I get:

71.911265: xfs_perag_get:   dev 253:32 agno 0 refcount 11 caller xfs_extent_busy_insert
71.911266: xfs_perag_put:   dev 253:32 agno 0 refcount 10 caller xfs_extent_busy_insert

Which is exactly what we should be getting from the tracing. I'm
using trace-cmd to gather and print the events, and it breaks
both old and current versions of trace-cmd.

Can you please look into why this change broke the tracing output
on x86-64 - if there is no obvious/easy fix for it, then I'm simply
going to revert it because having the tracing work correctly on
x86-64 is far more important to us than ppc64 or ia64....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 21/22] xfs: %pF is only for function pointers

[Steven Rostedt]

On Mon, 31 Aug 2015 14:24:25 -0500
Scott Wood <scottwood@freescale.com> wrote:
 
> > Can you please look into why this change broke the tracing output
> > on x86-64 - if there is no obvious/easy fix for it, then I'm simply
> > going to revert it because having the tracing work correctly on
> > x86-64 is far more important to us than ppc64 or ia64....
> 
> It looks like the cause is that TP_printk() is not really printk() -- it 
> actually passes the format to userspace which has its own, not 100% 
> compatible implementation pretty_print() in tools/lib/traceevent/event-
> parse.c.  %pf in that function behaves like %ps in the kernel, and %ps is 
> absent.
> 

We can fix that with adding %ps to the traceevent library.

-- Steve

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 21/22] xfs: %pF is only for function pointers

[Steven Rostedt]

On Mon, 31 Aug 2015 14:58:24 -0500
Scott Wood <scottwood@freescale.com> wrote:

> > We can fix that with adding %ps to the traceevent library.
> 
> I wasn't sure if this would be considered a stable ABI issue, as it's not 
> about the events themselves, but about the event mechanism.

When it comes to trace events, there's a fine line about the use space
stable ABI. Even Linus has mentioned that tracing and perf counters are
"special", as the two are not about a feature of the kernel, but
instead a way to see how the kernel works internally.

I've fixed up trace-cmd and libtraceevent more than once in the past
due to changes in the kernel. Unless it truly breaks the tool, it
should be fine to fix up tracepoints to handle small changes like this.

-- Steve

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs