Date:   Tue, 8 Sep 2020 20:20:53 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Andrii Nakryiko <andriin@...com>
Subject: Re: [PATCH bpf-next] selftests/bpf: fix test_sysctl_loop{1,2} failure
 due to clang change

On Tue, Sep 8, 2020 at 8:13 PM Yonghong Song <yhs@...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@...com>
> Signed-off-by: Yonghong Song <yhs@...com>
> ---

thanks!

Acked-by: Andrii Nakryiko <andriin@...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
>

Powered by blists - more mailing lists