Re: [PATCH bpf-next v2] bpftool: Compute map size of light skeletons at runtime

[Leon Hwang]

On 15/9/26 00:39, bot+bpf-ci@xxxxxxxxxx wrote:
>> diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
>> index 1f3f332dffbe..6f23ea9410eb 100644
>> --- a/tools/lib/bpf/skel_internal.h
>> +++ b/tools/lib/bpf/skel_internal.h
>> @@ -11,6 +11,7 @@
>>  #include <linux/bpf.h>
>>  #else
>>  #include <unistd.h>
>> +#include <sys/param.h>
>>  #include <sys/syscall.h>
>>  #include <sys/mman.h>
>>  #include <linux/keyctl.h>
> 
> [ ... ]
> 
>> @@ -193,25 +197,34 @@ static inline void skel_free(void *p)
>>  	free(p);
>>  }
>>
>> -static inline void skel_free_map_data(void *p, __u64 addr, size_t sz)
>> +static inline size_t skel_map_mmap_sz(size_t val_sz, __u32 max_entries)
>>  {
>> -	munmap(p, sz);
>> +	return roundup(roundup(val_sz, 8) * max_entries, sysconf(_SC_PAGE_SIZE));
>>  }
> 
> This isn't a bug, but would it read closer to array_map_mmap_sz() to hoist
> the page size into a local here? sysconf(_SC_PAGE_SIZE) is passed straight
> into roundup()'s second argument, and roundup() is a macro that expands its
> second argument several times in the non-constant case. The three nearby
> equivalents all hoist it: the bpf_map_mmap_sz() this patch deletes from
> gen.c used 'long page_sz = sysconf(_SC_PAGE_SIZE);', and libbpf's
> array_map_mmap_sz() and bpf_map_mmap_sz() both open with 'const long page_sz
> = sysconf(_SC_PAGE_SIZE);'. Hoisting it here would also make the helper
> independent of which roundup() definition wins, and might let the new
> <sys/param.h> include be dropped.

Good catch.

'sysconf(_SC_PAGE_SIZE)' will be calculated several times in
'roundup(roundup(val_sz, 8) * max_entries, sysconf(_SC_PAGE_SIZE))'.

printf '%s\n' \
  '#include <sys/param.h>' \
  'roundup(roundup(val_sz, 8) * max_entries, sysconf(_SC_PAGE_SIZE))' |
gcc -E -P -x c - |
tail -n 1

(__builtin_constant_p (sysconf(_SC_PAGESIZE)) &&
((((sysconf(_SC_PAGESIZE)) - 1) & (sysconf(_SC_PAGESIZE))) == 0) ?
((((__builtin_constant_p (8) && ((((8) - 1) & (8)) == 0) ? (((val_sz) +
(8) - 1) & ~((8) - 1)) : ((((val_sz) + ((8) - 1)) / (8)) * (8))) *
max_entries) + (sysconf(_SC_PAGESIZE)) - 1) & ~((sysconf(_SC_PAGESIZE))
- 1)) : (((((__builtin_constant_p (8) && ((((8) - 1) & (8)) == 0) ?
(((val_sz) + (8) - 1) & ~((8) - 1)) : ((((val_sz) + ((8) - 1)) / (8)) *
(8))) * max_entries) + ((sysconf(_SC_PAGESIZE)) - 1)) /
(sysconf(_SC_PAGESIZE))) * (sysconf(_SC_PAGESIZE))))

Will hoist 'sysconf(_SC_PAGE_SIZE);' and 'roundup(val_sz, 8) * max_entries'.

However, the new <sys/param.h> is needed for roundup().

Thanks,
Leon

> [...]