Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints

Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints

[Daniel Borkmann]

On 03/24/2018 03:30 AM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> v4->v5:
> - adopted Daniel's fancy REPEAT macro in bpf_trace.c in patch 7
>   
> v3->v4:
> - adopted Linus's CAST_TO_U64 macro to cast any integer, pointer, or small
>   struct to u64. That nicely reduced the size of patch 1
> 
> v2->v3:
> - with Linus's suggestion introduced generic COUNT_ARGS and CONCATENATE macros
>   (or rather moved them from apparmor)
>   that cleaned up patches 6 and 7
> - added patch 4 to refactor trace_iwlwifi_dev_ucode_error() from 17 args to 4
>   Now any tracepoint with >12 args will have build error
> 
> v1->v2:
> - simplified api by combing bpf_raw_tp_open(name) + bpf_attach(prog_fd) into
>   bpf_raw_tp_open(name, prog_fd) as suggested by Daniel.
>   That simplifies bpf_detach as well which is now simple close() of fd.
> - fixed memory leak in error path which was spotted by Daniel.
> - fixed bpf_get_stackid(), bpf_perf_event_output() called from raw tracepoints
> - added more tests
> - fixed allyesconfig build caught by buildbot
> 
> v1:
> This patch set is a different way to address the pressing need to access
> task_struct pointers in sched tracepoints from bpf programs.
> 
> The first approach simply added these pointers to sched tracepoints:
> https://lkml.org/lkml/2017/12/14/753
> which Peter nacked.
> Few options were discussed and eventually the discussion converged on
> doing bpf specific tracepoint_probe_register() probe functions.
> Details here:
> https://lkml.org/lkml/2017/12/20/929
> 
> Patch 1 is kernel wide cleanup of pass-struct-by-value into
> pass-struct-by-reference into tracepoints.
> 
> Patches 2 and 3 are minor cleanups to address allyesconfig build
> 
> Patch 4 refactor trace_iwlwifi_dev_ucode_error from 17 to 4 args
> 
> Patch 5 introduces COUNT_ARGS macro
> 
> Patch 6 minor prep work to expose number of arguments passed
> into tracepoints.
> 
> Patch 7 introduces BPF_RAW_TRACEPOINT api.
> the auto-cleanup and multiple concurrent users are must have
> features of tracing api. For bpf raw tracepoints it looks like:
>   // load bpf prog with BPF_PROG_TYPE_RAW_TRACEPOINT type
>   prog_fd = bpf_prog_load(...);
> 
>   // receive anon_inode fd for given bpf_raw_tracepoint
>   // and attach bpf program to it
>   raw_tp_fd = bpf_raw_tracepoint_open("xdp_exception", prog_fd);
> 
> Ctrl-C of tracing daemon or cmdline tool will automatically
> detach bpf program, unload it and unregister tracepoint probe.
> More details in patch 7.
> 
> Patch 8 - trivial support in libbpf
> Patches 9, 10 - user space tests
> 
> samples/bpf/test_overhead performance on 1 cpu:
> 
> tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
> task_rename   1.1M   769K        947K            1.0M
> urandom_read  789K   697K        750K            755K

Applied to bpf-next, thanks Alexei!

Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints

[Steven Rostedt]

On Mon, 26 Mar 2018 10:28:03 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> > tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
> > task_rename   1.1M   769K        947K            1.0M
> > urandom_read  789K   697K        750K            755K  
> 
> Applied to bpf-next, thanks Alexei!

Please wait till you have the proper acks. Some of this affects
tracing.

-- Steve

Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints

[Daniel Borkmann]

On 03/26/2018 05:04 PM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 10:28:03 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
>>> tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
>>> task_rename   1.1M   769K        947K            1.0M
>>> urandom_read  789K   697K        750K            755K  
>>
>> Applied to bpf-next, thanks Alexei!
> 
> Please wait till you have the proper acks. Some of this affects
> tracing.

Ok, I thought time up to v5 was long enough. Anyway, in case there are
objections I can still toss out the series from bpf-next tree worst case
should e.g. follow-up fixups not be appropriate.

Thanks,
Daniel

Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints

[Steven Rostedt]

On Mon, 26 Mar 2018 17:32:02 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 03/26/2018 05:04 PM, Steven Rostedt wrote:
> > On Mon, 26 Mar 2018 10:28:03 +0200
> > Daniel Borkmann <daniel@iogearbox.net> wrote:
> >   
> >>> tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
> >>> task_rename   1.1M   769K        947K            1.0M
> >>> urandom_read  789K   697K        750K            755K    
> >>
> >> Applied to bpf-next, thanks Alexei!  
> > 
> > Please wait till you have the proper acks. Some of this affects
> > tracing.  
> 
> Ok, I thought time up to v5 was long enough. Anyway, in case there are
> objections I can still toss out the series from bpf-next tree worst case
> should e.g. follow-up fixups not be appropriate.

Yeah, I've been traveling a bit which slowed down my review process
(trying to catch up). My main concern is with patch 6, as there are
external users of those functions. Although, we generally don't cater
to out of tree code, we play nice with LTTng, and I don't want to break
it.

I also should probably pull in the patches and run them through my
tests to make sure they don't have any other side effects.

-- Steve

Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints

[Alexei Starovoitov]

On 3/26/18 8:47 AM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 17:32:02 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 03/26/2018 05:04 PM, Steven Rostedt wrote:
>>> On Mon, 26 Mar 2018 10:28:03 +0200
>>> Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>
>>>>> tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
>>>>> task_rename   1.1M   769K        947K            1.0M
>>>>> urandom_read  789K   697K        750K            755K
>>>>
>>>> Applied to bpf-next, thanks Alexei!
>>>
>>> Please wait till you have the proper acks. Some of this affects
>>> tracing.
>>
>> Ok, I thought time up to v5 was long enough. Anyway, in case there are
>> objections I can still toss out the series from bpf-next tree worst case
>> should e.g. follow-up fixups not be appropriate.
>
> Yeah, I've been traveling a bit which slowed down my review process
> (trying to catch up).

v1 of this set was posted Feb 28.
imo one month is not an acceptable delay for maintainer to review
the patches. You really need to consider group maintainership as
we do with Daniel for bpf tree.

> My main concern is with patch 6, as there are
> external users of those functions. Although, we generally don't cater
> to out of tree code, we play nice with LTTng, and I don't want to break
> it.

out-of-tree module is out of tree. I'm beyond surprised that you
propose to keep for_each_kernel_tracepoint() as-is with zero in-tree
users in order to keep lttng working.

> I also should probably pull in the patches and run them through my
> tests to make sure they don't have any other side effects.

