On Fri, Apr 23, 2021 at 7:36 PM Yonghong Song <yhs@xxxxxx> wrote: > > > > On 4/23/21 5:13 PM, Andrii Nakryiko wrote: > > On Fri, Apr 23, 2021 at 4:48 PM Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> wrote: > >> > >> On Fri, Apr 23, 2021 at 4:35 PM Andrii Nakryiko > >> <andrii.nakryiko@xxxxxxxxx> wrote: > >>> > >>> On Fri, Apr 23, 2021 at 4:06 PM Alexei Starovoitov > >>> <alexei.starovoitov@xxxxxxxxx> wrote: > >>>> > >>>> On Fri, Apr 23, 2021 at 2:56 PM Yonghong Song <yhs@xxxxxx> 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?
