Re: [PATCH bpf-next v6 03/11] bpf: Populate kfunc BTF ID sets in struct btf

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, Jan 05, 2022 at 11:49:11AM IST, Alexei Starovoitov wrote:
> On Sun, Jan 02, 2022 at 09:51:07PM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > +enum btf_kfunc_hook {
> > +	BTF_KFUNC_HOOK_XDP,
> > +	BTF_KFUNC_HOOK_TC,
> > +	BTF_KFUNC_HOOK_STRUCT_OPS,
> > +	_BTF_KFUNC_HOOK_MAX,
>
> Why prefix with _ ?
>

Will fix.

> > +enum {
> > +	BTF_KFUNC_SET_MAX_CNT = 32,
> > +};
> ...
> > +	if (set_cnt + add_set->cnt > BTF_KFUNC_SET_MAX_CNT) {
> > +		ret = -E2BIG;
> > +		goto end;
> > +	}
>
> This artificial limit wouldn't be needed if you didn't insist on sorting.
> The later patches don't take advantage of this sorting feature and
> I don't see a test for sorting either.
>

I'm not insisting, but for vmlinux we will have multiple
register_btf_kfunc_id_set calls for same hook, so we have to concat multiple
sets into one, which may result in an unsorted set. It's ok to not sort for
modules where only one register call per hook is allowed.

Unless we switch to linear search for now (which is ok by me), we have to
re-sort for vmlinux BTF, to make btf_id_set_contains (in
btf_kfunc_id_set_contains) work.

> > +
> > +	/* Grow set */
> > +	set = krealloc(tab->sets[hook][type], offsetof(struct btf_id_set, ids[set_cnt + add_set->cnt]),
> > +		       GFP_KERNEL | __GFP_NOWARN);
> > +	if (!set) {
> > +		ret = -ENOMEM;
> > +		goto end;
> > +	}
> > +
> > +	/* For newly allocated set, initialize set->cnt to 0 */
> > +	if (!tab->sets[hook][type])
> > +		set->cnt = 0;
> > +	tab->sets[hook][type] = set;
> > +
> > +	/* Concatenate the two sets */
> > +	memcpy(set->ids + set->cnt, add_set->ids, add_set->cnt * sizeof(set->ids[0]));
> > +	set->cnt += add_set->cnt;
>
> Without sorting this function would just assign the pointer.
> No need for krealloc and memcpy.
>

Even if we didn't sort, we'd need to concat multiple sets for vmlinux case, so
krealloc and memcpy would still be needed for the vmlinux BTF case, right? For
modules I could certainly do a direct assignment, even if we keep sorting,
because only one set per hook is permitted.

> > +
> > +	if (sort_set)
> > +		sort(set->ids, set->cnt, sizeof(set->ids[0]), btf_id_cmp_func, NULL);
>
> All that looks like extra code for a dubious feature.
>

It's needed for the vmlinux case. I use WARN_ON_ONCE when modules try to
register more than one set for a certain hook.

> > +bool btf_kfunc_id_set_contains(const struct btf *btf,
> > +			       enum bpf_prog_type prog_type,
> > +			       enum btf_kfunc_type type, u32 kfunc_btf_id)
> > +{
> > +	enum btf_kfunc_hook hook;
> > +
> > +	switch (prog_type) {
> > +	case BPF_PROG_TYPE_XDP:
> > +		hook = BTF_KFUNC_HOOK_XDP;
> > +		break;
> > +	case BPF_PROG_TYPE_SCHED_CLS:
> > +		hook = BTF_KFUNC_HOOK_TC;
> > +		break;
> > +	case BPF_PROG_TYPE_STRUCT_OPS:
> > +		hook = BTF_KFUNC_HOOK_STRUCT_OPS;
> > +		break;
> > +	default:
> > +		return false;
> > +	}
>
> So this switch() is necessary only to compress prog_types into smaller hooks
> to save memory in the struct btf_kfunc_set_tab, right ?
> If so both kfunc_id_set_contains() and register_btf_kfunc() should
> probably use prog_type as an argument for symmetry.

Ok, will fix.

--
Kartikeya