Re: [PATCH bpf-next v2 19/25] bpf: Introduce bpf_kptr_new

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Dave Marchevsky <davemarchevsky@meta.com>,
	Delyan Kratunov <delyank@meta.com>
Subject: Re: [PATCH bpf-next v2 19/25] bpf: Introduce bpf_kptr_new
Date: Wed, 19 Oct 2022 11:28:34 +0530	[thread overview]
Message-ID: <20221019055834.ux5dfoot7hyuf4jk@apollo> (raw)
In-Reply-To: <20221019023124.47zzi3gs2zcdvxca@macbook-pro-4.dhcp.thefacebook.com>

On Wed, Oct 19, 2022 at 08:01:24AM IST, Alexei Starovoitov wrote:
> On Thu, Oct 13, 2022 at 11:52:57AM +0530, Kumar Kartikeya Dwivedi wrote:
> > +void *bpf_kptr_new_impl(u64 local_type_id__k, u64 flags, void *meta__ign)
> > +{
> > +	struct btf_struct_meta *meta = meta__ign;
> > +	u64 size = local_type_id__k;
> > +	void *p;
> > +
> > +	if (unlikely(flags || !bpf_global_ma_set))
> > +		return NULL;
>
> Unused 'flags' looks weird in unstable api. Just drop it?
> And keep it as:
> void *bpf_kptr_new(u64 local_type_id__k, struct btf_struct_meta *meta__ign);
>
> and in bpf_experimental.h:
>
> extern void *bpf_kptr_new(__u64 local_type_id) __ksym;
>
> since __ign args are ignored during kfunc type match
> the bpf progs can use it without #define.
>

It's ignored during check_kfunc_call, but libbpf doesn't ignore that. The
prototypes will not be the same. I guess I'll have to teach it do that during
type match, but IDK how you feel about that.

Otherwise unless you want people to manually pass something to the ignored
argument, we have to hide it behind a macro.

I actually like the macro on top, then I don't even pass the type ID but the
type. But that's a personal preference, and I don't feel strongly about it.

So in C one does malloc(sizeof(*p)), here we'll just write
bpf_kptr_new(typeof(*p)). YMMV.

> > +	p = bpf_mem_alloc(&bpf_global_ma, size);
> > +	if (!p)
> > +		return NULL;
> > +	if (meta)
> > +		bpf_obj_init(meta->off_arr, p);
>
> I'm starting to dislike all that _arr and _tab suffixes in the verifier code base.
> It reminds me of programming style where people tried to add types into
> variable names. imo dropping _arr wouldn't be just fine.

Ack, I'll do it in v3.

Also, I'd like to invite people to please bikeshed a bit over the naming of the
APIs, e.g. whether it should be bpf_kptr_drop vs bpf_kptr_delete.

In the BPF list API, it's named bpf_list_del but it's actually distinct from how
list_del in the kernel works. So it does make sense to give them a different
name (like pop_front/pop_back and push_front/push_back)?

Because even bpf_list_add takes bpf_list_head, in the kernel there's no
distinction between node and head, so you can do list_add on a node as well, but
it won't be possible with the kfunc (unless we overload the head argument to
also work with nodes).

Later we'll probably have to add bpf_list_node_add etc. that add before or after
a node to make that work.

The main question is whether it should closely resembly the linked list API in
the kernel, or can it steer away considerably from that?