Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object

From: Arnaldo Carvalho de Melo
Date: Thu Nov 26 2015 - 15:57:25 EST

Next message: Nicolas Sugino: "[PATCH] dib8000: Add support for Mygica/Geniatech S2870"
Previous message: Arnaldo Carvalho de Melo: "[PATCH 03/13] perf script: Remove default_scripting_ops"
In reply to: Wang Nan: "[PATCH 04/16] bpf tools: Collect map definition in bpf_object"
Next in thread: Wangnan (F): "Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <stdio.h>

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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Nicolas Sugino: "[PATCH] dib8000: Add support for Mygica/Geniatech S2870"
Previous message: Arnaldo Carvalho de Melo: "[PATCH 03/13] perf script: Remove default_scripting_ops"
In reply to: Wang Nan: "[PATCH 04/16] bpf tools: Collect map definition in bpf_object"
Next in thread: Wangnan (F): "Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]