[PATCHv2 bpf-next 0/4] bpf: Speed up symbol resolving in kprobe multi link

[PATCHv2 bpf-next 0/4] bpf: Speed up symbol resolving in kprobe multi link

[Jiri Olsa]

hi,
sending additional fix for symbol resolving in kprobe multi link
requested by Alexei and Andrii [1].

This speeds up bpftrace kprobe attachment, when using pure symbols
(3344 symbols) to attach:

Before:

  # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
  ...
  6.5681 +- 0.0225 seconds time elapsed  ( +-  0.34% )

After:

  # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
  ...
  0.5661 +- 0.0275 seconds time elapsed  ( +-  4.85% )


v2 changes (first version [2]):
  - removed the 2 seconds check [Alexei]
  - moving/forcing symbols sorting out of kallsyms_lookup_names function [Alexei]
  - skipping one array allocation and copy_from_user [Andrii]
  - several small fixes [Masami,Andrii]
  - build fix [kernel test robot]

thanks,
jirka


[1] https://lore.kernel.org/bpf/CAEf4BzZtQaiUxQ-sm_hH2qKPRaqGHyOfEsW96DxtBHRaKLoL3Q@mail.gmail.com/
[2] https://lore.kernel.org/bpf/20220407125224.310255-1-jolsa@kernel.org/
---
Jiri Olsa (4):
      kallsyms: Add kallsyms_lookup_names function
      fprobe: Resolve symbols with kallsyms_lookup_names
      bpf: Resolve symbols with kallsyms_lookup_names for kprobe multi link
      selftests/bpf: Add attach bench test

 include/linux/kallsyms.h                                   |   6 ++++++
 kernel/kallsyms.c                                          |  70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 kernel/trace/bpf_trace.c                                   | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
 kernel/trace/fprobe.c                                      |  32 ++++++++++++--------------------
 tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/kprobe_multi_empty.c     |  12 ++++++++++++
 6 files changed, 302 insertions(+), 67 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_empty.c

[PATCHv2 bpf-next 1/4] kallsyms: Add kallsyms_lookup_names function

[Jiri Olsa]

Adding kallsyms_lookup_names function that resolves array of symbols
with single pass over kallsyms.

The user provides array of string pointers with count and pointer to
allocated array for resolved values.

  int kallsyms_lookup_names(const char **syms, size_t cnt,
                            unsigned long *addrs)

It iterates all kalsyms symbols and tries to loop up each in provided
symbols array with bsearch. The symbols array needs to be sorted by
name for this reason.

We also check each symbol to pass ftrace_location, because this API
will be used for fprobe symbols resolving. This can be optional in
future if there's a need.

We need kallsyms_on_each_symbol function, so enabling it and also
the new function for CONFIG_FPROBE option.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/kallsyms.h |  6 ++++
 kernel/kallsyms.c        | 70 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index ce1bd2fbf23e..7c82fa7445d4 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -72,6 +72,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
 #ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
+int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs);
 
 extern int kallsyms_lookup_size_offset(unsigned long addr,
 				  unsigned long *symbolsize,
@@ -103,6 +104,11 @@ static inline unsigned long kallsyms_lookup_name(const char *name)
 	return 0;
 }
 
+static inline int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs)
+{
+	return -ERANGE;
+}
+
 static inline int kallsyms_lookup_size_offset(unsigned long addr,
 					      unsigned long *symbolsize,
 					      unsigned long *offset)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 79f2eb617a62..ef940b25f3fc 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -29,6 +29,7 @@
 #include <linux/compiler.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/bsearch.h>
 
 /*
  * These will be re-linked against their real values
@@ -228,7 +229,7 @@ unsigned long kallsyms_lookup_name(const char *name)
 	return module_kallsyms_lookup_name(name);
 }
 
-#ifdef CONFIG_LIVEPATCH
+#if defined(CONFIG_LIVEPATCH) || defined(CONFIG_FPROBE)
 /*
  * Iterate over all symbols in vmlinux.  For symbols from modules use
  * module_kallsyms_on_each_symbol instead.
@@ -572,6 +573,73 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address)
 	return __sprint_symbol(buffer, address, -1, 1, 1);
 }
 
+#ifdef CONFIG_FPROBE
+static int symbols_cmp(const void *a, const void *b)
+{
+	const char **str_a = (const char **) a;
+	const char **str_b = (const char **) b;
+
+	return strcmp(*str_a, *str_b);
+}
+
+struct kallsyms_data {
+	unsigned long *addrs;
+	const char **syms;
+	size_t cnt;
+	size_t found;
+};
+
+static int kallsyms_callback(void *data, const char *name,
+			     struct module *mod, unsigned long addr)
+{
+	struct kallsyms_data *args = data;
+
+	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
+		return 0;
+
+	addr = ftrace_location(addr);
+	if (!addr)
+		return 0;
+
+	args->addrs[args->found++] = addr;
+	return args->found == args->cnt ? 1 : 0;
+}
+
+/**
+ * kallsyms_lookup_names - Lookup addresses for array of symbols
+ *
+ * @syms: array of symbols pointers symbols to resolve, must be
+ * alphabetically sorted
+ * @cnt: number of symbols/addresses in @syms/@addrs arrays
+ * @addrs: array for storing resulting addresses
+ *
+ * This function looks up addresses for array of symbols provided in
+ * @syms array (must be alphabetically sorted) and stores them in
+ * @addrs array, which needs to be big enough to store at least @cnt
+ * addresses.
+ *
+ * This function returns 0 if all provided symbols are found,
+ * -ESRCH otherwise.
+ */
+int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs)
+{
+	struct kallsyms_data args;
+
+	args.addrs = addrs;
+	args.syms = syms;
+	args.cnt = cnt;
+	args.found = 0;
+	kallsyms_on_each_symbol(kallsyms_callback, &args);
+
+	return args.found == args.cnt ? 0 : -ESRCH;
+}
+#else
+int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs)
+{
+	return -ERANGE;
+}
+#endif /* CONFIG_FPROBE */
+
 /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
 struct kallsym_iter {
 	loff_t pos;
-- 
2.35.1

Re: [PATCHv2 bpf-next 1/4] kallsyms: Add kallsyms_lookup_names function

[Masami Hiramatsu]

On Mon, 18 Apr 2022 14:48:31 +0200
Jiri Olsa <jolsa@kernel.org> wrote:

> Adding kallsyms_lookup_names function that resolves array of symbols
> with single pass over kallsyms.
> 
> The user provides array of string pointers with count and pointer to
> allocated array for resolved values.
> 
>   int kallsyms_lookup_names(const char **syms, size_t cnt,
>                             unsigned long *addrs)

What about renaming the 'syms' argument to 'sorted_syms' so that user
is easily notice what is required?
Or renaming the function as kallsyms_lookup_sorted_names()?


> 
> It iterates all kalsyms symbols and tries to loop up each in provided
> symbols array with bsearch. The symbols array needs to be sorted by
> name for this reason.
> 
> We also check each symbol to pass ftrace_location, because this API
> will be used for fprobe symbols resolving. This can be optional in
> future if there's a need.
> 
> We need kallsyms_on_each_symbol function, so enabling it and also
> the new function for CONFIG_FPROBE option.
> 
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/kallsyms.h |  6 ++++
>  kernel/kallsyms.c        | 70 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index ce1bd2fbf23e..7c82fa7445d4 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -72,6 +72,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
>  #ifdef CONFIG_KALLSYMS
>  /* Lookup the address for a symbol. Returns 0 if not found. */
>  unsigned long kallsyms_lookup_name(const char *name);
> +int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs);
>  
>  extern int kallsyms_lookup_size_offset(unsigned long addr,
>  				  unsigned long *symbolsize,
> @@ -103,6 +104,11 @@ static inline unsigned long kallsyms_lookup_name(const char *name)
>  	return 0;
>  }
>  
> +static inline int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs)
> +{
> +	return -ERANGE;
> +}
> +
>  static inline int kallsyms_lookup_size_offset(unsigned long addr,
>  					      unsigned long *symbolsize,
>  					      unsigned long *offset)
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 79f2eb617a62..ef940b25f3fc 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -29,6 +29,7 @@
>  #include <linux/compiler.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/bsearch.h>
>  
>  /*
>   * These will be re-linked against their real values
> @@ -228,7 +229,7 @@ unsigned long kallsyms_lookup_name(const char *name)
>  	return module_kallsyms_lookup_name(name);
>  }
>  
> -#ifdef CONFIG_LIVEPATCH
> +#if defined(CONFIG_LIVEPATCH) || defined(CONFIG_FPROBE)
>  /*
>   * Iterate over all symbols in vmlinux.  For symbols from modules use
>   * module_kallsyms_on_each_symbol instead.
> @@ -572,6 +573,73 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address)
>  	return __sprint_symbol(buffer, address, -1, 1, 1);
>  }
>  
> +#ifdef CONFIG_FPROBE
> +static int symbols_cmp(const void *a, const void *b)
> +{
> +	const char **str_a = (const char **) a;
> +	const char **str_b = (const char **) b;
> +
> +	return strcmp(*str_a, *str_b);
> +}
> +
> +struct kallsyms_data {
> +	unsigned long *addrs;
> +	const char **syms;
> +	size_t cnt;
> +	size_t found;
> +};
> +
> +static int kallsyms_callback(void *data, const char *name,
> +			     struct module *mod, unsigned long addr)
> +{
> +	struct kallsyms_data *args = data;
> +
> +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> +		return 0;
> +
> +	addr = ftrace_location(addr);
> +	if (!addr)
> +		return 0;

Ooops, wait. Did you do this last version? I missed this point.
This changes the meanings of the kernel function.

> +
> +	args->addrs[args->found++] = addr;
> +	return args->found == args->cnt ? 1 : 0;
> +}
> +
> +/**
> + * kallsyms_lookup_names - Lookup addresses for array of symbols

More correctly "Lookup 'ftraced' addresses for array of sorted symbols", right?

I'm not sure, we can call it as a 'kallsyms' API, since this is using
kallsyms but doesn't return symbol address, but ftrace address.
I think this name misleads user to expect returning symbol address.

> + *
> + * @syms: array of symbols pointers symbols to resolve, must be
> + * alphabetically sorted
> + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> + * @addrs: array for storing resulting addresses
> + *
> + * This function looks up addresses for array of symbols provided in
> + * @syms array (must be alphabetically sorted) and stores them in
> + * @addrs array, which needs to be big enough to store at least @cnt
> + * addresses.

Hmm, sorry I changed my mind. I rather like to expose kallsyms_on_each_symbol()
and provide this API from fprobe or ftrace, because this returns ftrace address
and thus this is only used from fprobe.

Thank you,

> + *
> + * This function returns 0 if all provided symbols are found,
> + * -ESRCH otherwise.
> + */
> +int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs)
> +{
> +	struct kallsyms_data args;
> +
> +	args.addrs = addrs;
> +	args.syms = syms;
> +	args.cnt = cnt;
> +	args.found = 0;
> +	kallsyms_on_each_symbol(kallsyms_callback, &args);
> +
> +	return args.found == args.cnt ? 0 : -ESRCH;
> +}
> +#else
> +int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs)
> +{
> +	return -ERANGE;
> +}
> +#endif /* CONFIG_FPROBE */
> +
>  /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
>  struct kallsym_iter {
>  	loff_t pos;
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCHv2 bpf-next 1/4] kallsyms: Add kallsyms_lookup_names function

