On Tue, Oct 18, 2022 at 10:58 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> 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.
libbpf does the full type match, really?
Could you point me to the code?
> 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.
bpf_kptr_new(typeof(*p)) is cleaner.
> > > + 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.
bpf_kptr_drop is more precise.
delete assumes instant free which is not the case here.
How about
extern void *__bpf_obj_new(__u64 local_type_id) __ksym;
extern void bpf_obj_drop(void *obj) __ksym;
#define bpf_obj_new(t) \
(t *)__bpf_obj_new(bpf_core_type_id_local(t));
kptr means 'kernel pointer'.
Here we have program supplied object.
It feels 'obj' is better than 'kptr' in this context.
> 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?
If we do doubly linked list we should allow delete in
the middle with
bpf_list_del_any(head, node)
and
bpf_list_pop_front/pop_back(head)
bpf_list_add(node, head) would match kernel style,
but I think it's cleaner to have head as 1st arg.
In that sense new pop/push/_front/_back are cleaner.
And similar for rbtree.
If we keep (node, head) and (rb_node, rb_root) order
we should keep kernel names.
