[PATCH 0/7] add syscall tracepoints

[PATCH 0/7] add syscall tracepoints

[Jason Baron]

hi,

The following is an implementation of Frederic's syscall tracer on top of
tracepoints. It adds the ability to toggle the entry/exit of each syscall
via the standard events/syscalls/syscall_blah/enable interface. The 
implementation is done by adding 2 tracepoints. One on entry and one for exit.

This implementation required a few 'core' api changes. I've added 
'DECLARE_TRACE_REG()' macro which takes a register and and an unregister
function as arguments. This allowed me to toggle the ftrace tif flag
when the first tracepoint callback is added and the last is removed. Current
callers of 'DECLARE_TRACE()' are not impacted.

Another change was to call arch_init_ftrace_syscalls via an 'arch_initall'. In
this implmentation I needed to access the syscalls_metadata structure at
runtime in order to determine which syscalls were 'traceable'. Although the
implementation uses SYSCALL_DEFINE() to set up the the trace events, for
some reason at runtime there is no syscalls_metadata, associated with some of
the SYSCALL_DEFINE() calls. I'm not quite sure why that is. However, by
calling arch_init_ftrace_syscalls() at boot I can make sure the lists are in
sync.

thanks,

-Jason 


 arch/x86/include/asm/ftrace.h |    4 +-
 arch/x86/kernel/ftrace.c      |   24 +++++-
 arch/x86/kernel/ptrace.c      |    6 +-
 include/linux/syscalls.h      |   63 +++++++++++++++
 include/linux/tracepoint.h    |   27 ++++++-
 include/trace/syscall.h       |   37 +++++++---
 kernel/trace/trace_events.c   |   29 +++++---
 kernel/trace/trace_syscalls.c |  172 +++++++++++++++++++----------------------
 kernel/tracepoint.c           |   38 +++++++++
 9 files changed, 278 insertions(+), 122 deletions(-)

[PATCH 1/7] add syscall tracepoints

[Jason Baron]

Add a new function to support translating a syscall name to number at runtime.
This allows the syscall event tracer to map syscall names to number.


Signed-off-by: Jason Baron <jbaron@redhat.com>

---
 arch/x86/kernel/ftrace.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index b79c553..30baa5b 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -502,6 +502,22 @@ struct syscall_metadata *syscall_nr_to_meta(int nr)
 	return syscalls_metadata[nr];
 }
 
+int syscall_name_to_nr(char *name)
+{
+	int i;
+
+	if (!syscalls_metadata)
+		return -1;
+
+	for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
+		if (syscalls_metadata[i]) {
+			if (!strcmp((syscalls_metadata[i])->name, name))
+				return i;
+		}
+	}
+	return -1;
+}
+
 void arch_init_ftrace_syscalls(void)
 {
 	int i;
-- 
1.6.0.6

Re: [PATCH 1/7] add syscall tracepoints

[Li Zefan]

> +int syscall_name_to_nr(char *name)
> +{
> +	int i;
> +
> +	if (!syscalls_metadata)
> +		return -1;
> +
> +	for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
> +		if (syscalls_metadata[i]) {
> +			if (!strcmp((syscalls_metadata[i])->name, name))

Remove a pair of parenthesis looks cleaner:

			if (!strcmp(syscalls_metadata[i]->name, name))

> +				return i;
> +		}
> +	}
> +	return -1;
> +}
> +
>  void arch_init_ftrace_syscalls(void)
>  {
>  	int i;

[PATCH 2/7] add syscall tracepoints

[Jason Baron]

Call arch_init_ftrace_syscalls at boot, so we can determine the set of syscalls
for the syscall trace events.


Signed-off-by: Jason Baron <jbaron@redhat.com>

---
 arch/x86/kernel/ftrace.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 30baa5b..9f2aa83 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -518,7 +518,7 @@ int syscall_name_to_nr(char *name)
 	return -1;
 }
 
-void arch_init_ftrace_syscalls(void)
+int arch_init_ftrace_syscalls(void)
 {
 	int i;
 	struct syscall_metadata *meta;
@@ -532,17 +532,19 @@ void arch_init_ftrace_syscalls(void)
 					FTRACE_SYSCALL_MAX, GFP_KERNEL);
 	if (!syscalls_metadata) {
 		WARN_ON(1);
-		return;
+		return -ENOMEM;
 	}
 
 	for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
 		meta = find_syscall_meta(psys_syscall_table[i]);
 		syscalls_metadata[i] = meta;
 	}
-	return;
+	return 0;
 
 	/* Paranoid: avoid overflow */
 end:
 	atomic_dec(&refs);
+	return 0;
 }
+arch_initcall(arch_init_ftrace_syscalls);
 #endif
-- 
1.6.0.6

Re: [PATCH 2/7] add syscall tracepoints

[Steven Rostedt]

On Fri, 12 Jun 2009, Jason Baron wrote:

> 
> Call arch_init_ftrace_syscalls at boot, so we can determine the set of syscalls
> for the syscall trace events.
> 
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> ---
>  arch/x86/kernel/ftrace.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 30baa5b..9f2aa83 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -518,7 +518,7 @@ int syscall_name_to_nr(char *name)
>  	return -1;
>  }
>  
> -void arch_init_ftrace_syscalls(void)
> +int arch_init_ftrace_syscalls(void)
>  {
>  	int i;
>  	struct syscall_metadata *meta;
> @@ -532,17 +532,19 @@ void arch_init_ftrace_syscalls(void)
>  					FTRACE_SYSCALL_MAX, GFP_KERNEL);
>  	if (!syscalls_metadata) {
>  		WARN_ON(1);
> -		return;
> +		return -ENOMEM;
>  	}
>  
>  	for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
>  		meta = find_syscall_meta(psys_syscall_table[i]);
>  		syscalls_metadata[i] = meta;
>  	}
> -	return;
> +	return 0;
>  
>  	/* Paranoid: avoid overflow */
>  end:
>  	atomic_dec(&refs);
> +	return 0;
>  }
> +arch_initcall(arch_init_ftrace_syscalls);

If we add this, can't we make it static and remove the call to it from 
kernel/trace/trace_syscalls.c ?

-- Steve

>  #endif
> -- 
> 1.6.0.6
> 
> 

Re: [PATCH 2/7] add syscall tracepoints

[Li Zefan]

Jason Baron wrote:
> Call arch_init_ftrace_syscalls at boot, so we can determine the set of syscalls
> for the syscall trace events.
> 
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> ---
>  arch/x86/kernel/ftrace.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 30baa5b..9f2aa83 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -518,7 +518,7 @@ int syscall_name_to_nr(char *name)
>  	return -1;
>  }
>  
> -void arch_init_ftrace_syscalls(void)
> +int arch_init_ftrace_syscalls(void)

1. It's called at boot, so can be __init.

2. This patch breaks bisectability, now the declaration and definition
   of this function don't match.

>  {
>  	int i;
>  	struct syscall_metadata *meta;
> @@ -532,17 +532,19 @@ void arch_init_ftrace_syscalls(void)
>  					FTRACE_SYSCALL_MAX, GFP_KERNEL);
>  	if (!syscalls_metadata) {
>  		WARN_ON(1);
> -		return;
> +		return -ENOMEM;
>  	}
>  
>  	for (i = 0; i < FTRACE_SYSCALL_MAX; i++) {
>  		meta = find_syscall_meta(psys_syscall_table[i]);
>  		syscalls_metadata[i] = meta;
>  	}
> -	return;
> +	return 0;
>  
>  	/* Paranoid: avoid overflow */
>  end:
>  	atomic_dec(&refs);
> +	return 0;
>  }
> +arch_initcall(arch_init_ftrace_syscalls);
>  #endif

[PATCH 3/7] add syscall tracepoints

[Jason Baron]

Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
an external register/unregister function.


Signed-off-by: Jason Baron <jbaron@redhat.com>

