Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1276473 > unrolled thread

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

Started byWang Nan <wangnan0@huawei.com>
First post2015-11-24 14:50 +0100
Last post2015-11-27 07:30 +0100
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 04/16] bpf tools: Collect map definition in bpf_object Wang Nan <wangnan0@huawei.com> - 2015-11-24 14:50 +0100
    Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-11-26 22:00 +0100
      Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-27 07:20 +0100
        Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-27 07:30 +0100

#1276473 — [PATCH 04/16] bpf tools: Collect map definition in bpf_object

FromWang Nan <wangnan0@huawei.com>
Date2015-11-24 14:50 +0100
Subject[PATCH 04/16] bpf tools: Collect map definition in bpf_object
Message-ID<qym8O-jw-21@gated-at.bofh.it>
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(-)

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
-- 
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/

[toc] | [next] | [standalone]

#1278449

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-11-26 22:00 +0100
Message-ID<qzbO1-1JG-3@gated-at.bofh.it>
In reply to#1276473
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278554

From"Wangnan (F)" <wangnan0@huawei.com>
Date2015-11-27 07:20 +0100
Message-ID<qzkxX-7zh-5@gated-at.bofh.it>
In reply to#1278449

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/

[toc] | [prev] | [next] | [standalone]

#1278555

From"Wangnan (F)" <wangnan0@huawei.com>
Date2015-11-27 07:30 +0100
Message-ID<qzkHD-7CD-1@gated-at.bofh.it>
In reply to#1278554

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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web