* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 15:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers
In-Reply-To: <20180324023038.938665-7-ast@fb.com>
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);
>
^ permalink raw reply
* Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints
From: Steven Rostedt @ 2018-03-26 15:04 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, davem, torvalds, peterz, netdev, kernel-team,
linux-api
In-Reply-To: <c8be6d52-3f86-0402-9fdc-cc98f8e3e940@iogearbox.net>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-26 15:14 UTC (permalink / raw)
To: rostedt
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api
In-Reply-To: <20180326110204.042801dd@gandalf.local.home>
----- 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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints
From: Daniel Borkmann @ 2018-03-26 15:32 UTC (permalink / raw)
To: Steven Rostedt
Cc: Alexei Starovoitov, davem, torvalds, peterz, netdev, kernel-team,
linux-api
In-Reply-To: <20180326110435.4cdd7e7d@gandalf.local.home>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 15:42 UTC (permalink / raw)
To: Mathieu Desnoyers, rostedt
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
netdev, kernel-team, linux-api
In-Reply-To: <1787605856.4574.1522077244597.JavaMail.zimbra@efficios.com>
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'.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints
From: Steven Rostedt @ 2018-03-26 15:47 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, davem, torvalds, peterz, netdev, kernel-team,
linux-api
In-Reply-To: <ba96cda5-c4d6-4d66-0128-b7053d25fcd8@iogearbox.net>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-26 15:55 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <5bcacdb5-e72f-b67a-4884-61fcedf0938a@fb.com>
----- 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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 15:56 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api
In-Reply-To: <20180324023038.938665-7-ast@fb.com>
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;
> }
^ permalink raw reply
* Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints
From: Alexei Starovoitov @ 2018-03-26 16:00 UTC (permalink / raw)
To: Steven Rostedt, Daniel Borkmann
Cc: davem, torvalds, peterz, netdev, kernel-team, linux-api
In-Reply-To: <20180326114725.1999288a@gandalf.local.home>
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?
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 16:08 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <523311773.184.1522079745421.JavaMail.zimbra@efficios.com>
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 ?
^ permalink raw reply
* Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints
From: Steven Rostedt @ 2018-03-26 16:16 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, davem, torvalds, peterz, netdev, kernel-team,
linux-api
In-Reply-To: <3967d838-e737-ec44-d03b-54f11f85d21b@fb.com>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-26 16:17 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <f39181e0-3d46-2154-044c-dfc985053b3a@fb.com>
----- 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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 16:25 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <1055377367.195.1522081045131.JavaMail.zimbra@efficios.com>
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?
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 16:31 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api
In-Reply-To: <20180326115615.0ca53410@gandalf.local.home>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 16:35 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <89fbc745-c290-c82c-a837-8998cf2988e7@fb.com>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 16:47 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <20180326123530.66ced6ae@gandalf.local.home>
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.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-26 16:57 UTC (permalink / raw)
To: rostedt, Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <20180326123530.66ced6ae@gandalf.local.home>
----- 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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 17:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <b07ea9b4-6b73-60aa-9391-002db42c571b@fb.com>
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
^ permalink raw reply
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Alan Tull @ 2018-03-26 17:21 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <20180323043304.GA14733@hao-dev>
On Thu, Mar 22, 2018 at 11:33 PM, Wu Hao <hao.wu@intel.com> wrote:
>> > +
>> > +/*
>> > + * This function resets the FPGA Port and its accelerator (AFU) by function
>> > + * __fpga_port_disable and __fpga_port_enable (set port soft reset bit and
>> > + * then clear it). Userspace can do Port reset at any time, e.g during DMA
>> > + * or Partial Reconfiguration. But it should never cause any system level
>> > + * issue, only functional failure (e.g DMA or PR operation failure) and be
>> > + * recoverable from the failure.
>> > + *
>> > + * Note: the accelerator (AFU) is not accessible when its port is in reset
>> > + * (disabled). Any attempts on MMIO access to AFU while in reset, will
>> > + * result errors reported via port error reporting sub feature (if present).
>> > + */
>> > +static inline int __fpga_port_reset(struct platform_device *pdev)
>> > +{
>> > + int ret;
>> > +
>> > + ret = __fpga_port_disable(pdev);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + __fpga_port_enable(pdev);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static inline int fpga_port_reset(struct platform_device *pdev)
>> > +{
>> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> > + int ret;
>> > +
>> > + mutex_lock(&pdata->lock);
>> > + ret = __fpga_port_reset(pdev);
>> > + mutex_unlock(&pdata->lock);
>> > +
>> > + return ret;
>> > +}
>>
>> I'm still scratching my head about how the enumeration code also has
>> code that handles resetting the PL in a FPGA region and
>> enabling/disabling the bridge. We've discussed this before [1] and I
>> know you've looked into it, I'm still trying to figure out how this
>> can be made modular, so when someone needs to support a different port
>> in the future, it isn't a complete rewrite.
>>
>> Speaking of resets, one way forward would be to create a reset
>> controller for the port (and if possible move the port code to the
>> bridge platform driver). The current linux-next repo adds support for
>> reset lookups, so that reset controllers are supported for non-DT
>> platforms [2].
>>
>> So the bridge driver would implement the enable/disable functions and
>> create a reset controller, the fpga-region (or whoever else needs it)
>> could look the reset controller and use the reset. By using the
>> kernel reset framework, we don't have to have that piece of code
>> shared around by having a reset function in a .h file. And it avoids
>> adding extra dependencies between modules. Also, where necessary, I'd
>> rather add functionality to the existing bridge/mgr/region frameworks,
>> adding common interfaces at that level to allow reuse (like adding
>> status to fpga-mgr). Ideally, this DFL framework would sit on top of
>> mgr and bridge and allow those to be swapped out for reuse of the DFL
>> framework on other devices. Also it will save future headaches as mgr
>> or port implementations evolve.
>
> Thanks a lot for the suggestion. I really really appreciate this.
Yes, this is a good discussion, thanks.
>
> Actually if we consider the virutalization case as I mentioned in [1] below,
> that means AFU and its Port will be turned into a PCI VF and assigned (passed
> through) to a virtual machine. There is no FME block on that PCI VF device,
> (the FME is always kept in PCI PF device in the host) and currently the bridge
> is created by FME module for PR functionatily. So in the guest virtual machine,
> nobody creates the reset controller actually.
>
> As I mentioned in [1], one possible method is, put these port reset functions to
> AFU (Port) module, and share those functions with FME bridge module.
Yes, the port reset functions could move into an AFU driver, and then
also the AFU driver could also create a reset controller and register
a lookup [2] for the reset. That would be just a few lines of code.
The reset controller would control enabling/disabling the port. The
bridge driver could get the reset controller to use during FPGA
programming. That is instead of sharing a reset function with the
bridge driver. It decouples the FPGA bridge driver and simplifies it
to be something that just needs to control a reset instead of needing
to include a specific .h file that makes a port reset function
available.
> I think
> that will make the code in the common DFL framework a little more clean,
Yes, IIUC that may also make it easier as the port/AFU gets added
functionality that is intended to be controlled by the VF anyway
(while the only port-related thing that is needed by the FME is port
enable/disable).
> but it
> will introduce some module dependency here for sure, (e.g FME modules can't
> finish PR without AFU (Port) Module loaded).
That sounds like an OK type of dependency, i.e. if the modules are not
all loaded, it doesn't work. :-)
> But anyway it may be still
> acceptable for users as all these modules could be loaded automatically. How do
> you think? :)
The other thing I want to get right now is if there is a different
AFU/port that needs a different driver. Can the DFL be changed to
specify what AFU/port to load? I really really want to avoid large
code rewrites in the future that we can anticipate now. Such as
someone implements their own static image, it has DFL, but the port is
somewhat different. Instead of seeing features as just something that
gets added, the DFL also specifies what port driver and mgr driver to
load. The stuff we discussed above is a good step towards that, but
not all of it.
Alan
>
> Thanks
> Hao
>
>
>>
>> Alan
>>
>> [1] https://lkml.org/lkml/2017/12/22/398
>> [2] https://patchwork.kernel.org/patch/10247475/
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 17:55 UTC (permalink / raw)
To: Mathieu Desnoyers, rostedt
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <1218234422.259.1522083422808.JavaMail.zimbra@efficios.com>
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?!
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 18:11 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <17073efa-d833-7348-bef1-79376ad43bc6@fb.com>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 18:39 UTC (permalink / raw)
To: Steven Rostedt
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <20180326141104.6d63660a@gandalf.local.home>
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.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 18:48 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <543f953a-8284-68f0-6a22-ec2d5f247199@fb.com>
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
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-26 21:27 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <17073efa-d833-7348-bef1-79376ad43bc6@fb.com>
----- 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
^ permalink raw reply
* [PATCH bpf-next] bpf, tracing: unbreak lttng
From: Alexei Starovoitov @ 2018-03-26 22:08 UTC (permalink / raw)
To: davem
Cc: daniel, torvalds, peterz, rostedt, mathieu.desnoyers, netdev,
kernel-team, linux-api
for_each_kernel_tracepoint() is used by out-of-tree lttng module
and therefore cannot be changed.
Instead introduce kernel_tracepoint_find_by_name() to find
tracepoint by name.
Fixes: 9e9afbae6514 ("tracepoint: compute num_args at build time")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/tracepoint.h | 14 ++++++++++----
kernel/bpf/syscall.c | 11 +----------
kernel/tracepoint.c | 36 ++++++++++++++++++++----------------
3 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 2194e7c31484..035887dc4ed3 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -42,16 +42,22 @@ extern int
tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data);
#ifdef CONFIG_TRACEPOINTS
-void *
-for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
+void
+for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
void *priv);
+struct tracepoint *kernel_tracepoint_find_by_name(const char *name);
#else
-static inline void *
-for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
+static inline void
+for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
void *priv)
{
return NULL;
}
+static inline struct tracepoint *
+kernel_tracepoint_find_by_name(const char *name)
+{
+ return NULL;
+}
#endif
#ifdef CONFIG_MODULES
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ae8b43f1cee3..644311777d8e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1334,15 +1334,6 @@ static const struct file_operations bpf_raw_tp_fops = {
.write = bpf_dummy_write,
};
-static void *__find_tp(struct tracepoint *tp, void *priv)
-{
- char *name = priv;
-
- if (!strcmp(tp->name, name))
- return tp;
- return NULL;
-}
-
#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
@@ -1358,7 +1349,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
return -EFAULT;
tp_name[sizeof(tp_name) - 1] = 0;
- tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+ tp = kernel_tracepoint_find_by_name(tp_name);
if (!tp)
return -ENOENT;
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 3f2dc5738c2b..764d02fbe782 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -502,22 +502,17 @@ 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 NULL;
- for (iter = begin; iter < end; iter++) {
- ret = fct(*iter, priv);
- if (ret)
- return ret;
- }
- return NULL;
+ return;
+ for (iter = begin; iter < end; iter++)
+ fct(*iter, priv);
}
/**
@@ -525,14 +520,23 @@ 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)
{
- return for_each_tracepoint_range(__start___tracepoints_ptrs,
- __stop___tracepoints_ptrs, fct, priv);
+ for_each_tracepoint_range(__start___tracepoints_ptrs,
+ __stop___tracepoints_ptrs, fct, priv);
}
EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);
+struct tracepoint *kernel_tracepoint_find_by_name(const char *name)
+{
+ struct tracepoint * const *tp = __start___tracepoints_ptrs;
+
+ for (; tp < __stop___tracepoints_ptrs; tp++)
+ if (!strcmp((*tp)->name, name))
+ return *tp;
+ return NULL;
+}
#ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
/* NB: reg/unreg are called while guarded with the tracepoints_mutex */
--
2.9.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox