Re: [PATCH RFC bpf-next v1 16/32] bpf: Introduce BPF memory object model

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Dave Marchevsky <davemarchevsky@fb.com>,
	Delyan Kratunov <delyank@fb.com>
Subject: Re: [PATCH RFC bpf-next v1 16/32] bpf: Introduce BPF memory object model
Date: Thu, 8 Sep 2022 07:18:09 -0700	[thread overview]
Message-ID: <CAADnVQ+hgprNMCSk0bjZnRveEzv=t8zoZXH44Gy8tVPJKoPt_A@mail.gmail.com> (raw)
In-Reply-To: <CAP01T76YqSKUMFCVz-WqQQL29SFFn4DG6wqwm0HVpN2-DqJuFA@mail.gmail.com>

On Thu, Sep 8, 2022 at 4:50 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> I slept over this. I think I can get behind this idea of implicit
> ctor/dtor. We might have open coded construction/destruction later if
> we want.
>
> I am however thinking of naming these helpers:
> bpf_kptr_new
> bpf_kptr_delete
> to make it clear it does a little more than just allocating the type.
> The open coded cases can later derive their allocation from the more
> bare bones bpf_kptr_alloc instead in the future.

New names make complete sense. Good idea.

> The main reason to have open coded-ness was being able to 'manage'
> resources once visibility reduces to current CPU (bpf_refcount_put,
> single ownership after xchg, etc.). Even with RCU, we won't allow
> touching the BPF special fields without refcount. bpf_spin_lock is
> different, as it protects more than just bpf special fields.
>
> But one can still splice or kptr_xchg before passing to bpf_kptr_free
> to do that. bpf_kptr_free is basically cleaning up whatever is left by
> then, forcefully. In the future, we might even be able to do elision
> of implicit dtors based on the seen data flow (splicing in single
> ownership implies list is empty, any other op will undo that, etc.) if
> there are big structs with too many fields. Can also support that in
> open coded cases.

Right.

>
> What I want to think about more is whether we should still force
> calling bpf_refcount_set vs always setting it to 1.
>
> I know we don't agree about whether list_add in shared mode should
> take ref vs transfer ref. I'm leaning towards transfer since that will
> be most intuitive. It then works the same way in both cases, single
> ownership only transfers the sole reference you have, so you lose
> access, but in shared you may have more than one. If you have just one
> you will still lose access.
>
> It will be odd for list_add to consume it in one case and not the
> other. People should already be fully conscious of how they are
> managing the lifetime of their object.
>
> It then seems better to require users to set the initial refcount
> themselves. When doing the initial linking it can be very cheap.
> Later get/put/inc are always available.
>
> But forcing it to be called is going to be much simpler than this patch.

I'm not convinced yet :)
Pls hold on implementing one way or another.
Let's land the single ownership case for locks, lists,
rbtrees, allocators. That's plenty of patches.
Then we can start a deeper discussion into the shared case.
Whether it will be different in terms of 'lose access after list_add'
is not critical to decide now. It can change in the future too.

The other reason to do implicit inits and ref count sets is to
avoid fighting llvm.
obj = bpf_kptr_new();
obj->var1 = 1;
some_func(&obj->var2);
In many cases the compiler is allowed to sink stores.
If there are two calls that "init" two different fields
the compiler is allowed to change the order as well
even if it doesn't see the body of the function and the function is
marked as __pure. Technically initializers as pure functions.
The verifier and llvm already "fight" a lot.
We gotta be very careful in the verifier and not assume
that the code stays as written in C.