so let me rephrase.
You're saying that a change to a function with zero in-tree users
can somehow break your tests?
How is that possible?
Does it mean you also have some out-of-tree modules that will break?
and that _is_ the real reason for objection?

Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints

[Steven Rostedt]

On Mon, 26 Mar 2018 09:00:33 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> On 3/26/18 8:47 AM, Steven Rostedt wrote:
> > On Mon, 26 Mar 2018 17:32:02 +0200
> > Daniel Borkmann <daniel@iogearbox.net>


> >> On 03/26/2018 05:04 PM, Steven Rostedt wrote:  
> >>> On Mon, 26 Mar 2018 10:28:03 +0200
> >>> Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>  
> >>>>> tracepoint    base  kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
> >>>>> task_rename   1.1M   769K        947K            1.0M
> >>>>> urandom_read  789K   697K        750K            755K  
> >>>>
> >>>> Applied to bpf-next, thanks Alexei!  
> >>>
> >>> Please wait till you have the proper acks. Some of this affects
> >>> tracing.  
> >>
> >> Ok, I thought time up to v5 was long enough. Anyway, in case there are
> >> objections I can still toss out the series from bpf-next tree worst case
> >> should e.g. follow-up fixups not be appropriate.  
> >
> > Yeah, I've been traveling a bit which slowed down my review process
> > (trying to catch up).  
> 
> v1 of this set was posted Feb 28.

Yep, Where I traveled to the West coast 2/26 - 3/1 (but due to snow
storms, I didn't get home till late 3/2). Then I went back 3/6 and came
home 3/8 (again due to another snow storm, it was 3/9). Then I went to
ELC from 3/11 to 3/15 (Luckily, the third snow storm hit 3/14, and
didn't affect my return trip).

> imo one month is not an acceptable delay for maintainer to review
> the patches. You really need to consider group maintainership as
> we do with Daniel for bpf tree.

Perhaps, (which I talked to Masami about, just need to go through
logistics). But the tracing code isn't high volume, and the three weeks
of traveling for me was a fluke (didn't look at my schedule when I
agreed to make that second one).

> 
> > My main concern is with patch 6, as there are
> > external users of those functions. Although, we generally don't cater
> > to out of tree code, we play nice with LTTng, and I don't want to break
> > it.  
> 
> out-of-tree module is out of tree. I'm beyond surprised that you
> propose to keep for_each_kernel_tracepoint() as-is with zero in-tree
> users in order to keep lttng working.

I'm nice.

> 
> > I also should probably pull in the patches and run them through my
> > tests to make sure they don't have any other side effects.  
> 
> so let me rephrase.
> You're saying that a change to a function with zero in-tree users
> can somehow break your tests?
> How is that possible?
> Does it mean you also have some out-of-tree modules that will break?
> and that _is_ the real reason for objection?

That function isn't what I'm worried about. You changed much more than
that.

-- Steve

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Steven Rostedt]

On Fri, 23 Mar 2018 19:30:34 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> From: Alexei Starovoitov <ast@kernel.org>
> 
> add fancy macro to compute number of arguments passed into tracepoint
> at compile time and store it as part of 'struct tracepoint'.
> The number is necessary to check safety of bpf program access that
> is coming in subsequent patch.
> 
> for_each_tracepoint_range() api has no users inside the kernel.
> Make it more useful with ability to stop for_each() loop depending
> via callback return value.
> In such form it's used in subsequent patch.

I believe this is used by LTTng.

-- Steve

> 
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  include/linux/tracepoint-defs.h |  1 +
>  include/linux/tracepoint.h      | 28 +++++++++++++++++++---------
>  include/trace/define_trace.h    | 14 +++++++-------
>  kernel/tracepoint.c             | 27 ++++++++++++++++-----------
>  4 files changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index 64ed7064f1fa..39a283c61c51 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -33,6 +33,7 @@ struct tracepoint {
>  	int (*regfunc)(void);
>  	void (*unregfunc)(void);
>  	struct tracepoint_func __rcu *funcs;
> +	u32 num_args;
>  };
>  
>  #endif
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index c94f466d57ef..2194e7c31484 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -40,9 +40,19 @@ tracepoint_probe_register_prio(struct tracepoint *tp, void *probe, void *data,
>  			       int prio);
>  extern int
>  tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data);
> -extern void
> -for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
> -		void *priv);
> +
> +#ifdef CONFIG_TRACEPOINTS
> +void *
> +for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
> +			   void *priv);
> +#else
> +static inline void *
> +for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
> +			   void *priv)
> +{
> +	return NULL;
> +}
> +#endif
>  
>  #ifdef CONFIG_MODULES
>  struct tp_module {
> @@ -230,18 +240,18 @@ extern void syscall_unregfunc(void);
>   * structures, so we create an array of pointers that will be used for iteration
>   * on the tracepoints.
>   */
> -#define DEFINE_TRACE_FN(name, reg, unreg)				 \
> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args)			 \
>  	static const char __tpstrtab_##name[]				 \
>  	__attribute__((section("__tracepoints_strings"))) = #name;	 \
>  	struct tracepoint __tracepoint_##name				 \
>  	__attribute__((section("__tracepoints"))) =			 \
> -		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\
> +		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
>  	static struct tracepoint * const __tracepoint_ptr_##name __used	 \
>  	__attribute__((section("__tracepoints_ptrs"))) =		 \
>  		&__tracepoint_##name;
>  
> -#define DEFINE_TRACE(name)						\
> -	DEFINE_TRACE_FN(name, NULL, NULL);
> +#define DEFINE_TRACE(name, num_args)					\
> +	DEFINE_TRACE_FN(name, NULL, NULL, num_args);
>  
>  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)				\
>  	EXPORT_SYMBOL_GPL(__tracepoint_##name)
> @@ -275,8 +285,8 @@ extern void syscall_unregfunc(void);
>  		return false;						\
>  	}
>  
> -#define DEFINE_TRACE_FN(name, reg, unreg)
> -#define DEFINE_TRACE(name)
> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args)
> +#define DEFINE_TRACE(name, num_args)
>  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
>  #define EXPORT_TRACEPOINT_SYMBOL(name)
>  
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index d9e3d4aa3f6e..96b22ace9ae7 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -25,7 +25,7 @@
>  
>  #undef TRACE_EVENT
>  #define TRACE_EVENT(name, proto, args, tstruct, assign, print)	\
> -	DEFINE_TRACE(name)
> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>  
>  #undef TRACE_EVENT_CONDITION
>  #define TRACE_EVENT_CONDITION(name, proto, args, cond, tstruct, assign, print) \
> @@ -39,24 +39,24 @@
>  #undef TRACE_EVENT_FN
>  #define TRACE_EVENT_FN(name, proto, args, tstruct,		\
>  		assign, print, reg, unreg)			\
> -	DEFINE_TRACE_FN(name, reg, unreg)
> +	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>  
>  #undef TRACE_EVENT_FN_COND
>  #define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct,		\
>  		assign, print, reg, unreg)			\
> -	DEFINE_TRACE_FN(name, reg, unreg)
> +	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>  
>  #undef DEFINE_EVENT
>  #define DEFINE_EVENT(template, name, proto, args) \
> -	DEFINE_TRACE(name)
> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>  
>  #undef DEFINE_EVENT_FN
>  #define DEFINE_EVENT_FN(template, name, proto, args, reg, unreg) \
> -	DEFINE_TRACE_FN(name, reg, unreg)
> +	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>  
>  #undef DEFINE_EVENT_PRINT
>  #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
> -	DEFINE_TRACE(name)
> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>  
>  #undef DEFINE_EVENT_CONDITION
>  #define DEFINE_EVENT_CONDITION(template, name, proto, args, cond) \
> @@ -64,7 +64,7 @@
>  
>  #undef DECLARE_TRACE
>  #define DECLARE_TRACE(name, proto, args)	\
> -	DEFINE_TRACE(name)
> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>  
>  #undef TRACE_INCLUDE
>  #undef __TRACE_INCLUDE
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 671b13457387..3f2dc5738c2b 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -502,17 +502,22 @@ static __init int init_tracepoints(void)
>  __initcall(init_tracepoints);
>  #endif /* CONFIG_MODULES */
>  
> -static void for_each_tracepoint_range(struct tracepoint * const *begin,
> -		struct tracepoint * const *end,
> -		void (*fct)(struct tracepoint *tp, void *priv),
> -		void *priv)
> +static void *for_each_tracepoint_range(struct tracepoint * const *begin,
> +				       struct tracepoint * const *end,
> +				       void *(*fct)(struct tracepoint *tp, void *priv),
> +				       void *priv)
>  {
>  	struct tracepoint * const *iter;
> +	void *ret;
>  
>  	if (!begin)
> -		return;
> -	for (iter = begin; iter < end; iter++)
> -		fct(*iter, priv);
> +		return NULL;
> +	for (iter = begin; iter < end; iter++) {
> +		ret = fct(*iter, priv);
> +		if (ret)
> +			return ret;
> +	}
> +	return NULL;
>  }
>  
>  /**
> @@ -520,11 +525,11 @@ static void for_each_tracepoint_range(struct tracepoint * const *begin,
>   * @fct: callback
>   * @priv: private data
>   */
> -void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
> -		void *priv)
> +void *for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
> +				 void *priv)
>  {
> -	for_each_tracepoint_range(__start___tracepoints_ptrs,
> -		__stop___tracepoints_ptrs, fct, priv);
> +	return for_each_tracepoint_range(__start___tracepoints_ptrs,
> +					 __stop___tracepoints_ptrs, fct, priv);
>  }
>  EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);
>  

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Mathieu Desnoyers]

