[PATCHv3 00/18] bpf: Add trampoline and dispatcher to /proc/kallsyms

[PATCHv3 00/18] bpf: Add trampoline and dispatcher to /proc/kallsyms

[Jiri Olsa]

hi,
this patchset adds trampoline and dispatcher objects
to be visible in /proc/kallsyms. The last patch also
adds sorting for all bpf objects in /proc/kallsyms.

  $ sudo cat /proc/kallsyms | tail -20
  ...
  ffffffffa050f000 t bpf_prog_5a2b06eab81b8f51    [bpf]
  ffffffffa0511000 t bpf_prog_6deef7357e7b4530    [bpf]
  ffffffffa0542000 t bpf_trampoline_13832 [bpf]
  ffffffffa0548000 t bpf_prog_96f1b5bf4e4cc6dc_mutex_lock [bpf]
  ffffffffa0572000 t bpf_prog_d1c63e29ad82c4ab_bpf_prog1  [bpf]
  ffffffffa0585000 t bpf_prog_e314084d332a5338__dissect   [bpf]
  ffffffffa0587000 t bpf_prog_59785a79eac7e5d2_mutex_unlock       [bpf]
  ffffffffa0589000 t bpf_prog_d0db6e0cac050163_mutex_lock [bpf]
  ffffffffa058d000 t bpf_prog_d8f047721e4d8321_bpf_prog2  [bpf]
  ffffffffa05df000 t bpf_trampoline_25637 [bpf]
  ffffffffa05e3000 t bpf_prog_d8f047721e4d8321_bpf_prog2  [bpf]
  ffffffffa05e5000 t bpf_prog_3b185187f1855c4c    [bpf]
  ffffffffa05e7000 t bpf_prog_d8f047721e4d8321_bpf_prog2  [bpf]
  ffffffffa05eb000 t bpf_prog_93cebb259dd5c4b2_do_sys_open        [bpf]
  ffffffffa0677000 t bpf_dispatcher_xdp   [bpf]

v3 changes:
  - use container_of directly in bpf_get_ksym_start  [Daniel]
  - add more changelog explanations for ksym addresses [Daniel]

v2 changes:
  - omit extra condition in __bpf_ksym_add for sorting code (Andrii)
  - rename bpf_kallsyms_tree_ops to bpf_ksym_tree (Andrii)
  - expose only executable code in kallsyms (Andrii)
  - use full trampoline key as its kallsyms id (Andrii)
  - explained the BPF_TRAMP_REPLACE case (Andrii)
  - small format changes in bpf_trampoline_link_prog/bpf_trampoline_unlink_prog (Andrii)
  - propagate error value in bpf_dispatcher_update and update kallsym if it's successful (Andrii)
  - get rid of __always_inline for bpf_ksym_tree callbacks (Andrii)
  - added KSYMBOL notification for bpf_image add/removal
  - added perf tools changes to properly display trampoline/dispatcher


For perf tool to properly display trampoline/dispatcher you need
also Arnaldo's perf/urgent branch changes. I merged everything
into following branch:

    git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git bpf/kallsyms

thanks,
jirka


---
Björn Töpel (1):
      bpf: Add bpf_trampoline_ name prefix for DECLARE_BPF_DISPATCHER

Jiri Olsa (17):
      x86/mm: Rename is_kernel_text to __is_kernel_text
      bpf: Add struct bpf_ksym
      bpf: Add name to struct bpf_ksym
      bpf: Add lnode list node to struct bpf_ksym
      bpf: Add bpf_ksym_tree tree
      bpf: Move bpf_tree add/del from bpf_prog_ksym_node_add/del
      bpf: Separate kallsyms add/del functions
      bpf: Add bpf_ksym_add/del functions
      bpf: Re-initialize lnode in bpf_ksym_del
      bpf: Rename bpf_tree to bpf_progs_tree
      bpf: Add trampolines to kallsyms
      bpf: Return error value in bpf_dispatcher_update
      bpf: Add dispatchers to kallsyms
      bpf: Sort bpf kallsyms symbols
      perf tools: Synthesize bpf_trampoline/dispatcher ksymbol event
      perf tools: Set ksymbol dso as loaded on arrival
      perf annotate: Add base support for bpf_image

 arch/x86/mm/init_32.c       |  14 +++++++++-----
 include/linux/bpf.h         |  55 ++++++++++++++++++++++++++++++++++++++-----------------
 include/linux/filter.h      |  13 +++----------
 kernel/bpf/core.c           | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------
 kernel/bpf/dispatcher.c     |  19 ++++++++++++++-----
 kernel/bpf/trampoline.c     |  38 +++++++++++++++++++++++++++++++++++++-
 kernel/events/core.c        |   9 ++++-----
 net/core/filter.c           |   5 ++---
 tools/perf/util/annotate.c  |  20 ++++++++++++++++++++
 tools/perf/util/bpf-event.c |  98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/dso.c       |   1 +
 tools/perf/util/dso.h       |   1 +
 tools/perf/util/machine.c   |  12 ++++++++++++
 tools/perf/util/symbol.c    |   1 +
 14 files changed, 371 insertions(+), 91 deletions(-)

[PATCH 01/18] x86/mm: Rename is_kernel_text to __is_kernel_text

[Jiri Olsa]

The kbuild test robot reported compile issue on x86 in one of
the following patches that adds <linux/kallsyms.h> include into
<linux/bpf.h>, which is picked up by init_32.c object.

The problem is that <linux/kallsyms.h> defines global function
is_kernel_text which colides with the static function of the
same name defined in init_32.c:

  $ make ARCH=i386
  ...
  >> arch/x86/mm/init_32.c:241:19: error: redefinition of 'is_kernel_text'
    static inline int is_kernel_text(unsigned long addr)
                      ^~~~~~~~~~~~~~
   In file included from include/linux/bpf.h:21:0,
                    from include/linux/bpf-cgroup.h:5,
                    from include/linux/cgroup-defs.h:22,
                    from include/linux/cgroup.h:28,
                    from include/linux/hugetlb.h:9,
                    from arch/x86/mm/init_32.c:18:
   include/linux/kallsyms.h:31:19: note: previous definition of 'is_kernel_text' was here
    static inline int is_kernel_text(unsigned long addr)

Renaming the init_32.c is_kernel_text function to __is_kernel_text.

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/mm/init_32.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 23df4885bbed..eb6ede2c3d43 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -238,7 +238,11 @@ page_table_range_init(unsigned long start, unsigned long end, pgd_t *pgd_base)
 	}
 }
 
