Re: [PATCH bpf-next v8 03/10] bpf: add struct_ops_tab to btf.

[Martin KaFai Lau <martin.lau@linux.dev>]

On 10/30/23 12:28 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> 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.

> 
> 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?

[ ... ]

> 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.

>   {
>   	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.

>   {
>   	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.

> +		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/

> +
> +	return &btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++];
> +}