diff mbox series

[bpf,2/3] selftests/bpf: Double the size of test_loader log

Message ID 20240102193531.3169422-3-iii@linux.ibm.com (mailing list archive)
State Accepted
Commit 445aea5afda4759c13dc5c492b309cc1d5c1c486
Delegated to: BPF
Headers show
Series s390/bpf: Fix gotol with large offsets | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 11 maintainers not CCed: sdf@google.com haoluo@google.com martin.lau@linux.dev jolsa@kernel.org kpsingh@kernel.org shuah@kernel.org yonghong.song@linux.dev linux-kselftest@vger.kernel.org mykolal@fb.com song@kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-37 fail Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Ilya Leoshkevich Jan. 2, 2024, 7:30 p.m. UTC 
Testing long jumps requires having >32k instructions. That many
instructions require the verifier log buffer of 2 megabytes.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/test_loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yonghong Song Jan. 3, 2024, 12:41 a.m. UTC | #1 
On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> Testing long jumps requires having >32k instructions. That many
> instructions require the verifier log buffer of 2 megabytes.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tools/testing/selftests/bpf/test_loader.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 37ffa57f28a1..b0bfcc8d4638 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -12,7 +12,7 @@
>   #define str_has_pfx(str, pfx) \
>   	(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
>   
> -#define TEST_LOADER_LOG_BUF_SZ 1048576
> +#define TEST_LOADER_LOG_BUF_SZ 2097152

I think this patch is not necessary.
If the log buffer size is not enough, the kernel
verifier will wrap around and overwrite some initial states,
but all later states are still preserved. In my opinion,
there is really no need to increase the buffer size in this case,
esp. it is a verification success case.

>   
>   #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
>   #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
Ilya Leoshkevich Jan. 3, 2024, 7:05 a.m. UTC | #2 
On Tue, 2024-01-02 at 16:41 -0800, Yonghong Song wrote:
> 
> On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> > Testing long jumps requires having >32k instructions. That many
> > instructions require the verifier log buffer of 2 megabytes.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   tools/testing/selftests/bpf/test_loader.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/test_loader.c
> > b/tools/testing/selftests/bpf/test_loader.c
> > index 37ffa57f28a1..b0bfcc8d4638 100644
> > --- a/tools/testing/selftests/bpf/test_loader.c
> > +++ b/tools/testing/selftests/bpf/test_loader.c
> > @@ -12,7 +12,7 @@
> >   #define str_has_pfx(str, pfx) \
> >         (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx)
> > - 1 : strlen(pfx)) == 0)
> >   
> > -#define TEST_LOADER_LOG_BUF_SZ 1048576
> > +#define TEST_LOADER_LOG_BUF_SZ 2097152
> 
> I think this patch is not necessary.
> If the log buffer size is not enough, the kernel
> verifier will wrap around and overwrite some initial states,
> but all later states are still preserved. In my opinion,
> there is really no need to increase the buffer size in this case,
> esp. it is a verification success case.

What I observed in this case was that bpf_check() still returned 
-ENOSPC and failed the prog load. IIUC you are referring to the
functionality introduced by the following commit:

commit 1216640938035e63bdbd32438e91c9bcc1fd8ee1
Author: Andrii Nakryiko <andrii@kernel.org>
Date:   Thu Apr 6 16:41:49 2023 -0700

    bpf: Switch BPF verifier log to be a rotating log by default

The commit message says, among other things:

    The only user-visible change is which portion of verifier log user
    ends up seeing *if buffer is too small*.

So if we don't increase the log size, we would still have to deal with
-ENOSPC. An alternative would be to reallocate the log buffer and try
again. But I thought that for the test code we better keep it as simple
as possible.
 