----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:

> On Fri, 23 Mar 2018 19:30:34 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
> 
>> From: Alexei Starovoitov <ast@kernel.org>
>> 
>> add fancy macro to compute number of arguments passed into tracepoint
>> at compile time and store it as part of 'struct tracepoint'.
>> The number is necessary to check safety of bpf program access that
>> is coming in subsequent patch.
>> 
>> for_each_tracepoint_range() api has no users inside the kernel.
>> Make it more useful with ability to stop for_each() loop depending
>> via callback return value.
>> In such form it's used in subsequent patch.
> 
> I believe this is used by LTTng.

Indeed, and by SystemTAP as well.

What justifies the need to stop mid-iteration ? A less intrusive alternative
would be to use the "priv" data pointer to keep state telling further calls
to return immediately. Does performance of iteration over tracepoints really
matter here so much that stopping iteration immediately is worth it ?

Thanks,

Mathieu

> 
> -- Steve
> 
>> 
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>> ---
>>  include/linux/tracepoint-defs.h |  1 +
>>  include/linux/tracepoint.h      | 28 +++++++++++++++++++---------
>>  include/trace/define_trace.h    | 14 +++++++-------
>>  kernel/tracepoint.c             | 27 ++++++++++++++++-----------
>>  4 files changed, 43 insertions(+), 27 deletions(-)
>> 
>> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
>> index 64ed7064f1fa..39a283c61c51 100644
>> --- a/include/linux/tracepoint-defs.h
>> +++ b/include/linux/tracepoint-defs.h
>> @@ -33,6 +33,7 @@ struct tracepoint {
>>  	int (*regfunc)(void);
>>  	void (*unregfunc)(void);
>>  	struct tracepoint_func __rcu *funcs;
>> +	u32 num_args;
>>  };
>>  
>>  #endif
>> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
>> index c94f466d57ef..2194e7c31484 100644
>> --- a/include/linux/tracepoint.h
>> +++ b/include/linux/tracepoint.h
>> @@ -40,9 +40,19 @@ tracepoint_probe_register_prio(struct tracepoint *tp, void
>> *probe, void *data,
>>  			       int prio);
>>  extern int
>>  tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data);
>> -extern void
>> -for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
>> -		void *priv);
>> +
>> +#ifdef CONFIG_TRACEPOINTS
>> +void *
>> +for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
>> +			   void *priv);
>> +#else
>> +static inline void *
>> +for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
>> +			   void *priv)
>> +{
>> +	return NULL;
>> +}
>> +#endif
>>  
>>  #ifdef CONFIG_MODULES
>>  struct tp_module {
>> @@ -230,18 +240,18 @@ extern void syscall_unregfunc(void);
>>   * structures, so we create an array of pointers that will be used for iteration
>>   * on the tracepoints.
>>   */
>> -#define DEFINE_TRACE_FN(name, reg, unreg)				 \
>> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args)			 \
>>  	static const char __tpstrtab_##name[]				 \
>>  	__attribute__((section("__tracepoints_strings"))) = #name;	 \
>>  	struct tracepoint __tracepoint_##name				 \
>>  	__attribute__((section("__tracepoints"))) =			 \
>> -		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL };\
>> +		{ __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
>>  	static struct tracepoint * const __tracepoint_ptr_##name __used	 \
>>  	__attribute__((section("__tracepoints_ptrs"))) =		 \
>>  		&__tracepoint_##name;
>>  
>> -#define DEFINE_TRACE(name)						\
>> -	DEFINE_TRACE_FN(name, NULL, NULL);
>> +#define DEFINE_TRACE(name, num_args)					\
>> +	DEFINE_TRACE_FN(name, NULL, NULL, num_args);
>>  
>>  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)				\
>>  	EXPORT_SYMBOL_GPL(__tracepoint_##name)
>> @@ -275,8 +285,8 @@ extern void syscall_unregfunc(void);
>>  		return false;						\
>>  	}
>>  
>> -#define DEFINE_TRACE_FN(name, reg, unreg)
>> -#define DEFINE_TRACE(name)
>> +#define DEFINE_TRACE_FN(name, reg, unreg, num_args)
>> +#define DEFINE_TRACE(name, num_args)
>>  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
>>  #define EXPORT_TRACEPOINT_SYMBOL(name)
>>  
>> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
>> index d9e3d4aa3f6e..96b22ace9ae7 100644
>> --- a/include/trace/define_trace.h
>> +++ b/include/trace/define_trace.h
>> @@ -25,7 +25,7 @@
>>  
>>  #undef TRACE_EVENT
>>  #define TRACE_EVENT(name, proto, args, tstruct, assign, print)	\
>> -	DEFINE_TRACE(name)
>> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>>  
>>  #undef TRACE_EVENT_CONDITION
>>  #define TRACE_EVENT_CONDITION(name, proto, args, cond, tstruct, assign, print) \
>> @@ -39,24 +39,24 @@
>>  #undef TRACE_EVENT_FN
>>  #define TRACE_EVENT_FN(name, proto, args, tstruct,		\
>>  		assign, print, reg, unreg)			\
>> -	DEFINE_TRACE_FN(name, reg, unreg)
>> +	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>>  
>>  #undef TRACE_EVENT_FN_COND
>>  #define TRACE_EVENT_FN_COND(name, proto, args, cond, tstruct,		\
>>  		assign, print, reg, unreg)			\
>> -	DEFINE_TRACE_FN(name, reg, unreg)
>> +	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>>  
>>  #undef DEFINE_EVENT
>>  #define DEFINE_EVENT(template, name, proto, args) \
>> -	DEFINE_TRACE(name)
>> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>>  
>>  #undef DEFINE_EVENT_FN
>>  #define DEFINE_EVENT_FN(template, name, proto, args, reg, unreg) \
>> -	DEFINE_TRACE_FN(name, reg, unreg)
>> +	DEFINE_TRACE_FN(name, reg, unreg, COUNT_ARGS(args))
>>  
>>  #undef DEFINE_EVENT_PRINT
>>  #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
>> -	DEFINE_TRACE(name)
>> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>>  
>>  #undef DEFINE_EVENT_CONDITION
>>  #define DEFINE_EVENT_CONDITION(template, name, proto, args, cond) \
>> @@ -64,7 +64,7 @@
>>  
>>  #undef DECLARE_TRACE
>>  #define DECLARE_TRACE(name, proto, args)	\
>> -	DEFINE_TRACE(name)
>> +	DEFINE_TRACE(name, COUNT_ARGS(args))
>>  
>>  #undef TRACE_INCLUDE
>>  #undef __TRACE_INCLUDE
>> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
>> index 671b13457387..3f2dc5738c2b 100644
>> --- a/kernel/tracepoint.c
>> +++ b/kernel/tracepoint.c
>> @@ -502,17 +502,22 @@ static __init int init_tracepoints(void)
>>  __initcall(init_tracepoints);
>>  #endif /* CONFIG_MODULES */
>>  
>> -static void for_each_tracepoint_range(struct tracepoint * const *begin,
>> -		struct tracepoint * const *end,
>> -		void (*fct)(struct tracepoint *tp, void *priv),
>> -		void *priv)
>> +static void *for_each_tracepoint_range(struct tracepoint * const *begin,
>> +				       struct tracepoint * const *end,
>> +				       void *(*fct)(struct tracepoint *tp, void *priv),
>> +				       void *priv)
>>  {
>>  	struct tracepoint * const *iter;
>> +	void *ret;
>>  
>>  	if (!begin)
>> -		return;
>> -	for (iter = begin; iter < end; iter++)
>> -		fct(*iter, priv);
>> +		return NULL;
>> +	for (iter = begin; iter < end; iter++) {
>> +		ret = fct(*iter, priv);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return NULL;
>>  }
>>  
>>  /**
>> @@ -520,11 +525,11 @@ static void for_each_tracepoint_range(struct tracepoint *
>> const *begin,
>>   * @fct: callback
>>   * @priv: private data
>>   */
>> -void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
>> -		void *priv)
>> +void *for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void
>> *priv),
>> +				 void *priv)
>>  {
>> -	for_each_tracepoint_range(__start___tracepoints_ptrs,
>> -		__stop___tracepoints_ptrs, fct, priv);
>> +	return for_each_tracepoint_range(__start___tracepoints_ptrs,
>> +					 __stop___tracepoints_ptrs, fct, priv);
>>  }
>>  EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Alexei Starovoitov]

