Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753518AbbKZU5I (ORCPT ); Thu, 26 Nov 2015 15:57:08 -0500 Received: from mail.kernel.org ([198.145.29.136]:54538 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753378AbbKZU5C (ORCPT ); Thu, 26 Nov 2015 15:57:02 -0500 Date: Thu, 26 Nov 2015 17:56:57 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: masami.hiramatsu.pt@hitachi.com, ast@kernel.org, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org, Namhyung Kim Subject: Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object Message-ID: <20151126205657.GN28162@kernel.org> References: <1448372181-151723-1-git-send-email-wangnan0@huawei.com> <1448372181-151723-5-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1448372181-151723-5-git-send-email-wangnan0@huawei.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9098 Lines: 343 Em Tue, Nov 24, 2015 at 01:36:09PM +0000, Wang Nan escreveu: > 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); We have %zd for these cases, please avoid using cast unnecessarily. > + > + obj->maps = calloc(1, sizeof(obj->maps[0]) * nr_maps); Hey, calloc does more than just zeroing the allocated memory, that first member is the number of entries, so you could do it as: obj->maps = calloc(nr_maps, sizeof(obj->maps[0])); To use that 'nmemb' first parameter and to make the whole thing a bit more compact. > + 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)); This gets more clear/compact as: *def = ((struct bpf_map_def *)data)[i]; Testing: [acme@zoo c]$ cat void_long.c #include void main(void) { long a[] = { 0, 21, 332, 4443, 5554 }; void *v = &a; int i; for (i = 0; i < 5; ++i) printf("++((long *)v)[%d]=%ld\n", i, ++((long *)v)[i]); } [acme@zoo c]$ make void_long cc void_long.c -o void_long [acme@zoo c]$ ./void_long ++((long *)v)[0]=1 ++((long *)v)[1]=22 ++((long *)v)[2]=333 ++((long *)v)[3]=4444 ++((long *)v)[4]=5555 [acme@zoo c]$ > + } > 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? > - 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"); > + 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". > + */ > +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 > -- > 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/