> >   #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
> >   #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
Yonghong Song Jan. 3, 2024, 6:15 p.m. UTC | #3 
On 1/2/24 11:05 PM, Ilya Leoshkevich wrote:
> On Tue, 2024-01-02 at 16:41 -0800, Yonghong Song wrote:
>> On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
>>> Testing long jumps requires having >32k instructions. That many
>>> instructions require the verifier log buffer of 2 megabytes.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    tools/testing/selftests/bpf/test_loader.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/test_loader.c
>>> b/tools/testing/selftests/bpf/test_loader.c
>>> index 37ffa57f28a1..b0bfcc8d4638 100644
>>> --- a/tools/testing/selftests/bpf/test_loader.c
>>> +++ b/tools/testing/selftests/bpf/test_loader.c
>>> @@ -12,7 +12,7 @@
>>>    #define str_has_pfx(str, pfx) \
>>>          (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx)
>>> - 1 : strlen(pfx)) == 0)
>>>    
>>> -#define TEST_LOADER_LOG_BUF_SZ 1048576
>>> +#define TEST_LOADER_LOG_BUF_SZ 2097152
>> I think this patch is not necessary.
>> If the log buffer size is not enough, the kernel
>> verifier will wrap around and overwrite some initial states,
>> but all later states are still preserved. In my opinion,
>> there is really no need to increase the buffer size in this case,
>> esp. it is a verification success case.
> What I observed in this case was that bpf_check() still returned
> -ENOSPC and failed the prog load. IIUC you are referring to the
> functionality introduced by the following commit:
>
> commit 1216640938035e63bdbd32438e91c9bcc1fd8ee1
> Author: Andrii Nakryiko <andrii@kernel.org>
> Date:   Thu Apr 6 16:41:49 2023 -0700
>
>      bpf: Switch BPF verifier log to be a rotating log by default
>
> The commit message says, among other things:
>
>      The only user-visible change is which portion of verifier log user
>      ends up seeing *if buffer is too small*.
>
> So if we don't increase the log size, we would still have to deal with
> -ENOSPC. An alternative would be to reallocate the log buffer and try
> again. But I thought that for the test code we better keep it as simple
> as possible.

Okay, thanks for the explanation. I applied the patch set to
my local env and indeed, with this patch, I can see libbpf returns
an error. So as you suggested, let us increase the buffer size to
avoid extra handling in test_progs. So

Acked-by: Yonghong Song <yonghong.song@linux.dev>

>   
>>>    #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
>>>    #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
John Fastabend Jan. 3, 2024, 6:43 p.m. UTC | #4 
Yonghong Song wrote:
> 
> On 1/2/24 11:05 PM, Ilya Leoshkevich wrote:
> > On Tue, 2024-01-02 at 16:41 -0800, Yonghong Song wrote:
> >> On 1/2/24 11:30 AM, Ilya Leoshkevich wrote:
> >>> Testing long jumps requires having >32k instructions. That many
> >>> instructions require the verifier log buffer of 2 megabytes.
> >>>
> >>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> >>> ---
> >>>    tools/testing/selftests/bpf/test_loader.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/test_loader.c
> >>> b/tools/testing/selftests/bpf/test_loader.c
> >>> index 37ffa57f28a1..b0bfcc8d4638 100644
> >>> --- a/tools/testing/selftests/bpf/test_loader.c
> >>> +++ b/tools/testing/selftests/bpf/test_loader.c
> >>> @@ -12,7 +12,7 @@
> >>>    #define str_has_pfx(str, pfx) \
> >>>          (strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx)
> >>> - 1 : strlen(pfx)) == 0)
> >>>    
> >>> -#define TEST_LOADER_LOG_BUF_SZ 1048576
> >>> +#define TEST_LOADER_LOG_BUF_SZ 2097152
> >> I think this patch is not necessary.
> >> If the log buffer size is not enough, the kernel
> >> verifier will wrap around and overwrite some initial states,
> >> but all later states are still preserved. In my opinion,
> >> there is really no need to increase the buffer size in this case,
> >> esp. it is a verification success case.
> > What I observed in this case was that bpf_check() still returned
> > -ENOSPC and failed the prog load. IIUC you are referring to the
> > functionality introduced by the following commit:
> >
> > commit 1216640938035e63bdbd32438e91c9bcc1fd8ee1
> > Author: Andrii Nakryiko <andrii@kernel.org>
> > Date:   Thu Apr 6 16:41:49 2023 -0700
> >
> >      bpf: Switch BPF verifier log to be a rotating log by default
> >
> > The commit message says, among other things:
> >
> >      The only user-visible change is which portion of verifier log user
> >      ends up seeing *if buffer is too small*.
> >
> > So if we don't increase the log size, we would still have to deal with
> > -ENOSPC. An alternative would be to reallocate the log buffer and try
> > again. But I thought that for the test code we better keep it as simple
> > as possible.
> 
> Okay, thanks for the explanation. I applied the patch set to
> my local env and indeed, with this patch, I can see libbpf returns
> an error. So as you suggested, let us increase the buffer size to
> avoid extra handling in test_progs. So
> 
> Acked-by: Yonghong Song <yonghong.song@linux.dev>

