On 11/1/23 18:32, Martin KaFai Lau wrote:
On 11/1/23 5:59 PM, Kui-Feng Lee wrote:
On 11/1/23 17:17, Martin KaFai Lau wrote:
On 10/31/23 5:19 PM, Kui-Feng Lee wrote:
On 10/31/23 17:02, Martin KaFai Lau wrote:
On 10/31/23 4:34 PM, Kui-Feng Lee wrote:
diff --git a/include/linux/btf.h b/include/linux/btf.h
index a8813605f2f6..954536431e0b 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -12,6 +12,8 @@
#include <uapi/linux/bpf.h>
#define BTF_TYPE_EMIT(type) ((void)(type *)0)
+#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type
*)0); \
((void)(struct type *)0); is new. Why is it needed?
This is a trick of BTF to force compiler generate type info for
the given type. Without trick, compiler may skip these types if these
type are not used at all in the module. For example, modules usually
don't use value types of struct_ops directly.
It is not the value type and value type emit is understood. It is
the struct_ops type itself and it is new addition in this patchset
afaict. The value type emit is in the next line which was cut out
from the context here.
I mean both of them are required.
In the case of a dummy implementation, struct_ops type itself
properly never being used, only being declared by the module.
Without this line,
Other than bpf_dummy_ops, after reg(), the struct_ops->func() must be
used somewhere in the kernel or module. Like tcp must be using the
tcp_congestion_ops after reg(). bpf_dummy_ops is very special and
probably should be moved out to bpf_testmod somehow but this is for
later. Even bpf_dummy_ops does not have an issue now. Why it is
needed after the kmod support change?
or it is a preemptive addition to be future proof only?
Addition is fine if it is required to work. I am trying to understand
why this new addition is needed after the kmod support change. The
reason why this is needed after the kmod support change is not
obvious from looking at the code. The commit message didn't mention
why and what broke after this kmod change. If someone wants to clean
it up a few months later, we will need to figure out why it was added
in the first place.
It is a future proof.
What do you think if I add a comment in the code?
If it is not required to work, I prefer not adding it to avoid confusion
and avoid future cleanup temptation. Even the artificial bpf_dummy_ops
does not need it, so not enough reason to introduce this code redundancy.
Got it!
Switch topic.
While we are on a new macro topic, I think a new macro will be useful to
emit the value type and register_bpf_struct_ops together. wdyt?
Like this?
#define REGISTER_STRUCT_OPS(st_type, st_ops) { \
BTF_STRUCT_OPS_TYPE_EMIT(st_type); \
register_bpf_struct_ops(st_ops); } while(0)
static int bpf_testmod_init(void) {
....
REGISTER_STRUCT_OPS(bpf_testmod_ops, &bpf_bpf_testmod_ops);
....
}
or you like something more aggressive
#define REGISTER_STRUCT_OPS(st_type) { \
BTF_STRUCT_OPS_TYPE_EMIT(st_type); \
register_bpf_struct_ops(&bpf_##st_type); } while(0)
the module developer will fail to load a struct_ops map of the dummy
type. This line is added to avoid this awful situation.