[Jiri Olsa]

On Mon, Apr 18, 2022 at 11:35:46PM +0900, Masami Hiramatsu wrote:
> On Mon, 18 Apr 2022 14:48:31 +0200
> Jiri Olsa <jolsa@kernel.org> wrote:
> 
> > Adding kallsyms_lookup_names function that resolves array of symbols
> > with single pass over kallsyms.
> > 
> > The user provides array of string pointers with count and pointer to
> > allocated array for resolved values.
> > 
> >   int kallsyms_lookup_names(const char **syms, size_t cnt,
> >                             unsigned long *addrs)
> 
> What about renaming the 'syms' argument to 'sorted_syms' so that user
> is easily notice what is required?
> Or renaming the function as kallsyms_lookup_sorted_names()?
> 
> 
> > 
> > It iterates all kalsyms symbols and tries to loop up each in provided
> > symbols array with bsearch. The symbols array needs to be sorted by
> > name for this reason.
> > 
> > We also check each symbol to pass ftrace_location, because this API
> > will be used for fprobe symbols resolving. This can be optional in
> > future if there's a need.
> > 
> > We need kallsyms_on_each_symbol function, so enabling it and also
> > the new function for CONFIG_FPROBE option.
> > 
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/kallsyms.h |  6 ++++
> >  kernel/kallsyms.c        | 70 +++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 75 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> > index ce1bd2fbf23e..7c82fa7445d4 100644
> > --- a/include/linux/kallsyms.h
> > +++ b/include/linux/kallsyms.h
> > @@ -72,6 +72,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
> >  #ifdef CONFIG_KALLSYMS
> >  /* Lookup the address for a symbol. Returns 0 if not found. */
> >  unsigned long kallsyms_lookup_name(const char *name);
> > +int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs);
> >  
> >  extern int kallsyms_lookup_size_offset(unsigned long addr,
> >  				  unsigned long *symbolsize,
> > @@ -103,6 +104,11 @@ static inline unsigned long kallsyms_lookup_name(const char *name)
> >  	return 0;
> >  }
> >  
> > +static inline int kallsyms_lookup_names(const char **syms, size_t cnt, unsigned long *addrs)
> > +{
> > +	return -ERANGE;
> > +}
> > +
> >  static inline int kallsyms_lookup_size_offset(unsigned long addr,
> >  					      unsigned long *symbolsize,
> >  					      unsigned long *offset)
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 79f2eb617a62..ef940b25f3fc 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/compiler.h>
> >  #include <linux/module.h>
> >  #include <linux/kernel.h>
> > +#include <linux/bsearch.h>
> >  
> >  /*
> >   * These will be re-linked against their real values
> > @@ -228,7 +229,7 @@ unsigned long kallsyms_lookup_name(const char *name)
> >  	return module_kallsyms_lookup_name(name);
> >  }
> >  
> > -#ifdef CONFIG_LIVEPATCH
> > +#if defined(CONFIG_LIVEPATCH) || defined(CONFIG_FPROBE)
> >  /*
> >   * Iterate over all symbols in vmlinux.  For symbols from modules use
> >   * module_kallsyms_on_each_symbol instead.
> > @@ -572,6 +573,73 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address)
> >  	return __sprint_symbol(buffer, address, -1, 1, 1);
> >  }
> >  
> > +#ifdef CONFIG_FPROBE
> > +static int symbols_cmp(const void *a, const void *b)
> > +{
> > +	const char **str_a = (const char **) a;
> > +	const char **str_b = (const char **) b;
> > +
> > +	return strcmp(*str_a, *str_b);
> > +}
> > +
> > +struct kallsyms_data {
> > +	unsigned long *addrs;
> > +	const char **syms;
> > +	size_t cnt;
> > +	size_t found;
> > +};
> > +
> > +static int kallsyms_callback(void *data, const char *name,
> > +			     struct module *mod, unsigned long addr)
> > +{
> > +	struct kallsyms_data *args = data;
> > +
> > +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> > +		return 0;
> > +
> > +	addr = ftrace_location(addr);
> > +	if (!addr)
> > +		return 0;
> 
> Ooops, wait. Did you do this last version? I missed this point.
> This changes the meanings of the kernel function.

yes, it was there before ;-) and you're right.. so some archs can
return different address, I did not realize that

> 
> > +
> > +	args->addrs[args->found++] = addr;
> > +	return args->found == args->cnt ? 1 : 0;
> > +}
> > +
> > +/**
> > + * kallsyms_lookup_names - Lookup addresses for array of symbols
> 
> More correctly "Lookup 'ftraced' addresses for array of sorted symbols", right?
> 
> I'm not sure, we can call it as a 'kallsyms' API, since this is using
> kallsyms but doesn't return symbol address, but ftrace address.
> I think this name misleads user to expect returning symbol address.
> 
> > + *
> > + * @syms: array of symbols pointers symbols to resolve, must be
> > + * alphabetically sorted
> > + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> > + * @addrs: array for storing resulting addresses
> > + *
> > + * This function looks up addresses for array of symbols provided in
> > + * @syms array (must be alphabetically sorted) and stores them in
> > + * @addrs array, which needs to be big enough to store at least @cnt
> > + * addresses.
> 
> Hmm, sorry I changed my mind. I rather like to expose kallsyms_on_each_symbol()
> and provide this API from fprobe or ftrace, because this returns ftrace address
> and thus this is only used from fprobe.

ok, so how about:

  int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);


thanks,
jirka

Re: [PATCHv2 bpf-next 1/4] kallsyms: Add kallsyms_lookup_names function

[Jiri Olsa]

On Tue, Apr 19, 2022 at 10:26:05AM +0200, Jiri Olsa wrote:

SNIP

> > > +static int kallsyms_callback(void *data, const char *name,
> > > +			     struct module *mod, unsigned long addr)
> > > +{
> > > +	struct kallsyms_data *args = data;
> > > +
> > > +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> > > +		return 0;
> > > +
> > > +	addr = ftrace_location(addr);
> > > +	if (!addr)
> > > +		return 0;
> > 
> > Ooops, wait. Did you do this last version? I missed this point.
> > This changes the meanings of the kernel function.
> 
> yes, it was there before ;-) and you're right.. so some archs can
> return different address, I did not realize that
> 
> > 
> > > +
> > > +	args->addrs[args->found++] = addr;
> > > +	return args->found == args->cnt ? 1 : 0;
> > > +}
> > > +
> > > +/**
> > > + * kallsyms_lookup_names - Lookup addresses for array of symbols
> > 
> > More correctly "Lookup 'ftraced' addresses for array of sorted symbols", right?
> > 
> > I'm not sure, we can call it as a 'kallsyms' API, since this is using
> > kallsyms but doesn't return symbol address, but ftrace address.
> > I think this name misleads user to expect returning symbol address.
> > 
> > > + *
> > > + * @syms: array of symbols pointers symbols to resolve, must be
> > > + * alphabetically sorted
> > > + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> > > + * @addrs: array for storing resulting addresses
> > > + *
> > > + * This function looks up addresses for array of symbols provided in
> > > + * @syms array (must be alphabetically sorted) and stores them in
> > > + * @addrs array, which needs to be big enough to store at least @cnt
> > > + * addresses.
> > 
> > Hmm, sorry I changed my mind. I rather like to expose kallsyms_on_each_symbol()
> > and provide this API from fprobe or ftrace, because this returns ftrace address
> > and thus this is only used from fprobe.
> 
> ok, so how about:
> 
>   int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);

quick question.. is it ok if it stays in kalsyms.c object?

so we don't need to expose kallsyms_on_each_symbol,
and it stays in 'kalsyms' place

jirka



diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index ce1bd2fbf23e..177e0b13c8c5 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -72,6 +72,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
 #ifdef CONFIG_KALLSYMS
 /* Lookup the address for a symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name);
+int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
 
 extern int kallsyms_lookup_size_offset(unsigned long addr,
 				  unsigned long *symbolsize,
@@ -103,6 +104,11 @@ static inline unsigned long kallsyms_lookup_name(const char *name)
 	return 0;
 }
 
+static inline int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
+{
+	return -ERANGE;
+}
+
 static inline int kallsyms_lookup_size_offset(unsigned long addr,
 					      unsigned long *symbolsize,
 					      unsigned long *offset)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 79f2eb617a62..1e7136a765a9 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -29,6 +29,7 @@
 #include <linux/compiler.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/bsearch.h>
 
 /*
  * These will be re-linked against their real values
@@ -228,7 +229,7 @@ unsigned long kallsyms_lookup_name(const char *name)
 	return module_kallsyms_lookup_name(name);
 }
 
-#ifdef CONFIG_LIVEPATCH
+#if defined(CONFIG_LIVEPATCH) || defined(CONFIG_FPROBE)
 /*
  * Iterate over all symbols in vmlinux.  For symbols from modules use
  * module_kallsyms_on_each_symbol instead.
@@ -572,6 +573,73 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address)
 	return __sprint_symbol(buffer, address, -1, 1, 1);
 }
 
+#ifdef CONFIG_FPROBE
+static int symbols_cmp(const void *a, const void *b)
+{
+	const char **str_a = (const char **) a;
+	const char **str_b = (const char **) b;
+
+	return strcmp(*str_a, *str_b);
+}
+
+struct kallsyms_data {
+	unsigned long *addrs;
+	const char **syms;
+	size_t cnt;
+	size_t found;
+};
+
+static int kallsyms_callback(void *data, const char *name,
+			     struct module *mod, unsigned long addr)
+{
+	struct kallsyms_data *args = data;
+
+	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
+		return 0;
+
+	addr = ftrace_location(addr);
+	if (!addr)
+		return 0;
+
+	args->addrs[args->found++] = addr;
+	return args->found == args->cnt ? 1 : 0;
+}
+
+/**
+ * ftrace_lookup_symbols - Lookup addresses for array of symbols
+ *
+ * @sorted_syms: array of symbols pointers symbols to resolve,
+ * must be alphabetically sorted
+ * @cnt: number of symbols/addresses in @syms/@addrs arrays
+ * @addrs: array for storing resulting addresses
+ *
+ * This function looks up addresses for array of symbols provided in
+ * @syms array (must be alphabetically sorted) and stores them in
+ * @addrs array, which needs to be big enough to store at least @cnt
+ * addresses.
+ *
+ * This function returns 0 if all provided symbols are found,
+ * -ESRCH otherwise.
+ */
+int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
+{
+	struct kallsyms_data args;
+
+	args.addrs = addrs;
+	args.syms = sorted_syms;
+	args.cnt = cnt;
+	args.found = 0;
+	kallsyms_on_each_symbol(kallsyms_callback, &args);
+
+	return args.found == args.cnt ? 0 : -ESRCH;
+}
+#else
+int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
+{
+	return -ERANGE;
+}
+#endif /* CONFIG_FPROBE */
+
 /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
 struct kallsym_iter {
 	loff_t pos;

Re: [PATCHv2 bpf-next 1/4] kallsyms: Add kallsyms_lookup_names function

[Masami Hiramatsu]

Hi Jiri,

Sorry for replying late.

On Fri, 22 Apr 2022 08:47:13 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:

> On Tue, Apr 19, 2022 at 10:26:05AM +0200, Jiri Olsa wrote:
> 
> SNIP
> 
> > > > +static int kallsyms_callback(void *data, const char *name,
> > > > +			     struct module *mod, unsigned long addr)
> > > > +{
> > > > +	struct kallsyms_data *args = data;
> > > > +
> > > > +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> > > > +		return 0;
> > > > +
> > > > +	addr = ftrace_location(addr);
> > > > +	if (!addr)
> > > > +		return 0;
> > > 
> > > Ooops, wait. Did you do this last version? I missed this point.
> > > This changes the meanings of the kernel function.
> > 
> > yes, it was there before ;-) and you're right.. so some archs can
> > return different address, I did not realize that
> > 
> > > 
> > > > +
> > > > +	args->addrs[args->found++] = addr;
> > > > +	return args->found == args->cnt ? 1 : 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * kallsyms_lookup_names - Lookup addresses for array of symbols
> > > 
> > > More correctly "Lookup 'ftraced' addresses for array of sorted symbols", right?
> > > 
> > > I'm not sure, we can call it as a 'kallsyms' API, since this is using
> > > kallsyms but doesn't return symbol address, but ftrace address.
> > > I think this name misleads user to expect returning symbol address.
> > > 
> > > > + *
> > > > + * @syms: array of symbols pointers symbols to resolve, must be
> > > > + * alphabetically sorted
> > > > + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> > > > + * @addrs: array for storing resulting addresses
> > > > + *
> > > > + * This function looks up addresses for array of symbols provided in
> > > > + * @syms array (must be alphabetically sorted) and stores them in
> > > > + * @addrs array, which needs to be big enough to store at least @cnt
> > > > + * addresses.
> > > 
> > > Hmm, sorry I changed my mind. I rather like to expose kallsyms_on_each_symbol()
> > > and provide this API from fprobe or ftrace, because this returns ftrace address
> > > and thus this is only used from fprobe.
> > 
> > ok, so how about:
> > 
> >   int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
> 
> quick question.. is it ok if it stays in kalsyms.c object?

I think if this is for the ftrace API, I think it should be in the ftrace.c, and
it can remove unneeded #ifdefs in C code.

> 
> so we don't need to expose kallsyms_on_each_symbol,
> and it stays in 'kalsyms' place

We don't need to expose it to modules, but just make it into a global scope.
I don't think that doesn't cause a problem.

Thank you,

> 
> jirka
> 
> 
> 
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index ce1bd2fbf23e..177e0b13c8c5 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -72,6 +72,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
>  #ifdef CONFIG_KALLSYMS
>  /* Lookup the address for a symbol. Returns 0 if not found. */
>  unsigned long kallsyms_lookup_name(const char *name);
> +int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
>  
>  extern int kallsyms_lookup_size_offset(unsigned long addr,
>  				  unsigned long *symbolsize,
> @@ -103,6 +104,11 @@ static inline unsigned long kallsyms_lookup_name(const char *name)
>  	return 0;
>  }
>  
> +static inline int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
> +{
> +	return -ERANGE;
> +}
> +
>  static inline int kallsyms_lookup_size_offset(unsigned long addr,
>  					      unsigned long *symbolsize,
>  					      unsigned long *offset)
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 79f2eb617a62..1e7136a765a9 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -29,6 +29,7 @@
>  #include <linux/compiler.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/bsearch.h>
>  
>  /*
>   * These will be re-linked against their real values
> @@ -228,7 +229,7 @@ unsigned long kallsyms_lookup_name(const char *name)
>  	return module_kallsyms_lookup_name(name);
>  }
>  
> -#ifdef CONFIG_LIVEPATCH
> +#if defined(CONFIG_LIVEPATCH) || defined(CONFIG_FPROBE)
>  /*
>   * Iterate over all symbols in vmlinux.  For symbols from modules use
>   * module_kallsyms_on_each_symbol instead.
> @@ -572,6 +573,73 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address)
>  	return __sprint_symbol(buffer, address, -1, 1, 1);
>  }
>  
> +#ifdef CONFIG_FPROBE
> +static int symbols_cmp(const void *a, const void *b)
> +{
> +	const char **str_a = (const char **) a;
> +	const char **str_b = (const char **) b;
> +
> +	return strcmp(*str_a, *str_b);
> +}
> +
> +struct kallsyms_data {
> +	unsigned long *addrs;
> +	const char **syms;
> +	size_t cnt;
> +	size_t found;
> +};
> +
> +static int kallsyms_callback(void *data, const char *name,
> +			     struct module *mod, unsigned long addr)
> +{
> +	struct kallsyms_data *args = data;
> +
> +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> +		return 0;
> +
> +	addr = ftrace_location(addr);
> +	if (!addr)
> +		return 0;
> +
> +	args->addrs[args->found++] = addr;
> +	return args->found == args->cnt ? 1 : 0;
> +}
> +
> +/**
> + * ftrace_lookup_symbols - Lookup addresses for array of symbols
> + *
> + * @sorted_syms: array of symbols pointers symbols to resolve,
> + * must be alphabetically sorted
> + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> + * @addrs: array for storing resulting addresses
> + *
> + * This function looks up addresses for array of symbols provided in
> + * @syms array (must be alphabetically sorted) and stores them in
> + * @addrs array, which needs to be big enough to store at least @cnt
> + * addresses.
> + *
> + * This function returns 0 if all provided symbols are found,
> + * -ESRCH otherwise.
> + */
> +int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
> +{
> +	struct kallsyms_data args;
> +
> +	args.addrs = addrs;
> +	args.syms = sorted_syms;
> +	args.cnt = cnt;
> +	args.found = 0;
> +	kallsyms_on_each_symbol(kallsyms_callback, &args);
> +
> +	return args.found == args.cnt ? 0 : -ESRCH;
> +}
> +#else
> +int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs)
> +{
> +	return -ERANGE;
> +}
> +#endif /* CONFIG_FPROBE */
> +
>  /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
>  struct kallsym_iter {
>  	loff_t pos;


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCHv2 bpf-next 1/4] kallsyms: Add kallsyms_lookup_names function

