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

[Wangnan (F)]

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