Re: [PATCH v3 bpf-next 13/15] bpf: Prepare bpf_mem_alloc to be used by sleepable bpf programs.

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Sat, Aug 20, 2022 at 12:21:46AM +0200, Kumar Kartikeya Dwivedi wrote:
> On Fri, 19 Aug 2022 at 23:43, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > Use call_rcu_tasks_trace() to wait for sleepable progs to finish.
> > Then use call_rcu() to wait for normal progs to finish
> > and finally do free_one() on each element when freeing objects
> > into global memory pool.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> 
> I fear this can make OOM issues very easy to run into, because one
> sleepable prog that sleeps for a long period of time can hold the
> freeing of elements from another sleepable prog which either does not
> sleep often or sleeps for a very short period of time, and has a high
> update frequency. I'm mostly worried that unrelated sleepable programs
> not even using the same map will begin to affect each other.

'sleep for long time'? sleepable bpf prog doesn't mean that they can sleep.
sleepable progs can copy_from_user, but they're not allowed to waste time.
I don't share OOM concerns at all.
max_entries and memcg limits are still there and enforced.
dynamic map is strictly better and memory efficient than full prealloc.

> Have you considered other options? E.g. we could directly expose
> bpf_rcu_read_lock/bpf_rcu_read_unlock to the program and enforce that
> access to RCU protected map lookups only happens in such read
> sections, and unlock invalidates all RCU protected pointers? Sleepable
> helpers can then not be invoked inside the BPF RCU read section. The
> program uses RCU read section while accessing such maps, and sleeps
> after doing bpf_rcu_read_unlock. They can be kfuncs.

Yes. We can add explicit bpf_rcu_read_lock and teach verifier about RCU CS,
but I don't see the value specifically for sleepable progs.
Current sleepable progs can do map lookup without extra kfuncs.
Explicit CS would force progs to be rewritten which is not great.

> It might also be useful in general, to access RCU protected data from
> sleepable programs (i.e. make some sections of the program RCU
> protected and non-sleepable at runtime). It will allow use of elements

For other cases, sure. We can introduce RCU protected objects and
explicit bpf_rcu_read_lock.

> from dynamically allocated maps with bpf_mem_alloc while not having to
> wait for RCU tasks trace grace period, which can extend into minutes
> (or even longer if unlucky).

sleepable bpf prog that lasts minutes? In what kind of situation?
We don't have bpf_sleep() helper and not going to add one any time soon.