| Message ID | 20200909031227.963161-1-yhs@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: fix test_sysctl_loop{1,2} failure due to clang change | expand |
On Tue, Sep 8, 2020 at 8:13 PM Yonghong Song <yhs@fb.com> wrote: > > Andrii reported that with latest clang, when building selftests, we have > error likes: > error: progs/test_sysctl_loop1.c:23:16: in function sysctl_tcp_mem i32 (%struct.bpf_sysctl*): > Looks like the BPF stack limit of 512 bytes is exceeded. > Please move large on stack variables into BPF per-cpu array map. > > The error is triggered by the following LLVM patch: > https://reviews.llvm.org/D87134 > > For example, the following code is from test_sysctl_loop1.c: > static __always_inline int is_tcp_mem(struct bpf_sysctl *ctx) > { > volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; > ... > } > Without the above LLVM patch, the compiler did optimization to load the string > (59 bytes long) with 7 64bit loads, 1 8bit load and 1 16bit load, > occupying 64 byte stack size. > > With the above LLVM patch, the compiler only uses 8bit loads, but subregister is 32bit. > So stack requirements become 4 * 59 = 236 bytes. Together with other stuff on > the stack, total stack size exceeds 512 bytes, hence compiler complains and quits. > > To fix the issue, removing "volatile" key word or changing "volatile" to > "const"/"static const" does not work, the string is put in .rodata.str1.1 section, > which libbpf did not process it and errors out with Yeah, those .str1.1 sections are becoming more prominent, I think I'll be able to fix it soon, just need the right BTF APIs implemented first. > libbpf: elf: skipping unrecognized data section(6) .rodata.str1.1 > libbpf: prog 'sysctl_tcp_mem': bad map relo against '.L__const.is_tcp_mem.tcp_mem_name' > in section '.rodata.str1.1' > > Defining the string const as global variable can fix the issue as it puts the string constant > in '.rodata' section which is recognized by libbpf. In the future, when libbpf can process > '.rodata.str*.*' properly, the global definition can be changed back to local definition. > > Reported-by: Andrii Nakryiko <andriin@fb.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- thanks! Acked-by: Andrii Nakryiko <andriin@fb.com> > tools/testing/selftests/bpf/progs/test_sysctl_loop1.c | 2 +- > tools/testing/selftests/bpf/progs/test_sysctl_loop2.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c > index 458b0d69133e..4b600b1f522f 100644 > --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c > +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c > @@ -18,9 +18,9 @@ > #define MAX_ULONG_STR_LEN 7 > #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) > > +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; Do you think we should put "volatile" there just in case Clang decides to be very optimizing and smart? > static __always_inline int is_tcp_mem(struct bpf_sysctl *ctx) > { > - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; > unsigned char i; > char name[64]; > int ret; > diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c > index b2e6f9b0894d..3c292c087395 100644 > --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c > +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c > @@ -18,9 +18,9 @@ > #define MAX_ULONG_STR_LEN 7 > #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) > > +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; > static __attribute__((noinline)) int is_tcp_mem(struct bpf_sysctl *ctx) > { > - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; > unsigned char i; > char name[64]; > int ret; > -- > 2.24.1 >
On 9/8/20 8:20 PM, Andrii Nakryiko wrote: > On Tue, Sep 8, 2020 at 8:13 PM Yonghong Song <yhs@fb.com> wrote: >> >> Andrii reported that with latest clang, when building selftests, we have >> error likes: >> error: progs/test_sysctl_loop1.c:23:16: in function sysctl_tcp_mem i32 (%struct.bpf_sysctl*): >> Looks like the BPF stack limit of 512 bytes is exceeded. >> Please move large on stack variables into BPF per-cpu array map. >> >> The error is triggered by the following LLVM patch: >> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D87134&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=MY4KJcuRPEAEtKz22Ylp90Su0nIcw8XHrqU5G6JygYw&s=gGtiXV8_9aQuXsJxyzNmh6CNLKMibukbD2X2QJj3tgw&e= >> >> For example, the following code is from test_sysctl_loop1.c: >> static __always_inline int is_tcp_mem(struct bpf_sysctl *ctx) >> { >> volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; >> ... >> } >> Without the above LLVM patch, the compiler did optimization to load the string >> (59 bytes long) with 7 64bit loads, 1 8bit load and 1 16bit load, >> occupying 64 byte stack size. >> >> With the above LLVM patch, the compiler only uses 8bit loads, but subregister is 32bit. >> So stack requirements become 4 * 59 = 236 bytes. Together with other stuff on >> the stack, total stack size exceeds 512 bytes, hence compiler complains and quits. >> >> To fix the issue, removing "volatile" key word or changing "volatile" to >> "const"/"static const" does not work, the string is put in .rodata.str1.1 section, >> which libbpf did not process it and errors out with > > Yeah, those .str1.1 sections are becoming more prominent, I think I'll > be able to fix it soon, just need the right BTF APIs implemented > first. > >> libbpf: elf: skipping unrecognized data section(6) .rodata.str1.1 >> libbpf: prog 'sysctl_tcp_mem': bad map relo against '.L__const.is_tcp_mem.tcp_mem_name' >> in section '.rodata.str1.1' >> >> Defining the string const as global variable can fix the issue as it puts the string constant >> in '.rodata' section which is recognized by libbpf. In the future, when libbpf can process >> '.rodata.str*.*' properly, the global definition can be changed back to local definition. >> >> Reported-by: Andrii Nakryiko <andriin@fb.com> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- > > thanks! > > Acked-by: Andrii Nakryiko <andriin@fb.com> > >> tools/testing/selftests/bpf/progs/test_sysctl_loop1.c | 2 +- >> tools/testing/selftests/bpf/progs/test_sysctl_loop2.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c >> index 458b0d69133e..4b600b1f522f 100644 >> --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c >> +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c >> @@ -18,9 +18,9 @@ >> #define MAX_ULONG_STR_LEN 7 >> #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) >> >> +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; > > Do you think we should put "volatile" there just in case Clang decides > to be very optimizing and smart? In this particular case, 'volatile' is not needed since volatile is mostly used to keep compiler optimizing away the variable. In this case, since it is a global, the compiler has no way to optimize it away, so we should be fine here. It should appear in skeleton .rodata section. Since the variable is defined as a 'const', compiler feels free to do optimization, e.g., doing actual loads and put the const strings on the stack, in certain cases. Even for this, the constant string should still be available in .rodata section since it is a global and somebody else may references it. > >> static __always_inline int is_tcp_mem(struct bpf_sysctl *ctx) >> { >> - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; >> unsigned char i; >> char name[64]; >> int ret; >> diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c >> index b2e6f9b0894d..3c292c087395 100644 >> --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c >> +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c >> @@ -18,9 +18,9 @@ >> #define MAX_ULONG_STR_LEN 7 >> #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) >> >> +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; >> static __attribute__((noinline)) int is_tcp_mem(struct bpf_sysctl *ctx) >> { >> - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; >> unsigned char i; >> char name[64]; >> int ret; >> -- >> 2.24.1 >>
On Tue, Sep 8, 2020 at 8:12 PM Yonghong Song <yhs@fb.com> wrote: > diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c > index b2e6f9b0894d..3c292c087395 100644 > --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c > +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c > @@ -18,9 +18,9 @@ > #define MAX_ULONG_STR_LEN 7 > #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) > > +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; > static __attribute__((noinline)) int is_tcp_mem(struct bpf_sysctl *ctx) > { > - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; It fixes the issue with new llvm, but breaks slightly older llvm: ./test_progs -n 7/21 libbpf: load bpf program failed: Permission denied libbpf: -- BEGIN DUMP LOG --- libbpf: invalid stack off=0 size=1 verification time 6975 usec stack depth 160+64 processed 889 insns (limit 1000000) max_states_per_insn 4 total_states 14 peak_states 14 mark_read 10 libbpf: -- END LOG -- libbpf: failed to load program 'sysctl_tcp_mem' libbpf: failed to load object 'test_sysctl_loop2.o' test_bpf_verif_scale:FAIL:114 #7/21 test_sysctl_loop2.o:FAIL clang --version clang version 12.0.0 (https://github.com/llvm/llvm-project.git 6f3511a01a5227358a334803c264cfce4175ca5d)
On 9/8/20 9:04 PM, Alexei Starovoitov wrote: > On Tue, Sep 8, 2020 at 8:12 PM Yonghong Song <yhs@fb.com> wrote: >> diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c >> index b2e6f9b0894d..3c292c087395 100644 >> --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c >> +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c >> @@ -18,9 +18,9 @@ >> #define MAX_ULONG_STR_LEN 7 >> #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) >> >> +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; >> static __attribute__((noinline)) int is_tcp_mem(struct bpf_sysctl *ctx) >> { >> - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; > > It fixes the issue with new llvm, but breaks slightly older llvm: Let me take a look. > ./test_progs -n 7/21 > libbpf: load bpf program failed: Permission denied > libbpf: -- BEGIN DUMP LOG --- > libbpf: > invalid stack off=0 size=1 > verification time 6975 usec > stack depth 160+64 > processed 889 insns (limit 1000000) max_states_per_insn 4 total_states > 14 peak_states 14 mark_read 10 > > libbpf: -- END LOG -- > libbpf: failed to load program 'sysctl_tcp_mem' > libbpf: failed to load object 'test_sysctl_loop2.o' > test_bpf_verif_scale:FAIL:114 > #7/21 test_sysctl_loop2.o:FAIL > > clang --version > clang version 12.0.0 (https://github.com/llvm/llvm-project.git > 6f3511a01a5227358a334803c264cfce4175ca5d) >
diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c index 458b0d69133e..4b600b1f522f 100644 --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c @@ -18,9 +18,9 @@ #define MAX_ULONG_STR_LEN 7 #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; static __always_inline int is_tcp_mem(struct bpf_sysctl *ctx) { - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; unsigned char i; char name[64]; int ret; diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c index b2e6f9b0894d..3c292c087395 100644 --- a/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c +++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop2.c @@ -18,9 +18,9 @@ #define MAX_ULONG_STR_LEN 7 #define MAX_VALUE_STR_LEN (TCP_MEM_LOOPS * MAX_ULONG_STR_LEN) +const char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; static __attribute__((noinline)) int is_tcp_mem(struct bpf_sysctl *ctx) { - volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string_to_stress_byte_loop"; unsigned char i; char name[64]; int ret;
Andrii reported that with latest clang, when building selftests, we have error likes: error: progs/test_sysctl_loop1.c:23:16: in function sysctl_tcp_mem i32 (%struct.bpf_sysctl*): Looks like the BPF stack limit of 512 bytes is exceeded. Please move large on stack variables into BPF per-cpu array map. The error is triggered by the following LLVM patch: https://reviews.llvm.org/D87134 For example, the following code is from test_sysctl_loop1.c: static __always_inline int is_tcp_mem(struct bpf_sysctl *ctx) { volatile char tcp_mem_name[] = "net/ipv4/tcp_mem/very_very_very_very_long_pointless_string"; ... } Without the above LLVM patch, the compiler did optimization to load the string (59 bytes long) with 7 64bit loads, 1 8bit load and 1 16bit load, occupying 64 byte stack size. With the above LLVM patch, the compiler only uses 8bit loads, but subregister is 32bit. So stack requirements become 4 * 59 = 236 bytes. Together with other stuff on the stack, total stack size exceeds 512 bytes, hence compiler complains and quits. To fix the issue, removing "volatile" key word or changing "volatile" to "const"/"static const" does not work, the string is put in .rodata.str1.1 section, which libbpf did not process it and errors out with libbpf: elf: skipping unrecognized data section(6) .rodata.str1.1 libbpf: prog 'sysctl_tcp_mem': bad map relo against '.L__const.is_tcp_mem.tcp_mem_name' in section '.rodata.str1.1' Defining the string const as global variable can fix the issue as it puts the string constant in '.rodata' section which is recognized by libbpf. In the future, when libbpf can process '.rodata.str*.*' properly, the global definition can be changed back to local definition. Reported-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/testing/selftests/bpf/progs/test_sysctl_loop1.c | 2 +- tools/testing/selftests/bpf/progs/test_sysctl_loop2.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)