-static inline int is_kernel_text(unsigned long addr)
+/*
+ * The <linux/kallsyms.h> already defines is_kernel_text,
+ * using '__' prefix not to get in conflict.
+ */
+static inline int __is_kernel_text(unsigned long addr)
 {
 	if (addr >= (unsigned long)_text && addr <= (unsigned long)__init_end)
 		return 1;
@@ -328,8 +332,8 @@ kernel_physical_mapping_init(unsigned long start,
 				addr2 = (pfn + PTRS_PER_PTE-1) * PAGE_SIZE +
 					PAGE_OFFSET + PAGE_SIZE-1;
 
-				if (is_kernel_text(addr) ||
-				    is_kernel_text(addr2))
+				if (__is_kernel_text(addr) ||
+				    __is_kernel_text(addr2))
 					prot = PAGE_KERNEL_LARGE_EXEC;
 
 				pages_2m++;
@@ -354,7 +358,7 @@ kernel_physical_mapping_init(unsigned long start,
 				 */
 				pgprot_t init_prot = __pgprot(PTE_IDENT_ATTR);
 
-				if (is_kernel_text(addr))
+				if (__is_kernel_text(addr))
 					prot = PAGE_KERNEL_EXEC;
 
 				pages_4k++;
@@ -881,7 +885,7 @@ static void mark_nxdata_nx(void)
 	 */
 	unsigned long start = PFN_ALIGN(_etext);
 	/*
-	 * This comes from is_kernel_text upper limit. Also HPAGE where used:
+	 * This comes from __is_kernel_text upper limit. Also HPAGE where used:
 	 */
 	unsigned long size = (((unsigned long)__init_end + HPAGE_SIZE) & HPAGE_MASK) - start;
 
-- 
2.24.1

[PATCH 02/18] bpf: Add bpf_trampoline_ name prefix for DECLARE_BPF_DISPATCHER

[Jiri Olsa]

From: Björn Töpel <bjorn.topel@intel.com>

Adding bpf_trampoline_ name prefix for DECLARE_BPF_DISPATCHER,
so all the dispatchers have the common name prefix.

And also a small '_' cleanup for bpf_dispatcher_nopfunc function
name.

Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h    | 21 +++++++++++----------
 include/linux/filter.h |  7 +++----
 net/core/filter.c      |  5 ++---
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 49b1a70e12c8..be7afccc9459 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -512,7 +512,7 @@ struct bpf_dispatcher {
 	u32 image_off;
 };
 
-static __always_inline unsigned int bpf_dispatcher_nopfunc(
+static __always_inline unsigned int bpf_dispatcher_nop_func(
 	const void *ctx,
 	const struct bpf_insn *insnsi,
 	unsigned int (*bpf_func)(const void *,
@@ -527,7 +527,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
 #define BPF_DISPATCHER_INIT(name) {			\
 	.mutex = __MUTEX_INITIALIZER(name.mutex),	\
-	.func = &name##func,				\
+	.func = &name##_func,				\
 	.progs = {},					\
 	.num_progs = 0,					\
 	.image = NULL,					\
@@ -535,7 +535,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr);
 }
 
 #define DEFINE_BPF_DISPATCHER(name)					\
-	noinline unsigned int name##func(				\
+	noinline unsigned int bpf_dispatcher_##name##_func(		\
 		const void *ctx,					\
 		const struct bpf_insn *insnsi,				\
 		unsigned int (*bpf_func)(const void *,			\
@@ -543,17 +543,18 @@ void bpf_trampoline_put(struct bpf_trampoline *tr);
 	{								\
 		return bpf_func(ctx, insnsi);				\
 	}								\
-	EXPORT_SYMBOL(name##func);			\
-	struct bpf_dispatcher name = BPF_DISPATCHER_INIT(name);
+	EXPORT_SYMBOL(bpf_dispatcher_##name##_func);			\
+	struct bpf_dispatcher bpf_dispatcher_##name =			\
+		BPF_DISPATCHER_INIT(bpf_dispatcher_##name);
 #define DECLARE_BPF_DISPATCHER(name)					\
-	unsigned int name##func(					\
+	unsigned int bpf_dispatcher_##name##_func(			\
 		const void *ctx,					\
 		const struct bpf_insn *insnsi,				\
 		unsigned int (*bpf_func)(const void *,			\
 					 const struct bpf_insn *));	\
-	extern struct bpf_dispatcher name;
-#define BPF_DISPATCHER_FUNC(name) name##func
-#define BPF_DISPATCHER_PTR(name) (&name)
+	extern struct bpf_dispatcher bpf_dispatcher_##name;
+#define BPF_DISPATCHER_FUNC(name) bpf_dispatcher_##name##_func
+#define BPF_DISPATCHER_PTR(name) (&bpf_dispatcher_##name)
 void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
 				struct bpf_prog *to);
 struct bpf_image {
@@ -579,7 +580,7 @@ static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 static inline void bpf_trampoline_put(struct bpf_trampoline *tr) {}
 #define DEFINE_BPF_DISPATCHER(name)
 #define DECLARE_BPF_DISPATCHER(name)
-#define BPF_DISPATCHER_FUNC(name) bpf_dispatcher_nopfunc
+#define BPF_DISPATCHER_FUNC(name) bpf_dispatcher_nop_func
 #define BPF_DISPATCHER_PTR(name) NULL
 static inline void bpf_dispatcher_change_prog(struct bpf_dispatcher *d,
 					      struct bpf_prog *from,
diff --git a/include/linux/filter.h b/include/linux/filter.h
index f349e2c0884c..eafe72644282 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -577,7 +577,7 @@ DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
 	ret; })
 
 #define BPF_PROG_RUN(prog, ctx) __BPF_PROG_RUN(prog, ctx,		\
-					       bpf_dispatcher_nopfunc)
+					       bpf_dispatcher_nop_func)
 
 #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
 
@@ -701,7 +701,7 @@ static inline u32 bpf_prog_run_clear_cb(const struct bpf_prog *prog,
 	return res;
 }
 
-DECLARE_BPF_DISPATCHER(bpf_dispatcher_xdp)
+DECLARE_BPF_DISPATCHER(xdp)
 
 static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
 					    struct xdp_buff *xdp)
@@ -712,8 +712,7 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
 	 * already takes rcu_read_lock() when fetching the program, so
 	 * it's not necessary here anymore.
 	 */
-	return __BPF_PROG_RUN(prog, xdp,
-			      BPF_DISPATCHER_FUNC(bpf_dispatcher_xdp));
+	return __BPF_PROG_RUN(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
 }
 
 void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog);
diff --git a/net/core/filter.c b/net/core/filter.c
index c180871e606d..9048453e235c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -8835,10 +8835,9 @@ const struct bpf_prog_ops sk_reuseport_prog_ops = {
 };
 #endif /* CONFIG_INET */
 
-DEFINE_BPF_DISPATCHER(bpf_dispatcher_xdp)
+DEFINE_BPF_DISPATCHER(xdp)
 
 void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog)
 {
-	bpf_dispatcher_change_prog(BPF_DISPATCHER_PTR(bpf_dispatcher_xdp),
-				   prev_prog, prog);
+	bpf_dispatcher_change_prog(BPF_DISPATCHER_PTR(xdp), prev_prog, prog);
 }
-- 
2.24.1

[PATCH 03/18] bpf: Add struct bpf_ksym

[Jiri Olsa]

Adding 'struct bpf_ksym' object that will carry the
kallsym information for bpf symbol. Adding the start
and end address to begin with. It will be used by
bpf_prog, bpf_trampoline, bpf_dispatcher.

The symbol_start/symbol_end values were originally used
to sort bpf_prog objects. For the address displayed in
/proc/kallsyms we are using prog->bpf_func.

I'm using the bpf_func for program symbol start instead
of the symbol_start, because it makes no difference for
sorting bpf_prog objects and we can use it directly as
an address for display it in /proc/kallsyms.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h |  6 ++++++
 kernel/bpf/core.c   | 26 +++++++++++---------------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index be7afccc9459..5ad8eea1cd37 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -462,6 +462,11 @@ int arch_prepare_bpf_trampoline(void *image, void *image_end,
 u64 notrace __bpf_prog_enter(void);
 void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start);
 
+struct bpf_ksym {
+	unsigned long		 start;
+	unsigned long		 end;
+};
+
 enum bpf_tramp_prog_type {
 	BPF_TRAMP_FENTRY,
 	BPF_TRAMP_FEXIT,
@@ -643,6 +648,7 @@ struct bpf_prog_aux {
 	u32 size_poke_tab;
 	struct latch_tree_node ksym_tnode;
 	struct list_head ksym_lnode;
+	struct bpf_ksym ksym;
 	const struct bpf_prog_ops *ops;
 	struct bpf_map **used_maps;
 	struct bpf_prog *prog;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 973a20d49749..39a9e4184900 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -524,17 +524,15 @@ int bpf_jit_harden   __read_mostly;
 long bpf_jit_limit   __read_mostly;
 
 static __always_inline void
-bpf_get_prog_addr_region(const struct bpf_prog *prog,
-			 unsigned long *symbol_start,
-			 unsigned long *symbol_end)
+bpf_get_prog_addr_region(const struct bpf_prog *prog)
 {
 	const struct bpf_binary_header *hdr = bpf_jit_binary_hdr(prog);
 	unsigned long addr = (unsigned long)hdr;
 
 	WARN_ON_ONCE(!bpf_prog_ebpf_jited(prog));
 
-	*symbol_start = addr;
-	*symbol_end   = addr + hdr->pages * PAGE_SIZE;
+	prog->aux->ksym.start = (unsigned long) prog->bpf_func;
+	prog->aux->ksym.end   = addr + hdr->pages * PAGE_SIZE;
 }
 
 void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
@@ -575,13 +573,10 @@ void bpf_get_prog_name(const struct bpf_prog *prog, char *sym)
 static __always_inline unsigned long
 bpf_get_prog_addr_start(struct latch_tree_node *n)
 {
-	unsigned long symbol_start, symbol_end;
 	const struct bpf_prog_aux *aux;
 
 	aux = container_of(n, struct bpf_prog_aux, ksym_tnode);
-	bpf_get_prog_addr_region(aux->prog, &symbol_start, &symbol_end);
-
-	return symbol_start;
+	return aux->ksym.start;
 }
 
 static __always_inline bool bpf_tree_less(struct latch_tree_node *a,
@@ -593,15 +588,13 @@ static __always_inline bool bpf_tree_less(struct latch_tree_node *a,
 static __always_inline int bpf_tree_comp(void *key, struct latch_tree_node *n)
 {
 	unsigned long val = (unsigned long)key;
-	unsigned long symbol_start, symbol_end;
 	const struct bpf_prog_aux *aux;
 
 	aux = container_of(n, struct bpf_prog_aux, ksym_tnode);
-	bpf_get_prog_addr_region(aux->prog, &symbol_start, &symbol_end);
 
-	if (val < symbol_start)
+	if (val < aux->ksym.start)
 		return -1;
-	if (val >= symbol_end)
+	if (val >= aux->ksym.end)
 		return  1;
 
 	return 0;
@@ -649,6 +642,8 @@ void bpf_prog_kallsyms_add(struct bpf_prog *fp)
 	    !capable(CAP_SYS_ADMIN))
 		return;
 
+	bpf_get_prog_addr_region(fp);
+
 	spin_lock_bh(&bpf_lock);
 	bpf_prog_ksym_node_add(fp->aux);
 	spin_unlock_bh(&bpf_lock);
@@ -677,14 +672,15 @@ static struct bpf_prog *bpf_prog_kallsyms_find(unsigned long addr)
 const char *__bpf_address_lookup(unsigned long addr, unsigned long *size,
 				 unsigned long *off, char *sym)
 {
-	unsigned long symbol_start, symbol_end;
 	struct bpf_prog *prog;
 	char *ret = NULL;
 
 	rcu_read_lock();
 	prog = bpf_prog_kallsyms_find(addr);
 	if (prog) {
-		bpf_get_prog_addr_region(prog, &symbol_start, &symbol_end);
+		unsigned long symbol_start = prog->aux->ksym.start;
+		unsigned long symbol_end = prog->aux->ksym.end;
+
 		bpf_get_prog_name(prog, sym);
 
 		ret = sym;
-- 
2.24.1

[PATCH 04/18] bpf: Add name to struct bpf_ksym

[Jiri Olsa]

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(-)

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);
 		}
 	}
 }
-- 
2.24.1

[PATCH 05/18] bpf: Add lnode list node to struct bpf_ksym

[Jiri Olsa]

Adding lnode list node to 'struct bpf_ksym' object,
so the symbol itself can be chained and used in other
objects like bpf_trampoline and bpf_dispatcher.

Changing iterator to bpf_ksym in bpf_get_kallsym.

The ksym->start is holding the prog->bpf_func value,
so it's ok to use it in bpf_get_kallsym.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h |  2 +-
 kernel/bpf/core.c   | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e7b2e9fc256c..f1174d24c185 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -467,6 +467,7 @@ struct bpf_ksym {
 	unsigned long		 start;
 	unsigned long		 end;
 	char			 name[KSYM_NAME_LEN];
+	struct list_head	 lnode;
 };
 
 enum bpf_tramp_prog_type {
@@ -649,7 +650,6 @@ struct bpf_prog_aux {
 	struct bpf_jit_poke_descriptor *poke_tab;
 	u32 size_poke_tab;
 	struct latch_tree_node ksym_tnode;
-	struct list_head ksym_lnode;
 	struct bpf_ksym ksym;
 	const struct bpf_prog_ops *ops;
 	struct bpf_map **used_maps;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index a7aaa81035b1..604093d2153a 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -97,7 +97,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag
 	fp->aux->prog = fp;
 	fp->jit_requested = ebpf_jit_enabled();
 
-	INIT_LIST_HEAD_RCU(&fp->aux->ksym_lnode);
+	INIT_LIST_HEAD_RCU(&fp->aux->ksym.lnode);
 
 	return fp;
 }
@@ -612,18 +612,18 @@ static struct latch_tree_root bpf_tree __cacheline_aligned;
 
 static void bpf_prog_ksym_node_add(struct bpf_prog_aux *aux)
 {
-	WARN_ON_ONCE(!list_empty(&aux->ksym_lnode));
-	list_add_tail_rcu(&aux->ksym_lnode, &bpf_kallsyms);
+	WARN_ON_ONCE(!list_empty(&aux->ksym.lnode));
+	list_add_tail_rcu(&aux->ksym.lnode, &bpf_kallsyms);
 	latch_tree_insert(&aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
 }
 
 static void bpf_prog_ksym_node_del(struct bpf_prog_aux *aux)
 {
-	if (list_empty(&aux->ksym_lnode))
+	if (list_empty(&aux->ksym.lnode))
 		return;
 
 	latch_tree_erase(&aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
-	list_del_rcu(&aux->ksym_lnode);
+	list_del_rcu(&aux->ksym.lnode);
 }
 
 static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
@@ -633,8 +633,8 @@ static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
 
 static bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
 {
-	return list_empty(&fp->aux->ksym_lnode) ||
-	       fp->aux->ksym_lnode.prev == LIST_POISON2;
+	return list_empty(&fp->aux->ksym.lnode) ||
+	       fp->aux->ksym.lnode.prev == LIST_POISON2;
 }
 
 void bpf_prog_kallsyms_add(struct bpf_prog *fp)
@@ -728,7 +728,7 @@ const struct exception_table_entry *search_bpf_extables(unsigned long addr)
 int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 		    char *sym)
 {
-	struct bpf_prog_aux *aux;
+	struct bpf_ksym *ksym;
 	unsigned int it = 0;
 	int ret = -ERANGE;
 
@@ -736,13 +736,13 @@ int bpf_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 		return ret;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(aux, &bpf_kallsyms, ksym_lnode) {
+	list_for_each_entry_rcu(ksym, &bpf_kallsyms, lnode) {
 		if (it++ != symnum)
 			continue;
 
-		strncpy(sym, aux->ksym.name, KSYM_NAME_LEN);
+		strncpy(sym, ksym->name, KSYM_NAME_LEN);
 
-		*value = (unsigned long)aux->prog->bpf_func;
+		*value = ksym->start;
 		*type  = BPF_SYM_ELF_TYPE;
 
 		ret = 0;
-- 
2.24.1

[PATCH 06/18] bpf: Add bpf_ksym_tree tree

[Jiri Olsa]

The bpf_tree is used both for kallsyms iterations and searching
for exception tables of bpf programs, which is needed only for
bpf programs.

Adding bpf_ksym_tree that will hold symbols for all bpf_prog
bpf_trampoline and bpf_dispatcher objects and keeping bpf_tree
only for bpf_prog objects to keep it fast.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h |  1 +
 kernel/bpf/core.c   | 56 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f1174d24c185..5d6649cdc3df 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -468,6 +468,7 @@ struct bpf_ksym {
 	unsigned long		 end;
 	char			 name[KSYM_NAME_LEN];
 	struct list_head	 lnode;
+	struct latch_tree_node	 tnode;
 };
 
 enum bpf_tramp_prog_type {
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 604093d2153a..26d13dec3435 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -606,8 +606,42 @@ static const struct latch_tree_ops bpf_tree_ops = {
 	.comp	= bpf_tree_comp,
 };
 
+static unsigned long bpf_get_ksym_start(struct latch_tree_node *n)
+{
+	return container_of(n, struct bpf_ksym, tnode)->start;
+}
+
+static bool
+bpf_ksym_tree_less(struct latch_tree_node *a,
+		   struct latch_tree_node *b)
+{
+	return bpf_get_ksym_start(a) < bpf_get_ksym_start(b);
+}
+
+static int
+bpf_ksym_tree_comp(void *key, struct latch_tree_node *n)
+{
+	unsigned long val = (unsigned long)key;
+	const struct bpf_ksym *ksym;
+
+	ksym = container_of(n, struct bpf_ksym, tnode);
+
+	if (val < ksym->start)
+		return -1;
+	if (val >= ksym->end)
+		return  1;
+
+	return 0;
+}
+
+static const struct latch_tree_ops bpf_ksym_tree_ops = {
+	.less	= bpf_ksym_tree_less,
+	.comp	= bpf_ksym_tree_comp,
+};
+
 static DEFINE_SPINLOCK(bpf_lock);
 static LIST_HEAD(bpf_kallsyms);
+static struct latch_tree_root bpf_ksym_tree __cacheline_aligned;
 static struct latch_tree_root bpf_tree __cacheline_aligned;
 
 static void bpf_prog_ksym_node_add(struct bpf_prog_aux *aux)
@@ -615,6 +649,7 @@ static void bpf_prog_ksym_node_add(struct bpf_prog_aux *aux)
 	WARN_ON_ONCE(!list_empty(&aux->ksym.lnode));
 	list_add_tail_rcu(&aux->ksym.lnode, &bpf_kallsyms);
 	latch_tree_insert(&aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
+	latch_tree_insert(&aux->ksym.tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
 }
 
 static void bpf_prog_ksym_node_del(struct bpf_prog_aux *aux)
@@ -623,6 +658,7 @@ static void bpf_prog_ksym_node_del(struct bpf_prog_aux *aux)
 		return;
 
 	latch_tree_erase(&aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
+	latch_tree_erase(&aux->ksym.tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
 	list_del_rcu(&aux->ksym.lnode);
 }
 
@@ -671,19 +707,27 @@ static struct bpf_prog *bpf_prog_kallsyms_find(unsigned long addr)
 	       NULL;
 }
 
+static struct bpf_ksym *bpf_ksym_find(unsigned long addr)
+{
+	struct latch_tree_node *n;
+
+	n = latch_tree_find((void *)addr, &bpf_ksym_tree, &bpf_ksym_tree_ops);
+	return n ? container_of(n, struct bpf_ksym, tnode) : NULL;
+}
+
 const char *__bpf_address_lookup(unsigned long addr, unsigned long *size,
 				 unsigned long *off, char *sym)
 {
-	struct bpf_prog *prog;
+	struct bpf_ksym *ksym;
 	char *ret = NULL;
 
 	rcu_read_lock();
-	prog = bpf_prog_kallsyms_find(addr);
-	if (prog) {
-		unsigned long symbol_start = prog->aux->ksym.start;
-		unsigned long symbol_end = prog->aux->ksym.end;
+	ksym = bpf_ksym_find(addr);
+	if (ksym) {
+		unsigned long symbol_start = ksym->start;
+		unsigned long symbol_end = ksym->end;
 
-		strncpy(sym, prog->aux->ksym.name, KSYM_NAME_LEN);
+		strncpy(sym, ksym->name, KSYM_NAME_LEN);
 
 		ret = sym;
 		if (size)
-- 
2.24.1

[PATCH 07/18] bpf: Move bpf_tree add/del from bpf_prog_ksym_node_add/del

[Jiri Olsa]

Moving bpf_tree add/del from bpf_prog_ksym_node_add/del,
because it will be used (and renamed) in following patches
for bpf_ksym objects. The bpf_tree is specific for bpf_prog
objects.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 26d13dec3435..cdff3e4b207c 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -648,7 +648,6 @@ static void bpf_prog_ksym_node_add(struct bpf_prog_aux *aux)
 {
 	WARN_ON_ONCE(!list_empty(&aux->ksym.lnode));
 	list_add_tail_rcu(&aux->ksym.lnode, &bpf_kallsyms);
-	latch_tree_insert(&aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
 	latch_tree_insert(&aux->ksym.tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
 }
 
@@ -657,7 +656,6 @@ static void bpf_prog_ksym_node_del(struct bpf_prog_aux *aux)
 	if (list_empty(&aux->ksym.lnode))
 		return;
 
-	latch_tree_erase(&aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
 	latch_tree_erase(&aux->ksym.tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
 	list_del_rcu(&aux->ksym.lnode);
 }
@@ -683,6 +681,7 @@ void bpf_prog_kallsyms_add(struct bpf_prog *fp)
 	bpf_get_prog_name(fp);
 
 	spin_lock_bh(&bpf_lock);
+	latch_tree_insert(&fp->aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
 	bpf_prog_ksym_node_add(fp->aux);
 	spin_unlock_bh(&bpf_lock);
 }
@@ -693,6 +692,7 @@ void bpf_prog_kallsyms_del(struct bpf_prog *fp)
 		return;
 
 	spin_lock_bh(&bpf_lock);
+	latch_tree_erase(&fp->aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
 	bpf_prog_ksym_node_del(fp->aux);
 	spin_unlock_bh(&bpf_lock);
 }
-- 
2.24.1

[PATCH 08/18] bpf: Separate kallsyms add/del functions

[Jiri Olsa]

Moving bpf_prog_ksym_node_add/del to __bpf_ksym_add/del
and changing the argument to 'struct bpf_ksym' object.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index cdff3e4b207c..a8a4000f5ca8 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -644,20 +644,20 @@ static LIST_HEAD(bpf_kallsyms);
 static struct latch_tree_root bpf_ksym_tree __cacheline_aligned;
 static struct latch_tree_root bpf_tree __cacheline_aligned;
 
-static void bpf_prog_ksym_node_add(struct bpf_prog_aux *aux)
+static void __bpf_ksym_add(struct bpf_ksym *ksym)
 {
-	WARN_ON_ONCE(!list_empty(&aux->ksym.lnode));
-	list_add_tail_rcu(&aux->ksym.lnode, &bpf_kallsyms);
-	latch_tree_insert(&aux->ksym.tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
+	WARN_ON_ONCE(!list_empty(&ksym->lnode));
+	list_add_tail_rcu(&ksym->lnode, &bpf_kallsyms);
+	latch_tree_insert(&ksym->tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
 }
 
-static void bpf_prog_ksym_node_del(struct bpf_prog_aux *aux)
+static void __bpf_ksym_del(struct bpf_ksym *ksym)
 {
-	if (list_empty(&aux->ksym.lnode))
+	if (list_empty(&ksym->lnode))
 		return;
 
-	latch_tree_erase(&aux->ksym.tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
-	list_del_rcu(&aux->ksym.lnode);
+	latch_tree_erase(&ksym->tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
+	list_del_rcu(&ksym->lnode);
 }
 
 static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
@@ -682,7 +682,7 @@ void bpf_prog_kallsyms_add(struct bpf_prog *fp)
 
 	spin_lock_bh(&bpf_lock);
 	latch_tree_insert(&fp->aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
-	bpf_prog_ksym_node_add(fp->aux);
+	__bpf_ksym_add(&fp->aux->ksym);
 	spin_unlock_bh(&bpf_lock);
 }
 
@@ -693,7 +693,7 @@ void bpf_prog_kallsyms_del(struct bpf_prog *fp)
 
 	spin_lock_bh(&bpf_lock);
 	latch_tree_erase(&fp->aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
-	bpf_prog_ksym_node_del(fp->aux);
+	__bpf_ksym_del(&fp->aux->ksym);
 	spin_unlock_bh(&bpf_lock);
 }
 
-- 
2.24.1

[PATCH 09/18] bpf: Add bpf_ksym_add/del functions

[Jiri Olsa]

Adding bpf_ksym_add/del functions as locked version
for __bpf_ksym_add/del. It will be used in following
patches for bpf_trampoline and bpf_dispatcher.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h |  3 +++
 kernel/bpf/core.c   | 14 ++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5d6649cdc3df..76934893bccf 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -573,6 +573,9 @@ struct bpf_image {
 #define BPF_IMAGE_SIZE (PAGE_SIZE - sizeof(struct bpf_image))
 bool is_bpf_image_address(unsigned long address);
 void *bpf_image_alloc(void);
+/* Called only from code, so there's no need for stubs. */
+void bpf_ksym_add(struct bpf_ksym *ksym);
+void bpf_ksym_del(struct bpf_ksym *ksym);
 #else
 static inline struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 {
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index a8a4000f5ca8..c95424fc53de 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -651,6 +651,13 @@ static void __bpf_ksym_add(struct bpf_ksym *ksym)
 	latch_tree_insert(&ksym->tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
 }
 
+void bpf_ksym_add(struct bpf_ksym *ksym)
+{
+	spin_lock_bh(&bpf_lock);
+	__bpf_ksym_add(ksym);
+	spin_unlock_bh(&bpf_lock);
+}
+
 static void __bpf_ksym_del(struct bpf_ksym *ksym)
 {
 	if (list_empty(&ksym->lnode))
@@ -660,6 +667,13 @@ static void __bpf_ksym_del(struct bpf_ksym *ksym)
 	list_del_rcu(&ksym->lnode);
 }
 
+void bpf_ksym_del(struct bpf_ksym *ksym)
+{
+	spin_lock_bh(&bpf_lock);
+	__bpf_ksym_del(ksym);
+	spin_unlock_bh(&bpf_lock);
+}
+
 static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
 {
 	return fp->jited && !bpf_prog_was_classic(fp);
-- 
2.24.1

[PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Jiri Olsa]

When bpf_prog is removed from kallsyms it's on the way
out to be removed, so we don't care about lnode state.

However the bpf_ksym_del will be used also by bpf_trampoline
and bpf_dispatcher objects, which stay allocated even when
they are not in kallsyms list, hence the lnode re-init.

The list_del_rcu commentary states that we need to call
synchronize_rcu, before we can change/re-init the list_head
pointers.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index c95424fc53de..1af2109b45c7 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym)
 	spin_lock_bh(&bpf_lock);
 	__bpf_ksym_del(ksym);
 	spin_unlock_bh(&bpf_lock);
+
+	/*
+	 * As explained in list_del_rcu, We must call synchronize_rcu
+	 * before changing list_head pointers.
+	 */
+	synchronize_rcu();
+	INIT_LIST_HEAD_RCU(&ksym->lnode);
 }
 
 static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
-- 
2.24.1

[PATCH 11/18] bpf: Rename bpf_tree to bpf_progs_tree

[Jiri Olsa]

Renaming bpf_tree to bpf_progs_tree and bpf_tree_ops
to bpf_progs_tree_ops to better capture the usage of
the tree which is for the bpf_prog objects only.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 1af2109b45c7..9879fb02ee8e 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -580,13 +580,14 @@ bpf_get_prog_addr_start(struct latch_tree_node *n)
 	return aux->ksym.start;
 }
 
-static __always_inline bool bpf_tree_less(struct latch_tree_node *a,
-					  struct latch_tree_node *b)
+static __always_inline bool
+bpf_progs_tree_less(struct latch_tree_node *a,
+		    struct latch_tree_node *b)
 {
 	return bpf_get_prog_addr_start(a) < bpf_get_prog_addr_start(b);
 }
 
-static __always_inline int bpf_tree_comp(void *key, struct latch_tree_node *n)
+static __always_inline int bpf_progs_tree_comp(void *key, struct latch_tree_node *n)
 {
 	unsigned long val = (unsigned long)key;
 	const struct bpf_prog_aux *aux;
@@ -601,9 +602,9 @@ static __always_inline int bpf_tree_comp(void *key, struct latch_tree_node *n)
 	return 0;
 }
 
-static const struct latch_tree_ops bpf_tree_ops = {
-	.less	= bpf_tree_less,
-	.comp	= bpf_tree_comp,
+static const struct latch_tree_ops bpf_progs_tree_ops = {
+	.less	= bpf_progs_tree_less,
+	.comp	= bpf_progs_tree_comp,
 };
 
 static unsigned long bpf_get_ksym_start(struct latch_tree_node *n)
@@ -642,7 +643,7 @@ static const struct latch_tree_ops bpf_ksym_tree_ops = {
 static DEFINE_SPINLOCK(bpf_lock);
 static LIST_HEAD(bpf_kallsyms);
 static struct latch_tree_root bpf_ksym_tree __cacheline_aligned;
-static struct latch_tree_root bpf_tree __cacheline_aligned;
+static struct latch_tree_root bpf_progs_tree __cacheline_aligned;
 
 static void __bpf_ksym_add(struct bpf_ksym *ksym)
 {
@@ -702,7 +703,8 @@ void bpf_prog_kallsyms_add(struct bpf_prog *fp)
 	bpf_get_prog_name(fp);
 
 	spin_lock_bh(&bpf_lock);
-	latch_tree_insert(&fp->aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
+	latch_tree_insert(&fp->aux->ksym_tnode, &bpf_progs_tree,
+			  &bpf_progs_tree_ops);
 	__bpf_ksym_add(&fp->aux->ksym);
 	spin_unlock_bh(&bpf_lock);
 }
@@ -713,7 +715,8 @@ void bpf_prog_kallsyms_del(struct bpf_prog *fp)
 		return;
 
 	spin_lock_bh(&bpf_lock);
-	latch_tree_erase(&fp->aux->ksym_tnode, &bpf_tree, &bpf_tree_ops);
+	latch_tree_erase(&fp->aux->ksym_tnode, &bpf_progs_tree,
+			 &bpf_progs_tree_ops);
 	__bpf_ksym_del(&fp->aux->ksym);
 	spin_unlock_bh(&bpf_lock);
 }
@@ -722,7 +725,8 @@ static struct bpf_prog *bpf_prog_kallsyms_find(unsigned long addr)
 {
 	struct latch_tree_node *n;
 
-	n = latch_tree_find((void *)addr, &bpf_tree, &bpf_tree_ops);
+	n = latch_tree_find((void *)addr, &bpf_progs_tree,
+			    &bpf_progs_tree_ops);
 	return n ?
 	       container_of(n, struct bpf_prog_aux, ksym_tnode)->prog :
 	       NULL;
-- 
2.24.1

[PATCH 12/18] bpf: Add trampolines to kallsyms

[Jiri Olsa]

Adding trampolines to kallsyms. It's displayed as
  bpf_trampoline_<ID> [bpf]

where ID is the BTF id of the trampoline function.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h     |  3 +++
 kernel/bpf/trampoline.c | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 76934893bccf..c216af89254f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -502,6 +502,7 @@ struct bpf_trampoline {
 	/* Executable image of trampoline */
 	void *image;
 	u64 selector;
+	struct bpf_ksym ksym;
 };
 
 #define BPF_DISPATCHER_MAX 48 /* Fits in 2048B */
@@ -573,6 +574,8 @@ struct bpf_image {
 #define BPF_IMAGE_SIZE (PAGE_SIZE - sizeof(struct bpf_image))
 bool is_bpf_image_address(unsigned long address);
 void *bpf_image_alloc(void);
+void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym);
+void bpf_image_ksym_del(struct bpf_ksym *ksym);
 /* Called only from code, so there's no need for stubs. */
 void bpf_ksym_add(struct bpf_ksym *ksym);
 void bpf_ksym_del(struct bpf_ksym *ksym);
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 6b264a92064b..4b0ae976a6eb 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -5,6 +5,7 @@
 #include <linux/filter.h>
 #include <linux/ftrace.h>
 #include <linux/rbtree_latch.h>
+#include <linux/perf_event.h>
 
 /* dummy _ops. The verifier will operate on target program's ops. */
 const struct bpf_verifier_ops bpf_extension_verifier_ops = {
@@ -96,6 +97,22 @@ bool is_bpf_image_address(unsigned long addr)
 	return ret;
 }
 
+void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym)
+{
+	ksym->start = (unsigned long) data;
+	ksym->end = ksym->start + BPF_IMAGE_SIZE;
+	bpf_ksym_add(ksym);
+	perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF, ksym->start,
+			   BPF_IMAGE_SIZE, false, ksym->name);
+}
+
+void bpf_image_ksym_del(struct bpf_ksym *ksym)
+{
+	bpf_ksym_del(ksym);
+	perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF, ksym->start,
+			   BPF_IMAGE_SIZE, true, ksym->name);
+}
+
 struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 {
 	struct bpf_trampoline *tr;
@@ -131,6 +148,7 @@ struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
 	for (i = 0; i < BPF_TRAMP_MAX; i++)
 		INIT_HLIST_HEAD(&tr->progs_hlist[i]);
 	tr->image = image;
+	INIT_LIST_HEAD_RCU(&tr->ksym.lnode);
 out:
 	mutex_unlock(&trampoline_mutex);
 	return tr;
@@ -267,6 +285,14 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(enum bpf_attach_type t)
 	}
 }
 
+static void bpf_trampoline_ksym_add(struct bpf_trampoline *tr)
+{
+	struct bpf_ksym *ksym = &tr->ksym;
+
+	snprintf(ksym->name, KSYM_NAME_LEN, "bpf_trampoline_%llu", tr->key);
+	bpf_image_ksym_add(tr->image, ksym);
+}
+
 int bpf_trampoline_link_prog(struct bpf_prog *prog)
 {
 	enum bpf_tramp_prog_type kind;
@@ -291,6 +317,10 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
 			err = -EBUSY;
 			goto out;
 		}
+		/* With cnt == 0 image is not used (no symbol in kallsyms)
+		 * and will not be used for BPF_PROG_TYPE_EXT prog type,
+		 * so there's no symbol added for this case.
+		 */
 		tr->extension_prog = prog;
 		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
 					 prog->bpf_func);
@@ -311,7 +341,10 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
 	if (err) {
 		hlist_del(&prog->aux->tramp_hlist);
 		tr->progs_cnt[kind]--;
+		goto out;
 	}
+	if (cnt == 0)
+		bpf_trampoline_ksym_add(tr);
 out:
 	mutex_unlock(&tr->mutex);
 	return err;
@@ -322,7 +355,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 {
 	enum bpf_tramp_prog_type kind;
 	struct bpf_trampoline *tr;
-	int err;
+	int err, cnt;
 
 	tr = prog->aux->trampoline;
 	kind = bpf_attach_type_to_tramp(prog->expected_attach_type);
@@ -336,6 +369,9 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 	}
 	hlist_del(&prog->aux->tramp_hlist);
 	tr->progs_cnt[kind]--;
+	cnt = tr->progs_cnt[BPF_TRAMP_FENTRY] + tr->progs_cnt[BPF_TRAMP_FEXIT];
+	if (cnt == 0)
+		bpf_image_ksym_del(&tr->ksym);
 	err = bpf_trampoline_update(prog->aux->trampoline);
 out:
 	mutex_unlock(&tr->mutex);
-- 
2.24.1

[PATCH 13/18] bpf: Return error value in bpf_dispatcher_update

[Jiri Olsa]

We don't currently propagate error value from
bpf_dispatcher_update function. This will be
needed in following patch, that needs to update
kallsyms based on the success of dispatcher
update.

Suggested-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/dispatcher.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
index b3e5b214fed8..3a5871bbd6d0 100644
--- a/kernel/bpf/dispatcher.c
+++ b/kernel/bpf/dispatcher.c
@@ -102,7 +102,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
 	return arch_prepare_bpf_dispatcher(image, &ips[0], d->num_progs);
 }
 
-static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
+static int bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
 {
 	void *old, *new;
 	u32 noff;
@@ -118,15 +118,17 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
 
 	new = d->num_progs ? d->image + noff : NULL;
 	if (new) {
-		if (bpf_dispatcher_prepare(d, new))
-			return;
+		err = bpf_dispatcher_prepare(d, new);
+		if (err)
+			return err;
 	}
 
 	err = bpf_arch_text_poke(d->func, BPF_MOD_JUMP, old, new);
 	if (err || !new)
-		return;
+		return err;
 
 	d->image_off = noff;
+	return 0;
 }
 
 void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
-- 
2.24.1

[PATCH 14/18] bpf: Add dispatchers to kallsyms

[Jiri Olsa]

Adding dispatchers to kallsyms. It's displayed as
  bpf_dispatcher_<NAME>

where NAME is the name of dispatcher.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h     | 19 ++++++++++++-------
 kernel/bpf/dispatcher.c |  9 ++++++++-
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c216af89254f..4ae2273112ba 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -520,6 +520,7 @@ struct bpf_dispatcher {
 	int num_progs;
 	void *image;
 	u32 image_off;
+	struct bpf_ksym ksym;
 };
 
 static __always_inline unsigned int bpf_dispatcher_nop_func(
@@ -535,13 +536,17 @@ struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
 int bpf_trampoline_link_prog(struct bpf_prog *prog);
 int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
-#define BPF_DISPATCHER_INIT(name) {			\
-	.mutex = __MUTEX_INITIALIZER(name.mutex),	\
-	.func = &name##_func,				\
-	.progs = {},					\
-	.num_progs = 0,					\
-	.image = NULL,					\
-	.image_off = 0					\
+#define BPF_DISPATCHER_INIT(_name) {				\
+	.mutex = __MUTEX_INITIALIZER(_name.mutex),		\
+	.func = &_name##_func,					\
+	.progs = {},						\
+	.num_progs = 0,						\
+	.image = NULL,						\
+	.image_off = 0,						\
+	.ksym = {						\
+		.name  = #_name,				\
+		.lnode = LIST_HEAD_INIT(_name.ksym.lnode),	\
+	},							\
 }
 
 #define DEFINE_BPF_DISPATCHER(name)					\
diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
index 3a5871bbd6d0..74060c92b2f3 100644
--- a/kernel/bpf/dispatcher.c
+++ b/kernel/bpf/dispatcher.c
@@ -154,7 +154,14 @@ void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
 	if (!changed)
 		goto out;
 
-	bpf_dispatcher_update(d, prev_num_progs);
+	if (!bpf_dispatcher_update(d, prev_num_progs)) {
+		if (!prev_num_progs)
+			bpf_image_ksym_add(d->image, &d->ksym);
+
+		if (!d->num_progs)
+			bpf_image_ksym_del(&d->ksym);
+	}
+
 out:
 	mutex_unlock(&d->mutex);
 }
-- 
2.24.1

[PATCH 15/18] bpf: Sort bpf kallsyms symbols

[Jiri Olsa]

Currently we don't sort bpf_kallsyms and display symbols
in proc/kallsyms as they come in via __bpf_ksym_add.

Using the latch tree to get the next bpf_ksym object
and insert the new symbol ahead of it.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 9879fb02ee8e..3319019735f0 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -647,9 +647,28 @@ static struct latch_tree_root bpf_progs_tree __cacheline_aligned;
 
 static void __bpf_ksym_add(struct bpf_ksym *ksym)
 {
+	struct list_head *head = &bpf_kallsyms;
+	struct rb_node *next;
+
 	WARN_ON_ONCE(!list_empty(&ksym->lnode));
-	list_add_tail_rcu(&ksym->lnode, &bpf_kallsyms);
 	latch_tree_insert(&ksym->tnode, &bpf_ksym_tree, &bpf_ksym_tree_ops);
+
+	/*
+	 * Add ksym into bpf_kallsyms in ordered position,
+	 * which is prepared for us by latch tree addition.
+	 *
+	 * Find out the next symbol and insert ksym right
+	 * ahead of it. If ksym is the last one, just tail
+	 * add to the bpf_kallsyms.
+	 */
+	next = rb_next(&ksym->tnode.node[0]);
+	if (next) {
+		struct bpf_ksym *ptr;
+
+		ptr = container_of(next, struct bpf_ksym, tnode.node[0]);
+		head = &ptr->lnode;
+	}
+	list_add_tail_rcu(&ksym->lnode, head);
 }
 
 void bpf_ksym_add(struct bpf_ksym *ksym)
-- 
2.24.1

[PATCH 16/18] perf tools: Synthesize bpf_trampoline/dispatcher ksymbol event

[Jiri Olsa]

Synthesize bpf images (trampolines/dispatchers) on start,
as ksymbol events from /proc/kallsyms. Having this perf
can recognize samples from those images and perf report
and top shows them correctly.

The rest of the ksymbol handling is already in place from
for the bpf programs monitoring, so only the initial state
was needed.

perf report output:

  # Overhead  Command     Shared Object                  Symbol

    12.37%  test_progs  [kernel.vmlinux]                 [k] entry_SYSCALL_64
    11.80%  test_progs  [kernel.vmlinux]                 [k] syscall_return_via_sysret
     9.63%  test_progs  bpf_prog_bcf7977d3b93787c_prog2  [k] bpf_prog_bcf7977d3b93787c_prog2
     6.90%  test_progs  bpf_trampoline_24456             [k] bpf_trampoline_24456
     6.36%  test_progs  [kernel.vmlinux]                 [k] memcpy_erms

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/bpf-event.c | 98 +++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index a3207d900339..120ec547ae75 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -6,6 +6,9 @@
 #include <bpf/libbpf.h>
 #include <linux/btf.h>
 #include <linux/err.h>
+#include <linux/string.h>
+#include <internal/lib.h>
+#include <symbol/kallsyms.h>
 #include "bpf-event.h"
 #include "debug.h"
 #include "dso.h"
@@ -290,11 +293,87 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
 	return err ? -1 : 0;
 }
 
+struct kallsyms_parse {
+	union perf_event	*event;
+	perf_event__handler_t	 process;
+	struct machine		*machine;
+	struct perf_tool	*tool;
+};
+
+static int
+process_bpf_image(char *name, u64 addr, struct kallsyms_parse *data)
+{
+	struct machine *machine = data->machine;
+	union perf_event *event = data->event;
+	struct perf_record_ksymbol *ksymbol;
+	u32 size;
+
+	ksymbol = &event->ksymbol;
+
+	/*
+	 * The bpf image (trampoline/dispatcher) size is aligned to
+	 * page, while it starts little bit after the page boundary.
+	 */
+	size = page_size - (addr - PERF_ALIGN(addr, page_size));
+
+	*ksymbol = (struct perf_record_ksymbol) {
+		.header = {
+			.type = PERF_RECORD_KSYMBOL,
+			.size = offsetof(struct perf_record_ksymbol, name),
+		},
+		.addr      = addr,
+		.len       = size,
+		.ksym_type = PERF_RECORD_KSYMBOL_TYPE_BPF,
+		.flags     = 0,
+	};
+
+	strncpy(ksymbol->name, name, KSYM_NAME_LEN);
+	ksymbol->header.size += PERF_ALIGN(strlen(name) + 1, sizeof(u64));
+	memset((void *) event + event->header.size, 0, machine->id_hdr_size);
+	event->header.size += machine->id_hdr_size;
+
+	return perf_tool__process_synth_event(data->tool, event, machine,
+					      data->process);
+}
+
+static int
+kallsyms_process_symbol(void *data, const char *_name,
+			char type __maybe_unused, u64 start)
+{
+	char *module, *name;
+	unsigned long id;
+	int err = 0;
+
+	module = strchr(_name, '\t');
+	if (!module)
+		return 0;
+
+	/* We are going after [bpf] module ... */
+	if (strcmp(module + 1, "[bpf]"))
+		return 0;
+
+	name = memdup(_name, (module - _name) + 1);
+	if (!name)
+		return -ENOMEM;
+
+	name[module - _name] = 0;
+
+	/* .. and only for trampolines and dispatchers */
+	if ((sscanf(name, "bpf_trampoline_%lu", &id) == 1) ||
+	    (sscanf(name, "bpf_dispatcher_%lu", &id) == 1))
+		err = process_bpf_image(name, start, data);
+
+	free(name);
+	return err;
+}
+
 int perf_event__synthesize_bpf_events(struct perf_session *session,
 				      perf_event__handler_t process,
 				      struct machine *machine,
 				      struct record_opts *opts)
 {
+	const char *kallsyms_filename = "/proc/kallsyms";
+	struct kallsyms_parse arg;
 	union perf_event *event;
 	__u32 id = 0;
 	int err;
@@ -303,6 +382,8 @@ int perf_event__synthesize_bpf_events(struct perf_session *session,
 	event = malloc(sizeof(event->bpf) + KSYM_NAME_LEN + machine->id_hdr_size);
 	if (!event)
 		return -1;
+
+	/* Synthesize all the bpf programs in system. */
 	while (true) {
 		err = bpf_prog_get_next_id(id, &id);
 		if (err) {
@@ -335,6 +416,23 @@ int perf_event__synthesize_bpf_events(struct perf_session *session,
 			break;
 		}
 	}
+
+	/* Synthesize all the bpf images - trampolines/dispatchers. */
+	if (symbol_conf.kallsyms_name != NULL)
+		kallsyms_filename = symbol_conf.kallsyms_name;
+
+	arg = (struct kallsyms_parse) {
+		.event   = event,
+		.process = process,
+		.machine = machine,
+		.tool    = session->tool,
+	};
+
+	if (kallsyms__parse(kallsyms_filename, &arg, kallsyms_process_symbol)) {
+		pr_err("%s: failed to synthesize bpf images: %s\n",
+		       __func__, strerror(errno));
+	}
+
 	free(event);
 	return err;
 }
-- 
2.24.1

[PATCH 17/18] perf tools: Set ksymbol dso as loaded on arrival

[Jiri Olsa]

There's no special load action for ksymbol data on
map__load/dso__load action, where the kernel is getting
loaded. It only gets confused with kernel kallsyms/vmlinux
load for bpf object, which fails and could mess up with
the map.

Disabling any further load of the map for ksymbol related dso/map.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index fb5c2cd44d30..463ada5117f8 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -742,6 +742,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
 		map->start = event->ksymbol.addr;
 		map->end = map->start + event->ksymbol.len;
 		maps__insert(&machine->kmaps, map);
+		dso__set_loaded(dso);
 	}
 
 	sym = symbol__new(map->map_ip(map, map->start),
-- 
2.24.1

[PATCH 18/18] perf annotate: Add base support for bpf_image

[Jiri Olsa]

Adding the DSO_BINARY_TYPE__BPF_IMAGE dso binary type
to recognize bpf images that carry trampoline or dispatcher.

Upcoming patches will add support to read the image data,
store it within the BPF feature in perf.data and display
it for annotation purposes.

Currently we only display following message:

  # ./perf annotate bpf_trampoline_24456 --stdio
   Percent |      Source code & Disassembly of . for cycles (504  ...
  --------------------------------------------------------------- ...
           :       to be implemented

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/annotate.c | 20 ++++++++++++++++++++
 tools/perf/util/dso.c      |  1 +
 tools/perf/util/dso.h      |  1 +
 tools/perf/util/machine.c  | 11 +++++++++++
 tools/perf/util/symbol.c   |  1 +
 5 files changed, 34 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ca73fb74ad03..d9e606e11936 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1843,6 +1843,24 @@ static int symbol__disassemble_bpf(struct symbol *sym __maybe_unused,
 }
 #endif // defined(HAVE_LIBBFD_SUPPORT) && defined(HAVE_LIBBPF_SUPPORT)
 
+static int
+symbol__disassemble_bpf_image(struct symbol *sym,
+			      struct annotate_args *args)
+{
+	struct annotation *notes = symbol__annotation(sym);
+	struct disasm_line *dl;
+
+	args->offset = -1;
+	args->line = strdup("to be implemented");
+	args->line_nr = 0;
+	dl = disasm_line__new(args);
+	if (dl)
+		annotation_line__add(&dl->al, &notes->src->source);
+
+	free(args->line);
+	return 0;
+}
+
 /*
  * Possibly create a new version of line with tabs expanded. Returns the
  * existing or new line, storage is updated if a new line is allocated. If
@@ -1942,6 +1960,8 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
 
 	if (dso->binary_type == DSO_BINARY_TYPE__BPF_PROG_INFO) {
 		return symbol__disassemble_bpf(sym, args);
+	} else if (dso->binary_type == DSO_BINARY_TYPE__BPF_IMAGE) {
+		return symbol__disassemble_bpf_image(sym, args);
 	} else if (dso__is_kcore(dso)) {
 		kce.kcore_filename = symfs_filename;
 		kce.addr = map__rip_2objdump(map, sym->start);
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 91f21239608b..f338990e0fe6 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -191,6 +191,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
 	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
 	case DSO_BINARY_TYPE__JAVA_JIT:
 	case DSO_BINARY_TYPE__BPF_PROG_INFO:
+	case DSO_BINARY_TYPE__BPF_IMAGE:
 	case DSO_BINARY_TYPE__NOT_FOUND:
 		ret = -1;
 		break;
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 2db64b79617a..9553a1fd9e8a 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -40,6 +40,7 @@ enum dso_binary_type {
 	DSO_BINARY_TYPE__GUEST_KCORE,
 	DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO,
 	DSO_BINARY_TYPE__BPF_PROG_INFO,
+	DSO_BINARY_TYPE__BPF_IMAGE,
 	DSO_BINARY_TYPE__NOT_FOUND,
 };
 
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 463ada5117f8..372ed147bed5 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -719,6 +719,12 @@ int machine__process_switch_event(struct machine *machine __maybe_unused,
 	return 0;
 }
 
+static int is_bpf_image(const char *name)
+{
+	return strncmp(name, "bpf_trampoline_", sizeof("bpf_trampoline_") - 1) ||
+	       strncmp(name, "bpf_dispatcher_", sizeof("bpf_dispatcher_") - 1);
+}
+
 static int machine__process_ksymbol_register(struct machine *machine,
 					     union perf_event *event,
 					     struct perf_sample *sample __maybe_unused)
@@ -743,6 +749,11 @@ static int machine__process_ksymbol_register(struct machine *machine,
 		map->end = map->start + event->ksymbol.len;
 		maps__insert(&machine->kmaps, map);
 		dso__set_loaded(dso);
+
+		if (is_bpf_image(event->ksymbol.name)) {
+			dso->binary_type = DSO_BINARY_TYPE__BPF_IMAGE;
+			dso__set_long_name(dso, "", false);
+		}
 	}
 
 	sym = symbol__new(map->map_ip(map, map->start),
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 3b379b1296f1..e6caec4b6054 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1537,6 +1537,7 @@ static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod,
 		return true;
 
 	case DSO_BINARY_TYPE__BPF_PROG_INFO:
+	case DSO_BINARY_TYPE__BPF_IMAGE:
 	case DSO_BINARY_TYPE__NOT_FOUND:
 	default:
 		return false;
-- 
2.24.1

Re: [PATCH 01/18] x86/mm: Rename is_kernel_text to __is_kernel_text

[Song Liu]

On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The kbuild test robot reported compile issue on x86 in one of
> the following patches that adds <linux/kallsyms.h> include into
> <linux/bpf.h>, which is picked up by init_32.c object.
>
> The problem is that <linux/kallsyms.h> defines global function
> is_kernel_text which colides with the static function of the
> same name defined in init_32.c:
>
>   $ make ARCH=i386
>   ...
>   >> arch/x86/mm/init_32.c:241:19: error: redefinition of 'is_kernel_text'
>     static inline int is_kernel_text(unsigned long addr)
>                       ^~~~~~~~~~~~~~
>    In file included from include/linux/bpf.h:21:0,
>                     from include/linux/bpf-cgroup.h:5,
>                     from include/linux/cgroup-defs.h:22,
>                     from include/linux/cgroup.h:28,
>                     from include/linux/hugetlb.h:9,
>                     from arch/x86/mm/init_32.c:18:
>    include/linux/kallsyms.h:31:19: note: previous definition of 'is_kernel_text' was here
>     static inline int is_kernel_text(unsigned long addr)
>
> Renaming the init_32.c is_kernel_text function to __is_kernel_text.
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 02/18] bpf: Add bpf_trampoline_ name prefix for DECLARE_BPF_DISPATCHER

[Song Liu]

On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> From: Björn Töpel <bjorn.topel@intel.com>
>
> Adding bpf_trampoline_ name prefix for DECLARE_BPF_DISPATCHER,
> so all the dispatchers have the common name prefix.
>
> And also a small '_' cleanup for bpf_dispatcher_nopfunc function
> name.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 03/18] bpf: Add struct bpf_ksym

[Song Liu]

On Wed, Feb 26, 2020 at 5:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding 'struct bpf_ksym' object that will carry the
> kallsym information for bpf symbol. Adding the start
> and end address to begin with. It will be used by
> bpf_prog, bpf_trampoline, bpf_dispatcher.
>
> The symbol_start/symbol_end values were originally used
> to sort bpf_prog objects. For the address displayed in
> /proc/kallsyms we are using prog->bpf_func.
>
> I'm using the bpf_func for program symbol start instead
> of the symbol_start, because it makes no difference for
> sorting bpf_prog objects and we can use it directly as
> an address for display it in /proc/kallsyms.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 04/18] bpf: Add name to struct bpf_ksym

[Song Liu]

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

Re: [PATCH 05/18] bpf: Add lnode list node to struct bpf_ksym

[Song Liu]

On Wed, Feb 26, 2020 at 5:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding lnode list node to 'struct bpf_ksym' object,
> so the symbol itself can be chained and used in other
> objects like bpf_trampoline and bpf_dispatcher.
>
> Changing iterator to bpf_ksym in bpf_get_kallsym.
>
> The ksym->start is holding the prog->bpf_func value,
> so it's ok to use it in bpf_get_kallsym.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

nit: I think we should describe this as "move lnode list node to
struct bpf_ksym".

Re: [PATCH 06/18] bpf: Add bpf_ksym_tree tree

[Song Liu]

On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The bpf_tree is used both for kallsyms iterations and searching
> for exception tables of bpf programs, which is needed only for
> bpf programs.
>
> Adding bpf_ksym_tree that will hold symbols for all bpf_prog
> bpf_trampoline and bpf_dispatcher objects and keeping bpf_tree
> only for bpf_prog objects to keep it fast.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 07/18] bpf: Move bpf_tree add/del from bpf_prog_ksym_node_add/del

[Song Liu]

On Wed, Feb 26, 2020 at 5:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Moving bpf_tree add/del from bpf_prog_ksym_node_add/del,
> because it will be used (and renamed) in following patches
> for bpf_ksym objects. The bpf_tree is specific for bpf_prog
> objects.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 08/18] bpf: Separate kallsyms add/del functions

[Song Liu]

On Wed, Feb 26, 2020 at 5:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Moving bpf_prog_ksym_node_add/del to __bpf_ksym_add/del
> and changing the argument to 'struct bpf_ksym' object.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 09/18] bpf: Add bpf_ksym_add/del functions

[Song Liu]

On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding bpf_ksym_add/del functions as locked version
> for __bpf_ksym_add/del. It will be used in following
> patches for bpf_trampoline and bpf_dispatcher.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Song Liu]

On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> When bpf_prog is removed from kallsyms it's on the way
> out to be removed, so we don't care about lnode state.
>
> However the bpf_ksym_del will be used also by bpf_trampoline
> and bpf_dispatcher objects, which stay allocated even when
> they are not in kallsyms list, hence the lnode re-init.
>
> The list_del_rcu commentary states that we need to call
> synchronize_rcu, before we can change/re-init the list_head
> pointers.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 11/18] bpf: Rename bpf_tree to bpf_progs_tree

[Song Liu]

On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Renaming bpf_tree to bpf_progs_tree and bpf_tree_ops
> to bpf_progs_tree_ops to better capture the usage of
> the tree which is for the bpf_prog objects only.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 12/18] bpf: Add trampolines to kallsyms

[Song Liu]

On Wed, Feb 26, 2020 at 5:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding trampolines to kallsyms. It's displayed as
>   bpf_trampoline_<ID> [bpf]
>
> where ID is the BTF id of the trampoline function.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 13/18] bpf: Return error value in bpf_dispatcher_update

[Song Liu]

On Wed, Feb 26, 2020 at 5:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> We don't currently propagate error value from
> bpf_dispatcher_update function. This will be
> needed in following patch, that needs to update
> kallsyms based on the success of dispatcher
> update.
>
> Suggested-by: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 14/18] bpf: Add dispatchers to kallsyms

[Song Liu]

On Wed, Feb 26, 2020 at 5:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding dispatchers to kallsyms. It's displayed as
>   bpf_dispatcher_<NAME>
>
> where NAME is the name of dispatcher.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 15/18] bpf: Sort bpf kallsyms symbols

[Song Liu]

On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently we don't sort bpf_kallsyms and display symbols
> in proc/kallsyms as they come in via __bpf_ksym_add.
>
> Using the latch tree to get the next bpf_ksym object
> and insert the new symbol ahead of it.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 16/18] perf tools: Synthesize bpf_trampoline/dispatcher ksymbol event

[Song Liu]

On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Synthesize bpf images (trampolines/dispatchers) on start,
> as ksymbol events from /proc/kallsyms. Having this perf
> can recognize samples from those images and perf report
> and top shows them correctly.
>
> The rest of the ksymbol handling is already in place from
> for the bpf programs monitoring, so only the initial state
> was needed.
>
> perf report output:
>
>   # Overhead  Command     Shared Object                  Symbol
>
>     12.37%  test_progs  [kernel.vmlinux]                 [k] entry_SYSCALL_64
>     11.80%  test_progs  [kernel.vmlinux]                 [k] syscall_return_via_sysret
>      9.63%  test_progs  bpf_prog_bcf7977d3b93787c_prog2  [k] bpf_prog_bcf7977d3b93787c_prog2
>      6.90%  test_progs  bpf_trampoline_24456             [k] bpf_trampoline_24456
>      6.36%  test_progs  [kernel.vmlinux]                 [k] memcpy_erms
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 17/18] perf tools: Set ksymbol dso as loaded on arrival

[Song Liu]

On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> There's no special load action for ksymbol data on
> map__load/dso__load action, where the kernel is getting
> loaded. It only gets confused with kernel kallsyms/vmlinux
> load for bpf object, which fails and could mess up with
> the map.
>
> Disabling any further load of the map for ksymbol related dso/map.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 18/18] perf annotate: Add base support for bpf_image

[Song Liu]

On Wed, Feb 26, 2020 at 5:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding the DSO_BINARY_TYPE__BPF_IMAGE dso binary type
> to recognize bpf images that carry trampoline or dispatcher.
>
> Upcoming patches will add support to read the image data,
> store it within the BPF feature in perf.data and display
> it for annotation purposes.
>
> Currently we only display following message:
>
>   # ./perf annotate bpf_trampoline_24456 --stdio
>    Percent |      Source code & Disassembly of . for cycles (504  ...
>   --------------------------------------------------------------- ...
>            :       to be implemented
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH 12/18] bpf: Add trampolines to kallsyms

[Martin KaFai Lau]

On Wed, Feb 26, 2020 at 02:03:39PM +0100, Jiri Olsa wrote:
> Adding trampolines to kallsyms. It's displayed as
>   bpf_trampoline_<ID> [bpf]
> 
> where ID is the BTF id of the trampoline function.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h     |  3 +++
>  kernel/bpf/trampoline.c | 38 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 76934893bccf..c216af89254f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -502,6 +502,7 @@ struct bpf_trampoline {
>  	/* Executable image of trampoline */
>  	void *image;
>  	u64 selector;
> +	struct bpf_ksym ksym;
>  };
>  
>  #define BPF_DISPATCHER_MAX 48 /* Fits in 2048B */
> @@ -573,6 +574,8 @@ struct bpf_image {
>  #define BPF_IMAGE_SIZE (PAGE_SIZE - sizeof(struct bpf_image))
>  bool is_bpf_image_address(unsigned long address);
>  void *bpf_image_alloc(void);
> +void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym);
> +void bpf_image_ksym_del(struct bpf_ksym *ksym);
>  /* Called only from code, so there's no need for stubs. */
>  void bpf_ksym_add(struct bpf_ksym *ksym);
>  void bpf_ksym_del(struct bpf_ksym *ksym);
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 6b264a92064b..4b0ae976a6eb 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -5,6 +5,7 @@
>  #include <linux/filter.h>
>  #include <linux/ftrace.h>
>  #include <linux/rbtree_latch.h>
> +#include <linux/perf_event.h>
>  
>  /* dummy _ops. The verifier will operate on target program's ops. */
>  const struct bpf_verifier_ops bpf_extension_verifier_ops = {
> @@ -96,6 +97,22 @@ bool is_bpf_image_address(unsigned long addr)
>  	return ret;
>  }
>  
> +void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym)
> +{
> +	ksym->start = (unsigned long) data;
> +	ksym->end = ksym->start + BPF_IMAGE_SIZE;
> +	bpf_ksym_add(ksym);
> +	perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF, ksym->start,
> +			   BPF_IMAGE_SIZE, false, ksym->name);
> +}
> +
> +void bpf_image_ksym_del(struct bpf_ksym *ksym)
> +{
> +	bpf_ksym_del(ksym);
> +	perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF, ksym->start,
> +			   BPF_IMAGE_SIZE, true, ksym->name);
> +}
> +
>  struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
>  {
>  	struct bpf_trampoline *tr;
> @@ -131,6 +148,7 @@ struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
>  	for (i = 0; i < BPF_TRAMP_MAX; i++)
>  		INIT_HLIST_HEAD(&tr->progs_hlist[i]);
>  	tr->image = image;
> +	INIT_LIST_HEAD_RCU(&tr->ksym.lnode);
>  out:
>  	mutex_unlock(&trampoline_mutex);
>  	return tr;
> @@ -267,6 +285,14 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(enum bpf_attach_type t)
>  	}
>  }
>  
> +static void bpf_trampoline_ksym_add(struct bpf_trampoline *tr)
> +{
> +	struct bpf_ksym *ksym = &tr->ksym;
> +
> +	snprintf(ksym->name, KSYM_NAME_LEN, "bpf_trampoline_%llu", tr->key);
Do you have plan to support struct_ops which is also using
trampoline (in bpf_struct_ops_map_update_elem())?
Any idea on the name? bpf_struct_ops_<map_id>? 

> +	bpf_image_ksym_add(tr->image, ksym);
> +}
> +

Re: [PATCH 12/18] bpf: Add trampolines to kallsyms

[Jiri Olsa]

On Wed, Feb 26, 2020 at 10:26:40PM -0800, Martin KaFai Lau wrote:

SNIP

> > +	ksym->start = (unsigned long) data;
> > +	ksym->end = ksym->start + BPF_IMAGE_SIZE;
> > +	bpf_ksym_add(ksym);
> > +	perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF, ksym->start,
> > +			   BPF_IMAGE_SIZE, false, ksym->name);
> > +}
> > +
> > +void bpf_image_ksym_del(struct bpf_ksym *ksym)
> > +{
> > +	bpf_ksym_del(ksym);
> > +	perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF, ksym->start,
> > +			   BPF_IMAGE_SIZE, true, ksym->name);
> > +}
> > +
> >  struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
> >  {
> >  	struct bpf_trampoline *tr;
> > @@ -131,6 +148,7 @@ struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
> >  	for (i = 0; i < BPF_TRAMP_MAX; i++)
> >  		INIT_HLIST_HEAD(&tr->progs_hlist[i]);
> >  	tr->image = image;
> > +	INIT_LIST_HEAD_RCU(&tr->ksym.lnode);
> >  out:
> >  	mutex_unlock(&trampoline_mutex);
> >  	return tr;
> > @@ -267,6 +285,14 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(enum bpf_attach_type t)
> >  	}
> >  }
> >  
> > +static void bpf_trampoline_ksym_add(struct bpf_trampoline *tr)
> > +{
> > +	struct bpf_ksym *ksym = &tr->ksym;
> > +
> > +	snprintf(ksym->name, KSYM_NAME_LEN, "bpf_trampoline_%llu", tr->key);
> Do you have plan to support struct_ops which is also using
> trampoline (in bpf_struct_ops_map_update_elem())?
> Any idea on the name? bpf_struct_ops_<map_id>? 

right, I was wondering we should also do that,
I'll check on it

jirka

Re: [PATCH 05/18] bpf: Add lnode list node to struct bpf_ksym

[Jiri Olsa]

On Wed, Feb 26, 2020 at 02:51:14PM -0800, Song Liu wrote:
> On Wed, Feb 26, 2020 at 5:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding lnode list node to 'struct bpf_ksym' object,
> > so the symbol itself can be chained and used in other
> > objects like bpf_trampoline and bpf_dispatcher.
> >
> > Changing iterator to bpf_ksym in bpf_get_kallsym.
> >
> > The ksym->start is holding the prog->bpf_func value,
> > so it's ok to use it in bpf_get_kallsym.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> 
> nit: I think we should describe this as "move lnode list node to
> struct bpf_ksym".

true, will change

thanks,
jirka

Re: [PATCH 04/18] bpf: Add name to struct bpf_ksym

[Jiri Olsa]

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

Re: [PATCH 04/18] bpf: Add name to struct bpf_ksym

[Song Liu]

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>

Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Alexei Starovoitov]

On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote:
> When bpf_prog is removed from kallsyms it's on the way
> out to be removed, so we don't care about lnode state.
> 
> However the bpf_ksym_del will be used also by bpf_trampoline
> and bpf_dispatcher objects, which stay allocated even when
> they are not in kallsyms list, hence the lnode re-init.
> 
> The list_del_rcu commentary states that we need to call
> synchronize_rcu, before we can change/re-init the list_head
> pointers.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index c95424fc53de..1af2109b45c7 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym)
>  	spin_lock_bh(&bpf_lock);
>  	__bpf_ksym_del(ksym);
>  	spin_unlock_bh(&bpf_lock);
> +
> +	/*
> +	 * As explained in list_del_rcu, We must call synchronize_rcu
> +	 * before changing list_head pointers.
> +	 */
> +	synchronize_rcu();
> +	INIT_LIST_HEAD_RCU(&ksym->lnode);

I don't understand what this is for.
The comment made it even more confusing.
What kind of ksym reuse are you expecting?

Looking at trampoline and dispatcher patches I think cnt == 0
condition is unnecessary. Just add them to ksym at creation time
and remove from ksym at destroy. Both are executable code sections.
Though RIP should never point into them while there are no progs
I think it's better to keep them in ksym always.
Imagine sw race conditions in destruction. CPU bugs. What not.

In patch 3 the name
bpf_get_prog_addr_region(const struct bpf_prog *prog)
became wrong and 'const' pointer makes it even more misleading.
The function is not getting prog addr. It's setting ksym's addr.
I think it should be called:
bpf_ksym_set_addr(struct bpf_ksym *ksym);
__always_inline should be removed too.

Similar in patch 4:
static void bpf_get_prog_name(const struct bpf_prog *prog)
also is wrong for the same reasons.
It probably should be:
static void bpf_ksym_set_name(struct bpf_ksym *ksym);

I'm still not confortable with patch 15 sorting bit.
next = rb_next(&ksym->tnode.node[0]);
if (next)
is too tricky for me. I cannot wrap my head yet.
Since user space doesn't rely on sorted order could you drop it?

Do patches 16-18 strongly depend on patches 1-15 ?
We can take them via bpf-next tree. No problem. Just need Arnaldo's ack.

Overall looks great. All around important work.
Please address above and respin. I would like to land it soon.

Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Jiri Olsa]

On Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov wrote:
> On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote:
> > When bpf_prog is removed from kallsyms it's on the way
> > out to be removed, so we don't care about lnode state.
> > 
> > However the bpf_ksym_del will be used also by bpf_trampoline
> > and bpf_dispatcher objects, which stay allocated even when
> > they are not in kallsyms list, hence the lnode re-init.
> > 
> > The list_del_rcu commentary states that we need to call
> > synchronize_rcu, before we can change/re-init the list_head
> > pointers.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index c95424fc53de..1af2109b45c7 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym)
> >  	spin_lock_bh(&bpf_lock);
> >  	__bpf_ksym_del(ksym);
> >  	spin_unlock_bh(&bpf_lock);
> > +
> > +	/*
> > +	 * As explained in list_del_rcu, We must call synchronize_rcu
> > +	 * before changing list_head pointers.
> > +	 */
> > +	synchronize_rcu();
> > +	INIT_LIST_HEAD_RCU(&ksym->lnode);
> 
> I don't understand what this is for.
> The comment made it even more confusing.
> What kind of ksym reuse are you expecting?
> 
> Looking at trampoline and dispatcher patches I think cnt == 0
> condition is unnecessary. Just add them to ksym at creation time
> and remove from ksym at destroy. Both are executable code sections.
> Though RIP should never point into them while there are no progs
> I think it's better to keep them in ksym always.
> Imagine sw race conditions in destruction. CPU bugs. What not.

aah ok, that should also solve your first question,
because the code above won't be needed anymore

I wish I read this comment before I prepared elabored ascii/code
picture of why the code above is needed ;-))

> 
> In patch 3 the name
> bpf_get_prog_addr_region(const struct bpf_prog *prog)
> became wrong and 'const' pointer makes it even more misleading.
> The function is not getting prog addr. It's setting ksym's addr.
> I think it should be called:
> bpf_ksym_set_addr(struct bpf_ksym *ksym);
> __always_inline should be removed too.

ok, will change

> 
> Similar in patch 4:
> static void bpf_get_prog_name(const struct bpf_prog *prog)
> also is wrong for the same reasons.
> It probably should be:
> static void bpf_ksym_set_name(struct bpf_ksym *ksym);

ok

> 
> I'm still not confortable with patch 15 sorting bit.
> next = rb_next(&ksym->tnode.node[0]);
> if (next)
> is too tricky for me. I cannot wrap my head yet.
> Since user space doesn't rely on sorted order could you drop it?

yes, as I said I only added it because I liked how simple it
turned out to be

> 
> Do patches 16-18 strongly depend on patches 1-15 ?
> We can take them via bpf-next tree. No problem. Just need Arnaldo's ack.

actualy there're some changes on the list from this week, that touch
the same code, so we might need to take them through Arnaldo's code

I'l double check

> 
> Overall looks great. All around important work.
> Please address above and respin. I would like to land it soon.
> 

will respin soon, thanks for review

jirka

Re: [PATCH 16/18] perf tools: Synthesize bpf_trampoline/dispatcher ksymbol event

[Arnaldo Carvalho de Melo]

Em Wed, Feb 26, 2020 at 02:03:43PM +0100, Jiri Olsa escreveu:
> Synthesize bpf images (trampolines/dispatchers) on start,
> as ksymbol events from /proc/kallsyms. Having this perf
> can recognize samples from those images and perf report
> and top shows them correctly.
> 
> The rest of the ksymbol handling is already in place from
> for the bpf programs monitoring, so only the initial state
> was needed.

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

But at some point we should try and consolidate all those
kallsym__parse() calls we have in tools/perf/ not to do it that many
times, see below _before_ this patch:

[root@five ~]# perf probe -x ~/bin/perf kallsyms__parse
Added new event:
  probe_perf:kallsyms__parse (on kallsyms__parse in /home/acme/bin/perf)

You can now use it in all perf tools, such as:

	perf record -e probe_perf:kallsyms__parse -aR sleep 1

[root@five ~]# perf trace -e probe_perf:kallsyms__parse/max-stack=8/ -- perf record sleep 1
     0.000 perf/6444 probe_perf:kallsyms__parse(__probe_ip: 4904384)
                                       kallsyms__parse (/home/acme/bin/perf)
                                       machine__get_running_kernel_start (/home/acme/bin/perf)
                                       machine__create_kernel_maps (/home/acme/bin/perf)
                                       perf_session__new (/home/acme/bin/perf)
                                       cmd_record (/home/acme/bin/perf)
                                       run_builtin (/home/acme/bin/perf)
                                       main (/home/acme/bin/perf)
                                       __libc_start_main (/usr/lib64/libc-2.30.so)
     0.124 perf/6444 probe_perf:kallsyms__parse(__probe_ip: 4904384)
                                       kallsyms__parse (/home/acme/bin/perf)
                                       machine__get_running_kernel_start (/home/acme/bin/perf)
                                       machine__create_kernel_maps (/home/acme/bin/perf)
                                       perf_session__new (/home/acme/bin/perf)
                                       cmd_record (/home/acme/bin/perf)
                                       run_builtin (/home/acme/bin/perf)
                                       main (/home/acme/bin/perf)
                                       __libc_start_main (/usr/lib64/libc-2.30.so)
    15.489 perf/6444 probe_perf:kallsyms__parse(__probe_ip: 4904384)
                                       kallsyms__parse (/home/acme/bin/perf)
                                       machine__create_kernel_maps (/home/acme/bin/perf)
                                       perf_session__new (/home/acme/bin/perf)
                                       cmd_record (/home/acme/bin/perf)
                                       run_builtin (/home/acme/bin/perf)
                                       main (/home/acme/bin/perf)
                                       __libc_start_main (/usr/lib64/libc-2.30.so)
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.019 MB perf.data (7 samples) ]
[root@five ~]#

- Arnaldo

 
> perf report output:
> 
>   # Overhead  Command     Shared Object                  Symbol
> 
>     12.37%  test_progs  [kernel.vmlinux]                 [k] entry_SYSCALL_64
>     11.80%  test_progs  [kernel.vmlinux]                 [k] syscall_return_via_sysret
>      9.63%  test_progs  bpf_prog_bcf7977d3b93787c_prog2  [k] bpf_prog_bcf7977d3b93787c_prog2
>      6.90%  test_progs  bpf_trampoline_24456             [k] bpf_trampoline_24456
>      6.36%  test_progs  [kernel.vmlinux]                 [k] memcpy_erms
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/bpf-event.c | 98 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index a3207d900339..120ec547ae75 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
> @@ -6,6 +6,9 @@
>  #include <bpf/libbpf.h>
>  #include <linux/btf.h>
>  #include <linux/err.h>
> +#include <linux/string.h>
> +#include <internal/lib.h>
> +#include <symbol/kallsyms.h>
>  #include "bpf-event.h"
>  #include "debug.h"
>  #include "dso.h"
> @@ -290,11 +293,87 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
>  	return err ? -1 : 0;
>  }
>  
> +struct kallsyms_parse {
> +	union perf_event	*event;
> +	perf_event__handler_t	 process;
> +	struct machine		*machine;
> +	struct perf_tool	*tool;
> +};
> +
> +static int
> +process_bpf_image(char *name, u64 addr, struct kallsyms_parse *data)
> +{
> +	struct machine *machine = data->machine;
> +	union perf_event *event = data->event;
> +	struct perf_record_ksymbol *ksymbol;
> +	u32 size;
> +
> +	ksymbol = &event->ksymbol;
> +
> +	/*
> +	 * The bpf image (trampoline/dispatcher) size is aligned to
> +	 * page, while it starts little bit after the page boundary.
> +	 */
> +	size = page_size - (addr - PERF_ALIGN(addr, page_size));
> +
> +	*ksymbol = (struct perf_record_ksymbol) {
> +		.header = {
> +			.type = PERF_RECORD_KSYMBOL,
> +			.size = offsetof(struct perf_record_ksymbol, name),
> +		},
> +		.addr      = addr,
> +		.len       = size,
> +		.ksym_type = PERF_RECORD_KSYMBOL_TYPE_BPF,
> +		.flags     = 0,
> +	};
> +
> +	strncpy(ksymbol->name, name, KSYM_NAME_LEN);
> +	ksymbol->header.size += PERF_ALIGN(strlen(name) + 1, sizeof(u64));
> +	memset((void *) event + event->header.size, 0, machine->id_hdr_size);
> +	event->header.size += machine->id_hdr_size;
> +
> +	return perf_tool__process_synth_event(data->tool, event, machine,
> +					      data->process);
> +}
> +
> +static int
> +kallsyms_process_symbol(void *data, const char *_name,
> +			char type __maybe_unused, u64 start)
> +{
> +	char *module, *name;
> +	unsigned long id;
> +	int err = 0;
> +
> +	module = strchr(_name, '\t');
> +	if (!module)
> +		return 0;
> +
> +	/* We are going after [bpf] module ... */
> +	if (strcmp(module + 1, "[bpf]"))
> +		return 0;
> +
> +	name = memdup(_name, (module - _name) + 1);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	name[module - _name] = 0;
> +
> +	/* .. and only for trampolines and dispatchers */
> +	if ((sscanf(name, "bpf_trampoline_%lu", &id) == 1) ||
> +	    (sscanf(name, "bpf_dispatcher_%lu", &id) == 1))
> +		err = process_bpf_image(name, start, data);
> +
> +	free(name);
> +	return err;
> +}
> +
>  int perf_event__synthesize_bpf_events(struct perf_session *session,
>  				      perf_event__handler_t process,
>  				      struct machine *machine,
>  				      struct record_opts *opts)
>  {
> +	const char *kallsyms_filename = "/proc/kallsyms";
> +	struct kallsyms_parse arg;
>  	union perf_event *event;
>  	__u32 id = 0;
>  	int err;
> @@ -303,6 +382,8 @@ int perf_event__synthesize_bpf_events(struct perf_session *session,
>  	event = malloc(sizeof(event->bpf) + KSYM_NAME_LEN + machine->id_hdr_size);
>  	if (!event)
>  		return -1;
> +
> +	/* Synthesize all the bpf programs in system. */
>  	while (true) {
>  		err = bpf_prog_get_next_id(id, &id);
>  		if (err) {
> @@ -335,6 +416,23 @@ int perf_event__synthesize_bpf_events(struct perf_session *session,
>  			break;
>  		}
>  	}
> +
> +	/* Synthesize all the bpf images - trampolines/dispatchers. */
> +	if (symbol_conf.kallsyms_name != NULL)
> +		kallsyms_filename = symbol_conf.kallsyms_name;
> +
> +	arg = (struct kallsyms_parse) {
> +		.event   = event,
> +		.process = process,
> +		.machine = machine,
> +		.tool    = session->tool,
> +	};
> +
> +	if (kallsyms__parse(kallsyms_filename, &arg, kallsyms_process_symbol)) {
> +		pr_err("%s: failed to synthesize bpf images: %s\n",
> +		       __func__, strerror(errno));
> +	}
> +
>  	free(event);
>  	return err;
>  }
> -- 
> 2.24.1

Re: [PATCH 17/18] perf tools: Set ksymbol dso as loaded on arrival

[Arnaldo Carvalho de Melo]

Em Wed, Feb 26, 2020 at 02:03:44PM +0100, Jiri Olsa escreveu:
> There's no special load action for ksymbol data on
> map__load/dso__load action, where the kernel is getting
> loaded. It only gets confused with kernel kallsyms/vmlinux
> load for bpf object, which fails and could mess up with
> the map.
> 
> Disabling any further load of the map for ksymbol related dso/map.

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/machine.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index fb5c2cd44d30..463ada5117f8 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -742,6 +742,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  		map->start = event->ksymbol.addr;
>  		map->end = map->start + event->ksymbol.len;
>  		maps__insert(&machine->kmaps, map);
> +		dso__set_loaded(dso);
>  	}
>  
>  	sym = symbol__new(map->map_ip(map, map->start),
> -- 
> 2.24.1

Re: [PATCH 18/18] perf annotate: Add base support for bpf_image

[Arnaldo Carvalho de Melo]

Em Wed, Feb 26, 2020 at 02:03:45PM +0100, Jiri Olsa escreveu:
> Adding the DSO_BINARY_TYPE__BPF_IMAGE dso binary type
> to recognize bpf images that carry trampoline or dispatcher.
> 
> Upcoming patches will add support to read the image data,
> store it within the BPF feature in perf.data and display
> it for annotation purposes.
> 
> Currently we only display following message:

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
 
>   # ./perf annotate bpf_trampoline_24456 --stdio
>    Percent |      Source code & Disassembly of . for cycles (504  ...
>   --------------------------------------------------------------- ...
>            :       to be implemented
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/annotate.c | 20 ++++++++++++++++++++
>  tools/perf/util/dso.c      |  1 +
>  tools/perf/util/dso.h      |  1 +
>  tools/perf/util/machine.c  | 11 +++++++++++
>  tools/perf/util/symbol.c   |  1 +
>  5 files changed, 34 insertions(+)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index ca73fb74ad03..d9e606e11936 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1843,6 +1843,24 @@ static int symbol__disassemble_bpf(struct symbol *sym __maybe_unused,
>  }
>  #endif // defined(HAVE_LIBBFD_SUPPORT) && defined(HAVE_LIBBPF_SUPPORT)
>  
> +static int
> +symbol__disassemble_bpf_image(struct symbol *sym,
> +			      struct annotate_args *args)
> +{
> +	struct annotation *notes = symbol__annotation(sym);
> +	struct disasm_line *dl;
> +
> +	args->offset = -1;
> +	args->line = strdup("to be implemented");
> +	args->line_nr = 0;
> +	dl = disasm_line__new(args);
> +	if (dl)
> +		annotation_line__add(&dl->al, &notes->src->source);
> +
> +	free(args->line);
> +	return 0;
> +}
> +
>  /*
>   * Possibly create a new version of line with tabs expanded. Returns the
>   * existing or new line, storage is updated if a new line is allocated. If
> @@ -1942,6 +1960,8 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
>  
>  	if (dso->binary_type == DSO_BINARY_TYPE__BPF_PROG_INFO) {
>  		return symbol__disassemble_bpf(sym, args);
> +	} else if (dso->binary_type == DSO_BINARY_TYPE__BPF_IMAGE) {
> +		return symbol__disassemble_bpf_image(sym, args);
>  	} else if (dso__is_kcore(dso)) {
>  		kce.kcore_filename = symfs_filename;
>  		kce.addr = map__rip_2objdump(map, sym->start);
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 91f21239608b..f338990e0fe6 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -191,6 +191,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
>  	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
>  	case DSO_BINARY_TYPE__JAVA_JIT:
>  	case DSO_BINARY_TYPE__BPF_PROG_INFO:
> +	case DSO_BINARY_TYPE__BPF_IMAGE:
>  	case DSO_BINARY_TYPE__NOT_FOUND:
>  		ret = -1;
>  		break;
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 2db64b79617a..9553a1fd9e8a 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -40,6 +40,7 @@ enum dso_binary_type {
>  	DSO_BINARY_TYPE__GUEST_KCORE,
>  	DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO,
>  	DSO_BINARY_TYPE__BPF_PROG_INFO,
> +	DSO_BINARY_TYPE__BPF_IMAGE,
>  	DSO_BINARY_TYPE__NOT_FOUND,
>  };
>  
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 463ada5117f8..372ed147bed5 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -719,6 +719,12 @@ int machine__process_switch_event(struct machine *machine __maybe_unused,
>  	return 0;
>  }
>  
> +static int is_bpf_image(const char *name)
> +{
> +	return strncmp(name, "bpf_trampoline_", sizeof("bpf_trampoline_") - 1) ||
> +	       strncmp(name, "bpf_dispatcher_", sizeof("bpf_dispatcher_") - 1);
> +}
> +
>  static int machine__process_ksymbol_register(struct machine *machine,
>  					     union perf_event *event,
>  					     struct perf_sample *sample __maybe_unused)
> @@ -743,6 +749,11 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  		map->end = map->start + event->ksymbol.len;
>  		maps__insert(&machine->kmaps, map);
>  		dso__set_loaded(dso);
> +
> +		if (is_bpf_image(event->ksymbol.name)) {
> +			dso->binary_type = DSO_BINARY_TYPE__BPF_IMAGE;
> +			dso__set_long_name(dso, "", false);
> +		}
>  	}
>  
>  	sym = symbol__new(map->map_ip(map, map->start),
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 3b379b1296f1..e6caec4b6054 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1537,6 +1537,7 @@ static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod,
>  		return true;
>  
>  	case DSO_BINARY_TYPE__BPF_PROG_INFO:
> +	case DSO_BINARY_TYPE__BPF_IMAGE:
>  	case DSO_BINARY_TYPE__NOT_FOUND:
>  	default:
>  		return false;
> -- 
> 2.24.1

Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Arnaldo Carvalho de Melo]

Em Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov escreveu:
> On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote:
> > When bpf_prog is removed from kallsyms it's on the way
> > out to be removed, so we don't care about lnode state.
> > 
> > However the bpf_ksym_del will be used also by bpf_trampoline
> > and bpf_dispatcher objects, which stay allocated even when
> > they are not in kallsyms list, hence the lnode re-init.
> > 
> > The list_del_rcu commentary states that we need to call
> > synchronize_rcu, before we can change/re-init the list_head
> > pointers.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index c95424fc53de..1af2109b45c7 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym)
> >  	spin_lock_bh(&bpf_lock);
> >  	__bpf_ksym_del(ksym);
> >  	spin_unlock_bh(&bpf_lock);
> > +
> > +	/*
> > +	 * As explained in list_del_rcu, We must call synchronize_rcu
> > +	 * before changing list_head pointers.
> > +	 */
> > +	synchronize_rcu();
> > +	INIT_LIST_HEAD_RCU(&ksym->lnode);
> 
> I don't understand what this is for.
> The comment made it even more confusing.
> What kind of ksym reuse are you expecting?
> 
> Looking at trampoline and dispatcher patches I think cnt == 0
> condition is unnecessary. Just add them to ksym at creation time
> and remove from ksym at destroy. Both are executable code sections.
> Though RIP should never point into them while there are no progs
> I think it's better to keep them in ksym always.
> Imagine sw race conditions in destruction. CPU bugs. What not.
> 
> In patch 3 the name
> bpf_get_prog_addr_region(const struct bpf_prog *prog)
> became wrong and 'const' pointer makes it even more misleading.
> The function is not getting prog addr. It's setting ksym's addr.
> I think it should be called:
> bpf_ksym_set_addr(struct bpf_ksym *ksym);
> __always_inline should be removed too.
> 
> Similar in patch 4:
> static void bpf_get_prog_name(const struct bpf_prog *prog)
> also is wrong for the same reasons.
> It probably should be:
> static void bpf_ksym_set_name(struct bpf_ksym *ksym);
> 
> I'm still not confortable with patch 15 sorting bit.
> next = rb_next(&ksym->tnode.node[0]);
> if (next)
> is too tricky for me. I cannot wrap my head yet.
> Since user space doesn't rely on sorted order could you drop it?
> 
> Do patches 16-18 strongly depend on patches 1-15 ?
> We can take them via bpf-next tree. No problem. Just need Arnaldo's ack.

No problems, sent the acks, we can sort out problems later, but from the
top of my mind I can't antecipate any,

- Arnaldo
 
> Overall looks great. All around important work.
> Please address above and respin. I would like to land it soon.

Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Arnaldo Carvalho de Melo]

Em Fri, Feb 28, 2020 at 01:17:08PM +0100, Jiri Olsa escreveu:
> On Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov wrote:
> > On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote:
> > Do patches 16-18 strongly depend on patches 1-15 ?
> > We can take them via bpf-next tree. No problem. Just need Arnaldo's ack.
> 
> actualy there're some changes on the list from this week, that touch
> the same code, so we might need to take them through Arnaldo's code

Ravi's patches, yeah, will push them via perf/urgent now, since those
are fixes, but I guess those won't clash...
 
> I'l double check

Please
 
> > 
> > Overall looks great. All around important work.
> > Please address above and respin. I would like to land it soon.
> > 
> 
> will respin soon, thanks for review
> 
> jirka

Re: [PATCH 04/18] bpf: Add name to struct bpf_ksym

[Jiri Olsa]

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 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Jiri Olsa]

When bpf_prog is removed from kallsyms it's on the way
out to be removed, so we don't care about lnode state.

However the bpf_ksym_del will be used also by bpf_trampoline
and bpf_dispatcher objects, which stay allocated even when
they are not in kallsyms list, hence the lnode re-init.

The list_del_rcu commentary states that we need to call
synchronize_rcu, before we can change/re-init the list_head
pointers.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 739bef60d868..a0feba447e5d 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -676,6 +676,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym)
 	spin_lock_bh(&bpf_lock);
 	__bpf_ksym_del(ksym);
 	spin_unlock_bh(&bpf_lock);
+
+	/*
+	 * As explained in list_del_rcu, We must call synchronize_rcu
+	 * before changing list_head pointers.
+	 */
+	synchronize_rcu();
+	INIT_LIST_HEAD_RCU(&ksym->lnode);
 }
 
 static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp)
-- 
2.24.1