On Thu, Jan 25, 2024 at 3:13 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote: > > > > On 1/25/24 14:21, Andrii Nakryiko wrote: > > On Wed, Jan 24, 2024 at 2:41 PM <thinker.li@xxxxxxxxx> wrote: > >> > >> From: Kui-Feng Lee <thinker.li@xxxxxxxxx> > >> > >> Create shadow variables for the fields of struct_ops maps in a skeleton > >> with the same name as the respective fields. For instance, if struct > >> bpf_testmod_ops has a "data" field as following, you can access or modify > >> its value through a shadow variable also named "data" in the skeleton. > >> > >> SEC(".struct_ops.link") > >> struct bpf_testmod_ops testmod_1 = { > >> .data = 0x1, > >> }; > >> > >> Then, you can change the value in the following manner as shown in the code > >> below. > >> > >> skel->st_ops_vars.testmod_1.data = 13; > >> > >> It is helpful to configure a struct_ops map during the execution of a > >> program. For instance, you can include a flag in a struct_ops type to > >> modify the way the kernel handles an instance. By implementing this > >> feature, a user space program can alter the flag's value prior to loading > >> an instance into the kernel. > >> > >> The code generator for skeletons will produce code that copies values to > >> shadow variables from the internal data buffer when a skeleton is > >> opened. It will also copy values from shadow variables back to the internal > >> data buffer before a skeleton is loaded into the kernel. > >> > >> The code generator will calculate the offset of a field and generate code > >> that copies the value of the field from or to the shadow variable to or > >> from the struct_ops internal data, with an offset relative to the beginning > >> of the internal data. For instance, if the "data" field in struct > >> bpf_testmod_ops is situated 16 bytes from the beginning of the struct, the > >> address for the "data" field of struct bpf_testmod_ops will be the starting > >> address plus 16. > >> > >> The offset is calculated during code generation, so it is always in line > >> with the internal data in the skeleton. Even if the user space programs and > >> the BPF program were not compiled together, any differences in versions > >> would not impact correctness. This means that even if the BPF program and > >> the user space program reference different versions of the "struct > >> bpf_testmod_ops" and have different offsets for "data", these offsets > >> computed by the code generator would still function correctly. > >> > >> The user space programs can only modify values by using these shadow > >> variables when the skeleton is open, but before being loaded. Once the > >> skeleton is loaded, the value is copied to the kernel, and any future > >> changes only affect the shadow variables in the user space memory and do > >> not update the kernel space. The shadow variables are not initialized > >> before opening a skeleton, so you cannot update values through them before > >> opening. > >> > >> For function pointers (operators), you can change/replace their values with > >> other BPF programs. For example, the test case in test_struct_ops_module.c > >> points .test_2 to test_3() while it was pointed to test_2() by assigning a > >> new value to the shadow variable. > >> > >> skel->st_ops_vars.testmod_1.test_2 = skel->progs.test_3; > >> > >> However, you need to turn off autoload for test_2() since it is not > >> assigned to any struct_ops map anymore. Or, it will fails to load test_2(). > >> > >> bpf_program__set_autoload(skel->progs.test_2, false); > >> > >> You can define more struct_ops programs than necessary. However, you need > >> to turn autoload off for unused ones. > > > > Overall I like the idea, it seems like a pretty natural and powerful > > interface. Few things I'd do a bit differently: > > > > - less code gen in the skeleton. It feels like it's better to teach > > libbpf to allow getting/setting intial struct_ops map "image" using > > standard bpf_map__initial_value() and bpf_map__set_initial_value(). > > That fits with other global data maps. > > Right, it is doable to move some logic from the code generator to > libbpf. The only thing should keep in the code generator should be > generating shadow variable fields in the skeleton. > > > > > - I think all struct ops maps should be in skel->struct_ops.<name>, > > not struct_ops_vars. I'd probably also combine struct_ops.link and > > no-link struct ops in one section for simplicity > > agree! > > > > > - getting underlying struct_ops BTF type should be possible to do > > through bpf_map__btf_value_type_id(), no need for struct_ops-specific > > one > > AFAIK, libbpf doesn't set def.value_type_id for struct_ops maps. > bpf_map__btf_value_type_id() doesn't return the ID of a struct_ops type. > I will check what the side effects are if def.value_type_id is set for > struct_ops maps. Yes, it doesn't right now, not sure why, though. At least we can fix that on libbpf side. > > > > > - you have a bunch of specific logic to dump INT/ENUM/PTR, I wonder if > > we can just reuse libbpf's BTF dumping API to dump everything? Though > > for prog pointers we'd want to rewrite them to `struct bpf_program *` > > field, not sure yet how hard it is. > > I am not quite sure what part you are talking about. > Do you mean the code skipping type modifiers? > The implementation skips all const (and static/volatile/... etc) to make > sure the user space programs can change these values without any > tricky type casting. > No, I'm talking about gen_st_ops_shadow_vars and gen_st_ops_func_one, which don't handle members of type STRUCT/UNION, for example. I didn't look too deeply into details of the implementation in those parts, but I don't see any reason why we shouldn't support embedded struct members there? > > > > The above is brief, so please ask questions where it's not clear what > > I propose, thanks! > > > > [...] > > Thank you for the comments. >