LGTM.

Acked-by: John Fastabend <john.fastabend@gmail.com>
Alexei Starovoitov Jan. 4, 2024, 9:19 p.m. UTC | #5 
On Wed, Jan 3, 2024 at 10:15 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> Okay, thanks for the explanation. I applied the patch set to
> my local env and indeed, with this patch, I can see libbpf returns
> an error.

How did you repro this?
I've tried reverting this patch, but the test in patch 3 still passes
for me without errors.
Alexei Starovoitov Jan. 4, 2024, 10:33 p.m. UTC | #6 
On Thu, Jan 4, 2024 at 1:19 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jan 3, 2024 at 10:15 AM Yonghong Song <yonghong.song@linux.dev> wrote:
> >
> >
> > Okay, thanks for the explanation. I applied the patch set to
> > my local env and indeed, with this patch, I can see libbpf returns
> > an error.
>
> How did you repro this?
> I've tried reverting this patch, but the test in patch 3 still passes
> for me without errors.

Took me a long time... I was able to repro with:

diff --git a/tools/testing/selftests/bpf/progs/verifier_gotol.c
b/tools/testing/selftests/bpf/progs/verifier_gotol.c
index 05a329ee45ee..66bdb940a40b 100644
--- a/tools/testing/selftests/bpf/progs/verifier_gotol.c
+++ b/tools/testing/selftests/bpf/progs/verifier_gotol.c
@@ -36,6 +36,7 @@ l3_%=:
         \
 SEC("socket")
 __description("gotol, large_imm")
 __success __failure_unpriv __retval(40000)
+__log_level(1)
 __naked void gotol_large_imm(void)

and then I finally realized that this patch is fixing
the case when test_progs runs with -v. Like:
./test_progs -t gotol -v

I wish you mentioned this in the commit log.
Would have saved me a ton of time.
Yonghong Song Jan. 4, 2024, 10:38 p.m. UTC | #7 
On 1/4/24 2:33 PM, Alexei Starovoitov wrote:
> On Thu, Jan 4, 2024 at 1:19 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Wed, Jan 3, 2024 at 10:15 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>
>>> Okay, thanks for the explanation. I applied the patch set to
>>> my local env and indeed, with this patch, I can see libbpf returns
>>> an error.
>> How did you repro this?
>> I've tried reverting this patch, but the test in patch 3 still passes
>> for me without errors.
> Took me a long time... I was able to repro with:
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_gotol.c
> b/tools/testing/selftests/bpf/progs/verifier_gotol.c
> index 05a329ee45ee..66bdb940a40b 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_gotol.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_gotol.c
> @@ -36,6 +36,7 @@ l3_%=:
>           \
>   SEC("socket")
>   __description("gotol, large_imm")
>   __success __failure_unpriv __retval(40000)
> +__log_level(1)
>   __naked void gotol_large_imm(void)
>
> and then I finally realized that this patch is fixing
> the case when test_progs runs with -v. Like:
> ./test_progs -t gotol -v

Sorry for replying later. Just taking a nap...
I reproduced the issue when I tried to find out
why unpriv failed with cpuv4 as I cannot remember why.
IIRC, I am using is
   ./test_progs-cpuv4 -v -t gotol

>
> I wish you mentioned this in the commit log.
> Would have saved me a ton of time.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 37ffa57f28a1..b0bfcc8d4638 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -12,7 +12,7 @@ 
 #define str_has_pfx(str, pfx) \
 	(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
 
-#define TEST_LOADER_LOG_BUF_SZ 1048576
+#define TEST_LOADER_LOG_BUF_SZ 2097152
 
 #define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
 #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"