On Thu, Feb 15, 2024 at 6:40 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 2/15/24 15:50, Andrii Nakryiko wrote:
> > 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?
>
> I explain it in the response for part 1.
yep, replied there. Let's keep it simple and restrict this
functionality to 64-bit host architecture
>
> >
> >>
> >> == 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.
>
Just lore link to the previous revision should be enough if anyone is
interested in history. Cover letter should represent the current state
of things.
> >
> >>
> >> ---
> >>
> >> 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
> >>
