On Tue, Feb 13, 2024 at 6:08 PM <thinker.li@xxxxxxxxx> wrote:
>
> From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
>
> This RFC is for gathering feedback/opinions on the design.
> Based on the feedback received for v1, I made some modifications.
>
> == Pointers to Shadow Copies ==
>
> With the current implementation, the code generator will create a
> pointer to a shadow copy of the struct_ops map for each map. For
> instance, if we define a testmod_1 as a struct_ops map, we can access
> its corresponding shadow variable "data" using the pointer.
>
> skel->struct_ops.testmod1->data
>
> == Shadow Info ==
>
> The code generator also generates a shadow info to describe the layout
> of the data pointed to by all these pointers. For instance, the
> following shadow info describes the layout of a struct_ops map called
> testmod_1, which has 3 members: test_1, test_2, and data.
>
> static struct bpf_struct_ops_member_info member_info_testmod_1[] = {
> {
> .name = "test_1",
> .offset = .....,
> .size = .....,
> },
> {
> .name = "test_2",
> .offset = .....,
> .size = .....,
> },
> {
> .name = "data",
> .offset = .....,
> .size = .....,
> },
> };
> static struct bpf_struct_ops_map_info map_info[] = {
> {
> .name = "testmod_1",
> .members = member_info_testmod_1,
> .cnt = ARRAY_SIZE(member_info_testmod_1),
> .data_size = sizeof(struct_ops->testmod_1),
> },
> };
> static struct bpf_struct_ops_shadow_info shadow_info = {
> .maps = map_info,
> .cnt = ARRAY_SIZE(map_info),
> };
>
> A shadow info describes the layout of the shadow copies of all
> struct_ops maps included in a skeleton. (Defined in *__shadow_info())
I must be missing something, but libbpf knows the layout of struct_ops
struct through BTF, why do we need all these descriptors?
>
> == libbpf Creates Shadow Copies ==
>
> This shadow info should be passed to bpf_object__open_skeleton() as a
> part of "opts" so that libbpf can create shadow copies with the layout
> described by the shadow info. For now, *__open() in the skeleton will
> automatically pass the shadow info to bpf_object__open_skeleton(),
> looking like the following example.
>
> static inline struct struct_ops_module *
> struct_ops_module__open(void)
> {
> DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
>
> opts.struct_ops_shadow = struct_ops_module__shadow_info();
>
> return struct_ops_module__open_opts(*** BLURB HERE ***opts);
> }
>
> The function bpf_map__initial_value() will return the shadow copy that
> is created based on the received shadow info. Therefore, in the
> function *__open_opts() in the skeleton, the pointers to shadow copies
> will be initialized with the values returned from
> bpf_map__initial_value(). For instance,
>
> obj->struct_ops.testmod_1 =
> bpf_map__initial_value(obj->maps.testmod_1, NULL);
>
I also don't get why you need to allocate some extra "shadow memory"
if we already have struct bpf_struct_ops->data pointer malloc()'ed
during bpf_map initialization, and its size matches exactly the
struct_ops's type size.
> This line of code will be included in the *__open_opts() function. If
> the opts.struct_ops_shadow is not set, bpf_map__initial_value() will
> return a NULL.
>
> ========================================
> DESCRIPTION form v1
> ========================================
>
you probably don't need to keep cover letter from previous versions if
they are not relevant anymore
[...]
>
> ---
>
> v1: https://lore.kernel.org/all/20240124224130.859921-1-thinker.li@xxxxxxxxx/
>
> Kui-Feng Lee (3):
> libbpf: Create a shadow copy for each struct_ops map if necessary.
> bpftool: generated shadow variables for struct_ops maps.
> selftests/bpf: Test if shadow variables work.
>
> tools/bpf/bpftool/gen.c | 358 +++++++++++++++++-
> tools/lib/bpf/libbpf.c | 195 +++++++++-
> tools/lib/bpf/libbpf.h | 34 +-
> tools/lib/bpf/libbpf.map | 1 +
> tools/lib/bpf/libbpf_internal.h | 1 +
> .../selftests/bpf/bpf_testmod/bpf_testmod.c | 6 +-
> .../selftests/bpf/bpf_testmod/bpf_testmod.h | 1 +
> .../bpf/prog_tests/test_struct_ops_module.c | 16 +-
> .../selftests/bpf/progs/struct_ops_module.c | 8 +
> 9 files changed, 596 insertions(+), 24 deletions(-)
>
> --
> 2.34.1
>