[Jiri Olsa]

On Tue, Apr 26, 2022 at 07:01:08PM +0900, Masami Hiramatsu wrote:
> Hi Jiri,
> 
> Sorry for replying late.
> 
> On Fri, 22 Apr 2022 08:47:13 +0200
> Jiri Olsa <olsajiri@gmail.com> wrote:
> 
> > On Tue, Apr 19, 2022 at 10:26:05AM +0200, Jiri Olsa wrote:
> > 
> > SNIP
> > 
> > > > > +static int kallsyms_callback(void *data, const char *name,
> > > > > +			     struct module *mod, unsigned long addr)
> > > > > +{
> > > > > +	struct kallsyms_data *args = data;
> > > > > +
> > > > > +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> > > > > +		return 0;
> > > > > +
> > > > > +	addr = ftrace_location(addr);
> > > > > +	if (!addr)
> > > > > +		return 0;
> > > > 
> > > > Ooops, wait. Did you do this last version? I missed this point.
> > > > This changes the meanings of the kernel function.
> > > 
> > > yes, it was there before ;-) and you're right.. so some archs can
> > > return different address, I did not realize that
> > > 
> > > > 
> > > > > +
> > > > > +	args->addrs[args->found++] = addr;
> > > > > +	return args->found == args->cnt ? 1 : 0;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * kallsyms_lookup_names - Lookup addresses for array of symbols
> > > > 
> > > > More correctly "Lookup 'ftraced' addresses for array of sorted symbols", right?
> > > > 
> > > > I'm not sure, we can call it as a 'kallsyms' API, since this is using
> > > > kallsyms but doesn't return symbol address, but ftrace address.
> > > > I think this name misleads user to expect returning symbol address.
> > > > 
> > > > > + *
> > > > > + * @syms: array of symbols pointers symbols to resolve, must be
> > > > > + * alphabetically sorted
> > > > > + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> > > > > + * @addrs: array for storing resulting addresses
> > > > > + *
> > > > > + * This function looks up addresses for array of symbols provided in
> > > > > + * @syms array (must be alphabetically sorted) and stores them in
> > > > > + * @addrs array, which needs to be big enough to store at least @cnt
> > > > > + * addresses.
> > > > 
> > > > Hmm, sorry I changed my mind. I rather like to expose kallsyms_on_each_symbol()
> > > > and provide this API from fprobe or ftrace, because this returns ftrace address
> > > > and thus this is only used from fprobe.
> > > 
> > > ok, so how about:
> > > 
> > >   int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
> > 
> > quick question.. is it ok if it stays in kalsyms.c object?
> 
> I think if this is for the ftrace API, I think it should be in the ftrace.c, and
> it can remove unneeded #ifdefs in C code.
> 
> > 
> > so we don't need to expose kallsyms_on_each_symbol,
> > and it stays in 'kalsyms' place
> 
> We don't need to expose it to modules, but just make it into a global scope.
> I don't think that doesn't cause a problem.