---
 include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 14df7e6..9a3660b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -61,7 +61,7 @@ struct tracepoint {
  * not add unwanted padding between the beginning of the section and the
  * structure. Force alignment to the same alignment as the section start.
  */
-#define DECLARE_TRACE(name, proto, args)				\
+#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
 	extern struct tracepoint __tracepoint_##name;			\
 	static inline void trace_##name(proto)				\
 	{								\
@@ -71,13 +71,29 @@ struct tracepoint {
 	}								\
 	static inline int register_trace_##name(void (*probe)(proto))	\
 	{								\
-		return tracepoint_probe_register(#name, (void *)probe);	\
+		int ret;						\
+		void (*func)(void) = (void (*)(void))reg;		\
+									\
+		ret = tracepoint_probe_register(#name, (void *)probe);	\
+		if (func && !ret)					\
+			func();						\
+		return ret;						\
 	}								\
 	static inline int unregister_trace_##name(void (*probe)(proto))	\
 	{								\
-		return tracepoint_probe_unregister(#name, (void *)probe);\
+		int ret;						\
+		void (*func)(void) = (void (*)(void))unreg;		\
+									\
+		ret = tracepoint_probe_unregister(#name, (void *)probe);\
+		if (func && !ret)					\
+			func();						\
+		return ret;						\
 	}
 
+
+#define DECLARE_TRACE(name, proto, args)		\
+	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
+
 #define DEFINE_TRACE(name)						\
 	static const char __tpstrtab_##name[]				\
 	__attribute__((section("__tracepoints_strings"))) = #name;	\
@@ -94,7 +110,7 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
 	struct tracepoint *end);
 
 #else /* !CONFIG_TRACEPOINTS */
-#define DECLARE_TRACE(name, proto, args)				\
+#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
 	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
 	{ }								\
 	static inline void trace_##name(proto)				\
@@ -108,6 +124,9 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
 		return -ENOSYS;						\
 	}
 
+#define DECLARE_TRACE(name, proto, args)                \
+	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
+
 #define DEFINE_TRACE(name)
 #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
 #define EXPORT_TRACEPOINT_SYMBOL(name)
-- 
1.6.0.6

Re: [PATCH 3/7] add syscall tracepoints

[Mathieu Desnoyers]

* Jason Baron (jbaron@redhat.com) wrote:
> 
> Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> an external register/unregister function.
> 
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> ---
>  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
>  1 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 14df7e6..9a3660b 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -61,7 +61,7 @@ struct tracepoint {
>   * not add unwanted padding between the beginning of the section and the
>   * structure. Force alignment to the same alignment as the section start.
>   */
> -#define DECLARE_TRACE(name, proto, args)				\
> +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
>  	extern struct tracepoint __tracepoint_##name;			\
>  	static inline void trace_##name(proto)				\
>  	{								\
> @@ -71,13 +71,29 @@ struct tracepoint {
>  	}								\
>  	static inline int register_trace_##name(void (*probe)(proto))	\
>  	{								\
> -		return tracepoint_probe_register(#name, (void *)probe);	\
> +		int ret;						\
> +		void (*func)(void) = (void (*)(void))reg;		\
> +									\
> +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> +		if (func && !ret)					\
> +			func();						\

I don't see why you need to add this weird interface when all you really
need to do is to call the function to set the TIF flags explicitly in
reg_event_syscall_enter when registering a tracepoint.

Mathieu

> +		return ret;						\
>  	}								\
>  	static inline int unregister_trace_##name(void (*probe)(proto))	\
>  	{								\
> -		return tracepoint_probe_unregister(#name, (void *)probe);\
> +		int ret;						\
> +		void (*func)(void) = (void (*)(void))unreg;		\
> +									\
> +		ret = tracepoint_probe_unregister(#name, (void *)probe);\
> +		if (func && !ret)					\
> +			func();						\
> +		return ret;						\
>  	}
>  
> +
> +#define DECLARE_TRACE(name, proto, args)		\
> +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> +
>  #define DEFINE_TRACE(name)						\
>  	static const char __tpstrtab_##name[]				\
>  	__attribute__((section("__tracepoints_strings"))) = #name;	\
> @@ -94,7 +110,7 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
>  	struct tracepoint *end);
>  
>  #else /* !CONFIG_TRACEPOINTS */
> -#define DECLARE_TRACE(name, proto, args)				\
> +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
>  	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
>  	{ }								\
>  	static inline void trace_##name(proto)				\
> @@ -108,6 +124,9 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
>  		return -ENOSYS;						\
>  	}
>  
> +#define DECLARE_TRACE(name, proto, args)                \
> +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> +
>  #define DEFINE_TRACE(name)
>  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
>  #define EXPORT_TRACEPOINT_SYMBOL(name)
> -- 
> 1.6.0.6
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH 3/7] add syscall tracepoints

[Jason Baron]

On Fri, Jun 12, 2009 at 05:57:26PM -0400, Mathieu Desnoyers wrote:
> > Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> > an external register/unregister function.
> > 
> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > 
> > ---
> >  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
> >  1 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 14df7e6..9a3660b 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -61,7 +61,7 @@ struct tracepoint {
> >   * not add unwanted padding between the beginning of the section and the
> >   * structure. Force alignment to the same alignment as the section start.
> >   */
> > -#define DECLARE_TRACE(name, proto, args)				\
> > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> >  	extern struct tracepoint __tracepoint_##name;			\
> >  	static inline void trace_##name(proto)				\
> >  	{								\
> > @@ -71,13 +71,29 @@ struct tracepoint {
> >  	}								\
> >  	static inline int register_trace_##name(void (*probe)(proto))	\
> >  	{								\
> > -		return tracepoint_probe_register(#name, (void *)probe);	\
> > +		int ret;						\
> > +		void (*func)(void) = (void (*)(void))reg;		\
> > +									\
> > +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> > +		if (func && !ret)					\
> > +			func();						\
> 
> I don't see why you need to add this weird interface when all you really
> need to do is to call the function to set the TIF flags explicitly in
> reg_event_syscall_enter when registering a tracepoint.
> 
> Mathieu
> 

I could enable the TIF flag in reg_event_syscall_enter, however that
would not manage the TIF flag for other users of these traceoints. When
users 'register/unregister' with a tracepoint, they expect the tracepoint
to be enabled/disabled. If we move this functionality to the user, we are
changing how that interface works. Therefore, I associated the
enabling/disabling of the tracepoint, with the tracepoint definition.

thanks,

-Jason

Re: [PATCH 3/7] add syscall tracepoints

[Mathieu Desnoyers]

* Jason Baron (jbaron@redhat.com) wrote:
> On Fri, Jun 12, 2009 at 05:57:26PM -0400, Mathieu Desnoyers wrote:
> > > Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> > > an external register/unregister function.
> > > 
> > > 
> > > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > > 
> > > ---
> > >  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
> > >  1 files changed, 23 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > index 14df7e6..9a3660b 100644
> > > --- a/include/linux/tracepoint.h
> > > +++ b/include/linux/tracepoint.h
> > > @@ -61,7 +61,7 @@ struct tracepoint {
> > >   * not add unwanted padding between the beginning of the section and the
> > >   * structure. Force alignment to the same alignment as the section start.
> > >   */
> > > -#define DECLARE_TRACE(name, proto, args)				\
> > > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> > >  	extern struct tracepoint __tracepoint_##name;			\
> > >  	static inline void trace_##name(proto)				\
> > >  	{								\
> > > @@ -71,13 +71,29 @@ struct tracepoint {
> > >  	}								\
> > >  	static inline int register_trace_##name(void (*probe)(proto))	\
> > >  	{								\
> > > -		return tracepoint_probe_register(#name, (void *)probe);	\
> > > +		int ret;						\
> > > +		void (*func)(void) = (void (*)(void))reg;		\
> > > +									\
> > > +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> > > +		if (func && !ret)					\
> > > +			func();						\
> > 
> > I don't see why you need to add this weird interface when all you really
> > need to do is to call the function to set the TIF flags explicitly in
> > reg_event_syscall_enter when registering a tracepoint.
> > 
> > Mathieu
> > 
> 
> I could enable the TIF flag in reg_event_syscall_enter, however that
> would not manage the TIF flag for other users of these traceoints. When
> users 'register/unregister' with a tracepoint, they expect the tracepoint
> to be enabled/disabled. If we move this functionality to the user, we are
> changing how that interface works. Therefore, I associated the
> enabling/disabling of the tracepoint, with the tracepoint definition.
> 

I agree it should be hidden from userspace APIs, but I don't think we
should hide it or from the "in kernel" API users, really. People
interfacing with this kind of API from the kernel-side expect to have a
great level of control on how they use it, and we can expect people to
know what they are doing.

Mathieu


> thanks,
> 
> -Jason

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH 3/7] add syscall tracepoints

[Frederic Weisbecker]

On Mon, Jun 15, 2009 at 11:24:28AM -0400, Mathieu Desnoyers wrote:
> * Jason Baron (jbaron@redhat.com) wrote:
> > On Fri, Jun 12, 2009 at 05:57:26PM -0400, Mathieu Desnoyers wrote:
> > > > Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> > > > an external register/unregister function.
> > > > 
> > > > 
> > > > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > > > 
> > > > ---
> > > >  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
> > > >  1 files changed, 23 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > > index 14df7e6..9a3660b 100644
> > > > --- a/include/linux/tracepoint.h
> > > > +++ b/include/linux/tracepoint.h
> > > > @@ -61,7 +61,7 @@ struct tracepoint {
> > > >   * not add unwanted padding between the beginning of the section and the
> > > >   * structure. Force alignment to the same alignment as the section start.
> > > >   */
> > > > -#define DECLARE_TRACE(name, proto, args)				\
> > > > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> > > >  	extern struct tracepoint __tracepoint_##name;			\
> > > >  	static inline void trace_##name(proto)				\
> > > >  	{								\
> > > > @@ -71,13 +71,29 @@ struct tracepoint {
> > > >  	}								\
> > > >  	static inline int register_trace_##name(void (*probe)(proto))	\
> > > >  	{								\
> > > > -		return tracepoint_probe_register(#name, (void *)probe);	\
> > > > +		int ret;						\
> > > > +		void (*func)(void) = (void (*)(void))reg;		\
> > > > +									\
> > > > +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> > > > +		if (func && !ret)					\
> > > > +			func();						\
> > > 
> > > I don't see why you need to add this weird interface when all you really
> > > need to do is to call the function to set the TIF flags explicitly in
> > > reg_event_syscall_enter when registering a tracepoint.
> > > 
> > > Mathieu
> > > 
> > 
> > I could enable the TIF flag in reg_event_syscall_enter, however that
> > would not manage the TIF flag for other users of these traceoints. When
> > users 'register/unregister' with a tracepoint, they expect the tracepoint
> > to be enabled/disabled. If we move this functionality to the user, we are
> > changing how that interface works. Therefore, I associated the
> > enabling/disabling of the tracepoint, with the tracepoint definition.
> > 
> 
> I agree it should be hidden from userspace APIs, but I don't think we
> should hide it or from the "in kernel" API users, really. People
> interfacing with this kind of API from the kernel-side expect to have a
> great level of control on how they use it, and we can expect people to
> know what they are doing.
> 
> Mathieu


Indeed it's fine to let the user of the tracepoint have a good
control of what is happening, but actually there is no point in
registering this one without having the TIF_FLAGS set, so it
seems legitimate to handle it like Jason did.
Remember it's a very specific tracepoint that needs these thread
flags to be activated.

Also this management of thread flags would become fragile once
you let the user deal with it concurrently with the event
registering.

I think it's more sane/safe to encapsulate it as it is.

 
> 
> > thanks,
> > 
> > -Jason
> 
> -- 
> Mathieu Desnoyers
> OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH 3/7] add syscall tracepoints

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Mon, Jun 15, 2009 at 11:24:28AM -0400, Mathieu Desnoyers wrote:
> > * Jason Baron (jbaron@redhat.com) wrote:
> > > On Fri, Jun 12, 2009 at 05:57:26PM -0400, Mathieu Desnoyers wrote:
> > > > > Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> > > > > an external register/unregister function.
> > > > > 
> > > > > 
> > > > > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > > > > 
> > > > > ---
> > > > >  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
> > > > >  1 files changed, 23 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > > > index 14df7e6..9a3660b 100644
> > > > > --- a/include/linux/tracepoint.h
> > > > > +++ b/include/linux/tracepoint.h
> > > > > @@ -61,7 +61,7 @@ struct tracepoint {
> > > > >   * not add unwanted padding between the beginning of the section and the
> > > > >   * structure. Force alignment to the same alignment as the section start.
> > > > >   */
> > > > > -#define DECLARE_TRACE(name, proto, args)				\
> > > > > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> > > > >  	extern struct tracepoint __tracepoint_##name;			\
> > > > >  	static inline void trace_##name(proto)				\
> > > > >  	{								\
> > > > > @@ -71,13 +71,29 @@ struct tracepoint {
> > > > >  	}								\
> > > > >  	static inline int register_trace_##name(void (*probe)(proto))	\
> > > > >  	{								\
> > > > > -		return tracepoint_probe_register(#name, (void *)probe);	\
> > > > > +		int ret;						\
> > > > > +		void (*func)(void) = (void (*)(void))reg;		\
> > > > > +									\
> > > > > +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> > > > > +		if (func && !ret)					\
> > > > > +			func();						\
> > > > 
> > > > I don't see why you need to add this weird interface when all you really
> > > > need to do is to call the function to set the TIF flags explicitly in
> > > > reg_event_syscall_enter when registering a tracepoint.
> > > > 
> > > > Mathieu
> > > > 
> > > 
> > > I could enable the TIF flag in reg_event_syscall_enter, however that
> > > would not manage the TIF flag for other users of these traceoints. When
> > > users 'register/unregister' with a tracepoint, they expect the tracepoint
> > > to be enabled/disabled. If we move this functionality to the user, we are
> > > changing how that interface works. Therefore, I associated the
> > > enabling/disabling of the tracepoint, with the tracepoint definition.
> > > 
> > 
> > I agree it should be hidden from userspace APIs, but I don't think we
> > should hide it or from the "in kernel" API users, really. People
> > interfacing with this kind of API from the kernel-side expect to have a
> > great level of control on how they use it, and we can expect people to
> > know what they are doing.
> > 
> > Mathieu
> 
> 
> Indeed it's fine to let the user of the tracepoint have a good
> control of what is happening, but actually there is no point in
> registering this one without having the TIF_FLAGS set, so it
> seems legitimate to handle it like Jason did.
> Remember it's a very specific tracepoint that needs these thread
> flags to be activated.
> 
> Also this management of thread flags would become fragile once
> you let the user deal with it concurrently with the event
> registering.
> 
> I think it's more sane/safe to encapsulate it as it is.
> 

OK, I don't feel very strongly about it. It's fine with me.

Mathieu

>  
> > 
> > > thanks,
> > > 
> > > -Jason
> > 
> > -- 
> > Mathieu Desnoyers
> > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH 3/7] add syscall tracepoints

[Frederic Weisbecker]

On Fri, Jun 12, 2009 at 05:24:49PM -0400, Jason Baron wrote:
> 
> Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> an external register/unregister function.
> 
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> ---
>  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
>  1 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 14df7e6..9a3660b 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -61,7 +61,7 @@ struct tracepoint {
>   * not add unwanted padding between the beginning of the section and the
>   * structure. Force alignment to the same alignment as the section start.
>   */
> -#define DECLARE_TRACE(name, proto, args)				\
> +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\



May be it deserves a comment? DECLARE_TRACE_REG() as a name is not enough
obvious on its own :)

Thanks,
Frederic.




>  	extern struct tracepoint __tracepoint_##name;			\
>  	static inline void trace_##name(proto)				\
>  	{								\
> @@ -71,13 +71,29 @@ struct tracepoint {
>  	}								\
>  	static inline int register_trace_##name(void (*probe)(proto))	\
>  	{								\
> -		return tracepoint_probe_register(#name, (void *)probe);	\
> +		int ret;						\
> +		void (*func)(void) = (void (*)(void))reg;		\
> +									\
> +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> +		if (func && !ret)					\
> +			func();						\
> +		return ret;						\
>  	}								\
>  	static inline int unregister_trace_##name(void (*probe)(proto))	\
>  	{								\
> -		return tracepoint_probe_unregister(#name, (void *)probe);\
> +		int ret;						\
> +		void (*func)(void) = (void (*)(void))unreg;		\
> +									\
> +		ret = tracepoint_probe_unregister(#name, (void *)probe);\
> +		if (func && !ret)					\
> +			func();						\
> +		return ret;						\
>  	}
>  
> +
> +#define DECLARE_TRACE(name, proto, args)		\
> +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> +
>  #define DEFINE_TRACE(name)						\
>  	static const char __tpstrtab_##name[]				\
>  	__attribute__((section("__tracepoints_strings"))) = #name;	\
> @@ -94,7 +110,7 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
>  	struct tracepoint *end);
>  
>  #else /* !CONFIG_TRACEPOINTS */
> -#define DECLARE_TRACE(name, proto, args)				\
> +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
>  	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
>  	{ }								\
>  	static inline void trace_##name(proto)				\
> @@ -108,6 +124,9 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
>  		return -ENOSYS;						\
>  	}
>  
> +#define DECLARE_TRACE(name, proto, args)                \
> +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> +
>  #define DEFINE_TRACE(name)
>  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
>  #define EXPORT_TRACEPOINT_SYMBOL(name)
> -- 
> 1.6.0.6
> 

Re: [PATCH 3/7] add syscall tracepoints

[Steven Rostedt]

On Fri, 19 Jun 2009, Frederic Weisbecker wrote:

> On Fri, Jun 12, 2009 at 05:24:49PM -0400, Jason Baron wrote:
> > 
> > Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> > an external register/unregister function.
> > 
> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > 
> > ---
> >  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
> >  1 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 14df7e6..9a3660b 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -61,7 +61,7 @@ struct tracepoint {
> >   * not add unwanted padding between the beginning of the section and the
> >   * structure. Force alignment to the same alignment as the section start.
> >   */
> > -#define DECLARE_TRACE(name, proto, args)				\
> > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> 
> 
> 
> May be it deserves a comment? DECLARE_TRACE_REG() as a name is not enough
> obvious on its own :)

I agree. I was talking with Christoph Hellwig about doing something 
similar with TRACE_EVENT, since he needs to do special things when enabled 
and disabled (like enable other trace points).

But anyway, I totally NAK the name. What about something like 
DECLARE_TRACE_WITH_CALLBACK()

Yes, it is a bit more verbose, but it at least tells what it does. The 
first thing that came to my mind when I saw DECLARE_TRACE_REG, was that 
this trace point will pass in "regs" to do things like backtraces.

-- Steve

> 
> 
> 
> >  	extern struct tracepoint __tracepoint_##name;			\
> >  	static inline void trace_##name(proto)				\
> >  	{								\
> > @@ -71,13 +71,29 @@ struct tracepoint {
> >  	}								\
> >  	static inline int register_trace_##name(void (*probe)(proto))	\
> >  	{								\
> > -		return tracepoint_probe_register(#name, (void *)probe);	\
> > +		int ret;						\
> > +		void (*func)(void) = (void (*)(void))reg;		\
> > +									\
> > +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> > +		if (func && !ret)					\
> > +			func();						\
> > +		return ret;						\
> >  	}								\
> >  	static inline int unregister_trace_##name(void (*probe)(proto))	\
> >  	{								\
> > -		return tracepoint_probe_unregister(#name, (void *)probe);\
> > +		int ret;						\
> > +		void (*func)(void) = (void (*)(void))unreg;		\
> > +									\
> > +		ret = tracepoint_probe_unregister(#name, (void *)probe);\
> > +		if (func && !ret)					\
> > +			func();						\
> > +		return ret;						\
> >  	}
> >  
> > +
> > +#define DECLARE_TRACE(name, proto, args)		\
> > +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> > +
> >  #define DEFINE_TRACE(name)						\
> >  	static const char __tpstrtab_##name[]				\
> >  	__attribute__((section("__tracepoints_strings"))) = #name;	\
> > @@ -94,7 +110,7 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> >  	struct tracepoint *end);
> >  
> >  #else /* !CONFIG_TRACEPOINTS */
> > -#define DECLARE_TRACE(name, proto, args)				\
> > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> >  	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> >  	{ }								\
> >  	static inline void trace_##name(proto)				\
> > @@ -108,6 +124,9 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> >  		return -ENOSYS;						\
> >  	}
> >  
> > +#define DECLARE_TRACE(name, proto, args)                \
> > +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> > +
> >  #define DEFINE_TRACE(name)
> >  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
> >  #define EXPORT_TRACEPOINT_SYMBOL(name)
> > -- 
> > 1.6.0.6
> > 
> 
> 

Re: [PATCH 3/7] add syscall tracepoints

[Frederic Weisbecker]

On Thu, Jun 18, 2009 at 11:29:29PM -0400, Steven Rostedt wrote:
> 
> On Fri, 19 Jun 2009, Frederic Weisbecker wrote:
> 
> > On Fri, Jun 12, 2009 at 05:24:49PM -0400, Jason Baron wrote:
> > > 
> > > Introduce a new 'DECLARE_TRACE_REG()' macro, so that tracepoints can associate
> > > an external register/unregister function.
> > > 
> > > 
> > > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > > 
> > > ---
> > >  include/linux/tracepoint.h |   27 +++++++++++++++++++++++----
> > >  1 files changed, 23 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > index 14df7e6..9a3660b 100644
> > > --- a/include/linux/tracepoint.h
> > > +++ b/include/linux/tracepoint.h
> > > @@ -61,7 +61,7 @@ struct tracepoint {
> > >   * not add unwanted padding between the beginning of the section and the
> > >   * structure. Force alignment to the same alignment as the section start.
> > >   */
> > > -#define DECLARE_TRACE(name, proto, args)				\
> > > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> > 
> > 
> > 
> > May be it deserves a comment? DECLARE_TRACE_REG() as a name is not enough
> > obvious on its own :)
> 
> I agree. I was talking with Christoph Hellwig about doing something 
> similar with TRACE_EVENT, since he needs to do special things when enabled 
> and disabled (like enable other trace points).
> 
> But anyway, I totally NAK the name. What about something like 
> DECLARE_TRACE_WITH_CALLBACK()
> 
> Yes, it is a bit more verbose, but it at least tells what it does.


Yeah, sounds good.



> The first thing that came to my mind when I saw DECLARE_TRACE_REG, was that 
> this trace point will pass in "regs" to do things like backtraces.


Ah, the first thing that came to me was a tracepoint that did something
weird with a mysterious registry...and then my imagination got stucked
there.


Frederic.

 
> -- Steve
> 
> > 
> > 
> > 
> > >  	extern struct tracepoint __tracepoint_##name;			\
> > >  	static inline void trace_##name(proto)				\
> > >  	{								\
> > > @@ -71,13 +71,29 @@ struct tracepoint {
> > >  	}								\
> > >  	static inline int register_trace_##name(void (*probe)(proto))	\
> > >  	{								\
> > > -		return tracepoint_probe_register(#name, (void *)probe);	\
> > > +		int ret;						\
> > > +		void (*func)(void) = (void (*)(void))reg;		\
> > > +									\
> > > +		ret = tracepoint_probe_register(#name, (void *)probe);	\
> > > +		if (func && !ret)					\
> > > +			func();						\
> > > +		return ret;						\
> > >  	}								\
> > >  	static inline int unregister_trace_##name(void (*probe)(proto))	\
> > >  	{								\
> > > -		return tracepoint_probe_unregister(#name, (void *)probe);\
> > > +		int ret;						\
> > > +		void (*func)(void) = (void (*)(void))unreg;		\
> > > +									\
> > > +		ret = tracepoint_probe_unregister(#name, (void *)probe);\
> > > +		if (func && !ret)					\
> > > +			func();						\
> > > +		return ret;						\
> > >  	}
> > >  
> > > +
> > > +#define DECLARE_TRACE(name, proto, args)		\
> > > +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> > > +
> > >  #define DEFINE_TRACE(name)						\
> > >  	static const char __tpstrtab_##name[]				\
> > >  	__attribute__((section("__tracepoints_strings"))) = #name;	\
> > > @@ -94,7 +110,7 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> > >  	struct tracepoint *end);
> > >  
> > >  #else /* !CONFIG_TRACEPOINTS */
> > > -#define DECLARE_TRACE(name, proto, args)				\
> > > +#define DECLARE_TRACE_REG(name, proto, args, reg, unreg)		\
> > >  	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> > >  	{ }								\
> > >  	static inline void trace_##name(proto)				\
> > > @@ -108,6 +124,9 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> > >  		return -ENOSYS;						\
> > >  	}
> > >  
> > > +#define DECLARE_TRACE(name, proto, args)                \
> > > +	DECLARE_TRACE_REG(name, TP_PROTO(proto), TP_ARGS(args), 0, 0);
> > > +
> > >  #define DEFINE_TRACE(name)
> > >  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
> > >  #define EXPORT_TRACEPOINT_SYMBOL(name)
> > > -- 
> > > 1.6.0.6
> > > 
> > 
> > 

[PATCH 4/7] add syscall tracepoints

[Jason Baron]

add two tracepoints in syscall exit and entry path, conditioned on
TIF_SYSCALL_FTRACE. Supports the syscall trace event code.


Signed-off-by: Jason Baron <jbaron@redhat.com>

---
 arch/x86/kernel/ptrace.c |    6 ++++--
 include/trace/syscall.h  |   18 ++++++++++++++++++
 kernel/tracepoint.c      |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 09ecbde..1c7301a 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -36,6 +36,8 @@
 #include <asm/ds.h>
 
 #include <trace/syscall.h>
+DEFINE_TRACE(syscall_enter);
+DEFINE_TRACE(syscall_exit);
 
 #include "tls.h"
 
@@ -1498,7 +1500,7 @@ asmregparm long syscall_trace_enter(struct pt_regs *regs)
 		ret = -1L;
 
 	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
-		ftrace_syscall_enter(regs);
+		trace_syscall_enter(regs, regs->orig_ax);
 
 	if (unlikely(current->audit_context)) {
 		if (IS_IA32)
@@ -1524,7 +1526,7 @@ asmregparm void syscall_trace_leave(struct pt_regs *regs)
 		audit_syscall_exit(AUDITSC_RESULT(regs->ax), regs->ax);
 
 	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
-		ftrace_syscall_exit(regs);
+		trace_syscall_exit(regs, regs->ax);
 
 	if (test_thread_flag(TIF_SYSCALL_TRACE))
 		tracehook_report_syscall_exit(regs, 0);
diff --git a/include/trace/syscall.h b/include/trace/syscall.h
index 8cfe515..d5d8310 100644
--- a/include/trace/syscall.h
+++ b/include/trace/syscall.h
@@ -2,6 +2,24 @@
 #define _TRACE_SYSCALL_H
 
 #include <asm/ptrace.h>
+#include <linux/tracepoint.h>
+
+extern void syscall_regfunc(void);
+extern void syscall_unregfunc(void);
+
+DECLARE_TRACE_REG(syscall_enter,
+	TP_PROTO(struct pt_regs *regs, long id),
+	TP_ARGS(regs, id),
+	syscall_regfunc,
+	syscall_unregfunc
+);
+
+DECLARE_TRACE_REG(syscall_exit,
+	TP_PROTO(struct pt_regs *regs, long ret),
+	TP_ARGS(regs, ret),
+	syscall_regfunc,
+	syscall_unregfunc
+);
 
 /*
  * A syscall entry in the ftrace syscalls array.
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 1ef5d3a..5b34ff9 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -24,6 +24,7 @@
 #include <linux/tracepoint.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/sched.h>
 
 extern struct tracepoint __start___tracepoints[];
 extern struct tracepoint __stop___tracepoints[];
@@ -577,3 +578,40 @@ static int init_tracepoints(void)
 __initcall(init_tracepoints);
 
 #endif /* CONFIG_MODULES */
+
+DEFINE_MUTEX(regfunc_mutex);
+int sys_tracepoint_refcount;
+
+void syscall_regfunc(void)
+{
+	unsigned long flags;
+	struct task_struct *g, *t;
+
+	mutex_lock(&regfunc_mutex);
+	if (!sys_tracepoint_refcount) {
+		read_lock_irqsave(&tasklist_lock, flags);
+		do_each_thread(g, t) {
+			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
+		} while_each_thread(g, t);
+		read_unlock_irqrestore(&tasklist_lock, flags);
+	}
+	sys_tracepoint_refcount++;
+	mutex_unlock(&regfunc_mutex);
+}
+
+void syscall_unregfunc(void)
+{
+	unsigned long flags;
+	struct task_struct *g, *t;
+
+	mutex_lock(&regfunc_mutex);
+	sys_tracepoint_refcount--;
+	if (!sys_tracepoint_refcount) {
+		read_lock_irqsave(&tasklist_lock, flags);
+		do_each_thread(g, t) {
+			clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
+		} while_each_thread(g, t);
+		read_unlock_irqrestore(&tasklist_lock, flags);
+	}
+	mutex_unlock(&regfunc_mutex);
+}
-- 
1.6.0.6

Re: [PATCH 4/7] add syscall tracepoints

[Frederic Weisbecker]

On Fri, Jun 12, 2009 at 05:24:54PM -0400, Jason Baron wrote:
> 
> add two tracepoints in syscall exit and entry path, conditioned on
> TIF_SYSCALL_FTRACE. Supports the syscall trace event code.
> 
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> ---
>  arch/x86/kernel/ptrace.c |    6 ++++--
>  include/trace/syscall.h  |   18 ++++++++++++++++++
>  kernel/tracepoint.c      |   38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 09ecbde..1c7301a 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -36,6 +36,8 @@
>  #include <asm/ds.h>
>  
>  #include <trace/syscall.h>
> +DEFINE_TRACE(syscall_enter);
> +DEFINE_TRACE(syscall_exit);
>  
>  #include "tls.h"
>  
> @@ -1498,7 +1500,7 @@ asmregparm long syscall_trace_enter(struct pt_regs *regs)
>  		ret = -1L;
>  
>  	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
> -		ftrace_syscall_enter(regs);
> +		trace_syscall_enter(regs, regs->orig_ax);
>  
>  	if (unlikely(current->audit_context)) {
>  		if (IS_IA32)
> @@ -1524,7 +1526,7 @@ asmregparm void syscall_trace_leave(struct pt_regs *regs)
>  		audit_syscall_exit(AUDITSC_RESULT(regs->ax), regs->ax);
>  
>  	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
> -		ftrace_syscall_exit(regs);
> +		trace_syscall_exit(regs, regs->ax);
>  
>  	if (test_thread_flag(TIF_SYSCALL_TRACE))
>  		tracehook_report_syscall_exit(regs, 0);
> diff --git a/include/trace/syscall.h b/include/trace/syscall.h
> index 8cfe515..d5d8310 100644
> --- a/include/trace/syscall.h
> +++ b/include/trace/syscall.h
> @@ -2,6 +2,24 @@
>  #define _TRACE_SYSCALL_H
>  
>  #include <asm/ptrace.h>
> +#include <linux/tracepoint.h>
> +
> +extern void syscall_regfunc(void);
> +extern void syscall_unregfunc(void);
> +
> +DECLARE_TRACE_REG(syscall_enter,
> +	TP_PROTO(struct pt_regs *regs, long id),
> +	TP_ARGS(regs, id),
> +	syscall_regfunc,
> +	syscall_unregfunc
> +);
> +
> +DECLARE_TRACE_REG(syscall_exit,
> +	TP_PROTO(struct pt_regs *regs, long ret),
> +	TP_ARGS(regs, ret),
> +	syscall_regfunc,
> +	syscall_unregfunc
> +);
>  
>  /*
>   * A syscall entry in the ftrace syscalls array.
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 1ef5d3a..5b34ff9 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c



At a first glance I wasn't sure tracepoint.c is the right
place for these.
But indeed putting those two callbacks here avoids any
dependency to the syscall tracer when someone else needs
the syscall tracepoints.

Well, I guess we can keep them there.



> @@ -24,6 +24,7 @@
>  #include <linux/tracepoint.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/sched.h>
>  
>  extern struct tracepoint __start___tracepoints[];
>  extern struct tracepoint __stop___tracepoints[];
> @@ -577,3 +578,40 @@ static int init_tracepoints(void)
>  __initcall(init_tracepoints);
>  
>  #endif /* CONFIG_MODULES */
> +
> +DEFINE_MUTEX(regfunc_mutex);
> +int sys_tracepoint_refcount;


Looks like regfunc_mutex is only there to protect
sys_tracepoint_refcount. May be you can just make it atomic_t?

Thanks,
Frederic.


> +
> +void syscall_regfunc(void)
> +{
> +	unsigned long flags;
> +	struct task_struct *g, *t;
> +
> +	mutex_lock(&regfunc_mutex);
> +	if (!sys_tracepoint_refcount) {
> +		read_lock_irqsave(&tasklist_lock, flags);
> +		do_each_thread(g, t) {
> +			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> +		} while_each_thread(g, t);
> +		read_unlock_irqrestore(&tasklist_lock, flags);
> +	}
> +	sys_tracepoint_refcount++;
> +	mutex_unlock(&regfunc_mutex);
> +}
> +
> +void syscall_unregfunc(void)
> +{
> +	unsigned long flags;
> +	struct task_struct *g, *t;
> +
> +	mutex_lock(&regfunc_mutex);
> +	sys_tracepoint_refcount--;
> +	if (!sys_tracepoint_refcount) {
> +		read_lock_irqsave(&tasklist_lock, flags);
> +		do_each_thread(g, t) {
> +			clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> +		} while_each_thread(g, t);
> +		read_unlock_irqrestore(&tasklist_lock, flags);
> +	}
> +	mutex_unlock(&regfunc_mutex);
> +}
> -- 
> 1.6.0.6
> 

Re: [PATCH 4/7] add syscall tracepoints

[Mathieu Desnoyers]

* Frederic Weisbecker (fweisbec@gmail.com) wrote:
> On Fri, Jun 12, 2009 at 05:24:54PM -0400, Jason Baron wrote:
> > 
> > add two tracepoints in syscall exit and entry path, conditioned on
> > TIF_SYSCALL_FTRACE. Supports the syscall trace event code.
> > 
> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > 
> > ---
> >  arch/x86/kernel/ptrace.c |    6 ++++--
> >  include/trace/syscall.h  |   18 ++++++++++++++++++
> >  kernel/tracepoint.c      |   38 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 60 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > index 09ecbde..1c7301a 100644
> > --- a/arch/x86/kernel/ptrace.c
> > +++ b/arch/x86/kernel/ptrace.c
> > @@ -36,6 +36,8 @@
> >  #include <asm/ds.h>
> >  
> >  #include <trace/syscall.h>
> > +DEFINE_TRACE(syscall_enter);
> > +DEFINE_TRACE(syscall_exit);
> >  
> >  #include "tls.h"
> >  
> > @@ -1498,7 +1500,7 @@ asmregparm long syscall_trace_enter(struct pt_regs *regs)
> >  		ret = -1L;
> >  
> >  	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
> > -		ftrace_syscall_enter(regs);
> > +		trace_syscall_enter(regs, regs->orig_ax);
> >  
> >  	if (unlikely(current->audit_context)) {
> >  		if (IS_IA32)
> > @@ -1524,7 +1526,7 @@ asmregparm void syscall_trace_leave(struct pt_regs *regs)
> >  		audit_syscall_exit(AUDITSC_RESULT(regs->ax), regs->ax);
> >  
> >  	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
> > -		ftrace_syscall_exit(regs);
> > +		trace_syscall_exit(regs, regs->ax);
> >  
> >  	if (test_thread_flag(TIF_SYSCALL_TRACE))
> >  		tracehook_report_syscall_exit(regs, 0);
> > diff --git a/include/trace/syscall.h b/include/trace/syscall.h
> > index 8cfe515..d5d8310 100644
> > --- a/include/trace/syscall.h
> > +++ b/include/trace/syscall.h
> > @@ -2,6 +2,24 @@
> >  #define _TRACE_SYSCALL_H
> >  
> >  #include <asm/ptrace.h>
> > +#include <linux/tracepoint.h>
> > +
> > +extern void syscall_regfunc(void);
> > +extern void syscall_unregfunc(void);
> > +
> > +DECLARE_TRACE_REG(syscall_enter,
> > +	TP_PROTO(struct pt_regs *regs, long id),
> > +	TP_ARGS(regs, id),
> > +	syscall_regfunc,
> > +	syscall_unregfunc
> > +);
> > +
> > +DECLARE_TRACE_REG(syscall_exit,
> > +	TP_PROTO(struct pt_regs *regs, long ret),
> > +	TP_ARGS(regs, ret),
> > +	syscall_regfunc,
> > +	syscall_unregfunc
> > +);
> >  
> >  /*
> >   * A syscall entry in the ftrace syscalls array.
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 1ef5d3a..5b34ff9 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> 
> 
> 
> At a first glance I wasn't sure tracepoint.c is the right
> place for these.
> But indeed putting those two callbacks here avoids any
> dependency to the syscall tracer when someone else needs
> the syscall tracepoints.
> 
> Well, I guess we can keep them there.
> 
> 
> 
> > @@ -24,6 +24,7 @@
> >  #include <linux/tracepoint.h>
> >  #include <linux/err.h>
> >  #include <linux/slab.h>
> > +#include <linux/sched.h>
> >  
> >  extern struct tracepoint __start___tracepoints[];
> >  extern struct tracepoint __stop___tracepoints[];
> > @@ -577,3 +578,40 @@ static int init_tracepoints(void)
> >  __initcall(init_tracepoints);
> >  
> >  #endif /* CONFIG_MODULES */
> > +
> > +DEFINE_MUTEX(regfunc_mutex);
> > +int sys_tracepoint_refcount;
> 
> 
> Looks like regfunc_mutex is only there to protect
> sys_tracepoint_refcount. May be you can just make it atomic_t?
> 

Nope, regfunc_mutex makes sure there is no disable/enable occuring
concurrently. They only take a tasklist read lock, so they can happen
concurrently if it wasn't for the regfunc_mutex.

Unless I'm missing something totally obvious about the refcount, the
mutex is needed, and there is no need to go for an atomic type in this
case.

Thanks,

Mathieu

> Thanks,
> Frederic.
> 
> 
> > +
> > +void syscall_regfunc(void)
> > +{
> > +	unsigned long flags;
> > +	struct task_struct *g, *t;
> > +
> > +	mutex_lock(&regfunc_mutex);
> > +	if (!sys_tracepoint_refcount) {
> > +		read_lock_irqsave(&tasklist_lock, flags);
> > +		do_each_thread(g, t) {
> > +			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> > +		} while_each_thread(g, t);
> > +		read_unlock_irqrestore(&tasklist_lock, flags);
> > +	}
> > +	sys_tracepoint_refcount++;
> > +	mutex_unlock(&regfunc_mutex);
> > +}
> > +
> > +void syscall_unregfunc(void)
> > +{
> > +	unsigned long flags;
> > +	struct task_struct *g, *t;
> > +
> > +	mutex_lock(&regfunc_mutex);
> > +	sys_tracepoint_refcount--;
> > +	if (!sys_tracepoint_refcount) {
> > +		read_lock_irqsave(&tasklist_lock, flags);
> > +		do_each_thread(g, t) {
> > +			clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> > +		} while_each_thread(g, t);
> > +		read_unlock_irqrestore(&tasklist_lock, flags);
> > +	}
> > +	mutex_unlock(&regfunc_mutex);
> > +}
> > -- 
> > 1.6.0.6
> > 
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH 4/7] add syscall tracepoints

[Frederic Weisbecker]

On Fri, Jun 19, 2009 at 08:35:04AM -0400, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> > On Fri, Jun 12, 2009 at 05:24:54PM -0400, Jason Baron wrote:
> > > 
> > > add two tracepoints in syscall exit and entry path, conditioned on
> > > TIF_SYSCALL_FTRACE. Supports the syscall trace event code.
> > > 
> > > 
> > > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > > 
> > > ---
> > >  arch/x86/kernel/ptrace.c |    6 ++++--
> > >  include/trace/syscall.h  |   18 ++++++++++++++++++
> > >  kernel/tracepoint.c      |   38 ++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 60 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > index 09ecbde..1c7301a 100644
> > > --- a/arch/x86/kernel/ptrace.c
> > > +++ b/arch/x86/kernel/ptrace.c
> > > @@ -36,6 +36,8 @@
> > >  #include <asm/ds.h>
> > >  
> > >  #include <trace/syscall.h>
> > > +DEFINE_TRACE(syscall_enter);
> > > +DEFINE_TRACE(syscall_exit);
> > >  
> > >  #include "tls.h"
> > >  
> > > @@ -1498,7 +1500,7 @@ asmregparm long syscall_trace_enter(struct pt_regs *regs)
> > >  		ret = -1L;
> > >  
> > >  	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
> > > -		ftrace_syscall_enter(regs);
> > > +		trace_syscall_enter(regs, regs->orig_ax);
> > >  
> > >  	if (unlikely(current->audit_context)) {
> > >  		if (IS_IA32)
> > > @@ -1524,7 +1526,7 @@ asmregparm void syscall_trace_leave(struct pt_regs *regs)
> > >  		audit_syscall_exit(AUDITSC_RESULT(regs->ax), regs->ax);
> > >  
> > >  	if (unlikely(test_thread_flag(TIF_SYSCALL_FTRACE)))
> > > -		ftrace_syscall_exit(regs);
> > > +		trace_syscall_exit(regs, regs->ax);
> > >  
> > >  	if (test_thread_flag(TIF_SYSCALL_TRACE))
> > >  		tracehook_report_syscall_exit(regs, 0);
> > > diff --git a/include/trace/syscall.h b/include/trace/syscall.h
> > > index 8cfe515..d5d8310 100644
> > > --- a/include/trace/syscall.h
> > > +++ b/include/trace/syscall.h
> > > @@ -2,6 +2,24 @@
> > >  #define _TRACE_SYSCALL_H
> > >  
> > >  #include <asm/ptrace.h>
> > > +#include <linux/tracepoint.h>
> > > +
> > > +extern void syscall_regfunc(void);
> > > +extern void syscall_unregfunc(void);
> > > +
> > > +DECLARE_TRACE_REG(syscall_enter,
> > > +	TP_PROTO(struct pt_regs *regs, long id),
> > > +	TP_ARGS(regs, id),
> > > +	syscall_regfunc,
> > > +	syscall_unregfunc
> > > +);
> > > +
> > > +DECLARE_TRACE_REG(syscall_exit,
> > > +	TP_PROTO(struct pt_regs *regs, long ret),
> > > +	TP_ARGS(regs, ret),
> > > +	syscall_regfunc,
> > > +	syscall_unregfunc
> > > +);
> > >  
> > >  /*
> > >   * A syscall entry in the ftrace syscalls array.
> > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > > index 1ef5d3a..5b34ff9 100644
> > > --- a/kernel/tracepoint.c
> > > +++ b/kernel/tracepoint.c
> > 
> > 
> > 
> > At a first glance I wasn't sure tracepoint.c is the right
> > place for these.
> > But indeed putting those two callbacks here avoids any
> > dependency to the syscall tracer when someone else needs
> > the syscall tracepoints.
> > 
> > Well, I guess we can keep them there.
> > 
> > 
> > 
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/tracepoint.h>
> > >  #include <linux/err.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/sched.h>
> > >  
> > >  extern struct tracepoint __start___tracepoints[];
> > >  extern struct tracepoint __stop___tracepoints[];
> > > @@ -577,3 +578,40 @@ static int init_tracepoints(void)
> > >  __initcall(init_tracepoints);
> > >  
> > >  #endif /* CONFIG_MODULES */
> > > +
> > > +DEFINE_MUTEX(regfunc_mutex);
> > > +int sys_tracepoint_refcount;
> > 
> > 
> > Looks like regfunc_mutex is only there to protect
> > sys_tracepoint_refcount. May be you can just make it atomic_t?
> > 
> 
> Nope, regfunc_mutex makes sure there is no disable/enable occuring
> concurrently. They only take a tasklist read lock, so they can happen
> concurrently if it wasn't for the regfunc_mutex.
> 
> Unless I'm missing something totally obvious about the refcount, the
> mutex is needed, and there is no need to go for an atomic type in this
> case.
> 
> Thanks,
> 
> Mathieu



Ah...
Eh, you're right :)

Although concurrent enable/disable of the TIF flags would reveal a bug
in the tracepoint user (enable and disable not serialized).

But yeah let's keep it as a mutex.

Thanks.

 
> > Thanks,
> > Frederic.
> > 
> > 
> > > +
> > > +void syscall_regfunc(void)
> > > +{
> > > +	unsigned long flags;
> > > +	struct task_struct *g, *t;
> > > +
> > > +	mutex_lock(&regfunc_mutex);
> > > +	if (!sys_tracepoint_refcount) {
> > > +		read_lock_irqsave(&tasklist_lock, flags);
> > > +		do_each_thread(g, t) {
> > > +			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> > > +		} while_each_thread(g, t);
> > > +		read_unlock_irqrestore(&tasklist_lock, flags);
> > > +	}
> > > +	sys_tracepoint_refcount++;
> > > +	mutex_unlock(&regfunc_mutex);
> > > +}
> > > +
> > > +void syscall_unregfunc(void)
> > > +{
> > > +	unsigned long flags;
> > > +	struct task_struct *g, *t;
> > > +
> > > +	mutex_lock(&regfunc_mutex);
> > > +	sys_tracepoint_refcount--;
> > > +	if (!sys_tracepoint_refcount) {
> > > +		read_lock_irqsave(&tasklist_lock, flags);
> > > +		do_each_thread(g, t) {
> > > +			clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> > > +		} while_each_thread(g, t);
> > > +		read_unlock_irqrestore(&tasklist_lock, flags);
> > > +	}
> > > +	mutex_unlock(&regfunc_mutex);
> > > +}
> > > -- 
> > > 1.6.0.6
> > > 
> > 
> 
> -- 
> Mathieu Desnoyers
> OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH 4/7] add syscall tracepoints

[Li Zefan]

>  /*
>   * A syscall entry in the ftrace syscalls array.
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 1ef5d3a..5b34ff9 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -24,6 +24,7 @@
>  #include <linux/tracepoint.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/sched.h>
>  
>  extern struct tracepoint __start___tracepoints[];
>  extern struct tracepoint __stop___tracepoints[];
> @@ -577,3 +578,40 @@ static int init_tracepoints(void)
>  __initcall(init_tracepoints);
>  
>  #endif /* CONFIG_MODULES */
> +
> +DEFINE_MUTEX(regfunc_mutex);

static?

> +int sys_tracepoint_refcount;

static?

and atomic_t should be fine as Frederic pointed out.

> +
> +void syscall_regfunc(void)
> +{
> +	unsigned long flags;
> +	struct task_struct *g, *t;
> +
> +	mutex_lock(&regfunc_mutex);
> +	if (!sys_tracepoint_refcount) {
> +		read_lock_irqsave(&tasklist_lock, flags);
> +		do_each_thread(g, t) {
> +			set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> +		} while_each_thread(g, t);
> +		read_unlock_irqrestore(&tasklist_lock, flags);
> +	}
> +	sys_tracepoint_refcount++;
> +	mutex_unlock(&regfunc_mutex);
> +}
> +
> +void syscall_unregfunc(void)
> +{
> +	unsigned long flags;
> +	struct task_struct *g, *t;
> +
> +	mutex_lock(&regfunc_mutex);
> +	sys_tracepoint_refcount--;
> +	if (!sys_tracepoint_refcount) {
> +		read_lock_irqsave(&tasklist_lock, flags);
> +		do_each_thread(g, t) {
> +			clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
> +		} while_each_thread(g, t);
> +		read_unlock_irqrestore(&tasklist_lock, flags);
> +	}
> +	mutex_unlock(&regfunc_mutex);
> +}

[PATCH 5/7] add syscall tracepoints

[Jason Baron]

update FTRACE_SYSCALL_MAX to the current number of syscalls


Signed-off-by: Jason Baron <jbaron@redhat.com>

---
 arch/x86/include/asm/ftrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index bd2c651..d16d195 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -30,9 +30,9 @@
 
 /* FIXME: I don't want to stay hardcoded */
 #ifdef CONFIG_X86_64
-# define FTRACE_SYSCALL_MAX     296
+# define FTRACE_SYSCALL_MAX     298
 #else
-# define FTRACE_SYSCALL_MAX     333
+# define FTRACE_SYSCALL_MAX     336
 #endif
 
 #ifdef CONFIG_FUNCTION_TRACER
-- 
1.6.0.6

Re: [PATCH 5/7] add syscall tracepoints

[Frederic Weisbecker]

On Fri, Jun 12, 2009 at 05:24:59PM -0400, Jason Baron wrote:
> update FTRACE_SYSCALL_MAX to the current number of syscalls
> 
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> ---
>  arch/x86/include/asm/ftrace.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index bd2c651..d16d195 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -30,9 +30,9 @@
>  
>  /* FIXME: I don't want to stay hardcoded */


BTW, is there a way to know this size dynamically?
Or is there already a hardcoded number of syscalls somewhere
in x86?

Frederic.


>  #ifdef CONFIG_X86_64
> -# define FTRACE_SYSCALL_MAX     296
> +# define FTRACE_SYSCALL_MAX     298
>  #else
> -# define FTRACE_SYSCALL_MAX     333
> +# define FTRACE_SYSCALL_MAX     336
>  #endif
>  
>  #ifdef CONFIG_FUNCTION_TRACER
> -- 
> 1.6.0.6
> 

Re: [PATCH 5/7] add syscall tracepoints

[Li Zefan]

Frederic Weisbecker wrote:
> On Fri, Jun 12, 2009 at 05:24:59PM -0400, Jason Baron wrote:
>> update FTRACE_SYSCALL_MAX to the current number of syscalls
>>
>>
>> Signed-off-by: Jason Baron <jbaron@redhat.com>
>>
>> ---
>>  arch/x86/include/asm/ftrace.h |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
>> index bd2c651..d16d195 100644
>> --- a/arch/x86/include/asm/ftrace.h
>> +++ b/arch/x86/include/asm/ftrace.h
>> @@ -30,9 +30,9 @@
>>  
>>  /* FIXME: I don't want to stay hardcoded */
> 
> 
> BTW, is there a way to know this size dynamically?
> Or is there already a hardcoded number of syscalls somewhere
> in x86?
> 

Does this patchset head for .31 or .32? If for .32, then this patch should
go into .31 I think.

Re: [PATCH 5/7] add syscall tracepoints

[Steven Rostedt]

On Fri, 19 Jun 2009, Li Zefan wrote:

> Frederic Weisbecker wrote:
> > On Fri, Jun 12, 2009 at 05:24:59PM -0400, Jason Baron wrote:
> >> update FTRACE_SYSCALL_MAX to the current number of syscalls
> >>
> >>
> >> Signed-off-by: Jason Baron <jbaron@redhat.com>
> >>
> >> ---
> >>  arch/x86/include/asm/ftrace.h |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> >> index bd2c651..d16d195 100644
> >> --- a/arch/x86/include/asm/ftrace.h
> >> +++ b/arch/x86/include/asm/ftrace.h
> >> @@ -30,9 +30,9 @@
> >>  
> >>  /* FIXME: I don't want to stay hardcoded */
> > 
> > 
> > BTW, is there a way to know this size dynamically?
> > Or is there already a hardcoded number of syscalls somewhere
> > in x86?
> > 
> 
> Does this patchset head for .31 or .32? If for .32, then this patch should
> go into .31 I think.

Probably the best to run it through tip for a release. So, yes this patch 
should go to the beginning of the queue so that we can push it to 31 if 
need be.

-- Steve

Re: [PATCH 5/7] add syscall tracepoints

[Frederic Weisbecker]

On Fri, Jun 19, 2009 at 11:14:20AM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Fri, Jun 12, 2009 at 05:24:59PM -0400, Jason Baron wrote:
> >> update FTRACE_SYSCALL_MAX to the current number of syscalls
> >>
> >>
> >> Signed-off-by: Jason Baron <jbaron@redhat.com>
> >>
> >> ---
> >>  arch/x86/include/asm/ftrace.h |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> >> index bd2c651..d16d195 100644
> >> --- a/arch/x86/include/asm/ftrace.h
> >> +++ b/arch/x86/include/asm/ftrace.h
> >> @@ -30,9 +30,9 @@
> >>  
> >>  /* FIXME: I don't want to stay hardcoded */
> > 
> > 
> > BTW, is there a way to know this size dynamically?
> > Or is there already a hardcoded number of syscalls somewhere
> > in x86?
> > 
> 
> Does this patchset head for .31 or .32? If for .32, then this patch should
> go into .31 I think.


Hmm, I was thinking about .32 but if Ingo has the time to test it and
if he agrees with it, why not.

[PATCH 6/7] add syscall tracepoints

[Jason Baron]

Allow the return value of raw_init() to bail us out of creating a trace event
file.

Signed-off-by: Jason Baron <jbaron@redhat.com>

---
 kernel/trace/trace_events.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 9e91c4a..c0da5e2 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -907,15 +907,6 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 	if (strcmp(call->system, TRACE_SYSTEM) != 0)
 		d_events = event_subsystem_dir(call->system, d_events);
 
-	if (call->raw_init) {
-		ret = call->raw_init();
-		if (ret < 0) {
-			pr_warning("Could not initialize trace point"
-				   " events/%s\n", call->name);
-			return ret;
-		}
-	}
-
 	call->dir = debugfs_create_dir(call->name, d_events);
 	if (!call->dir) {
 		pr_warning("Could not create debugfs "
@@ -1014,6 +1005,7 @@ static void trace_module_add_events(struct module *mod)
 	struct ftrace_module_file_ops *file_ops = NULL;
 	struct ftrace_event_call *call, *start, *end;
 	struct dentry *d_events;
+	int ret;
 
 	start = mod->trace_events;
 	end = mod->trace_events + mod->num_trace_events;
@@ -1029,7 +1021,15 @@ static void trace_module_add_events(struct module *mod)
 		/* The linker may leave blanks */
 		if (!call->name)
 			continue;
-
+		if (call->raw_init) {
+			ret = call->raw_init();
+			if (ret < 0) {
+				if (ret != -ENOSYS)
+					pr_warning("Could not initialize trace"
+					"point events/%s\n", call->name);
+				continue;
+			}
+		}
 		/*
 		 * This module has events, create file ops for this module
 		 * if not already done.
@@ -1167,6 +1167,15 @@ static __init int event_trace_init(void)
 		/* The linker may leave blanks */
 		if (!call->name)
 			continue;
+		if (call->raw_init) {
+			ret = call->raw_init();
+			if (ret < 0) {
+				if (ret != -ENOSYS)
+					pr_warning("Could not initialize trace"
+					"point events/%s\n", call->name);
+				continue;
+			}
+		}
 		list_add(&call->list, &ftrace_events);
 		event_create_dir(call, d_events, &ftrace_event_id_fops,
 				 &ftrace_enable_fops, &ftrace_event_filter_fops,
-- 
1.6.0.6

Re: [PATCH 6/7] add syscall tracepoints

[Frederic Weisbecker]

On Fri, Jun 12, 2009 at 05:25:04PM -0400, Jason Baron wrote:
> Allow the return value of raw_init() to bail us out of creating a trace event
> file.
> 
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> ---
>  kernel/trace/trace_events.c |   29 +++++++++++++++++++----------
>  1 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 9e91c4a..c0da5e2 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -907,15 +907,6 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
>  	if (strcmp(call->system, TRACE_SYSTEM) != 0)
>  		d_events = event_subsystem_dir(call->system, d_events);
>  
> -	if (call->raw_init) {
> -		ret = call->raw_init();
> -		if (ret < 0) {
> -			pr_warning("Could not initialize trace point"
> -				   " events/%s\n", call->name);
> -			return ret;
> -		}
> -	}
> -
>  	call->dir = debugfs_create_dir(call->name, d_events);
>  	if (!call->dir) {
>  		pr_warning("Could not create debugfs "
> @@ -1014,6 +1005,7 @@ static void trace_module_add_events(struct module *mod)
>  	struct ftrace_module_file_ops *file_ops = NULL;
>  	struct ftrace_event_call *call, *start, *end;
>  	struct dentry *d_events;
> +	int ret;
>  
>  	start = mod->trace_events;
>  	end = mod->trace_events + mod->num_trace_events;
> @@ -1029,7 +1021,15 @@ static void trace_module_add_events(struct module *mod)
>  		/* The linker may leave blanks */
>  		if (!call->name)
>  			continue;
> -
> +		if (call->raw_init) {
> +			ret = call->raw_init();
> +			if (ret < 0) {
> +				if (ret != -ENOSYS)



I see you've set it up in case syscall_to_nr doesn't find the
matching metadata.

How is it possible? The traced syscalls, ie those which are defined through
DEFINE_SYSCALLx(), should be all in the arch syscall table. Or may be I'm
missing something?

In this case we should warn because it would reveal a bug.



> +					pr_warning("Could not initialize trace"
> +					"point events/%s\n", call->name);
> +				continue;
> +			}
> +		}
>  		/*
>  		 * This module has events, create file ops for this module
>  		 * if not already done.
> @@ -1167,6 +1167,15 @@ static __init int event_trace_init(void)
>  		/* The linker may leave blanks */
>  		if (!call->name)
>  			continue;
> +		if (call->raw_init) {
> +			ret = call->raw_init();
> +			if (ret < 0) {
> +				if (ret != -ENOSYS)
> +					pr_warning("Could not initialize trace"
> +					"point events/%s\n", call->name);
> +				continue;
> +			}
> +		}
>  		list_add(&call->list, &ftrace_events);
>  		event_create_dir(call, d_events, &ftrace_event_id_fops,
>  				 &ftrace_enable_fops, &ftrace_event_filter_fops,
> -- 
> 1.6.0.6
> 

Re: [PATCH 6/7] add syscall tracepoints

[Jason Baron]

On Fri, Jun 19, 2009 at 04:28:26AM +0200, Frederic Weisbecker wrote:
> On Fri, Jun 12, 2009 at 05:25:04PM -0400, Jason Baron wrote:
> > Allow the return value of raw_init() to bail us out of creating a trace event
> > file.
> > 
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > 
> > ---
> >  kernel/trace/trace_events.c |   29 +++++++++++++++++++----------
> >  1 files changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 9e91c4a..c0da5e2 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -907,15 +907,6 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
> >  	if (strcmp(call->system, TRACE_SYSTEM) != 0)
> >  		d_events = event_subsystem_dir(call->system, d_events);
> >  
> > -	if (call->raw_init) {
> > -		ret = call->raw_init();
> > -		if (ret < 0) {
> > -			pr_warning("Could not initialize trace point"
> > -				   " events/%s\n", call->name);
> > -			return ret;
> > -		}
> > -	}
> > -
> >  	call->dir = debugfs_create_dir(call->name, d_events);
> >  	if (!call->dir) {
> >  		pr_warning("Could not create debugfs "
> > @@ -1014,6 +1005,7 @@ static void trace_module_add_events(struct module *mod)
> >  	struct ftrace_module_file_ops *file_ops = NULL;
> >  	struct ftrace_event_call *call, *start, *end;
> >  	struct dentry *d_events;
> > +	int ret;
> >  
> >  	start = mod->trace_events;
> >  	end = mod->trace_events + mod->num_trace_events;
> > @@ -1029,7 +1021,15 @@ static void trace_module_add_events(struct module *mod)
> >  		/* The linker may leave blanks */
> >  		if (!call->name)
> >  			continue;
> > -
> > +		if (call->raw_init) {
> > +			ret = call->raw_init();
> > +			if (ret < 0) {
> > +				if (ret != -ENOSYS)
> 
> 
> 
> I see you've set it up in case syscall_to_nr doesn't find the
> matching metadata.
> 
> How is it possible? The traced syscalls, ie those which are defined through
> DEFINE_SYSCALLx(), should be all in the arch syscall table. Or may be I'm
> missing something?
> 

yes, I've been wondering about the same thing...I think the answer is
that DEFINE_SYSCALLx() is used to define some of the 'compat' layer ia32
syscalls. Thus, they show up as events to register but they are not in
the syscall table. I'm not sure if its easy to handle these at compile
time, but in any case that is where this case is coming from...

I've also notice that I forgot to define the meta data for
SYSCALL_DEFINE0 case...

thanks,

-Jason

[PATCH 7/7] add syscall tracepoints

[Jason Baron]

Layer Frederic's syscall tracer on tracepoints. We create trace events via
hooking into the SYCALL_DEFINE macros. This allows us to individually toggle
syscall entry and exit points on/off.



Signed-off-by: Jason Baron <jbaron@redhat.com>

---
 include/linux/syscalls.h      |   63 +++++++++++++++
 include/trace/syscall.h       |   19 ++---
 kernel/trace/trace_syscalls.c |  172 +++++++++++++++++++----------------------
 3 files changed, 153 insertions(+), 101 deletions(-)

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 79faae9..17dc567 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -64,6 +64,7 @@ struct perf_counter_hw_event;
 #include <linux/sem.h>
 #include <asm/siginfo.h>
 #include <asm/signal.h>
+#include <linux/unistd.h>
 #include <linux/quota.h>
 #include <linux/key.h>
 #include <trace/syscall.h>
@@ -121,6 +122,68 @@ struct perf_counter_hw_event;
 		.nb_args 	= nb,				\
 		.types		= types_##sname,		\
 		.args		= args_##sname,			\
+	};							\
+	static struct ftrace_event_call event_enter_##sname;	\
+	static int init_enter_##sname(void)			\
+	{							\
+		int num;					\
+		num = syscall_name_to_nr("sys"#sname);		\
+		if (num < 0)					\
+			return -ENOSYS;				\
+		register_ftrace_event(&event_syscall_enter);	\
+		INIT_LIST_HEAD(&event_enter_##sname.fields);	\
+		init_preds(&event_enter_##sname);		\
+		return 0;					\
+	}							\
+	static int reg_enter_##sname(void)			\
+	{							\
+		return reg_event_syscall_enter("sys"#sname);	\
+	}							\
+	static void unreg_enter_##sname(void)			\
+	{							\
+		unreg_event_syscall_enter("sys"#sname);		\
+	}							\
+	static struct ftrace_event_call __used			\
+	  __attribute__((__aligned__(4)))			\
+	  __attribute__((section("_ftrace_events")))		\
+	  event_enter_##sname = {				\
+		.name                   = "sys_enter"#sname,	\
+		.system                 = "syscalls",		\
+		.event                  = &event_syscall_enter,	\
+		.raw_init		= init_enter_##sname,	\
+		.regfunc		= reg_enter_##sname,	\
+		.unregfunc		= unreg_enter_##sname,	\
+	};							\
+	static struct ftrace_event_call event_exit_##sname;	\
+	static int init_exit_##sname(void)			\
+	{							\
+		int num;					\
+		num = syscall_name_to_nr("sys"#sname);		\
+		if (num < 0)					\
+			return -ENOSYS;				\
+		register_ftrace_event(&event_syscall_exit);	\
+		INIT_LIST_HEAD(&event_exit_##sname.fields);	\
+		init_preds(&event_exit_##sname);		\
+		return 0;					\
+	}							\
+	static int reg_exit_##sname(void)			\
+	{							\
+		return reg_event_syscall_exit("sys"#sname);	\
+	}							\
+	static void unreg_exit_##sname(void)			\
+	{							\
+		unreg_event_syscall_exit("sys"#sname);		\
+	}							\
+	static struct ftrace_event_call __used			\
+	  __attribute__((__aligned__(4)))			\
+	  __attribute__((section("_ftrace_events")))		\
+	  event_exit_##sname = {				\
+		.name                   = "sys_exit"#sname,	\
+		.system                 = "syscalls",		\
+		.event                  = &event_syscall_exit,	\
+		.raw_init		= init_exit_##sname,	\
+		.regfunc		= reg_exit_##sname,	\
+		.unregfunc		= unreg_exit_##sname,	\
 	}
 
 #define SYSCALL_DEFINE0(sname)					\
diff --git a/include/trace/syscall.h b/include/trace/syscall.h
index d5d8310..461f7dd 100644
--- a/include/trace/syscall.h
+++ b/include/trace/syscall.h
@@ -3,6 +3,8 @@
 
 #include <asm/ptrace.h>
 #include <linux/tracepoint.h>
+#include <linux/unistd.h>
+#include <linux/ftrace_event.h>
 
 extern void syscall_regfunc(void);
 extern void syscall_unregfunc(void);
@@ -37,17 +39,14 @@ struct syscall_metadata {
 };
 
 #ifdef CONFIG_FTRACE_SYSCALLS
-extern void arch_init_ftrace_syscalls(void);
 extern struct syscall_metadata *syscall_nr_to_meta(int nr);
-extern void start_ftrace_syscalls(void);
-extern void stop_ftrace_syscalls(void);
-extern void ftrace_syscall_enter(struct pt_regs *regs);
-extern void ftrace_syscall_exit(struct pt_regs *regs);
-#else
-static inline void start_ftrace_syscalls(void)			{ }
-static inline void stop_ftrace_syscalls(void)			{ }
-static inline void ftrace_syscall_enter(struct pt_regs *regs)	{ }
-static inline void ftrace_syscall_exit(struct pt_regs *regs)	{ }
+extern int syscall_name_to_nr(char *name);
+extern struct trace_event event_syscall_enter;
+extern struct trace_event event_syscall_exit;
+extern int reg_event_syscall_enter(char *name);
+extern void unreg_event_syscall_enter(char *name);
+extern int reg_event_syscall_exit(char *name);
+extern void unreg_event_syscall_exit(char *name);
 #endif
 
 #endif /* _TRACE_SYSCALL_H */
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 5e57964..a7a3962 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -1,15 +1,16 @@
 #include <trace/syscall.h>
 #include <linux/kernel.h>
+#include <linux/ftrace.h>
 #include <asm/syscall.h>
 
 #include "trace_output.h"
 #include "trace.h"
 
-/* Keep a counter of the syscall tracing users */
-static int refcount;
-
-/* Prevent from races on thread flags toggling */
 static DEFINE_MUTEX(syscall_trace_lock);
+int sys_refcount_enter;
+int sys_refcount_exit;
+static DECLARE_BITMAP(enabled_enter_syscalls, FTRACE_SYSCALL_MAX + 1);
+static DECLARE_BITMAP(enabled_exit_syscalls, FTRACE_SYSCALL_MAX + 1);
 
 /* Option to display the parameters types */
 enum {
@@ -95,54 +96,7 @@ print_syscall_exit(struct trace_iterator *iter, int flags)
 	return TRACE_TYPE_HANDLED;
 }
 
-void start_ftrace_syscalls(void)
-{
-	unsigned long flags;
-	struct task_struct *g, *t;
-
-	mutex_lock(&syscall_trace_lock);
-
-	/* Don't enable the flag on the tasks twice */
-	if (++refcount != 1)
-		goto unlock;
-
-	arch_init_ftrace_syscalls();
-	read_lock_irqsave(&tasklist_lock, flags);
-
-	do_each_thread(g, t) {
-		set_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
-	} while_each_thread(g, t);
-
-	read_unlock_irqrestore(&tasklist_lock, flags);
-
-unlock:
-	mutex_unlock(&syscall_trace_lock);
-}
-
-void stop_ftrace_syscalls(void)
-{
-	unsigned long flags;
-	struct task_struct *g, *t;
-
-	mutex_lock(&syscall_trace_lock);
-
-	/* There are perhaps still some users */
-	if (--refcount)
-		goto unlock;
-
-	read_lock_irqsave(&tasklist_lock, flags);
-
-	do_each_thread(g, t) {
-		clear_tsk_thread_flag(t, TIF_SYSCALL_FTRACE);
-	} while_each_thread(g, t);
-
-	read_unlock_irqrestore(&tasklist_lock, flags);
-
-unlock:
-	mutex_unlock(&syscall_trace_lock);
-}
-
-void ftrace_syscall_enter(struct pt_regs *regs)
+void ftrace_syscall_enter(struct pt_regs *regs, long id)
 {
 	struct syscall_trace_enter *entry;
 	struct syscall_metadata *sys_data;
@@ -151,6 +105,8 @@ void ftrace_syscall_enter(struct pt_regs *regs)
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_enter_syscalls))
+		return;
 
 	sys_data = syscall_nr_to_meta(syscall_nr);
 	if (!sys_data)
@@ -171,7 +127,7 @@ void ftrace_syscall_enter(struct pt_regs *regs)
 	trace_wake_up();
 }
 
-void ftrace_syscall_exit(struct pt_regs *regs)
+void ftrace_syscall_exit(struct pt_regs *regs, long ret)
 {
 	struct syscall_trace_exit *entry;
 	struct syscall_metadata *sys_data;
@@ -179,6 +135,8 @@ void ftrace_syscall_exit(struct pt_regs *regs)
 	int syscall_nr;
 
 	syscall_nr = syscall_get_nr(current, regs);
+	if (!test_bit(syscall_nr, enabled_exit_syscalls))
+		return;
 
 	sys_data = syscall_nr_to_meta(syscall_nr);
 	if (!sys_data)
@@ -197,54 +155,86 @@ void ftrace_syscall_exit(struct pt_regs *regs)
 	trace_wake_up();
 }
 
-static int init_syscall_tracer(struct trace_array *tr)
+int reg_event_syscall_enter(char *name)
 {
-	start_ftrace_syscalls();
+	int ret = 0;
+	int num;
 
-	return 0;
+	num = syscall_name_to_nr(name);
+	if (num < 0)
+		return -ENOSYS;
+	mutex_lock(&syscall_trace_lock);
+	if (!sys_refcount_enter)
+		ret = register_trace_syscall_enter(ftrace_syscall_enter);
+	if (ret) {
+		pr_info("event trace: Could not activate"
+				"syscall entry trace point");
+	} else {
+		set_bit(num, enabled_enter_syscalls);
+		sys_refcount_enter++;
+	}
+	mutex_unlock(&syscall_trace_lock);
+	return ret;
 }
 
-static void reset_syscall_tracer(struct trace_array *tr)
+void unreg_event_syscall_enter(char *name)
 {
-	stop_ftrace_syscalls();
-	tracing_reset_online_cpus(tr);
-}
-
-static struct trace_event syscall_enter_event = {
-	.type	 	= TRACE_SYSCALL_ENTER,
-	.trace		= print_syscall_enter,
-};
-
-static struct trace_event syscall_exit_event = {
-	.type	 	= TRACE_SYSCALL_EXIT,
-	.trace		= print_syscall_exit,
-};
+	int num;
 
-static struct tracer syscall_tracer __read_mostly = {
-	.name	     	= "syscall",
-	.init		= init_syscall_tracer,
-	.reset		= reset_syscall_tracer,
-	.flags		= &syscalls_flags,
-};
+	num = syscall_name_to_nr(name);
+	if (num < 0)
+		return;
+	mutex_lock(&syscall_trace_lock);
+	sys_refcount_enter--;
+	clear_bit(num, enabled_enter_syscalls);
+	if (!sys_refcount_enter)
+		unregister_trace_syscall_enter(ftrace_syscall_enter);
+	mutex_unlock(&syscall_trace_lock);
+}
 
-__init int register_ftrace_syscalls(void)
+int reg_event_syscall_exit(char *name)
 {
-	int ret;
+	int ret = 0;
+	int num;
 
-	ret = register_ftrace_event(&syscall_enter_event);
-	if (!ret) {
-		printk(KERN_WARNING "event %d failed to register\n",
-		       syscall_enter_event.type);
-		WARN_ON_ONCE(1);
+	num = syscall_name_to_nr(name);
+	if (num < 0)
+		return -ENOSYS;
+	mutex_lock(&syscall_trace_lock);
+	if (!sys_refcount_exit)
+		ret = register_trace_syscall_exit(ftrace_syscall_exit);
+	if (ret) {
+		pr_info("event trace: Could not activate"
+				"syscall exit trace point");
+	} else {
+		set_bit(num, enabled_exit_syscalls);
+		sys_refcount_exit++;
 	}
+	mutex_unlock(&syscall_trace_lock);
+	return ret;
+}
 
-	ret = register_ftrace_event(&syscall_exit_event);
-	if (!ret) {
-		printk(KERN_WARNING "event %d failed to register\n",
-		       syscall_exit_event.type);
-		WARN_ON_ONCE(1);
-	}
+void unreg_event_syscall_exit(char *name)
+{
+	int num;
 
-	return register_tracer(&syscall_tracer);
+	num = syscall_name_to_nr(name);
+	if (num < 0)
+		return;
+	mutex_lock(&syscall_trace_lock);
+	sys_refcount_exit--;
+	clear_bit(num, enabled_exit_syscalls);
+	if (!sys_refcount_exit)
+		unregister_trace_syscall_exit(ftrace_syscall_exit);
+	mutex_unlock(&syscall_trace_lock);
 }
-device_initcall(register_ftrace_syscalls);
+
+struct trace_event event_syscall_enter = {
+	.trace			= print_syscall_enter,
+	.type			= TRACE_SYSCALL_ENTER
+};
+
+struct trace_event event_syscall_exit = {
+	.trace			= print_syscall_exit,
+	.type			= TRACE_SYSCALL_EXIT
+};
-- 
1.6.0.6

Re: [PATCH 0/7] add syscall tracepoints

[Ingo Molnar]

* Jason Baron <jbaron@redhat.com> wrote:

> hi,
> 
> The following is an implementation of Frederic's syscall tracer on 
> top of tracepoints. It adds the ability to toggle the entry/exit 
> of each syscall via the standard 
> events/syscalls/syscall_blah/enable interface. The implementation 
> is done by adding 2 tracepoints. One on entry and one for exit.
> 
> This implementation required a few 'core' api changes. I've added 
> 'DECLARE_TRACE_REG()' macro which takes a register and and an 
> unregister function as arguments. This allowed me to toggle the 
> ftrace tif flag when the first tracepoint callback is added and 
> the last is removed. Current callers of 'DECLARE_TRACE()' are not 
> impacted.
> 
> Another change was to call arch_init_ftrace_syscalls via an 
> 'arch_initall'. In this implmentation I needed to access the 
> syscalls_metadata structure at runtime in order to determine which 
> syscalls were 'traceable'. Although the implementation uses 
> SYSCALL_DEFINE() to set up the the trace events, for some reason 
> at runtime there is no syscalls_metadata, associated with some of 
> the SYSCALL_DEFINE() calls. I'm not quite sure why that is. 
> However, by calling arch_init_ftrace_syscalls() at boot I can make 
> sure the lists are in sync.
> 
> thanks,
> 
> -Jason 
> 
> 
>  arch/x86/include/asm/ftrace.h |    4 +-
>  arch/x86/kernel/ftrace.c      |   24 +++++-
>  arch/x86/kernel/ptrace.c      |    6 +-
>  include/linux/syscalls.h      |   63 +++++++++++++++
>  include/linux/tracepoint.h    |   27 ++++++-
>  include/trace/syscall.h       |   37 +++++++---
>  kernel/trace/trace_events.c   |   29 +++++---
>  kernel/trace/trace_syscalls.c |  172 +++++++++++++++++++----------------------
>  kernel/tracepoint.c           |   38 +++++++++
>  9 files changed, 278 insertions(+), 122 deletions(-)

This looks far nicer structurally. Steve, Frederic, what's your take 
on this series?

	Ingo

Re: [PATCH 0/7] add syscall tracepoints

[Frederic Weisbecker]

On Tue, Jun 16, 2009 at 09:32:20PM +0200, Ingo Molnar wrote:
> 
> * Jason Baron <jbaron@redhat.com> wrote:
> 
> > hi,
> > 
> > The following is an implementation of Frederic's syscall tracer on 
> > top of tracepoints. It adds the ability to toggle the entry/exit 
> > of each syscall via the standard 
> > events/syscalls/syscall_blah/enable interface. The implementation 
> > is done by adding 2 tracepoints. One on entry and one for exit.
> > 
> > This implementation required a few 'core' api changes. I've added 
> > 'DECLARE_TRACE_REG()' macro which takes a register and and an 
> > unregister function as arguments. This allowed me to toggle the 
> > ftrace tif flag when the first tracepoint callback is added and 
> > the last is removed. Current callers of 'DECLARE_TRACE()' are not 
> > impacted.
> > 
> > Another change was to call arch_init_ftrace_syscalls via an 
> > 'arch_initall'. In this implmentation I needed to access the 
> > syscalls_metadata structure at runtime in order to determine which 
> > syscalls were 'traceable'. Although the implementation uses 
> > SYSCALL_DEFINE() to set up the the trace events, for some reason 
> > at runtime there is no syscalls_metadata, associated with some of 
> > the SYSCALL_DEFINE() calls. I'm not quite sure why that is. 
> > However, by calling arch_init_ftrace_syscalls() at boot I can make 
> > sure the lists are in sync.
> > 
> > thanks,
> > 
> > -Jason 
> > 
> > 
> >  arch/x86/include/asm/ftrace.h |    4 +-
> >  arch/x86/kernel/ftrace.c      |   24 +++++-
> >  arch/x86/kernel/ptrace.c      |    6 +-
> >  include/linux/syscalls.h      |   63 +++++++++++++++
> >  include/linux/tracepoint.h    |   27 ++++++-
> >  include/trace/syscall.h       |   37 +++++++---
> >  kernel/trace/trace_events.c   |   29 +++++---
> >  kernel/trace/trace_syscalls.c |  172 +++++++++++++++++++----------------------
> >  kernel/tracepoint.c           |   38 +++++++++
> >  9 files changed, 278 insertions(+), 122 deletions(-)
> 
> This looks far nicer structurally. Steve, Frederic, what's your take 
> on this series?
> 
> 	Ingo


Sorry, I hadn't a lot of time recently to review it but I'll
have some time to do it soon.

Thanks.

Re: [PATCH 0/7] add syscall tracepoints

[Frederic Weisbecker]

On Tue, Jun 16, 2009 at 09:32:20PM +0200, Ingo Molnar wrote:
> 
> * Jason Baron <jbaron@redhat.com> wrote:
> 
> > hi,
> > 
> > The following is an implementation of Frederic's syscall tracer on 
> > top of tracepoints. It adds the ability to toggle the entry/exit 
> > of each syscall via the standard 
> > events/syscalls/syscall_blah/enable interface. The implementation 
> > is done by adding 2 tracepoints. One on entry and one for exit.
> > 
> > This implementation required a few 'core' api changes. I've added 
> > 'DECLARE_TRACE_REG()' macro which takes a register and and an 
> > unregister function as arguments. This allowed me to toggle the 
> > ftrace tif flag when the first tracepoint callback is added and 
> > the last is removed. Current callers of 'DECLARE_TRACE()' are not 
> > impacted.
> > 
> > Another change was to call arch_init_ftrace_syscalls via an 
> > 'arch_initall'. In this implmentation I needed to access the 
> > syscalls_metadata structure at runtime in order to determine which 
> > syscalls were 'traceable'. Although the implementation uses 
> > SYSCALL_DEFINE() to set up the the trace events, for some reason 
> > at runtime there is no syscalls_metadata, associated with some of 
> > the SYSCALL_DEFINE() calls. I'm not quite sure why that is. 
> > However, by calling arch_init_ftrace_syscalls() at boot I can make 
> > sure the lists are in sync.
> > 
> > thanks,
> > 
> > -Jason 
> > 
> > 
> >  arch/x86/include/asm/ftrace.h |    4 +-
> >  arch/x86/kernel/ftrace.c      |   24 +++++-
> >  arch/x86/kernel/ptrace.c      |    6 +-
> >  include/linux/syscalls.h      |   63 +++++++++++++++
> >  include/linux/tracepoint.h    |   27 ++++++-
> >  include/trace/syscall.h       |   37 +++++++---
> >  kernel/trace/trace_events.c   |   29 +++++---
> >  kernel/trace/trace_syscalls.c |  172 +++++++++++++++++++----------------------
> >  kernel/tracepoint.c           |   38 +++++++++
> >  9 files changed, 278 insertions(+), 122 deletions(-)
> 
> This looks far nicer structurally. Steve, Frederic, what's your take 
> on this series?
> 
> 	Ingo


I've reviewed all of them and only found small neats to comment.
Yeah the direction looks better now.

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>