[04/18] bpf: Add name to struct bpf_ksym
diff mbox series

Message ID 20200226130345.209469-5-jolsa@kernel.org
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series
  • bpf: Add trampoline and dispatcher to /proc/kallsyms
Related show

Commit Message

Jiri Olsa Feb. 26, 2020, 1:03 p.m. UTC
Adding name to 'struct bpf_ksym' object to carry the name
of the symbol for bpf_prog, bpf_trampoline, bpf_dispatcher.

The current benefit is that name is now generated only when
the symbol is added to the list, so we don't need to generate
it every time it's accessed.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h    | 2 ++
 include/linux/filter.h | 6 ------
 kernel/bpf/core.c      | 8 +++++---
 kernel/events/core.c   | 9 ++++-----
 4 files changed, 11 insertions(+), 14 deletions(-)

Comments

Song Liu Feb. 26, 2020, 9:14 p.m. UTC | #1
On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding name to 'struct bpf_ksym' object to carry the name
> of the symbol for bpf_prog, bpf_trampoline, bpf_dispatcher.
>
> The current benefit is that name is now generated only when
> the symbol is added to the list, so we don't need to generate
> it every time it's accessed.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

The patch looks good. But I wonder whether we want pay the cost of
extra 128 bytes per bpf program. Maybe make it a pointer and only
generate the string when it is first used?

Thanks,
Song
Jiri Olsa Feb. 27, 2020, 8:50 a.m. UTC | #2
On Wed, Feb 26, 2020 at 01:14:43PM -0800, Song Liu wrote:
> On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding name to 'struct bpf_ksym' object to carry the name
> > of the symbol for bpf_prog, bpf_trampoline, bpf_dispatcher.
> >
> > The current benefit is that name is now generated only when
> > the symbol is added to the list, so we don't need to generate
> > it every time it's accessed.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> The patch looks good. But I wonder whether we want pay the cost of
> extra 128 bytes per bpf program. Maybe make it a pointer and only
> generate the string when it is first used?

I thought 128 would not be that bad, also the code is quite
simple because of that.. if that's really a concern I could
make the changes, but that would probably mean changing the
design

jirka
Song Liu Feb. 27, 2020, 6:59 p.m. UTC | #3
On Thu, Feb 27, 2020 at 12:50 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Feb 26, 2020 at 01:14:43PM -0800, Song Liu wrote:
> > On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding name to 'struct bpf_ksym' object to carry the name
> > > of the symbol for bpf_prog, bpf_trampoline, bpf_dispatcher.
> > >
> > > The current benefit is that name is now generated only when
> > > the symbol is added to the list, so we don't need to generate
> > > it every time it's accessed.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >
> > The patch looks good. But I wonder whether we want pay the cost of
> > extra 128 bytes per bpf program. Maybe make it a pointer and only
> > generate the string when it is first used?
>
> I thought 128 would not be that bad, also the code is quite
> simple because of that.. if that's really a concern I could
> make the changes, but that would probably mean changing the
> design

I guess this is OK. We can further optimize it if needed.

Acked-by: Song Liu <songliubraving@fb.com>
Jiri Olsa March 1, 2020, 6:31 p.m. UTC | #4
On Thu, Feb 27, 2020 at 10:59:57AM -0800, Song Liu wrote:
> On Thu, Feb 27, 2020 at 12:50 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Wed, Feb 26, 2020 at 01:14:43PM -0800, Song Liu wrote:
> > > On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Adding name to 'struct bpf_ksym' object to carry the name
> > > > of the symbol for bpf_prog, bpf_trampoline, bpf_dispatcher.
> > > >
> > > > The current benefit is that name is now generated only when
> > > > the symbol is added to the list, so we don't need to generate
> > > > it every time it's accessed.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > >
> > > The patch looks good. But I wonder whether we want pay the cost of
> > > extra 128 bytes per bpf program. Maybe make it a pointer and only
> > > generate the string when it is first used?
> >
> > I thought 128 would not be that bad, also the code is quite
> > simple because of that.. if that's really a concern I could
> > make the changes, but that would probably mean changing the
> > design
> 
> I guess this is OK. We can further optimize it if needed.
> 
> Acked-by: Song Liu <songliubraving@fb.com>
>

ok, thanks for the review, I still have to make some changes,
so I'll keep your acked-by on patches that won't be changed,
please scream otherwise ;-)

