Thread (24 messages) 24 messages, 4 authors, 2022-01-25

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

From: Alexei Starovoitov <hidden>
Date: 2022-01-05 06:19:25
Also in: bpf, netfilter-devel

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help