Re: [PATCH bpf-next v2 29/35] bpf: libbpf: cleanup RLIMIT_MEMLOCK usage

On Wed, Jul 29, 2020 at 6:38 PM Roman Gushchin [off-list ref] wrote:

On Mon, Jul 27, 2020 at 10:59:33PM -0700, Andrii Nakryiko wrote:

quoted

On Mon, Jul 27, 2020 at 4:15 PM Roman Gushchin [off-list ref] wrote:

quoted

On Mon, Jul 27, 2020 at 03:05:11PM -0700, Andrii Nakryiko wrote:

quoted

On Mon, Jul 27, 2020 at 12:21 PM Roman Gushchin [off-list ref] wrote:

quoted

As bpf is not using memlock rlimit for memory accounting anymore,
let's remove the related code from libbpf.

Bpf operations can't fail because of exceeding the limit anymore.

They can't in the newest kernel, but libbpf will keep working and
supporting old kernels for a very long time now. So please don't
remove any of this.

Yeah, good point, agree.
So we just can drop this patch from the series, no other changes
are needed.

quoted

But it would be nice to add a detection of whether kernel needs a
RLIMIT_MEMLOCK bump or not. Is there some simple and reliable way to
detect this from user-space?

Btw, do you mean we should add a new function to the libbpf API?
Or just extend pr_perm_msg() to skip guessing on new kernels?

I think we have to do both. There is libbpf_util.h in libbpf, we could
add two functions there:

- libbpf_needs_memlock() that would return true/false if kernel is old
and needs RLIMIT_MEMLOCK
- as a convenience, we can also add libbpf_inc_memlock_by() and
libbpf_set_memlock_to(), which will optionally (if kernel needs it)
adjust RLIMIT_MEMLOCK?

I think for your patch set, given it's pretty big already, let's not
touch runqslower, libbpf, and perf code (I think samples/bpf are fine
to just remove memlock adjustment), and we'll deal with detection and
optional bumping of RLIMIT_MEMLOCK as a separate patch once your
change land.

The problem with the latter one is that it's called on a failed attempt
to create a map, so unlikely we'll be able to create a new one just to test
for the "memlock" value. But it also raises a question what should we do
if the creation of this temporarily map fails? Assume the old kernel and
bump the limit?

Yeah, I think we'll have to make assumptions like that. Ideally, of
course, detection of this would be just a simple sysfs value or
something, don't know. Maybe there is already a way for kernel to
communicate something like that?

Idk, maybe it's better to just leave the userspace code as it is for some time.

Thanks!