Re: [PATCH v2 bpf-next 2/6] libbpf: rename static variables during linking

On Fri, Apr 23, 2021 at 5:13 PM Andrii Nakryiko
[off-list ref] wrote:

On Fri, Apr 23, 2021 at 4:48 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Fri, Apr 23, 2021 at 4:35 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Fri, Apr 23, 2021 at 4:06 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Fri, Apr 23, 2021 at 2:56 PM Yonghong Song [off-list ref] wrote:

quoted

quoted

quoted

quoted

-static volatile const __u32 print_len;
-static volatile const __u32 ret1;
+volatile const __u32 print_len = 0;
+volatile const __u32 ret1 = 0;

I am little bit puzzled why bpf_iter_test_kern4.c is impacted. I think
this is not in a static link test, right? The same for a few tests below.

All the selftests are passed through a static linker, so it will
append obj_name to each static variable. So I just minimized use of
static variables to avoid too much code churn. If this variable was
static, it would have to be accessed as
skel->rodata->bpf_iter_test_kern4__print_len, for example.

Okay this should be fine. selftests/bpf specific. I just feel that
some people may get confused if they write/see a single program in
selftest and they have to use obj_varname format and thinking this
is a new standard, but actually it is due to static linking buried
in Makefile. Maybe add a note in selftests/README.rst so we
can point to people if there is confusion.

I'm not sure I understand.
Are you saying that
bpftool gen object out_file.o in_file.o
is no longer equivalent to llvm-strip ?
Since during that step static vars will get their names mangled?

Yes. Static vars and static maps. We don't allow (yet?) static
entry-point BPF programs, so those don't change.

quoted

So a good chunk of code that uses skeleton right now should either
1. don't do the linking step
or
2. adjust their code to use global vars
or
3. adjust the usage of skel.h in their corresponding user code
  to accommodate mangled static names?
Did it get it right?

Yes, you are right. But so far most cases outside of selftest that
I've seen don't use static variables (partially because they need
pesky volatile to be visible from user-space at all), global vars are
much nicer in that regard.

Right.
but wait...
why linker is mangling them at all and why they appear in the skeleton?
static vars without volatile should not be in a skeleton, since changing
them from user space might have no meaning on the bpf program.
The behavior of the bpf prog is unpredictable.

It's up to the compiler. If compiler decides that it shouldn't inline
all the uses (or e.g. if static variable is an array accessed with
index known only at runtime, or many other cases where compiler can't
just deduce constant value), then compiler will emit ELF symbols, will
allocate storage, and code will use that storage. static volatile just
forces the compiler to not assume anything at all.

If the compiler does inline all the uses of static, then we won't have
storage allocated for it and it won't be even present in BTF. So for
libbpf, linker and skeleton statics are no different than globals.

Not quite.
The compiler can inline static values in some cases and still
keep the var in global data in others places in the same C file.
If it's a static struct the compiler is free to do any crazy optimization
on this data it can come up with.
So the presence of it in the elf symbols doesn't guarantee anything.

Static maps are slightly different, because we use SEC() which marks
them as used, so they should always be present.

yes. The used attribute makes the compiler keep the data,
but it can still inline it and lose the reference in the .text.

Sub-skeleton will present those statics to the BPF library without
name mangling, but for the final linked BPF object file we need to
handle statics. Definitely for maps, because static means that library
or library user shouldn't be able to just extern that definition and
update/lookup/corrupt its state. But I think for static variables it
should be the same. Both are visible to user-space, but invisible
between linked BPF compilation units.

I agree with motivation and also agree that static map def is a clean
way to hide such a map inside the library.
I'm not sure yet that static + used is enough.
More study needed.
For maps it's only the reference inside .text that is relevant.
If the compiler messes with it then we might have issues.
Like two static maps might look the same to the compiler.
They will have different field names, but their contents are zeros
in both cases. I'm not sure that the compiler will not decide to merge them
into one data location. In such case .text insns will be pointing
to the same data offset. The symbol names are probably going to be different.
So we might be ok with static+used only.