Re: [PATCH bpf-next 8/9] libbpf: automatically fix up BPF_MAP_TYPE_RINGBUF size, if necessary

From: Nathan Chancellor <nathan@kernel.org>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	kernel-team@fb.com, llvm@lists.linux.dev
Subject: Re: [PATCH bpf-next 8/9] libbpf: automatically fix up BPF_MAP_TYPE_RINGBUF size, if necessary
Date: Tue, 10 May 2022 08:34:30 -0700	[thread overview]
Message-ID: <YnqGBmOHIZhrZBFJ@dev-arch.thelio-3990X> (raw)
In-Reply-To: <20220509004148.1801791-9-andrii@kernel.org>

Hi Andrii,

On Sun, May 08, 2022 at 05:41:47PM -0700, Andrii Nakryiko wrote:
> Kernel imposes a pretty particular restriction on ringbuf map size. It
> has to be a power-of-2 multiple of page size. While generally this isn't
> hard for user to satisfy, sometimes it's impossible to do this
> declaratively in BPF source code or just plain inconvenient to do at
> runtime.
> 
> One such example might be BPF libraries that are supposed to work on
> different architectures, which might not agree on what the common page
> size is.
> 
> Let libbpf find the right size for user instead, if it turns out to not
> satisfy kernel requirements. If user didn't set size at all, that's most
> probably a mistake so don't upsize such zero size to one full page,
> though. Also we need to be careful about not overflowing __u32
> max_entries.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

<snip>

> +static size_t adjust_ringbuf_sz(size_t sz)
> +{
> +	__u32 page_sz = sysconf(_SC_PAGE_SIZE);
> +	__u32 i, mul;
> +
> +	/* if user forgot to set any size, make sure they see error */
> +	if (sz == 0)
> +		return 0;
> +	/* Kernel expects BPF_MAP_TYPE_RINGBUF's max_entries to be
> +	 * a power-of-2 multiple of kernel's page size. If user diligently
> +	 * satisified these conditions, pass the size through.
> +	 */
> +	if ((sz % page_sz) == 0 && is_pow_of_2(sz / page_sz))
> +		return sz;
> +
> +	/* Otherwise find closest (page_sz * power_of_2) product bigger than
> +	 * user-set size to satisfy both user size request and kernel
> +	 * requirements and substitute correct max_entries for map creation.
> +	 */
> +	for (i = 0, mul = 1; ; i++, mul <<= 1) {
> +		if (mul > UINT_MAX / page_sz) /* prevent __u32 overflow */
> +			break;
> +		if (mul * page_sz > sz)
> +			return mul * page_sz;
> +	}
> +
> +	/* if it's impossible to satisfy the conditions (i.e., user size is
> +	 * very close to UINT_MAX but is not a power-of-2 multiple of
> +	 * page_size) then just return original size and let kernel reject it
> +	 */
> +	return sz;
> +}

This patch in -next as commit 0087a681fa8c ("libbpf: Automatically fix
up BPF_MAP_TYPE_RINGBUF size, if necessary") breaks the build with tip
of tree LLVM due to [1] strengthening -Wunused-but-set-variable:

libbpf.c:4954:8: error: variable 'i' set but not used [-Werror,-Wunused-but-set-variable]
        __u32 i, mul;
              ^
1 error generated.

Should i be removed or was it intended to be used somewhere that it is
not?

[1]: https://github.com/llvm/llvm-project/commit/2af845a6519c9cde5c8f58db5554f8b1084ce1ed

Cheers,
Nathan