np, will move it to ftrace

thanks,
jirka

Re: [PATCHv2 bpf-next 1/4] kallsyms: Add kallsyms_lookup_names function

[Masami Hiramatsu]

On Tue, 26 Apr 2022 14:27:04 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:

> On Tue, Apr 26, 2022 at 07:01:08PM +0900, Masami Hiramatsu wrote:
> > Hi Jiri,
> > 
> > Sorry for replying late.
> > 
> > On Fri, 22 Apr 2022 08:47:13 +0200
> > Jiri Olsa <olsajiri@gmail.com> wrote:
> > 
> > > On Tue, Apr 19, 2022 at 10:26:05AM +0200, Jiri Olsa wrote:
> > > 
> > > SNIP
> > > 
> > > > > > +static int kallsyms_callback(void *data, const char *name,
> > > > > > +			     struct module *mod, unsigned long addr)
> > > > > > +{
> > > > > > +	struct kallsyms_data *args = data;
> > > > > > +
> > > > > > +	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	addr = ftrace_location(addr);
> > > > > > +	if (!addr)
> > > > > > +		return 0;
> > > > > 
> > > > > Ooops, wait. Did you do this last version? I missed this point.
> > > > > This changes the meanings of the kernel function.
> > > > 
> > > > yes, it was there before ;-) and you're right.. so some archs can
> > > > return different address, I did not realize that
> > > > 
> > > > > 
> > > > > > +
> > > > > > +	args->addrs[args->found++] = addr;
> > > > > > +	return args->found == args->cnt ? 1 : 0;
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * kallsyms_lookup_names - Lookup addresses for array of symbols
> > > > > 
> > > > > More correctly "Lookup 'ftraced' addresses for array of sorted symbols", right?
> > > > > 
> > > > > I'm not sure, we can call it as a 'kallsyms' API, since this is using
> > > > > kallsyms but doesn't return symbol address, but ftrace address.
> > > > > I think this name misleads user to expect returning symbol address.
> > > > > 
> > > > > > + *
> > > > > > + * @syms: array of symbols pointers symbols to resolve, must be
> > > > > > + * alphabetically sorted
> > > > > > + * @cnt: number of symbols/addresses in @syms/@addrs arrays
> > > > > > + * @addrs: array for storing resulting addresses
> > > > > > + *
> > > > > > + * This function looks up addresses for array of symbols provided in
> > > > > > + * @syms array (must be alphabetically sorted) and stores them in
> > > > > > + * @addrs array, which needs to be big enough to store at least @cnt
> > > > > > + * addresses.
> > > > > 
> > > > > Hmm, sorry I changed my mind. I rather like to expose kallsyms_on_each_symbol()
> > > > > and provide this API from fprobe or ftrace, because this returns ftrace address
> > > > > and thus this is only used from fprobe.
> > > > 
> > > > ok, so how about:
> > > > 
> > > >   int ftrace_lookup_symbols(const char **sorted_syms, size_t cnt, unsigned long *addrs);
> > > 
> > > quick question.. is it ok if it stays in kalsyms.c object?
> > 
> > I think if this is for the ftrace API, I think it should be in the ftrace.c, and
> > it can remove unneeded #ifdefs in C code.
> > 
> > > 
> > > so we don't need to expose kallsyms_on_each_symbol,
> > > and it stays in 'kalsyms' place
> > 
> > We don't need to expose it to modules, but just make it into a global scope.
> > I don't think that doesn't cause a problem.