thanks,
jirka

Patch
diff mbox series

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5ad8eea1cd37..e7b2e9fc256c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -18,6 +18,7 @@ 
 #include <linux/refcount.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
+#include <linux/kallsyms.h>
 
 struct bpf_verifier_env;
 struct bpf_verifier_log;
@@ -465,6 +466,7 @@  void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
 struct bpf_ksym {
 	unsigned long		 start;
 	unsigned long		 end;
+	char			 name[KSYM_NAME_LEN];
 };
 
 enum bpf_tramp_prog_type {
diff --git a/include/linux/filter.h b/include/linux/filter.h
index eafe72644282..a945c250ad53 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1062,7 +1062,6 @@  bpf_address_lookup(unsigned long addr, unsigned long *size,
 
 void bpf_prog_kallsyms_add(struct bpf_prog *fp);
 void bpf_prog_kallsyms_del(struct bpf_prog *fp);
-void bpf_get_prog_name(const struct bpf_prog *prog, char *sym);
 
 #else /* CONFIG_BPF_JIT */
 
@@ -1131,11 +1130,6 @@  static inline void bpf_prog_kallsyms_del(struct bpf_prog *fp)
 {
 }
 
-static inline void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
-{
-	sym[0] = '\0';
-}
-
 #endif /* CONFIG_BPF_JIT */
 
 void bpf_prog_kallsyms_del_all(struct bpf_prog *fp);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 39a9e4184900..a7aaa81035b1 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -535,8 +535,9 @@  bpf_get_prog_addr_region(const struct bpf_prog *prog)
 	prog->aux->ksym.end   = addr + hdr->pages * PAGE_SIZE;
 }
 
-void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
+static void bpf_get_prog_name(const struct bpf_prog *prog)
 {
+	char *sym = prog->aux->ksym.name;
 	const char *end = sym + KSYM_NAME_LEN;
 	const struct btf_type *type;
 	const char *func_name;
@@ -643,6 +644,7 @@  void bpf_prog_kallsyms_add(struct bpf_prog *fp)
 		return;
 
 	bpf_get_prog_addr_region(fp);
+	bpf_get_prog_name(fp);
 
 	spin_lock_bh(&bpf_lock);
 	bpf_prog_ksym_node_add(fp->aux);
@@ -681,7 +683,7 @@  const char *__bpf_address_lookup(unsigned long addr, unsigned long *size,
 		unsigned long symbol_start = prog->aux->ksym.start;
 		unsigned long symbol_end = prog->aux->ksym.end;
 
-		bpf_get_prog_name(prog, sym);
+		strncpy(sym, prog->aux->ksym.name, KSYM_NAME_LEN);
 
 		ret = sym;
 		if (size)
@@ -738,7 +740,7 @@  int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 		if (it++ != symnum)
 			continue;
 
-		bpf_get_prog_name(aux->prog, sym);
+		strncpy(sym, aux->ksym.name, KSYM_NAME_LEN);
 
 		*value = (unsigned long)aux->prog->bpf_func;
 		*type  = BPF_SYM_ELF_TYPE;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index e453589da97c..a2cfb9e5f262 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8255,23 +8255,22 @@  static void perf_event_bpf_emit_ksymbols(struct bpf_prog *prog,
 					 enum perf_bpf_event_type type)
 {
 	bool unregister = type == PERF_BPF_EVENT_PROG_UNLOAD;
-	char sym[KSYM_NAME_LEN];
 	int i;
 
 	if (prog->aux->func_cnt == 0) {
-		bpf_get_prog_name(prog, sym);
 		perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF,
 				   (u64)(unsigned long)prog->bpf_func,
-				   prog->jited_len, unregister, sym);
+				   prog->jited_len, unregister,
+				   prog->aux->ksym.name);
 	} else {
 		for (i = 0; i < prog->aux->func_cnt; i++) {
 			struct bpf_prog *subprog = prog->aux->func[i];
 
-			bpf_get_prog_name(subprog, sym);
 			perf_event_ksymbol(
 				PERF_RECORD_KSYMBOL_TYPE_BPF,
 				(u64)(unsigned long)subprog->bpf_func,
-				subprog->jited_len, unregister, sym);
+				subprog->jited_len, unregister,
+				prog->aux->ksym.name);
 		}
 	}
 }