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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Yonghong Song <yhs@fb.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	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: Mon, 26 Apr 2021 08:45:07 -0700	[thread overview]
Message-ID: <CAEf4BzahtXOck_TTtvY4bRtnyqEw=Cxd6T0QbTdwF=UH-p-5Dw@mail.gmail.com> (raw)
In-Reply-To: <f9d8328e-6f3e-59c7-05f5-d67b54e6b4ce@fb.com>

On Fri, Apr 23, 2021 at 7:36 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> 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.

I agree, `static volatile` definitely has to be supported. I wonder
what we should do about plain statics, though? What's your opinion,
Yonghong?