diff mbox series

[RFC,bpf-next,v2,1/9] bpf: refactory struct_ops type initialization to a function.

Message ID 20230913061449.1918219-2-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Registrating struct_ops types from modules | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR pending PR summary
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-0 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 pending Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 fail Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-28 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-10 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs_no_alu32 on aarch64 with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1343 this patch: 1343
netdev/cc_maintainers warning 8 maintainers not CCed: jolsa@kernel.org haoluo@google.com daniel@iogearbox.net sdf@google.com john.fastabend@gmail.com yonghong.song@linux.dev netdev@vger.kernel.org kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 1364 this patch: 1364
netdev/verify_signedoff fail author Signed-off-by missing
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1366 this patch: 1366
netdev/checkpatch warning WARNING: macros should not use a trailing semicolon
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kui-Feng Lee Sept. 13, 2023, 6:14 a.m. UTC
From: Kui-Feng Lee <thinker.li@gmail.com>

Move most of code to bpf_struct_ops_init_one() that can be use to
initialize new struct_ops types registered dynamically.
---
 kernel/bpf/bpf_struct_ops.c | 157 +++++++++++++++++++-----------------
 1 file changed, 83 insertions(+), 74 deletions(-)

Comments

Martin KaFai Lau Sept. 15, 2023, 10:43 p.m. UTC | #1
On 9/12/23 11:14 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Move most of code to bpf_struct_ops_init_one() that can be use to
> initialize new struct_ops types registered dynamically.

While in RFC, still better to have SOB so that it won't be overlooked in the future.