Oops, I meant "I don't think that cause any problem."

> 
> np, will move it to ftrace

Thank you!

> 
> thanks,
> jirka


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCHv2 bpf-next 2/4] fprobe: Resolve symbols with kallsyms_lookup_names

[Jiri Olsa]

Using kallsyms_lookup_names to speed up symbols lookup
in register_fprobe_syms API.

This requires syms array to be alphabetically sorted.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/fprobe.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 89d9f994ebb0..6419501a0036 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -85,39 +85,31 @@ static void fprobe_exit_handler(struct rethook_node *rh, void *data,
 }
 NOKPROBE_SYMBOL(fprobe_exit_handler);
 
+static int symbols_cmp(const void *a, const void *b)
+{
+	const char **str_a = (const char **) a;
+	const char **str_b = (const char **) b;
+
+	return strcmp(*str_a, *str_b);
+}
+
 /* Convert ftrace location address from symbols */
 static unsigned long *get_ftrace_locations(const char **syms, int num)
 {
-	unsigned long addr, size;
 	unsigned long *addrs;
-	int i;
 
 	/* Convert symbols to symbol address */
 	addrs = kcalloc(num, sizeof(*addrs), GFP_KERNEL);
 	if (!addrs)
 		return ERR_PTR(-ENOMEM);
 
-	for (i = 0; i < num; i++) {
-		addr = kallsyms_lookup_name(syms[i]);
-		if (!addr)	/* Maybe wrong symbol */
-			goto error;
-
-		/* Convert symbol address to ftrace location. */
-		if (!kallsyms_lookup_size_offset(addr, &size, NULL) || !size)
-			goto error;
+	/* kallsyms_lookup_names expects sorted symbols */
+	sort(syms, num, sizeof(*syms), symbols_cmp, NULL);
 
-		addr = ftrace_location_range(addr, addr + size - 1);
-		if (!addr) /* No dynamic ftrace there. */
-			goto error;
+	if (!kallsyms_lookup_names(syms, num, addrs))
+		return addrs;
 
-		addrs[i] = addr;
-	}
-
-	return addrs;
-
-error:
 	kfree(addrs);
-
 	return ERR_PTR(-ENOENT);
 }
 
-- 
2.35.1

Re: [PATCHv2 bpf-next 2/4] fprobe: Resolve symbols with kallsyms_lookup_names

[Masami Hiramatsu]

Hi Jiri,

On Mon, 18 Apr 2022 14:48:32 +0200
Jiri Olsa <jolsa@kernel.org> wrote:

> Using kallsyms_lookup_names to speed up symbols lookup
> in register_fprobe_syms API.
> 
> This requires syms array to be alphabetically sorted.

This looks good to me. But we may need to update this for changing
the function name "kallsyms_lookup_names()". I think "ftrace_lookup_sorted_syms()"
will be better to understand.
What would you think?

Thank you,

> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/fprobe.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 89d9f994ebb0..6419501a0036 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -85,39 +85,31 @@ static void fprobe_exit_handler(struct rethook_node *rh, void *data,
>  }
>  NOKPROBE_SYMBOL(fprobe_exit_handler);
>  
> +static int symbols_cmp(const void *a, const void *b)
> +{
> +	const char **str_a = (const char **) a;
> +	const char **str_b = (const char **) b;
> +
> +	return strcmp(*str_a, *str_b);
> +}
> +
>  /* Convert ftrace location address from symbols */
>  static unsigned long *get_ftrace_locations(const char **syms, int num)
>  {
> -	unsigned long addr, size;
>  	unsigned long *addrs;
> -	int i;
>  
>  	/* Convert symbols to symbol address */
>  	addrs = kcalloc(num, sizeof(*addrs), GFP_KERNEL);
>  	if (!addrs)
>  		return ERR_PTR(-ENOMEM);
>  
> -	for (i = 0; i < num; i++) {
> -		addr = kallsyms_lookup_name(syms[i]);
> -		if (!addr)	/* Maybe wrong symbol */
> -			goto error;
> -
> -		/* Convert symbol address to ftrace location. */
> -		if (!kallsyms_lookup_size_offset(addr, &size, NULL) || !size)
> -			goto error;
> +	/* kallsyms_lookup_names expects sorted symbols */
> +	sort(syms, num, sizeof(*syms), symbols_cmp, NULL);
>  
> -		addr = ftrace_location_range(addr, addr + size - 1);
> -		if (!addr) /* No dynamic ftrace there. */
> -			goto error;
> +	if (!kallsyms_lookup_names(syms, num, addrs))
> +		return addrs;
>  
> -		addrs[i] = addr;
> -	}
> -
> -	return addrs;
> -
> -error:
>  	kfree(addrs);
> -
>  	return ERR_PTR(-ENOENT);
>  }
>  
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCHv2 bpf-next 3/4] bpf: Resolve symbols with kallsyms_lookup_names for kprobe multi link