On 3/26/18 8:14 AM, Mathieu Desnoyers wrote:
> ----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:
>
>> On Fri, 23 Mar 2018 19:30:34 -0700
>> Alexei Starovoitov <ast@fb.com> wrote:
>>
>>> From: Alexei Starovoitov <ast@kernel.org>
>>>
>>> add fancy macro to compute number of arguments passed into tracepoint
>>> at compile time and store it as part of 'struct tracepoint'.
>>> The number is necessary to check safety of bpf program access that
>>> is coming in subsequent patch.
>>>
>>> for_each_tracepoint_range() api has no users inside the kernel.
>>> Make it more useful with ability to stop for_each() loop depending
>>> via callback return value.
>>> In such form it's used in subsequent patch.
>>
>> I believe this is used by LTTng.
>
> Indeed, and by SystemTAP as well.
>
> What justifies the need to stop mid-iteration ? A less intrusive alternative
> would be to use the "priv" data pointer to keep state telling further calls
> to return immediately. Does performance of iteration over tracepoints really
> matter here so much that stopping iteration immediately is worth it ?

I'm sure both you and Steven are not serious when you object
to _in-tree_ change to for_each_kernel_tracepoint() that
affects _out-of_tree_ modules?

Just change your module to 'return NULL' instead of plain 'return'.

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Mathieu Desnoyers]

----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov ast@fb.com wrote:

> On 3/26/18 8:14 AM, Mathieu Desnoyers wrote:
>> ----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:
>>
>>> On Fri, 23 Mar 2018 19:30:34 -0700
>>> Alexei Starovoitov <ast@fb.com> wrote:
>>>
>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>
>>>> add fancy macro to compute number of arguments passed into tracepoint
>>>> at compile time and store it as part of 'struct tracepoint'.
>>>> The number is necessary to check safety of bpf program access that
>>>> is coming in subsequent patch.
>>>>
>>>> for_each_tracepoint_range() api has no users inside the kernel.
>>>> Make it more useful with ability to stop for_each() loop depending
>>>> via callback return value.
>>>> In such form it's used in subsequent patch.
>>>
>>> I believe this is used by LTTng.
>>
>> Indeed, and by SystemTAP as well.
>>
>> What justifies the need to stop mid-iteration ? A less intrusive alternative
>> would be to use the "priv" data pointer to keep state telling further calls
>> to return immediately. Does performance of iteration over tracepoints really
>> matter here so much that stopping iteration immediately is worth it ?
> 
> I'm sure both you and Steven are not serious when you object
> to _in-tree_ change to for_each_kernel_tracepoint() that
> affects _out-of_tree_ modules?
> 
> Just change your module to 'return NULL' instead of plain 'return'.