> ---
>   kernel/bpf/bpf_struct_ops.c | 157 +++++++++++++++++++-----------------
>   1 file changed, 83 insertions(+), 74 deletions(-)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index fdc3e8705a3c..1662875e0ebe 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -110,102 +110,111 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
>   
>   static const struct btf_type *module_type;
>   
> -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> +static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
> +				    struct btf *btf,
> +				    struct bpf_verifier_log *log)
>   {
> -	s32 type_id, value_id, module_id;
>   	const struct btf_member *member;
> -	struct bpf_struct_ops *st_ops;
>   	const struct btf_type *t;
> +	s32 type_id, value_id;
>   	char value_name[128];
>   	const char *mname;
> -	u32 i, j;
> +	int i;
>   
> -	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
> -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> +	if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
> +	    sizeof(value_name)) {
> +		pr_warn("struct_ops name %s is too long\n",
> +			st_ops->name);
> +		return;
> +	}
> +	sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>   
> -	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
> -	if (module_id < 0) {
> -		pr_warn("Cannot find struct module in btf_vmlinux\n");
> +	value_id = btf_find_by_name_kind(btf, value_name,
> +					 BTF_KIND_STRUCT);

It needs to do some sanity checks on the value_type since this won't be 
statically enforced by bpf_struct_ops.c.

[ ... ]

> +void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> +{
> +	struct bpf_struct_ops *st_ops;
> +	s32 module_id;
> +	u32 i;
>   
> -			if (__btf_member_bitfield_size(t, member)) {
> -				pr_warn("bit field member %s in struct %s is not supported\n",
> -					mname, st_ops->name);
> -				break;
> -			}
> +	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
> +#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
> +#include "bpf_struct_ops_types.h"
> +#undef BPF_STRUCT_OPS_TYPE

Can this static way of defining struct_ops be removed? bpf_tcp_ca should be able 
to use the register_bpf_struct_ops() introduced in patch 2.

For the future subsystem supporting struct_ops, the subsystem could be compiled 
as a kernel module or as a built-in. register_bpf_struct_ops() should work for both.
Kui-Feng Lee Sept. 16, 2023, 1:35 a.m. UTC | #2
On 9/15/23 15:43, Martin KaFai Lau wrote:
> On 9/12/23 11:14 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Move most of code to bpf_struct_ops_init_one() that can be use to
>> initialize new struct_ops types registered dynamically.
> 
> While in RFC, still better to have SOB so that it won't be overlooked in 
> the future.
> 
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 157 +++++++++++++++++++-----------------
>>   1 file changed, 83 insertions(+), 74 deletions(-)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index fdc3e8705a3c..1662875e0ebe 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -110,102 +110,111 @@ const struct bpf_prog_ops 
>> bpf_struct_ops_prog_ops = {
>>   static const struct btf_type *module_type;
>> -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>> +static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>> +                    struct btf *btf,
>> +                    struct bpf_verifier_log *log)
>>   {
>> -    s32 type_id, value_id, module_id;
>>       const struct btf_member *member;
>> -    struct bpf_struct_ops *st_ops;
>>       const struct btf_type *t;
>> +    s32 type_id, value_id;
>>       char value_name[128];
>>       const char *mname;
>> -    u32 i, j;
>> +    int i;
>> -    /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
>> -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct 
>> bpf_struct_ops_##_name);
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> +    if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
>> +        sizeof(value_name)) {
>> +        pr_warn("struct_ops name %s is too long\n",
>> +            st_ops->name);
>> +        return;
>> +    }
>> +    sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
>> -    module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
>> -    if (module_id < 0) {
>> -        pr_warn("Cannot find struct module in btf_vmlinux\n");
>> +    value_id = btf_find_by_name_kind(btf, value_name,
>> +                     BTF_KIND_STRUCT);
> 
> It needs to do some sanity checks on the value_type since this won't be 
> statically enforced by bpf_struct_ops.c.

Do you mean to check if a value_type has refcnt, state and data field?

> 
> [ ... ]
> 
>> +void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>> +{
>> +    struct bpf_struct_ops *st_ops;
>> +    s32 module_id;
>> +    u32 i;
>> -            if (__btf_member_bitfield_size(t, member)) {
>> -                pr_warn("bit field member %s in struct %s is not 
>> supported\n",
>> -                    mname, st_ops->name);
>> -                break;
>> -            }
>> +    /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
>> +#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct 
>> bpf_struct_ops_##_name);
>> +#include "bpf_struct_ops_types.h"
>> +#undef BPF_STRUCT_OPS_TYPE
> 
> Can this static way of defining struct_ops be removed? bpf_tcp_ca should 
> be able to use the register_bpf_struct_ops() introduced in patch 2.

It sounds good for me.

> 
> For the future subsystem supporting struct_ops, the subsystem could be 
> compiled as a kernel module or as a built-in. register_bpf_struct_ops() 
> should work for both.
>
diff mbox series

Patch

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index fdc3e8705a3c..1662875e0ebe 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -110,102 +110,111 @@  const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
 
 static const struct btf_type *module_type;
 
-void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
+static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
+				    struct btf *btf,
+				    struct bpf_verifier_log *log)
 {
-	s32 type_id, value_id, module_id;
 	const struct btf_member *member;
-	struct bpf_struct_ops *st_ops;
 	const struct btf_type *t;
+	s32 type_id, value_id;
 	char value_name[128];
 	const char *mname;
-	u32 i, j;
+	int i;
 
-	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
-#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
+	if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
+	    sizeof(value_name)) {
+		pr_warn("struct_ops name %s is too long\n",
+			st_ops->name);
+		return;
+	}
+	sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
 
-	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
-	if (module_id < 0) {
-		pr_warn("Cannot find struct module in btf_vmlinux\n");
+	value_id = btf_find_by_name_kind(btf, value_name,
+					 BTF_KIND_STRUCT);
+	if (value_id < 0) {
+		pr_warn("Cannot find struct %s in btf_vmlinux\n",
+			value_name);
 		return;
 	}
-	module_type = btf_type_by_id(btf, module_id);
 
-	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
-		st_ops = bpf_struct_ops[i];
+	type_id = btf_find_by_name_kind(btf, st_ops->name,
+					BTF_KIND_STRUCT);
+	if (type_id < 0) {
+		pr_warn("Cannot find struct %s in btf_vmlinux\n",
+			st_ops->name);
+		return;
+	}
+	t = btf_type_by_id(btf, type_id);
+	if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
+		pr_warn("Cannot support #%u members in struct %s\n",
+			btf_type_vlen(t), st_ops->name);
+		return;
+	}
 
-		if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
-		    sizeof(value_name)) {
-			pr_warn("struct_ops name %s is too long\n",
+	for_each_member(i, t, member) {
+		const struct btf_type *func_proto;
+
+		mname = btf_name_by_offset(btf, member->name_off);
+		if (!*mname) {
+			pr_warn("anon member in struct %s is not supported\n",
 				st_ops->name);
-			continue;
+			break;
 		}
-		sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
 
-		value_id = btf_find_by_name_kind(btf, value_name,
-						 BTF_KIND_STRUCT);
-		if (value_id < 0) {
-			pr_warn("Cannot find struct %s in btf_vmlinux\n",
-				value_name);
-			continue;
+		if (__btf_member_bitfield_size(t, member)) {
+			pr_warn("bit field member %s in struct %s is not supported\n",
+				mname, st_ops->name);
+			break;
 		}
 
-		type_id = btf_find_by_name_kind(btf, st_ops->name,
-						BTF_KIND_STRUCT);
-		if (type_id < 0) {
-			pr_warn("Cannot find struct %s in btf_vmlinux\n",
-				st_ops->name);
-			continue;
-		}
-		t = btf_type_by_id(btf, type_id);
-		if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
-			pr_warn("Cannot support #%u members in struct %s\n",
-				btf_type_vlen(t), st_ops->name);
-			continue;
+		func_proto = btf_type_resolve_func_ptr(btf,
+						       member->type,
+						       NULL);
+		if (func_proto &&
+		    btf_distill_func_proto(log, btf,
+					   func_proto, mname,
+					   &st_ops->func_models[i])) {
+			pr_warn("Error in parsing func ptr %s in struct %s\n",
+				mname, st_ops->name);
+			break;
 		}
+	}
 
-		for_each_member(j, t, member) {
-			const struct btf_type *func_proto;
+	if (i == btf_type_vlen(t)) {
+		if (st_ops->init(btf)) {
+			pr_warn("Error in init bpf_struct_ops %s\n",
+				st_ops->name);
+		} else {
+			st_ops->type_id = type_id;
+			st_ops->type = t;
+			st_ops->value_id = value_id;
+			st_ops->value_type = btf_type_by_id(btf,
+							    value_id);
+		}
+	}
+}
 
-			mname = btf_name_by_offset(btf, member->name_off);
-			if (!*mname) {
-				pr_warn("anon member in struct %s is not supported\n",
-					st_ops->name);
-				break;
-			}
+void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
+{
+	struct bpf_struct_ops *st_ops;
+	s32 module_id;
+	u32 i;
 
-			if (__btf_member_bitfield_size(t, member)) {
-				pr_warn("bit field member %s in struct %s is not supported\n",
-					mname, st_ops->name);
-				break;
-			}
+	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
+#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
+#include "bpf_struct_ops_types.h"
+#undef BPF_STRUCT_OPS_TYPE
 
-			func_proto = btf_type_resolve_func_ptr(btf,
-							       member->type,
-							       NULL);
-			if (func_proto &&
-			    btf_distill_func_proto(log, btf,
-						   func_proto, mname,
-						   &st_ops->func_models[j])) {
-				pr_warn("Error in parsing func ptr %s in struct %s\n",
-					mname, st_ops->name);
-				break;
-			}
-		}
+	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
+	if (module_id < 0) {
+		pr_warn("Cannot find struct module in btf_vmlinux\n");
+		return;
+	}
+	module_type = btf_type_by_id(btf, module_id);
 
-		if (j == btf_type_vlen(t)) {
-			if (st_ops->init(btf)) {
-				pr_warn("Error in init bpf_struct_ops %s\n",
-					st_ops->name);
-			} else {
-				st_ops->type_id = type_id;
-				st_ops->type = t;
-				st_ops->value_id = value_id;
-				st_ops->value_type = btf_type_by_id(btf,
-								    value_id);
-			}
-		}
+	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
+		st_ops = bpf_struct_ops[i];
+		bpf_struct_ops_init_one(st_ops, btf, log);
 	}
 }