[Jiri Olsa]

Using kallsyms_lookup_names function to speed up symbols lookup in
kprobe multi link attachment and replacing with it the current
kprobe_multi_resolve_syms function.

This speeds up bpftrace kprobe attachment:

  # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
  ...
  6.5681 +- 0.0225 seconds time elapsed  ( +-  0.34% )

After:

  # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
  ...
  0.5661 +- 0.0275 seconds time elapsed  ( +-  4.85% )

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c | 113 +++++++++++++++++++++++----------------
 1 file changed, 67 insertions(+), 46 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b26f3da943de..f49cdc46a21f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2226,6 +2226,60 @@ struct bpf_kprobe_multi_run_ctx {
 	unsigned long entry_ip;
 };
 
+struct user_syms {
+	const char **syms;
+	char *buf;
+};
+
+static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
+{
+	unsigned long __user usymbol;
+	const char **syms = NULL;
+	char *buf = NULL, *p;
+	int err = -EFAULT;
+	unsigned int i;
+
+	err = -ENOMEM;
+	syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
+	if (!syms)
+		goto error;
+
+	buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
+	if (!buf)
+		goto error;
+
+	for (p = buf, i = 0; i < cnt; i++) {
+		if (__get_user(usymbol, usyms + i)) {
+			err = -EFAULT;
+			goto error;
+		}
+		err = strncpy_from_user(p, (const char __user *) usymbol, KSYM_NAME_LEN);
+		if (err == KSYM_NAME_LEN)
+			err = -E2BIG;
+		if (err < 0)
+			goto error;
+		syms[i] = p;
+		p += err + 1;
+	}
+
+	err = 0;
+	us->syms = syms;
+	us->buf = buf;
+
+error:
+	if (err) {
+		kvfree(syms);
+		kvfree(buf);
+	}
+	return err;
+}
+
+static void free_user_syms(struct user_syms *us)
+{
+	kvfree(us->syms);
+	kvfree(us->buf);
+}
+
 static void bpf_kprobe_multi_link_release(struct bpf_link *link)
 {
 	struct bpf_kprobe_multi_link *kmulti_link;
@@ -2346,53 +2400,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
 	kprobe_multi_link_prog_run(link, entry_ip, regs);
 }
 
-static int
-kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt,
-			  unsigned long *addrs)
+static int symbols_cmp(const void *a, const void *b)
 {
-	unsigned long addr, size;
-	const char __user **syms;
-	int err = -ENOMEM;
-	unsigned int i;
-	char *func;
-
-	size = cnt * sizeof(*syms);
-	syms = kvzalloc(size, GFP_KERNEL);
-	if (!syms)
-		return -ENOMEM;
-
-	func = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
-	if (!func)
-		goto error;
-
-	if (copy_from_user(syms, usyms, size)) {
-		err = -EFAULT;
-		goto error;
-	}
-
-	for (i = 0; i < cnt; i++) {
-		err = strncpy_from_user(func, syms[i], KSYM_NAME_LEN);
-		if (err == KSYM_NAME_LEN)
-			err = -E2BIG;
-		if (err < 0)
-			goto error;
-		err = -EINVAL;
-		addr = kallsyms_lookup_name(func);
-		if (!addr)
-			goto error;
-		if (!kallsyms_lookup_size_offset(addr, &size, NULL))
-			goto error;
-		addr = ftrace_location_range(addr, addr + size - 1);
-		if (!addr)
-			goto error;
-		addrs[i] = addr;
-	}
+	const char **str_a = (const char **) a;
+	const char **str_b = (const char **) b;
 
-	err = 0;
-error:
-	kvfree(syms);
-	kfree(func);
-	return err;
+	return strcmp(*str_a, *str_b);
 }
 
 int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
@@ -2438,7 +2451,15 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 			goto error;
 		}
 	} else {
-		err = kprobe_multi_resolve_syms(usyms, cnt, addrs);
+		struct user_syms us;
+
+		err = copy_user_syms(&us, usyms, cnt);
+		if (err)
+			goto error;
+
+		sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
+		err = kallsyms_lookup_names(us.syms, cnt, addrs);
+		free_user_syms(&us);
 		if (err)
 			goto error;
 	}
-- 
2.35.1

Re: [PATCHv2 bpf-next 3/4] bpf: Resolve symbols with kallsyms_lookup_names for kprobe multi link

[Andrii Nakryiko]

On Mon, Apr 18, 2022 at 5:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Using kallsyms_lookup_names function to speed up symbols lookup in
> kprobe multi link attachment and replacing with it the current
> kprobe_multi_resolve_syms function.
>
> This speeds up bpftrace kprobe attachment:
>
>   # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
>   ...
>   6.5681 +- 0.0225 seconds time elapsed  ( +-  0.34% )
>
> After:
>
>   # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
>   ...
>   0.5661 +- 0.0275 seconds time elapsed  ( +-  4.85% )
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  kernel/trace/bpf_trace.c | 113 +++++++++++++++++++++++----------------
>  1 file changed, 67 insertions(+), 46 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index b26f3da943de..f49cdc46a21f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2226,6 +2226,60 @@ struct bpf_kprobe_multi_run_ctx {
>         unsigned long entry_ip;
>  };
>
> +struct user_syms {
> +       const char **syms;
> +       char *buf;
> +};
> +
> +static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
> +{
> +       unsigned long __user usymbol;
> +       const char **syms = NULL;
> +       char *buf = NULL, *p;
> +       int err = -EFAULT;
> +       unsigned int i;
> +
> +       err = -ENOMEM;
> +       syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
> +       if (!syms)
> +               goto error;
> +
> +       buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
> +       if (!buf)
> +               goto error;
> +
> +       for (p = buf, i = 0; i < cnt; i++) {
> +               if (__get_user(usymbol, usyms + i)) {
> +                       err = -EFAULT;
> +                       goto error;
> +               }
> +               err = strncpy_from_user(p, (const char __user *) usymbol, KSYM_NAME_LEN);
> +               if (err == KSYM_NAME_LEN)
> +                       err = -E2BIG;
> +               if (err < 0)
> +                       goto error;
> +               syms[i] = p;
> +               p += err + 1;
> +       }
> +
> +       err = 0;
> +       us->syms = syms;
> +       us->buf = buf;

return 0 here instead of falling through into error: block?

> +
> +error:
> +       if (err) {
> +               kvfree(syms);
> +               kvfree(buf);
> +       }
> +       return err;
> +}
> +
> +static void free_user_syms(struct user_syms *us)
> +{
> +       kvfree(us->syms);
> +       kvfree(us->buf);
> +}
> +
>  static void bpf_kprobe_multi_link_release(struct bpf_link *link)
>  {
>         struct bpf_kprobe_multi_link *kmulti_link;

[...]

Re: [PATCHv2 bpf-next 3/4] bpf: Resolve symbols with kallsyms_lookup_names for kprobe multi link

[Jiri Olsa]

On Wed, Apr 20, 2022 at 02:49:59PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 18, 2022 at 5:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Using kallsyms_lookup_names function to speed up symbols lookup in
> > kprobe multi link attachment and replacing with it the current
> > kprobe_multi_resolve_syms function.
> >
> > This speeds up bpftrace kprobe attachment:
> >
> >   # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
> >   ...
> >   6.5681 +- 0.0225 seconds time elapsed  ( +-  0.34% )
> >
> > After:
> >
> >   # perf stat -r 5 -e cycles ./src/bpftrace -e 'kprobe:x* {  } i:ms:1 { exit(); }'
> >   ...
> >   0.5661 +- 0.0275 seconds time elapsed  ( +-  4.85% )
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> LGTM.
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> >  kernel/trace/bpf_trace.c | 113 +++++++++++++++++++++++----------------
> >  1 file changed, 67 insertions(+), 46 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index b26f3da943de..f49cdc46a21f 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2226,6 +2226,60 @@ struct bpf_kprobe_multi_run_ctx {
> >         unsigned long entry_ip;
> >  };
> >
> > +struct user_syms {
> > +       const char **syms;
> > +       char *buf;
> > +};
> > +
> > +static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 cnt)
> > +{
> > +       unsigned long __user usymbol;
> > +       const char **syms = NULL;
> > +       char *buf = NULL, *p;
> > +       int err = -EFAULT;
> > +       unsigned int i;
> > +
> > +       err = -ENOMEM;
> > +       syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL);
> > +       if (!syms)
> > +               goto error;
> > +
> > +       buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL);
> > +       if (!buf)
> > +               goto error;
> > +
> > +       for (p = buf, i = 0; i < cnt; i++) {
> > +               if (__get_user(usymbol, usyms + i)) {
> > +                       err = -EFAULT;
> > +                       goto error;
> > +               }
> > +               err = strncpy_from_user(p, (const char __user *) usymbol, KSYM_NAME_LEN);
> > +               if (err == KSYM_NAME_LEN)
> > +                       err = -E2BIG;
> > +               if (err < 0)
> > +                       goto error;
> > +               syms[i] = p;
> > +               p += err + 1;
> > +       }
> > +
> > +       err = 0;
> > +       us->syms = syms;
> > +       us->buf = buf;
> 
> return 0 here instead of falling through into error: block?