I never said I objected to adapt the LTTng out of tree code. If there is a
solid reason for changing the kernel API, I will adapt my code to those
changes.

What I'm trying to understand here is whether there is solid ground for
the added complexity you are proposing. Is it a performance enhancement ?
If so, explanation of the use cases targeted, and numbers that measure
performance improvements are needed.

How is your patch making tracepoints "more useful" ?

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Alexei Starovoitov]

On 3/26/18 8:55 AM, Mathieu Desnoyers wrote:
> ----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov ast@fb.com wrote:
>
>> On 3/26/18 8:14 AM, Mathieu Desnoyers wrote:
>>> ----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:
>>>
>>>> On Fri, 23 Mar 2018 19:30:34 -0700
>>>> Alexei Starovoitov <ast@fb.com> wrote:
>>>>
>>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>>
>>>>> add fancy macro to compute number of arguments passed into tracepoint
>>>>> at compile time and store it as part of 'struct tracepoint'.
>>>>> The number is necessary to check safety of bpf program access that
>>>>> is coming in subsequent patch.
>>>>>
>>>>> for_each_tracepoint_range() api has no users inside the kernel.
>>>>> Make it more useful with ability to stop for_each() loop depending
>>>>> via callback return value.
>>>>> In such form it's used in subsequent patch.
>>>>
>>>> I believe this is used by LTTng.
>>>
>>> Indeed, and by SystemTAP as well.
>>>
>>> What justifies the need to stop mid-iteration ? A less intrusive alternative
>>> would be to use the "priv" data pointer to keep state telling further calls
>>> to return immediately. Does performance of iteration over tracepoints really
>>> matter here so much that stopping iteration immediately is worth it ?
>>
>> I'm sure both you and Steven are not serious when you object
>> to _in-tree_ change to for_each_kernel_tracepoint() that
>> affects _out-of_tree_ modules?
>>
>> Just change your module to 'return NULL' instead of plain 'return'.
>
> I never said I objected to adapt the LTTng out of tree code. If there is a
> solid reason for changing the kernel API, I will adapt my code to those
> changes.
>
> What I'm trying to understand here is whether there is solid ground for
> the added complexity you are proposing. Is it a performance enhancement ?
> If so, explanation of the use cases targeted, and numbers that measure
> performance improvements are needed.
>
> How is your patch making tracepoints "more useful" ?

are you arguing about the whole set overall or about a change
to for_each_kernel_tracepoint() ?

I'm hearing your arguments as that now changes to all exported functions
need to be "solid" (not sure what exactly means 'solid' here) to
justify breakage of out-of-tree modules?

re: 'added complexity'...
-       for (iter = begin; iter < end; iter++)
-               fct(*iter, priv);
+               return NULL;
+       for (iter = begin; iter < end; iter++) {
+               ret = fct(*iter, priv);
+               if (ret)
+                       return ret;
+       }
+       return NULL;

where do you see 'added complexity' ?
Isn't the above diff self-explanatory that for_each_tracepoint_range()
can be used not only to iterate over all tracepoints
(just do 'return NULL') from callback _and_ to find one particular
tracepoint as patch 7 does ?

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Mathieu Desnoyers]

----- On Mar 26, 2018, at 12:08 PM, Alexei Starovoitov ast@fb.com wrote:

> On 3/26/18 8:55 AM, Mathieu Desnoyers wrote:
>> ----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov ast@fb.com wrote:
>>
>>> On 3/26/18 8:14 AM, Mathieu Desnoyers wrote:
>>>> ----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:
>>>>
>>>>> On Fri, 23 Mar 2018 19:30:34 -0700
>>>>> Alexei Starovoitov <ast@fb.com> wrote:
>>>>>
>>>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>>>
>>>>>> add fancy macro to compute number of arguments passed into tracepoint
>>>>>> at compile time and store it as part of 'struct tracepoint'.
>>>>>> The number is necessary to check safety of bpf program access that
>>>>>> is coming in subsequent patch.
>>>>>>
>>>>>> for_each_tracepoint_range() api has no users inside the kernel.
>>>>>> Make it more useful with ability to stop for_each() loop depending
>>>>>> via callback return value.
>>>>>> In such form it's used in subsequent patch.
>>>>>
>>>>> I believe this is used by LTTng.
>>>>
>>>> Indeed, and by SystemTAP as well.
>>>>
>>>> What justifies the need to stop mid-iteration ? A less intrusive alternative
>>>> would be to use the "priv" data pointer to keep state telling further calls
>>>> to return immediately. Does performance of iteration over tracepoints really
>>>> matter here so much that stopping iteration immediately is worth it ?
>>>
>>> I'm sure both you and Steven are not serious when you object
>>> to _in-tree_ change to for_each_kernel_tracepoint() that
>>> affects _out-of_tree_ modules?
>>>
>>> Just change your module to 'return NULL' instead of plain 'return'.
>>
>> I never said I objected to adapt the LTTng out of tree code. If there is a
>> solid reason for changing the kernel API, I will adapt my code to those
>> changes.
>>
>> What I'm trying to understand here is whether there is solid ground for
>> the added complexity you are proposing. Is it a performance enhancement ?
>> If so, explanation of the use cases targeted, and numbers that measure
>> performance improvements are needed.
>>
>> How is your patch making tracepoints "more useful" ?
> 
> are you arguing about the whole set overall or about a change
> to for_each_kernel_tracepoint() ?

I'm perfectly fine with adding the "num_args" stuff. I think it's
really useful. It's only the for_each_kernel_tracepoint change for
which I'm trying to understand the rationale.

> I'm hearing your arguments as that now changes to all exported functions
> need to be "solid" (not sure what exactly means 'solid' here) to
> justify breakage of out-of-tree modules?

No. Any added complexity to tracepoint.c needs to be justified
appropriately.

> 
> re: 'added complexity'...
> -       for (iter = begin; iter < end; iter++)
> -               fct(*iter, priv);
> +               return NULL;
> +       for (iter = begin; iter < end; iter++) {
> +               ret = fct(*iter, priv);
> +               if (ret)
> +                       return ret;
> +       }
> +       return NULL;
> 
> where do you see 'added complexity' ?
> Isn't the above diff self-explanatory that for_each_tracepoint_range()
> can be used not only to iterate over all tracepoints
> (just do 'return NULL') from callback _and_ to find one particular
> tracepoint as patch 7 does ?

