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

From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 2/6] libbpf: rename static variables during linking
Date: Fri, 23 Apr 2021 19:36:49 -0700	[thread overview]
Message-ID: <f9d8328e-6f3e-59c7-05f5-d67b54e6b4ce@fb.com> (raw)
In-Reply-To: <CAEf4BzZifOFHr4gozUuSFTh7rTWu2cE_-L4H1shLV5OKyQ92uw@mail.gmail.com>

On 4/23/21 5:13 PM, Andrii Nakryiko wrote:
> On Fri, Apr 23, 2021 at 4:48 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Fri, Apr 23, 2021 at 4:35 PM Andrii Nakryiko
>> <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Fri, Apr 23, 2021 at 4:06 PM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> On Fri, Apr 23, 2021 at 2:56 PM Yonghong Song <yhs@fb.com> wrote:
>>>>>>>>
>>>>>>>> -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.
>>>
>>>> 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.
> 
> Static maps are slightly different, because we use SEC() which marks
> them as used, so they should always be present.
> 
> 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.
> 
>> Only volatile static can theoretically be in the skeleton, but as you said
>> probably no one is using them yet, so we can omit them from skeleton too.

I think it is a good idea to keep volatile static use case in
the skeleton if we add support for static map. The volatile
static variable essentially a private map. Without this,
for skeleton users, they may need to use an explicit one-element
static array map which we probably want to avoid.