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 _ ?
> +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.
> +
> + /* 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.
> +
> + 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.
> +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.