I am not arguing about your proposed implementation. I am arguing about
the lack of justification behind this change. Why is this change needed ?
What is it allowing you to do that cannot be done using the private data
pointer ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Alexei Starovoitov]

On 3/26/18 9:17 AM, Mathieu Desnoyers wrote:
>>
>> re: 'added complexity'...
>> -       for (iter = begin; iter < end; iter++)
>> -               fct(*iter, priv);
>> +               return NULL;
>> +       for (iter = begin; iter < end; iter++) {
>> +               ret = fct(*iter, priv);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +       return NULL;
>>
>> where do you see 'added complexity' ?
>> Isn't the above diff self-explanatory that for_each_tracepoint_range()
>> can be used not only to iterate over all tracepoints
>> (just do 'return NULL') from callback _and_ to find one particular
>> tracepoint as patch 7 does ?
>
> I am not arguing about your proposed implementation. I am arguing about
> the lack of justification behind this change. Why is this change needed ?
> What is it allowing you to do that cannot be done using the private data
> pointer ?

commit log of patch 6 states:

"for_each_tracepoint_range() api has no users inside the kernel.
Make it more useful with ability to stop for_each() loop depending
via callback return value.
In such form it's used in subsequent patch."

and in patch 7:

+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+       char *name = priv;
+
+       if (!strcmp(tp->name, name))
+               return tp;
+       return NULL;
+}
...
+       struct tracepoint *tp;
...
+       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+       if (!tp)
+               return -ENOENT;

still not obvious?

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Steven Rostedt]

On Mon, 26 Mar 2018 09:25:07 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> commit log of patch 6 states:
> 
> "for_each_tracepoint_range() api has no users inside the kernel.
> Make it more useful with ability to stop for_each() loop depending
> via callback return value.
> In such form it's used in subsequent patch."
> 
> and in patch 7:
> 
> +static void *__find_tp(struct tracepoint *tp, void *priv)
> +{
> +       char *name = priv;
> +
> +       if (!strcmp(tp->name, name))
> +               return tp;
> +       return NULL;
> +}
> ...
> +       struct tracepoint *tp;
> ...
> +       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
> +       if (!tp)
> +               return -ENOENT;
> 
> still not obvious?

Please just create a new function called tracepoint_find_by_name(), and
use that. I don't see any benefit in using a for_each* function for
such a simple routine. Not to mention, you then don't need to know the
internals of a tracepoint in kernel/bpf/syscall.c.

-- Steve

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Alexei Starovoitov]

On 3/26/18 9:35 AM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 09:25:07 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> commit log of patch 6 states:
>>
>> "for_each_tracepoint_range() api has no users inside the kernel.
>> Make it more useful with ability to stop for_each() loop depending
>> via callback return value.
>> In such form it's used in subsequent patch."
>>
>> and in patch 7:
>>
>> +static void *__find_tp(struct tracepoint *tp, void *priv)
>> +{
>> +       char *name = priv;
>> +
>> +       if (!strcmp(tp->name, name))
>> +               return tp;
>> +       return NULL;
>> +}
>> ...
>> +       struct tracepoint *tp;
>> ...
>> +       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
>> +       if (!tp)
>> +               return -ENOENT;
>>
>> still not obvious?
>
> Please just create a new function called tracepoint_find_by_name(), and
> use that. I don't see any benefit in using a for_each* function for
> such a simple routine. Not to mention, you then don't need to know the
> internals of a tracepoint in kernel/bpf/syscall.c.

It's a standard pattern in the kernel to stop for_each*() iterator
when callback function returns non-null.
Few examples:
idr_for_each
cfs_hash_for_each
and there are plenty more.

I don't mind to _rename_ for_each_kernel_tracepoint() into
tracepoint_find_by_name(), but keeping exported function
just to be used by out-of-tree modules would be wrong message for
the kernel community in general.
With my patch the for_each_kernel_tracepoint() will be used by bpf side
and out-of-tree can trivially hack their callbacks to keep working.
imo that's a better approach then renaming it.

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Steven Rostedt]

On Mon, 26 Mar 2018 09:47:21 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> I don't mind to _rename_ for_each_kernel_tracepoint() into
> tracepoint_find_by_name(), but keeping exported function
> just to be used by out-of-tree modules would be wrong message for
> the kernel community in general.
> With my patch the for_each_kernel_tracepoint() will be used by bpf side
> and out-of-tree can trivially hack their callbacks to keep working.
> imo that's a better approach then renaming it.

Look, the tracepoint code was written by Mathieu for LTTng, and perf
and ftrace were able to benefit because of it, as well as your bpf code.
For this, we agreed to keep this function around for his use, as its the
only thing he requires. Everyone has been fine with that. Not all out
of tree code is evil. In fact, some out of tree modules help the kernel
community. You ask why I care. Because PREEMPT_RT has been one of those
out of tree modules that has helped the kernel community a lot. Have
you noticed that there are "raw_spin_lock()" and "spin_lock()"? There's
no difference between the two in the kernel. Why have them? Because
they are used by PREEMPT_RT.

Having that function for LTTng does not hurt us. And I will NACK
removing it.

-- Stevwe

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Mathieu Desnoyers]

----- On Mar 26, 2018, at 12:35 PM, rostedt rostedt@goodmis.org wrote:

> On Mon, 26 Mar 2018 09:25:07 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
> 
>> commit log of patch 6 states:
>> 
>> "for_each_tracepoint_range() api has no users inside the kernel.
>> Make it more useful with ability to stop for_each() loop depending
>> via callback return value.
>> In such form it's used in subsequent patch."
>> 
>> and in patch 7:
>> 
>> +static void *__find_tp(struct tracepoint *tp, void *priv)
>> +{
>> +       char *name = priv;
>> +
>> +       if (!strcmp(tp->name, name))
>> +               return tp;
>> +       return NULL;
>> +}
>> ...
>> +       struct tracepoint *tp;
>> ...
>> +       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
>> +       if (!tp)
>> +               return -ENOENT;
>> 
>> still not obvious?
> 
> Please just create a new function called tracepoint_find_by_name(), and
> use that. I don't see any benefit in using a for_each* function for
> such a simple routine. Not to mention, you then don't need to know the
> internals of a tracepoint in kernel/bpf/syscall.c.

Steven's approach is fine by me, considering there should never be duplicated
tracepoint definitions (it emits a __tracepoint_##name symbol which would cause
multiple symbols definition errors at link time if there are more than
a single definition per tracepoint name in the core kernel). The exported
API should probably be named "kernel_tracepoint_find_by_name()" or something
similar, thus indicating that it only lookup tracepoints in the core kernel.