ok, will change

jirka

> 
> > +
> > +error:
> > +       if (err) {
> > +               kvfree(syms);
> > +               kvfree(buf);
> > +       }
> > +       return err;
> > +}
> > +
> > +static void free_user_syms(struct user_syms *us)
> > +{
> > +       kvfree(us->syms);
> > +       kvfree(us->buf);
> > +}
> > +
> >  static void bpf_kprobe_multi_link_release(struct bpf_link *link)
> >  {
> >         struct bpf_kprobe_multi_link *kmulti_link;
> 
> [...]

[PATCHv2 bpf-next 4/4] selftests/bpf: Add attach bench test

[Jiri Olsa]

Adding test that reads all functions from ftrace available_filter_functions
file and attach them all through kprobe_multi API.

It also prints stats info with -v option, like on my setup:

  test_bench_attach: found 48712 functions
  test_bench_attach: attached in   1.069s
  test_bench_attach: detached in   0.373s

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/kprobe_multi_test.c        | 136 ++++++++++++++++++
 .../selftests/bpf/progs/kprobe_multi_empty.c  |  12 ++
 2 files changed, 148 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_empty.c

diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index b9876b55fc0c..05f0fab8af89 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -2,6 +2,9 @@
 #include <test_progs.h>
 #include "kprobe_multi.skel.h"
 #include "trace_helpers.h"
+#include "kprobe_multi_empty.skel.h"
+#include "bpf/libbpf_internal.h"
+#include "bpf/hashmap.h"
 
 static void kprobe_multi_test_run(struct kprobe_multi *skel, bool test_return)
 {
@@ -301,6 +304,137 @@ static void test_attach_api_fails(void)
 	kprobe_multi__destroy(skel);
 }
 
+static inline __u64 get_time_ns(void)
+{
+	struct timespec t;
+
+	clock_gettime(CLOCK_MONOTONIC, &t);
+	return (__u64) t.tv_sec * 1000000000 + t.tv_nsec;
+}
+
+static size_t symbol_hash(const void *key, void *ctx __maybe_unused)
+{
+	return str_hash((const char *) key);
+}
+
+static bool symbol_equal(const void *key1, const void *key2, void *ctx __maybe_unused)
+{
+	return strcmp((const char *) key1, (const char *) key2) == 0;
+}
+
+#define DEBUGFS "/sys/kernel/debug/tracing/"
+
+static int get_syms(char ***symsp, size_t *cntp)
+{
+	size_t cap = 0, cnt = 0, i;
+	char *name, **syms = NULL;
+	struct hashmap *map;
+	char buf[256];
+	FILE *f;
+	int err;
+
+	/*
+	 * The available_filter_functions contains many duplicates,
+	 * but other than that all symbols are usable in kprobe multi
+	 * interface.
+	 * Filtering out duplicates by using hashmap__add, which won't
+	 * add existing entry.
+	 */
+	f = fopen(DEBUGFS "available_filter_functions", "r");
+	if (!f)
+		return -EINVAL;
+
+	map = hashmap__new(symbol_hash, symbol_equal, NULL);
+	err = libbpf_get_error(map);
+	if (err)
+		goto error;
+
+	while (fgets(buf, sizeof(buf), f)) {
+		/* skip modules */
+		if (strchr(buf, '['))
+			continue;
+		if (sscanf(buf, "%ms$*[^\n]\n", &name) != 1)
+			continue;
+		err = hashmap__add(map, name, NULL);
+		if (err) {
+			free(name);
+			if (err == -EEXIST)
+				continue;
+			goto error;
+		}
+		err = libbpf_ensure_mem((void **) &syms, &cap,
+					sizeof(*syms), cnt + 1);
+		if (err) {
+			free(name);
+			goto error;
+		}
+		syms[cnt] = name;
+		cnt++;
+	}
+
+	*symsp = syms;
+	*cntp = cnt;
+
+error:
+	fclose(f);
+	hashmap__free(map);
+	if (err) {
+		for (i = 0; i < cnt; i++)
+			free(syms[cnt]);
+		free(syms);
+	}
+	return err;
+}
+
+static void test_bench_attach(void)
+{
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
+	struct kprobe_multi_empty *skel = NULL;
+	long attach_start_ns, attach_end_ns;
+	long detach_start_ns, detach_end_ns;
+	double attach_delta, detach_delta;
+	struct bpf_link *link = NULL;
+	char **syms = NULL;
+	size_t cnt, i;
+
+	if (!ASSERT_OK(get_syms(&syms, &cnt), "get_syms"))
+		return;
+
+	skel = kprobe_multi_empty__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "kprobe_multi_empty__open_and_load"))
+		goto cleanup;
+
+	opts.syms = (const char **) syms;
+	opts.cnt = cnt;
+
+	attach_start_ns = get_time_ns();
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_empty,
+						     NULL, &opts);
+	attach_end_ns = get_time_ns();
+
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_kprobe_multi_opts"))
+		goto cleanup;
+
+	detach_start_ns = get_time_ns();
+	bpf_link__destroy(link);
+	detach_end_ns = get_time_ns();
+
+	attach_delta = (attach_end_ns - attach_start_ns) / 1000000000.0;
+	detach_delta = (detach_end_ns - detach_start_ns) / 1000000000.0;
+
+	fprintf(stderr, "%s: found %lu functions\n", __func__, cnt);
+	fprintf(stderr, "%s: attached in %7.3lfs\n", __func__, attach_delta);
+	fprintf(stderr, "%s: detached in %7.3lfs\n", __func__, detach_delta);
+
+cleanup:
+	kprobe_multi_empty__destroy(skel);
+	if (syms) {
+		for (i = 0; i < cnt; i++)
+			free(syms[i]);
+		free(syms);
+	}
+}
+
 void test_kprobe_multi_test(void)
 {
 	if (!ASSERT_OK(load_kallsyms(), "load_kallsyms"))
@@ -320,4 +454,6 @@ void test_kprobe_multi_test(void)
 		test_attach_api_syms();
 	if (test__start_subtest("attach_api_fails"))
 		test_attach_api_fails();
+	if (test__start_subtest("bench_attach"))
+		test_bench_attach();
 }
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c b/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c
new file mode 100644
index 000000000000..be9e3d891d46
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+SEC("kprobe.multi/*")
+int test_kprobe_empty(struct pt_regs *ctx)
+{
+	return 0;
+}
-- 
2.35.1

Re: [PATCHv2 bpf-next 4/4] selftests/bpf: Add attach bench test

[Andrii Nakryiko]

