Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1216081 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 21/22] xfs: %pF is only for function pointers Dave Chinner <david@fromorbit.com> - 2015-08-31 10:10 +0200
    Re: [PATCH 21/22] xfs: %pF is only for function pointers Scott Wood <scottwood@freescale.com> - 2015-08-31 21:30 +0200
      Re: [PATCH 21/22] xfs: %pF is only for function pointers Steven Rostedt <rostedt@goodmis.org> - 2015-08-31 21:50 +0200
        Re: [PATCH 21/22] xfs: %pF is only for function pointers Scott Wood <scottwood@freescale.com> - 2015-08-31 22:00 +0200
          Re: [PATCH 21/22] xfs: %pF is only for function pointers Steven Rostedt <rostedt@goodmis.org> - 2015-08-31 22:10 +0200

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1216390

On Mon, 2015-08-31 at 18:06 +1000, Dave Chinner wrote:
> 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....

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.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216408

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216410

On Mon, 2015-08-31 at 15:45 -0400, Steven Rostedt wrote:
> 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.

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.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216412

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web