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

[bot+bpf-ci]

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

>
> -static inline void *skel_prep_map_data(const void *val, size_t mmap_sz, size_t val_sz)
> +static inline void skel_free_map_data(void *p, __u64 addr, size_t val_sz, __u32 max_entries)
>  {
> +	munmap(p, skel_map_mmap_sz(val_sz, max_entries));
> +}
> +
> +static inline void *skel_prep_map_data(const void *val, size_t val_sz, __u32 max_entries,
> +				       size_t data_sz)
> +{
> +	size_t mmap_sz = skel_map_mmap_sz(val_sz, max_entries);
>  	void *addr;
>
>  	addr = mmap(NULL, mmap_sz, PROT_READ | PROT_WRITE,
>  		    MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>  	if (addr == (void *) -1)
>  		return NULL;
> -	memcpy(addr, val, val_sz);
> +	memcpy(addr, val, data_sz);
>  	return addr;
>  }
>
> -static inline void *skel_finalize_map_data(__u64 *init_val, size_t mmap_sz, int flags, int fd)
> +static inline void *skel_finalize_map_data(__u64 *init_val, size_t val_sz, __u32 max_entries,
> +					   int flags, int fd)
>  {
> +	size_t mmap_sz = skel_map_mmap_sz(val_sz, max_entries);
>  	void *addr;
>
>  	addr = mmap((void *) (long) *init_val, mmap_sz, flags, MAP_SHARED | MAP_FIXED, fd, 0);
> @@ -220,11 +233,13 @@ static inline void *skel_finalize_map_data(__u64 *init_val, size_t mmap_sz, int
>  	return addr;
>  }
>
> -static inline int skel_protect_map_data(void *p, __u64 *init_val, size_t sz)
> +static inline int skel_protect_map_data(void *p, __u64 *init_val, size_t val_sz, __u32 max_entries)
>  {
> +	size_t mmap_sz = skel_map_mmap_sz(val_sz, max_entries);
> +
>  	(void)init_val;
>
> -	if (mprotect(p, sz, PROT_READ))
> +	if (mprotect(p, mmap_sz, PROT_READ))
>  		return -errno;
>  	return 0;
>  }


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34864978845