Which brings the next question: what are Alexei's plan to handle tracepoints
in modules, considering module load/unload scenarios ? The tracepoint API
has module notifiers for this, but it does not appear to be used in this
patch series.

Thanks,

Mathieu

> 
> -- Steve

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Alexei Starovoitov]

On 3/26/18 9:57 AM, Mathieu Desnoyers wrote:
> ----- On Mar 26, 2018, at 12:35 PM, rostedt rostedt@goodmis.org wrote:
>
>> On Mon, 26 Mar 2018 09:25:07 -0700
>> Alexei Starovoitov <ast@fb.com> wrote:
>>
>>> commit log of patch 6 states:
>>>
>>> "for_each_tracepoint_range() api has no users inside the kernel.
>>> Make it more useful with ability to stop for_each() loop depending
>>> via callback return value.
>>> In such form it's used in subsequent patch."
>>>
>>> and in patch 7:
>>>
>>> +static void *__find_tp(struct tracepoint *tp, void *priv)
>>> +{
>>> +       char *name = priv;
>>> +
>>> +       if (!strcmp(tp->name, name))
>>> +               return tp;
>>> +       return NULL;
>>> +}
>>> ...
>>> +       struct tracepoint *tp;
>>> ...
>>> +       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
>>> +       if (!tp)
>>> +               return -ENOENT;
>>>
>>> still not obvious?
>>
>> Please just create a new function called tracepoint_find_by_name(), and
>> use that. I don't see any benefit in using a for_each* function for
>> such a simple routine. Not to mention, you then don't need to know the
>> internals of a tracepoint in kernel/bpf/syscall.c.
>
> Steven's approach is fine by me, considering there should never be duplicated
> tracepoint definitions (it emits a __tracepoint_##name symbol which would cause
> multiple symbols definition errors at link time if there are more than
> a single definition per tracepoint name in the core kernel). The exported
> API should probably be named "kernel_tracepoint_find_by_name()" or something
> similar, thus indicating that it only lookup tracepoints in the core kernel.

An email ago you were ok to s/return/return NULL/ in your out-of-tree
module, but now flip flop to add new function approach just to
reduce the work you need to do in lttng?
We're not talking about changing __kmalloc signature here.
My patch extends for_each_kernel_tracepoint() api similar to other
for_each_*() iterators and improves possible uses of it.

It can event help lltng.

lttng-tracepoint.c is doing:

for_each_kernel_tracepoint(lttng_kernel_tracepoint_add, &ret);

to copy kernel tracepoints into its own hash table.

Only to later do:
/*
  * Get tracepoint if the tracepoint is present in the tracepoint hash 
table.
  * Must be called with lttng_tracepoint_mutex held.
  * Returns NULL if not present.
  */
static
struct tracepoint_entry *get_tracepoint(const char *name)
{
	struct hlist_head *head;
	struct tracepoint_entry *e;
	u32 hash = jhash(name, strlen(name), 0);

	head = &tracepoint_table[hash & (TRACEPOINT_TABLE_SIZE - 1)];
	lttng_hlist_for_each_entry(e, head, hlist) {
		if (!strcmp(name, e->name))
			return e;
	}
	return NULL;
}

this use case potentially can be reimplemented in lttng with
this extended for_each_kernel_tracepoint() api.
Like I do on bpf side with very similar callback:
+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+       char *name = priv;
+
+       if (!strcmp(tp->name, name))
+               return tp;
+       return NULL;
+}

> Which brings the next question: what are Alexei's plan to handle tracepoints
> in modules, considering module load/unload scenarios ? The tracepoint API
> has module notifiers for this, but it does not appear to be used in this
> patch series.

correct. this set deals with in-kernel tracepoints only.
No attempt to do anything with tracepoints inside modules.

but your question brings another question:
why kernel/module.c have this code:
  mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs"
                                       sizeof(*mod->tracepoints_ptrs),
                                       &mod->num_tracepoints);
and the whole module tracepoint notifier logic
that is only used by out-of-tree ?

I didn't realize so much of kernel code was taken hostage by lttng.

One thing is to be nice to out-of-tree and do not break them
for no reason, but arguing that kernel shouldn't add a minor extension
to for_each_kernel_tracepoint() api is really taking the whole thing
to next level.

Also I hope you noticed that the patch is doing:
+++ b/include/linux/tracepoint-defs.h
@@ -33,6 +33,7 @@ struct tracepoint {
         int (*regfunc)(void);
         void (*unregfunc)(void);
         struct tracepoint_func __rcu *funcs;
+       u32 num_args;
  };

To make sure that bpf programs are safe I need to do a static check
in the verifier that programs don't access arguments beyond
those specified by the tracepoint.

That was mentioned in the commit log of patch 6 too:
"
compute number of arguments passed into tracepoint
at compile time and store it as part of 'struct tracepoint'.
The number is necessary to check safety of bpf program access that
is coming in subsequent patch.
"

I suspect you will have the same objection?
It breaks out-of-tree modules?!

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Steven Rostedt]

On Mon, 26 Mar 2018 10:55:51 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> An email ago you were ok to s/return/return NULL/ in your out-of-tree
> module, but now flip flop to add new function approach just to
> reduce the work you need to do in lttng?
> We're not talking about changing __kmalloc signature here.
> My patch extends for_each_kernel_tracepoint() api similar to other
> for_each_*() iterators and improves possible uses of it.

Alexei, do you have another use case for using
for_each_kernel_tracepoint() other than the find_tp? If so, then I'm
sure Mathieu can handle the change.

But I think it's cleaner to add a tracepoint_find_by_name() function.
If you come up with another use case for using the for_each* function
then we'll consider changing it then.


> One thing is to be nice to out-of-tree and do not break them
> for no reason, but arguing that kernel shouldn't add a minor extension
> to for_each_kernel_tracepoint() api is really taking the whole thing
> to next level.

That's not the point. I disagree with the reason for the change, and
believe that it would be cleaner to add a find_by_name() function.
Which would make your patch set even cleaner. 

Instead of having in the bpf code:

static void *__find_tp(struct tracepoint *tp, void *priv)
{
	char *name = priv;

	if (!strcmp(tp->name, name))
		return tp;
	return NULL;
}

[..]

	tp = for_each_kernel_tracepoint(__find_tp, tp_name);
	if (!tp)
		return -ENOENT;


You would simply have:

	tp = tracepoint_find_by_name(tp_name);
	if (!tp)
		return -ENOENT;

That would make the code more obvious to what it is doing. And this
does not impede your patch set at all.

-- Steve

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Alexei Starovoitov]

