Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751439AbbK0GRG (ORCPT ); Fri, 27 Nov 2015 01:17:06 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:56563 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750739AbbK0GRE (ORCPT ); Fri, 27 Nov 2015 01:17:04 -0500 Message-ID: <5657F544.1080708@huawei.com> Date: Fri, 27 Nov 2015 14:16:36 +0800 From: "Wangnan (F)" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Arnaldo Carvalho de Melo CC: , , , , , Namhyung Kim Subject: Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object References: <1448372181-151723-1-git-send-email-wangnan0@huawei.com> <1448372181-151723-5-git-send-email-wangnan0@huawei.com> <20151126205657.GN28162@kernel.org> In-Reply-To: <20151126205657.GN28162@kernel.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020205.5657F550.0017,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 97246884a2dd95f22191d7daec428a9a Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7649 Lines: 271 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/