On 10/30/23 18:09, Martin KaFai Lau wrote:
On 10/30/23 12:28 PM, thinker.li@xxxxxxxxx wrote:
From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
Maintain a registry of registered struct_ops types in the per-btf
(module)
struct_ops_tab. This registry allows for easy lookup of struct_ops types
that are registered by a specific module.
Every struct_ops type should have an associated module BTF to provide
type
information since we are going to allow modules to define and register
new
struct_ops types. Once this change is made, the bpf_struct_ops subsystem
knows where to look up type info with just a bpf_struct_ops.
I think this part needs better description. I found it hard to parse. In
particular:
...
the "bpf_struct_ops" subsystem
knows where to look up type info with just
a "bpf_struct_ops"
...
May be something like:
It is a preparation work for supporting kernel module struct_ops in a
latter patch. Each struct_ops will be registered under its own kernel
module btf and will be stored in the newly added btf->struct_ops_tab.
The bpf verifier and bpf syscall (e.g. prog and map cmd) can find the
struct_ops and its btf type/size/id... information from
btf->struct_ops_tab.
Got it!
The subsystem looks up struct_ops types from a given module BTF
although it
is always btf_vmlinux now. Once start using struct_ops_tab, btfs other
than
btf_vmlinux can be used as well.
I think this describes about the "struct btf *btf" argument change in
this patch. This seems unrelated to the "add struct_ops_tab to btf"
change. Can it be in its own preparation patch?
Sure!
[ ... ]
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index e35d6321a2f8..0bc21a39257d 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -186,6 +186,7 @@ static void bpf_struct_ops_init_one(struct
bpf_struct_ops_desc *st_ops_desc,
pr_warn("Error in init bpf_struct_ops %s\n",
st_ops->name);
} else {
+ st_ops_desc->btf = btf;
st_ops_desc->type_id = type_id;
st_ops_desc->type = t;
st_ops_desc->value_id = value_id;
@@ -222,7 +223,7 @@ void bpf_struct_ops_init(struct btf *btf, struct
bpf_verifier_log *log)
extern struct btf *btf_vmlinux;
static const struct bpf_struct_ops_desc *
-bpf_struct_ops_find_value(u32 value_id)
+bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
The "!btf_vmlinux" check a few lines below should also be changed to
"!btf". I think I had commented on a similar point in v5.
At this patch, btf is still always btf_vmlinux until the patch 6 and 7.
I will move these changes from the patch 6 and 7 to here or
the new patch mentioned above.
{
unsigned int i;
@@ -237,7 +238,8 @@ bpf_struct_ops_find_value(u32 value_id)
return NULL;
}
-const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id)
+const struct bpf_struct_ops_desc *
+bpf_struct_ops_find(struct btf *btf, u32 type_id)
same here.
Got it!
{
unsigned int i;
[ ... ]
+static struct bpf_struct_ops_desc *
+btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops)
+{
+ struct btf_struct_ops_tab *tab, *new_tab;
+ int i;
+
+ if (!btf)
+ return ERR_PTR(-ENOENT);
+
+ /* Assume this function is called for a module when the module is
+ * loading.
+ */
+
+ tab = btf->struct_ops_tab;
+ if (!tab) {
+ tab = kzalloc(offsetof(struct btf_struct_ops_tab, ops[4]),
+ GFP_KERNEL);
+ if (!tab)
+ return ERR_PTR(-ENOMEM);
+ tab->capacity = 4;
+ btf->struct_ops_tab = tab;
+ }
+
+ for (i = 0; i < tab->cnt; i++)
+ if (tab->ops[i].st_ops == st_ops)
+ return ERR_PTR(-EEXIST);
+
+ if (tab->cnt == tab->capacity) {
+ new_tab = krealloc(tab, sizeof(*tab) +
+ sizeof(struct bpf_struct_ops *) *
+ tab->capacity * 2, GFP_KERNEL);
nit. Use a similar offsetof() like a few lines above.
Sure!
+ if (!new_tab)
+ return ERR_PTR(-ENOMEM);
+ tab = new_tab;
+ tab->capacity *= 2;
+ btf->struct_ops_tab = tab;
+ }
+
+ btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt].st_ops = st_ops;
nit. s/btf->struct_ops_tab/tab/
Sure!
+
+ return &btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++];
+}
