| Message ID | 1448372181-151723-5-git-send-email-wangnan0@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
On 2015/11/27 4:56, Arnaldo Carvalho de Melo wrote: > Em Tue, Nov 24, 2015 at 01:36:09PM +0000, Wang Nan escreveu: [SNIP] >> + } >> return 0; >> } >> >> @@ -688,37 +707,15 @@ static int >> bpf_object__create_maps(struct bpf_object *obj) >> { >> unsigned int i; >> - size_t nr_maps; >> - int *pfd; >> - >> - nr_maps = obj->maps_buf_sz / sizeof(struct bpf_map_def); >> - if (!obj->maps_buf || !nr_maps) { >> - pr_debug("don't need create maps for %s\n", >> - obj->path); >> - return 0; >> - } >> - >> - obj->map_fds = malloc(sizeof(int) * nr_maps); > perhaps calloc? This line is being removed, so you'll still see this in my next version... > >> - if (!obj->map_fds) { >> - pr_warning("realloc perf_bpf_map_fds failed\n"); >> - return -ENOMEM; >> - } >> - obj->nr_map_fds = nr_maps; >> >> - /* fill all fd with -1 */ >> - memset(obj->map_fds, -1, sizeof(int) * nr_maps); >> + for (i = 0; i < obj->nr_maps; i++) { >> + struct bpf_map_def *def = &obj->maps[i].def; >> + int *pfd = &obj->maps[i].fd; >> >> - pfd = obj->map_fds; >> - for (i = 0; i < nr_maps; i++) { >> - struct bpf_map_def def; >> - >> - def = *(struct bpf_map_def *)(obj->maps_buf + >> - i * sizeof(struct bpf_map_def)); >> - >> - *pfd = bpf_create_map(def.type, >> - def.key_size, >> - def.value_size, >> - def.max_entries); >> + *pfd = bpf_create_map(def->type, >> + def->key_size, >> + def->value_size, >> + def->max_entries); >> if (*pfd < 0) { >> size_t j; >> int err = *pfd; >> @@ -726,22 +723,17 @@ bpf_object__create_maps(struct bpf_object *obj) >> pr_warning("failed to create map: %s\n", >> strerror(errno)); >> for (j = 0; j < i; j++) >> - zclose(obj->map_fds[j]); >> - obj->nr_map_fds = 0; >> - zfree(&obj->map_fds); >> + zclose(obj->maps[j].fd); >> return err; >> } >> pr_debug("create map: fd=%d\n", *pfd); >> - pfd++; >> } >> >> - zfree(&obj->maps_buf); >> - obj->maps_buf_sz = 0; >> return 0; >> } >> >> static int >> -bpf_program__relocate(struct bpf_program *prog, int *map_fds) >> +bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj) >> { >> int i; >> >> @@ -761,7 +753,7 @@ bpf_program__relocate(struct bpf_program *prog, int *map_fds) >> return -LIBBPF_ERRNO__RELOC; >> } >> insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD; >> - insns[insn_idx].imm = map_fds[map_idx]; >> + insns[insn_idx].imm = obj->maps[map_idx].fd; >> } >> >> zfree(&prog->reloc_desc); >> @@ -780,7 +772,7 @@ bpf_object__relocate(struct bpf_object *obj) >> for (i = 0; i < obj->nr_programs; i++) { >> prog = &obj->programs[i]; >> >> - err = bpf_program__relocate(prog, obj->map_fds); >> + err = bpf_program__relocate(prog, obj); >> if (err) { >> pr_warning("failed to relocate '%s'\n", >> prog->section_name); >> @@ -804,8 +796,7 @@ static int bpf_object__collect_reloc(struct bpf_object *obj) >> Elf_Data *data = obj->efile.reloc[i].data; >> int idx = shdr->sh_info; >> struct bpf_program *prog; >> - size_t nr_maps = obj->maps_buf_sz / >> - sizeof(struct bpf_map_def); >> + size_t nr_maps = obj->nr_maps; >> >> if (shdr->sh_type != SHT_REL) { >> pr_warning("internal error at %d\n", __LINE__); >> @@ -1050,10 +1041,8 @@ int bpf_object__unload(struct bpf_object *obj) >> if (!obj) >> return -EINVAL; >> >> - for (i = 0; i < obj->nr_map_fds; i++) >> - zclose(obj->map_fds[i]); >> - zfree(&obj->map_fds); >> - obj->nr_map_fds = 0; >> + for (i = 0; i < obj->nr_maps; i++) >> + zclose(obj->maps[i].fd); >> >> for (i = 0; i < obj->nr_programs; i++) >> bpf_program__unload(&obj->programs[i]); >> @@ -1096,7 +1085,15 @@ void bpf_object__close(struct bpf_object *obj) >> bpf_object__elf_finish(obj); >> bpf_object__unload(obj); >> >> - zfree(&obj->maps_buf); >> + for (i = 0; i < obj->nr_maps; i++) { >> + if (obj->maps[i].clear_priv) >> + obj->maps[i].clear_priv(&obj->maps[i], >> + obj->maps[i].priv); >> + obj->maps[i].priv = NULL; >> + obj->maps[i].clear_priv = NULL; >> + } >> + zfree(&obj->maps); >> + obj->nr_maps = 0; >> >> if (obj->programs && obj->nr_programs) { >> for (i = 0; i < obj->nr_programs; i++) >> @@ -1251,3 +1248,72 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n) >> >> return fd; >> } >> + >> +int bpf_map__get_fd(struct bpf_map *map) >> +{ >> + if (!map) >> + return -EINVAL; >> + >> + return map->fd; >> +} >> + >> +int bpf_map__get_def(struct bpf_map *map, struct bpf_map_def *pdef) >> +{ >> + if (!map || !pdef) >> + return -EINVAL; >> + >> + *pdef = map->def; >> + return 0; >> +} >> + >> +int bpf_map__set_private(struct bpf_map *map, void *priv, >> + bpf_map_clear_priv_t clear_priv) >> +{ >> + if (!map) >> + return -EINVAL; >> + >> + if (map->priv) { >> + if (map->clear_priv) >> + map->clear_priv(map, map->priv); >> + } >> + >> + map->priv = priv; >> + map->clear_priv = clear_priv; >> + return 0; >> +} >> + >> +int bpf_map__get_private(struct bpf_map *map, void **ppriv) >> +{ >> + if (!map) >> + return -EINVAL; >> + >> + if (ppriv) >> + *ppriv = map->priv; >> + return 0; >> +} >> + >> +struct bpf_map * >> +bpf_map__next(struct bpf_map *prev, struct bpf_object *obj) >> +{ >> + size_t idx; >> + struct bpf_map *s, *e; >> + >> + if (!obj || !obj->maps) >> + return NULL; >> + >> + s = obj->maps; >> + e = obj->maps + obj->nr_maps; >> + >> + if (prev == NULL) >> + return s; >> + >> + if ((prev < s) || (prev >= e)) { >> + pr_warning("error: map handler doesn't belong to object\n"); > I wonder if this shouldn't be made pr_debug, and as well have a function > prefix, otherwise we may think this is related to some other kind of > map, so I suggest: > pr_debug("%s: error: map handler doesn't belong to object\n", __func__); > > Or at least: > > pr_debug("BPF error: map handler doesn't belong to object\n"); We always have a libbpf prefix before debug info from libbpf. Please see static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr; static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr; static __printf(1, 2) libbpf_print_fn_t __pr_debug; #define __pr(func, fmt, ...) \ do { \ if ((func)) \ (func)("libbpf: " fmt, ##__VA_ARGS__); \ } while (0) #define pr_warning(fmt, ...) __pr(__pr_warning, fmt, ##__VA_ARGS__) #define pr_info(fmt, ...) __pr(__pr_info, fmt, ##__VA_ARGS__) #define pr_debug(fmt, ...) __pr(__pr_debug, fmt, ##__VA_ARGS__) We allow __pr_{warning,info,debug} be overwritten but the 'libbpf:' prefix is always there. Also, don't forget in perf's context libbpf is muted. >> + return NULL; >> + } >> + >> + idx = (prev - obj->maps) + 1; >> + if (idx >= obj->nr_maps) >> + return NULL; >> + return &obj->maps[idx]; >> +} >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h >> index 949df4b..709d2fa 100644 >> --- a/tools/lib/bpf/libbpf.h >> +++ b/tools/lib/bpf/libbpf.h >> @@ -165,4 +165,25 @@ struct bpf_map_def { >> unsigned int max_entries; >> }; >> >> +/* >> + * There is another 'struct bpf_map' in include/linux/map.h. However, >> + * it is not a uapi header so no need to consider name confliction. > s/confliction/conflict/g > > But I would use "name clash". Good word. Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 2015/11/27 14:16, Wangnan (F) wrote: > > > On 2015/11/27 4:56, Arnaldo Carvalho de Melo wrote: >> Em Tue, Nov 24, 2015 at 01:36:09PM +0000, Wang Nan escreveu: > > [SNIP] [SNIP] >>> + >>> + if ((prev < s) || (prev >= e)) { >>> + pr_warning("error: map handler doesn't belong to object\n"); >> I wonder if this shouldn't be made pr_debug, and as well have a function >> prefix, otherwise we may think this is related to some other kind of >> map, so I suggest: >> pr_debug("%s: error: map handler doesn't belong to object\n", >> __func__); >> >> Or at least: >> >> pr_debug("BPF error: map handler doesn't belong to object\n"); > > We always have a libbpf prefix before debug info from libbpf. Please see > > static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr; > static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr; > static __printf(1, 2) libbpf_print_fn_t __pr_debug; > #define __pr(func, fmt, ...) \ > do { \ > if ((func)) \ > (func)("libbpf: " fmt, ##__VA_ARGS__); \ > } while (0) > > #define pr_warning(fmt, ...) __pr(__pr_warning, fmt, ##__VA_ARGS__) > #define pr_info(fmt, ...) __pr(__pr_info, fmt, ##__VA_ARGS__) > #define pr_debug(fmt, ...) __pr(__pr_debug, fmt, ##__VA_ARGS__) > > We allow __pr_{warning,info,debug} be overwritten but the 'libbpf:' > prefix is > always there. > > Also, don't forget in perf's context libbpf is muted. > Sorry, Shoule be "muted in most case"... Here's an example: $ sudo ./perf record -v -e ./test_bpf_base.c ls ... ude/uapi -Iinclude/generated/uapi -include /home/w00229757/linux-hydrogen/include/linux/kconfig.h set env: WORKING_DIR=/lib/modules/4.3.0-rc4+/build set env: CLANG_SOURCE=/home/w00229757/./test_bpf_base.c llvm compiling command template: $CLANG_EXEC -D__KERNEL__ -D__NR_CPUS__=$NR_CPUS -DLINUX_VERSION_CODE=$LINUX_VERSION_CODE $CLANG_OPTIONS $KERNEL_INC_OPTIONS -Wno-unused-value -Wno-pointer-sign -working-directory $WORKING_DIR -c "$CLANG_SOURCE" -target bpf -O2 -o - libbpf: loading object './test_bpf_base.c' from buffer libbpf: section .strtab, size 112, link 0, flags 0, type=3 libbpf: section .text, size 0, link 0, flags 6, type=1 libbpf: section .data, size 0, link 0, flags 3, type=1 ... libbpf: kernel version of ./test_bpf_base.c is 40300 libbpf: section .symtab, size 96, link 1, flags 0, type=2 bpf: config program 'func_write=vfs_write file->f_mode' symbol:vfs_write file:(null) line:0 offset:0 return:0 lazy:(null) parsing arg: file->f_mode into file, f_mode(1) bpf: config 'func_write=vfs_write file->f_mode' is ok If the error you pointed out is triggered, it should be: libbpf: error: map handler doesn't belong to object ... So we can always find which error message is belone to libbpf and which is belone to perf. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index e3f4c33..61c9f40 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -163,22 +163,24 @@ struct bpf_program { bpf_program_clear_priv_t clear_priv; }; +struct bpf_map { + int fd; + struct bpf_map_def def; + void *priv; + bpf_map_clear_priv_t clear_priv; +}; + static LIST_HEAD(bpf_objects_list); struct bpf_object { char license[64]; u32 kern_version; - void *maps_buf; - size_t maps_buf_sz; struct bpf_program *programs; size_t nr_programs; - int *map_fds; - /* - * This field is required because maps_buf will be freed and - * maps_buf_sz will be set to 0 after loaded. - */ - size_t nr_map_fds; + struct bpf_map *maps; + size_t nr_maps; + bool loaded; /* @@ -489,21 +491,38 @@ static int bpf_object__init_maps(struct bpf_object *obj, void *data, size_t size) { - if (size == 0) { + size_t nr_maps; + int i; + + nr_maps = size / sizeof(struct bpf_map_def); + if (!data || !nr_maps) { pr_debug("%s doesn't need map definition\n", obj->path); return 0; } - obj->maps_buf = malloc(size); - if (!obj->maps_buf) { - pr_warning("malloc maps failed: %s\n", obj->path); + pr_debug("maps in %s: %ld bytes\n", obj->path, (long)size); + + obj->maps = calloc(1, sizeof(obj->maps[0]) * nr_maps); + if (!obj->maps) { + pr_warning("alloc maps for object failed\n"); return -ENOMEM; } + obj->nr_maps = nr_maps; - obj->maps_buf_sz = size; - memcpy(obj->maps_buf, data, size); - pr_debug("maps in %s: %ld bytes\n", obj->path, (long)size); + for (i = 0; i < nr_maps; i++) { + struct bpf_map_def *def = &obj->maps[i].def; + + /* + * fill all fd with -1 so won't close incorrect + * fd (0, stdin) when failure. + */ + obj->maps[i].fd = -1; + + /* Save map definition into obj->maps */ + *def = *(struct bpf_map_def *)(data + + i * sizeof(struct bpf_map_def)); + } return 0; } @@ -688,37 +707,15 @@ static int bpf_object__create_maps(struct bpf_object *obj) { unsigned int i; - size_t nr_maps; - int *pfd; - - nr_maps = obj->maps_buf_sz / sizeof(struct bpf_map_def); - if (!obj->maps_buf || !nr_maps) { - pr_debug("don't need create maps for %s\n", - obj->path); - return 0; - } - - obj->map_fds = malloc(sizeof(int) * nr_maps); - if (!obj->map_fds) { - pr_warning("realloc perf_bpf_map_fds failed\n"); - return -ENOMEM; - } - obj->nr_map_fds = nr_maps; - /* fill all fd with -1 */ - memset(obj->map_fds, -1, sizeof(int) * nr_maps); + for (i = 0; i < obj->nr_maps; i++) { + struct bpf_map_def *def = &obj->maps[i].def; + int *pfd = &obj->maps[i].fd; - pfd = obj->map_fds; - for (i = 0; i < nr_maps; i++) { - struct bpf_map_def def; - - def = *(struct bpf_map_def *)(obj->maps_buf + - i * sizeof(struct bpf_map_def)); - - *pfd = bpf_create_map(def.type, - def.key_size, - def.value_size, - def.max_entries); + *pfd = bpf_create_map(def->type, + def->key_size, + def->value_size, + def->max_entries); if (*pfd < 0) { size_t j; int err = *pfd; @@ -726,22 +723,17 @@ bpf_object__create_maps(struct bpf_object *obj) pr_warning("failed to create map: %s\n", strerror(errno)); for (j = 0; j < i; j++) - zclose(obj->map_fds[j]); - obj->nr_map_fds = 0; - zfree(&obj->map_fds); + zclose(obj->maps[j].fd); return err; } pr_debug("create map: fd=%d\n", *pfd); - pfd++; } - zfree(&obj->maps_buf); - obj->maps_buf_sz = 0; return 0; } static int -bpf_program__relocate(struct bpf_program *prog, int *map_fds) +bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj) { int i; @@ -761,7 +753,7 @@ bpf_program__relocate(struct bpf_program *prog, int *map_fds) return -LIBBPF_ERRNO__RELOC; } insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD; - insns[insn_idx].imm = map_fds[map_idx]; + insns[insn_idx].imm = obj->maps[map_idx].fd; } zfree(&prog->reloc_desc); @@ -780,7 +772,7 @@ bpf_object__relocate(struct bpf_object *obj) for (i = 0; i < obj->nr_programs; i++) { prog = &obj->programs[i]; - err = bpf_program__relocate(prog, obj->map_fds); + err = bpf_program__relocate(prog, obj); if (err) { pr_warning("failed to relocate '%s'\n", prog->section_name); @@ -804,8 +796,7 @@ static int bpf_object__collect_reloc(struct bpf_object *obj) Elf_Data *data = obj->efile.reloc[i].data; int idx = shdr->sh_info; struct bpf_program *prog; - size_t nr_maps = obj->maps_buf_sz / - sizeof(struct bpf_map_def); + size_t nr_maps = obj->nr_maps; if (shdr->sh_type != SHT_REL) { pr_warning("internal error at %d\n", __LINE__); @@ -1050,10 +1041,8 @@ int bpf_object__unload(struct bpf_object *obj) if (!obj) return -EINVAL; - for (i = 0; i < obj->nr_map_fds; i++) - zclose(obj->map_fds[i]); - zfree(&obj->map_fds); - obj->nr_map_fds = 0; + for (i = 0; i < obj->nr_maps; i++) + zclose(obj->maps[i].fd); for (i = 0; i < obj->nr_programs; i++) bpf_program__unload(&obj->programs[i]); @@ -1096,7 +1085,15 @@ void bpf_object__close(struct bpf_object *obj) bpf_object__elf_finish(obj); bpf_object__unload(obj); - zfree(&obj->maps_buf); + for (i = 0; i < obj->nr_maps; i++) { + if (obj->maps[i].clear_priv) + obj->maps[i].clear_priv(&obj->maps[i], + obj->maps[i].priv); + obj->maps[i].priv = NULL; + obj->maps[i].clear_priv = NULL; + } + zfree(&obj->maps); + obj->nr_maps = 0; if (obj->programs && obj->nr_programs) { for (i = 0; i < obj->nr_programs; i++) @@ -1251,3 +1248,72 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n) return fd; } + +int bpf_map__get_fd(struct bpf_map *map) +{ + if (!map) + return -EINVAL; + + return map->fd; +} + +int bpf_map__get_def(struct bpf_map *map, struct bpf_map_def *pdef) +{ + if (!map || !pdef) + return -EINVAL; + + *pdef = map->def; + return 0; +} + +int bpf_map__set_private(struct bpf_map *map, void *priv, + bpf_map_clear_priv_t clear_priv) +{ + if (!map) + return -EINVAL; + + if (map->priv) { + if (map->clear_priv) + map->clear_priv(map, map->priv); + } + + map->priv = priv; + map->clear_priv = clear_priv; + return 0; +} + +int bpf_map__get_private(struct bpf_map *map, void **ppriv) +{ + if (!map) + return -EINVAL; + + if (ppriv) + *ppriv = map->priv; + return 0; +} + +struct bpf_map * +bpf_map__next(struct bpf_map *prev, struct bpf_object *obj) +{ + size_t idx; + struct bpf_map *s, *e; + + if (!obj || !obj->maps) + return NULL; + + s = obj->maps; + e = obj->maps + obj->nr_maps; + + if (prev == NULL) + return s; + + if ((prev < s) || (prev >= e)) { + pr_warning("error: map handler doesn't belong to object\n"); + return NULL; + } + + idx = (prev - obj->maps) + 1; + if (idx >= obj->nr_maps) + return NULL; + return &obj->maps[idx]; +} diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 949df4b..709d2fa 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -165,4 +165,25 @@ struct bpf_map_def { unsigned int max_entries; }; +/* + * There is another 'struct bpf_map' in include/linux/map.h. However, + * it is not a uapi header so no need to consider name confliction. + */ +struct bpf_map; + +struct bpf_map * +bpf_map__next(struct bpf_map *map, struct bpf_object *obj); +#define bpf_map__for_each(pos, obj) \ + for ((pos) = bpf_map__next(NULL, (obj)); \ + (pos) != NULL; \ + (pos) = bpf_map__next((pos), (obj))) + +int bpf_map__get_fd(struct bpf_map *map); +int bpf_map__get_def(struct bpf_map *map, struct bpf_map_def *pdef); + +typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *); +int bpf_map__set_private(struct bpf_map *map, void *priv, + bpf_map_clear_priv_t clear_priv); +int bpf_map__get_private(struct bpf_map *map, void **ppriv); + #endif
This patch collects more information from maps sections in BPF object files into 'struct bpf_object', enables later patches access those information (such as the type and size of the map). In this patch, a new handler 'struct bpf_map' is extracted in parallel with bpf_object and bpf_program. Its iterator and accessor is also created. Signed-off-by: Wang Nan <wangnan0@huawei.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Zefan Li <lizefan@huawei.com> Cc: pi3orama@163.com --- tools/lib/bpf/libbpf.c | 186 +++++++++++++++++++++++++++++++++---------------- tools/lib/bpf/libbpf.h | 21 ++++++ 2 files changed, 147 insertions(+), 60 deletions(-) -- 1.8.3.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/