On 3/26/18 11:11 AM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 10:55:51 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> An email ago you were ok to s/return/return NULL/ in your out-of-tree
>> module, but now flip flop to add new function approach just to
>> reduce the work you need to do in lttng?
>> We're not talking about changing __kmalloc signature here.
>> My patch extends for_each_kernel_tracepoint() api similar to other
>> for_each_*() iterators and improves possible uses of it.
>
> Alexei, do you have another use case for using
> for_each_kernel_tracepoint() other than the find_tp? If so, then I'm
> sure Mathieu can handle the change.
>
> But I think it's cleaner to add a tracepoint_find_by_name() function.
> If you come up with another use case for using the for_each* function
> then we'll consider changing it then.

another use case ?! Frankly such reasoning smells.

I'm fine doing quick followup patch to add tracepoint_find_by_name()
and restore 'return void' behavior of for_each_kernel_tracepoint's
callback, but I'm struggling to accept the precedent it will create
that all exported functions of kernel/tracepoint.c are really
lttng extensions and we cannot easily change them.
I'd like to hear Linus take on this.

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Steven Rostedt]

On Mon, 26 Mar 2018 11:39:05 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> On 3/26/18 11:11 AM, Steven Rostedt wrote:
> > On Mon, 26 Mar 2018 10:55:51 -0700
> > Alexei Starovoitov <ast@fb.com> wrote:
> >  
> >> An email ago you were ok to s/return/return NULL/ in your out-of-tree
> >> module, but now flip flop to add new function approach just to
> >> reduce the work you need to do in lttng?
> >> We're not talking about changing __kmalloc signature here.
> >> My patch extends for_each_kernel_tracepoint() api similar to other
> >> for_each_*() iterators and improves possible uses of it.  
> >
> > Alexei, do you have another use case for using
> > for_each_kernel_tracepoint() other than the find_tp? If so, then I'm
> > sure Mathieu can handle the change.
> >
> > But I think it's cleaner to add a tracepoint_find_by_name() function.
> > If you come up with another use case for using the for_each* function
> > then we'll consider changing it then.  
> 
> another use case ?! Frankly such reasoning smells.

WTF is the big deal here?

> 
> I'm fine doing quick followup patch to add tracepoint_find_by_name()

And BTW, it would actually have to be called
tracepoint_core_find_by_name() as it will not deal with modules.
Modules would have to have much more work to deal with.

> and restore 'return void' behavior of for_each_kernel_tracepoint's

What? you can't rebase now? Just don't touch that function.

> callback, but I'm struggling to accept the precedent it will create
> that all exported functions of kernel/tracepoint.c are really
> lttng extensions and we cannot easily change them.

First, my argument about your use case has little to do with LTTng.
I have to maintain this code, and this is my preference. Just like I do
the silly

 /* Comment like this,
  * for multiple lines
  */

When I deal with the networking code. Because that's the preference for
the networking folks.

> I'd like to hear Linus take on this.

I doubt he cares about something this petty.

-- Steve

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Mathieu Desnoyers]

----- On Mar 26, 2018, at 1:55 PM, Alexei Starovoitov ast@fb.com wrote:

[...]

> 
> correct. this set deals with in-kernel tracepoints only.
> No attempt to do anything with tracepoints inside modules.

Please endeavor to handle in-module tracepoints properly, then we'll
be able to pursue a more constructive discussion.

[...]

> Also I hope you noticed that the patch is doing:
> +++ b/include/linux/tracepoint-defs.h
> @@ -33,6 +33,7 @@ struct tracepoint {
>         int (*regfunc)(void);
>         void (*unregfunc)(void);
>         struct tracepoint_func __rcu *funcs;
> +       u32 num_args;
>  };
> 
> To make sure that bpf programs are safe I need to do a static check
> in the verifier that programs don't access arguments beyond
> those specified by the tracepoint.
> 
> That was mentioned in the commit log of patch 6 too:
> "
> compute number of arguments passed into tracepoint
> at compile time and store it as part of 'struct tracepoint'.
> The number is necessary to check safety of bpf program access that
> is coming in subsequent patch.
> "

This part of the patch and its associated changelog is fine
with me. Please submit it as a separate commit from the rest
of the tracepoint.{c,h} changes.

[...]

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Steven Rostedt]

On Fri, 23 Mar 2018 19:30:34 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> +static void *for_each_tracepoint_range(struct tracepoint * const *begin,
> +				       struct tracepoint * const *end,
> +				       void *(*fct)(struct tracepoint *tp, void *priv),
> +				       void *priv)
>  {
>  	struct tracepoint * const *iter;
> +	void *ret;
>  
>  	if (!begin)
> -		return;
> -	for (iter = begin; iter < end; iter++)
> -		fct(*iter, priv);
> +		return NULL;
> +	for (iter = begin; iter < end; iter++) {
> +		ret = fct(*iter, priv);
> +		if (ret)
> +			return ret;

So you just stopped the loop here. You have an inconsistent state. What
about the functions that were called before. How do you undo them? Or
what about the rest that haven't been touched. This function gives no
feedback to the caller.

-- Steve


> +	}
> +	return NULL;
>  }

Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Steven Rostedt]

On Mon, 26 Mar 2018 11:56:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 23 Mar 2018 19:30:34 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
> 
> > +static void *for_each_tracepoint_range(struct tracepoint * const *begin,
> > +				       struct tracepoint * const *end,
> > +				       void *(*fct)(struct tracepoint *tp, void *priv),
> > +				       void *priv)
> >  {
> >  	struct tracepoint * const *iter;
> > +	void *ret;
> >  
> >  	if (!begin)
> > -		return;
> > -	for (iter = begin; iter < end; iter++)
> > -		fct(*iter, priv);
> > +		return NULL;
> > +	for (iter = begin; iter < end; iter++) {
> > +		ret = fct(*iter, priv);
> > +		if (ret)
> > +			return ret;  
> 
> So you just stopped the loop here. You have an inconsistent state. What
> about the functions that were called before. How do you undo them? Or
> what about the rest that haven't been touched. This function gives no
> feedback to the caller.
> 

OK, I see my confusion with this patch. I much rather have a new
function, and this isn't about being nice to out of tree modules. We
can keep this function as is (to be nice), but my biggest squabble
about this patch is that the function name is inconsistent to what its
doing. When I have a function that says "for_each" I expect it to go
through each function and not exit out. This is because of what I said
above. When you have a "for_each" function that stops in the middle, you
have a state that you may need to deal with. How would another use of
that function clean up the mess if it expected to fail at some random
location.

I see in the next patch that you are using it simply to find a
tracepoint with the given name. I'd be much happier to add a new
function called:

   tracepoint_find_by_name(const char *name)

Because using a "for_each" to implement such a simple function seems
more of a hack.

-- Steve