On Mon, Apr 18, 2022 at 5:49 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that reads all functions from ftrace available_filter_functions
> file and attach them all through kprobe_multi API.
>
> It also prints stats info with -v option, like on my setup:
>
>   test_bench_attach: found 48712 functions
>   test_bench_attach: attached in   1.069s
>   test_bench_attach: detached in   0.373s
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../bpf/prog_tests/kprobe_multi_test.c        | 136 ++++++++++++++++++
>  .../selftests/bpf/progs/kprobe_multi_empty.c  |  12 ++
>  2 files changed, 148 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_empty.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> index b9876b55fc0c..05f0fab8af89 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -2,6 +2,9 @@
>  #include <test_progs.h>
>  #include "kprobe_multi.skel.h"
>  #include "trace_helpers.h"
> +#include "kprobe_multi_empty.skel.h"
> +#include "bpf/libbpf_internal.h"
> +#include "bpf/hashmap.h"
>
>  static void kprobe_multi_test_run(struct kprobe_multi *skel, bool test_return)
>  {
> @@ -301,6 +304,137 @@ static void test_attach_api_fails(void)
>         kprobe_multi__destroy(skel);
>  }
>
> +static inline __u64 get_time_ns(void)
> +{
> +       struct timespec t;
> +
> +       clock_gettime(CLOCK_MONOTONIC, &t);
> +       return (__u64) t.tv_sec * 1000000000 + t.tv_nsec;
> +}
> +
> +static size_t symbol_hash(const void *key, void *ctx __maybe_unused)
> +{
> +       return str_hash((const char *) key);
> +}
> +
> +static bool symbol_equal(const void *key1, const void *key2, void *ctx __maybe_unused)
> +{
> +       return strcmp((const char *) key1, (const char *) key2) == 0;
> +}
> +
> +#define DEBUGFS "/sys/kernel/debug/tracing/"
> +
> +static int get_syms(char ***symsp, size_t *cntp)
> +{
> +       size_t cap = 0, cnt = 0, i;
> +       char *name, **syms = NULL;
> +       struct hashmap *map;
> +       char buf[256];
> +       FILE *f;
> +       int err;
> +
> +       /*
> +        * The available_filter_functions contains many duplicates,
> +        * but other than that all symbols are usable in kprobe multi
> +        * interface.
> +        * Filtering out duplicates by using hashmap__add, which won't
> +        * add existing entry.
> +        */
> +       f = fopen(DEBUGFS "available_filter_functions", "r");

nit: DEBUGFS "constant" just makes it harder to follow the code and
doesn't add anything, please just use the full path here directly

> +       if (!f)
> +               return -EINVAL;
> +
> +       map = hashmap__new(symbol_hash, symbol_equal, NULL);
> +       err = libbpf_get_error(map);

hashmap__new() is an internal API, so please use IS_ERR() directly
here. libbpf_get_error() should be used for public libbpf APIs, and
preferably not in libbpf 1.0 mode

> +       if (err)
> +               goto error;
> +
> +       while (fgets(buf, sizeof(buf), f)) {
> +               /* skip modules */
> +               if (strchr(buf, '['))
> +                       continue;

[...]

> +       attach_delta = (attach_end_ns - attach_start_ns) / 1000000000.0;
> +       detach_delta = (detach_end_ns - detach_start_ns) / 1000000000.0;
> +
> +       fprintf(stderr, "%s: found %lu functions\n", __func__, cnt);
> +       fprintf(stderr, "%s: attached in %7.3lfs\n", __func__, attach_delta);
> +       fprintf(stderr, "%s: detached in %7.3lfs\n", __func__, detach_delta);


why stderr? just do printf() and let test_progs handle output


> +
> +cleanup:
> +       kprobe_multi_empty__destroy(skel);
> +       if (syms) {
> +               for (i = 0; i < cnt; i++)
> +                       free(syms[i]);
> +               free(syms);
> +       }
> +}
> +
>  void test_kprobe_multi_test(void)
>  {
>         if (!ASSERT_OK(load_kallsyms(), "load_kallsyms"))
> @@ -320,4 +454,6 @@ void test_kprobe_multi_test(void)
>                 test_attach_api_syms();
>         if (test__start_subtest("attach_api_fails"))
>                 test_attach_api_fails();
> +       if (test__start_subtest("bench_attach"))
> +               test_bench_attach();
>  }
> diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c b/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c
> new file mode 100644
> index 000000000000..be9e3d891d46
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("kprobe.multi/*")

use SEC("kprobe.multi") to make it clear that we are attaching it "manually"?

> +int test_kprobe_empty(struct pt_regs *ctx)
> +{
> +       return 0;
> +}
> --
> 2.35.1
>

Re: [PATCHv2 bpf-next 4/4] selftests/bpf: Add attach bench test

[Jiri Olsa]

On Wed, Apr 20, 2022 at 02:56:53PM -0700, Andrii Nakryiko wrote:

SNIP

> > +#define DEBUGFS "/sys/kernel/debug/tracing/"
> > +
> > +static int get_syms(char ***symsp, size_t *cntp)
> > +{
> > +       size_t cap = 0, cnt = 0, i;
> > +       char *name, **syms = NULL;
> > +       struct hashmap *map;
> > +       char buf[256];
> > +       FILE *f;
> > +       int err;
> > +
> > +       /*
> > +        * The available_filter_functions contains many duplicates,
> > +        * but other than that all symbols are usable in kprobe multi
> > +        * interface.
> > +        * Filtering out duplicates by using hashmap__add, which won't
> > +        * add existing entry.
> > +        */
> > +       f = fopen(DEBUGFS "available_filter_functions", "r");
> 
> nit: DEBUGFS "constant" just makes it harder to follow the code and
> doesn't add anything, please just use the full path here directly

there's another one DEBUGFS in trace_helpers.c,
we could put it in trace_helpers.h

> 
> > +       if (!f)
> > +               return -EINVAL;
> > +
> > +       map = hashmap__new(symbol_hash, symbol_equal, NULL);
> > +       err = libbpf_get_error(map);
> 
> hashmap__new() is an internal API, so please use IS_ERR() directly
> here. libbpf_get_error() should be used for public libbpf APIs, and
> preferably not in libbpf 1.0 mode

ok

> 
> > +       if (err)
> > +               goto error;
> > +
> > +       while (fgets(buf, sizeof(buf), f)) {
> > +               /* skip modules */
> > +               if (strchr(buf, '['))
> > +                       continue;
> 
> [...]
> 
> > +       attach_delta = (attach_end_ns - attach_start_ns) / 1000000000.0;
> > +       detach_delta = (detach_end_ns - detach_start_ns) / 1000000000.0;
> > +
> > +       fprintf(stderr, "%s: found %lu functions\n", __func__, cnt);
> > +       fprintf(stderr, "%s: attached in %7.3lfs\n", __func__, attach_delta);
> > +       fprintf(stderr, "%s: detached in %7.3lfs\n", __func__, detach_delta);
> 
> 
> why stderr? just do printf() and let test_progs handle output

ok

> 
> 
> > +
> > +cleanup:
> > +       kprobe_multi_empty__destroy(skel);
> > +       if (syms) {
> > +               for (i = 0; i < cnt; i++)
> > +                       free(syms[i]);
> > +               free(syms);
> > +       }
> > +}
> > +
> >  void test_kprobe_multi_test(void)
> >  {
> >         if (!ASSERT_OK(load_kallsyms(), "load_kallsyms"))
> > @@ -320,4 +454,6 @@ void test_kprobe_multi_test(void)
> >                 test_attach_api_syms();
> >         if (test__start_subtest("attach_api_fails"))
> >                 test_attach_api_fails();
> > +       if (test__start_subtest("bench_attach"))
> > +               test_bench_attach();
> >  }
> > diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c b/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c
> > new file mode 100644
> > index 000000000000..be9e3d891d46
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/kprobe_multi_empty.c
> > @@ -0,0 +1,12 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("kprobe.multi/*")
> 
> use SEC("kprobe.multi") to make it clear that we are attaching it "manually"?

yep, will do

thanks,
jirka