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

["Wangnan (F)" <wangnan0@huawei.com>]

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.