Re: [RFC bpf-next v2 0/3] Create shadow variables for struct_ops in skeletons

[Kui-Feng Lee <sinquersw@gmail.com>]

On 2/15/24 15:50, Andrii Nakryiko wrote:
> On Tue, Feb 13, 2024 at 6:08 PM <thinker.li@gmail.com> wrote:
>>
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> 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?

I explain it in the response for part 1.

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


I assume that the alignments & padding of BPF and the user space
programs are different. (Check the response for part 1 as well)

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


Sure!
It explains what the feature is and how to use this feature.
So, I keep it here for people just found this discussion.

> 
>>
>> ---
>>
>> v1: https://lore.kernel.org/all/20240124224130.859921-1-thinker.li@gmail.com/
>>
>> 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
>>