diff mbox series

[bpf] selftests/bpf: Relax time_tai test for equal timestamps in tai_forward

Message ID 20231212182911.3784108-1-zhuyifei@google.com (mailing list archive)
State Accepted
Commit e1ba7f64b192f083b4423644be03bb9e3dc8ae84
Delegated to: BPF
Headers show
Series [bpf] selftests/bpf: Relax time_tai test for equal timestamps in tai_forward | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
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 9 maintainers not CCed: kpsingh@kernel.org shuah@kernel.org yonghong.song@linux.dev mykolal@fb.com song@kernel.org haoluo@google.com linux-kselftest@vger.kernel.org jolsa@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 Fixes tag looks correct
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-5 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-7 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-6 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_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 fail Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-16 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-31 fail Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-18 / test
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-VM_Test-32 fail Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-VM_Test-17 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-20 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-22 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 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-23 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 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-27 fail Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-28 fail 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-29 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-VM_Test-19 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-18 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-12 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-14 success Logs for x86_64-gcc / build-release

Commit Message

YiFei Zhu Dec. 12, 2023, 6:29 p.m. UTC 
We're observing test flakiness on an arm64 platform which might not
have timestamps as precise as x86. The test log looks like:

  test_time_tai:PASS:tai_open 0 nsec
  test_time_tai:PASS:test_run 0 nsec
  test_time_tai:PASS:tai_ts1 0 nsec
  test_time_tai:PASS:tai_ts2 0 nsec
  test_time_tai:FAIL:tai_forward unexpected tai_forward: actual 1702348135471494160 <= expected 1702348135471494160
  test_time_tai:PASS:tai_gettime 0 nsec
  test_time_tai:PASS:tai_future_ts1 0 nsec
  test_time_tai:PASS:tai_future_ts2 0 nsec
  test_time_tai:PASS:tai_range_ts1 0 nsec
  test_time_tai:PASS:tai_range_ts2 0 nsec
  #199     time_tai:FAIL

This patch changes ASSERT_GT to ASSERT_GE in the tai_forward assertion
so that equal timestamps are permitted.

Fixes: 64e15820b987 ("selftests/bpf: Add BPF-helper test for CLOCK_TAI access")
Signed-off-by: YiFei Zhu <zhuyifei@google.com>
---
 tools/testing/selftests/bpf/prog_tests/time_tai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yonghong Song Dec. 12, 2023, 9:39 p.m. UTC | #1 
On 12/12/23 10:29 AM, YiFei Zhu wrote:
> We're observing test flakiness on an arm64 platform which might not
> have timestamps as precise as x86. The test log looks like:
>
>    test_time_tai:PASS:tai_open 0 nsec
>    test_time_tai:PASS:test_run 0 nsec
>    test_time_tai:PASS:tai_ts1 0 nsec
>    test_time_tai:PASS:tai_ts2 0 nsec
>    test_time_tai:FAIL:tai_forward unexpected tai_forward: actual 1702348135471494160 <= expected 1702348135471494160
>    test_time_tai:PASS:tai_gettime 0 nsec
>    test_time_tai:PASS:tai_future_ts1 0 nsec
>    test_time_tai:PASS:tai_future_ts2 0 nsec
>    test_time_tai:PASS:tai_range_ts1 0 nsec
>    test_time_tai:PASS:tai_range_ts2 0 nsec
>    #199     time_tai:FAIL
>
> This patch changes ASSERT_GT to ASSERT_GE in the tai_forward assertion
> so that equal timestamps are permitted.
>
> Fixes: 64e15820b987 ("selftests/bpf: Add BPF-helper test for CLOCK_TAI access")
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> ---
>   tools/testing/selftests/bpf/prog_tests/time_tai.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/time_tai.c b/tools/testing/selftests/bpf/prog_tests/time_tai.c
> index a31119823666..f45af1b0ef2c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/time_tai.c
> +++ b/tools/testing/selftests/bpf/prog_tests/time_tai.c
> @@ -56,7 +56,7 @@ void test_time_tai(void)
>   	ASSERT_NEQ(ts2, 0, "tai_ts2");
>   
>   	/* TAI is moving forward only */
> -	ASSERT_GT(ts2, ts1, "tai_forward");
> +	ASSERT_GE(ts2, ts1, "tai_forward");

Can we guard the new change with arm64 specific macro?

>   
>   	/* Check for future */
>   	ret = clock_gettime(CLOCK_TAI, &now_tai);
YiFei Zhu Dec. 12, 2023, 9:52 p.m. UTC | #2 
On Tue, Dec 12, 2023 at 1:39 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 12/12/23 10:29 AM, YiFei Zhu wrote:
> > We're observing test flakiness on an arm64 platform which might not
> > have timestamps as precise as x86. The test log looks like:
> >
> >    test_time_tai:PASS:tai_open 0 nsec
> >    test_time_tai:PASS:test_run 0 nsec
> >    test_time_tai:PASS:tai_ts1 0 nsec
> >    test_time_tai:PASS:tai_ts2 0 nsec
> >    test_time_tai:FAIL:tai_forward unexpected tai_forward: actual 1702348135471494160 <= expected 1702348135471494160
> >    test_time_tai:PASS:tai_gettime 0 nsec
> >    test_time_tai:PASS:tai_future_ts1 0 nsec
> >    test_time_tai:PASS:tai_future_ts2 0 nsec
> >    test_time_tai:PASS:tai_range_ts1 0 nsec
> >    test_time_tai:PASS:tai_range_ts2 0 nsec
> >    #199     time_tai:FAIL
> >
> > This patch changes ASSERT_GT to ASSERT_GE in the tai_forward assertion
> > so that equal timestamps are permitted.
> >
> > Fixes: 64e15820b987 ("selftests/bpf: Add BPF-helper test for CLOCK_TAI access")
> > Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> > ---
> >   tools/testing/selftests/bpf/prog_tests/time_tai.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/time_tai.c b/tools/testing/selftests/bpf/prog_tests/time_tai.c
> > index a31119823666..f45af1b0ef2c 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/time_tai.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/time_tai.c
> > @@ -56,7 +56,7 @@ void test_time_tai(void)
> >       ASSERT_NEQ(ts2, 0, "tai_ts2");
> >
> >       /* TAI is moving forward only */
> > -     ASSERT_GT(ts2, ts1, "tai_forward");
> > +     ASSERT_GE(ts2, ts1, "tai_forward");
>
> Can we guard the new change with arm64 specific macro?

Problem with this is that I'm not sure what other architectures could
be affected. AFAICT from the test, what it cares about is that time is
moving forwards rather than going backwards, so I thought GE is good
enough for what it's testing for.

>
> >
> >       /* Check for future */
> >       ret = clock_gettime(CLOCK_TAI, &now_tai);
patchwork-bot+netdevbpf@kernel.org Dec. 13, 2023, midnight UTC | #3 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Tue, 12 Dec 2023 18:29:11 +0000 you wrote:
> We're observing test flakiness on an arm64 platform which might not
> have timestamps as precise as x86. The test log looks like:
> 
>   test_time_tai:PASS:tai_open 0 nsec
>   test_time_tai:PASS:test_run 0 nsec
>   test_time_tai:PASS:tai_ts1 0 nsec
>   test_time_tai:PASS:tai_ts2 0 nsec
>   test_time_tai:FAIL:tai_forward unexpected tai_forward: actual 1702348135471494160 <= expected 1702348135471494160
>   test_time_tai:PASS:tai_gettime 0 nsec
>   test_time_tai:PASS:tai_future_ts1 0 nsec
>   test_time_tai:PASS:tai_future_ts2 0 nsec
>   test_time_tai:PASS:tai_range_ts1 0 nsec
>   test_time_tai:PASS:tai_range_ts2 0 nsec
>   #199     time_tai:FAIL
> 
> [...]

Here is the summary with links:
  - [bpf] selftests/bpf: Relax time_tai test for equal timestamps in tai_forward
    https://git.kernel.org/bpf/bpf-next/c/e1ba7f64b192

You are awesome, thank you!
Andrii Nakryiko Dec. 13, 2023, 12:01 a.m. UTC | #4 
On Tue, Dec 12, 2023 at 1:53 PM YiFei Zhu <zhuyifei@google.com> wrote:
>
> On Tue, Dec 12, 2023 at 1:39 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >
> >
> > On 12/12/23 10:29 AM, YiFei Zhu wrote:
> > > We're observing test flakiness on an arm64 platform which might not
> > > have timestamps as precise as x86. The test log looks like:
> > >
> > >    test_time_tai:PASS:tai_open 0 nsec
> > >    test_time_tai:PASS:test_run 0 nsec
> > >    test_time_tai:PASS:tai_ts1 0 nsec
> > >    test_time_tai:PASS:tai_ts2 0 nsec
> > >    test_time_tai:FAIL:tai_forward unexpected tai_forward: actual 1702348135471494160 <= expected 1702348135471494160
> > >    test_time_tai:PASS:tai_gettime 0 nsec
> > >    test_time_tai:PASS:tai_future_ts1 0 nsec
> > >    test_time_tai:PASS:tai_future_ts2 0 nsec
> > >    test_time_tai:PASS:tai_range_ts1 0 nsec
> > >    test_time_tai:PASS:tai_range_ts2 0 nsec
> > >    #199     time_tai:FAIL
> > >
> > > This patch changes ASSERT_GT to ASSERT_GE in the tai_forward assertion
> > > so that equal timestamps are permitted.
> > >
> > > Fixes: 64e15820b987 ("selftests/bpf: Add BPF-helper test for CLOCK_TAI access")
> > > Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> > > ---
> > >   tools/testing/selftests/bpf/prog_tests/time_tai.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/time_tai.c b/tools/testing/selftests/bpf/prog_tests/time_tai.c
> > > index a31119823666..f45af1b0ef2c 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/time_tai.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/time_tai.c
> > > @@ -56,7 +56,7 @@ void test_time_tai(void)
> > >       ASSERT_NEQ(ts2, 0, "tai_ts2");
> > >
> > >       /* TAI is moving forward only */
> > > -     ASSERT_GT(ts2, ts1, "tai_forward");
> > > +     ASSERT_GE(ts2, ts1, "tai_forward");
> >
> > Can we guard the new change with arm64 specific macro?
>
> Problem with this is that I'm not sure what other architectures could
> be affected. AFAICT from the test, what it cares about is that time is
> moving forwards rather than going backwards, so I thought GE is good
> enough for what it's testing for.
>

Agreed. I think having architecture-specific GE vs GT here will just
add more complexity than necessary without providing any added safety.
So I applied the patch to bpf-next as is, thanks.

> >
> > >
> > >       /* Check for future */
> > >       ret = clock_gettime(CLOCK_TAI, &now_tai);
Kurt Kanzenbach Dec. 19, 2023, 9:57 a.m. UTC | #5 
On Tue Dec 12 2023, YiFei Zhu wrote:
>> > diff --git a/tools/testing/selftests/bpf/prog_tests/time_tai.c b/tools/testing/selftests/bpf/prog_tests/time_tai.c
>> > index a31119823666..f45af1b0ef2c 100644
>> > --- a/tools/testing/selftests/bpf/prog_tests/time_tai.c
>> > +++ b/tools/testing/selftests/bpf/prog_tests/time_tai.c
>> > @@ -56,7 +56,7 @@ void test_time_tai(void)
>> >       ASSERT_NEQ(ts2, 0, "tai_ts2");
>> >
>> >       /* TAI is moving forward only */
>> > -     ASSERT_GT(ts2, ts1, "tai_forward");
>> > +     ASSERT_GE(ts2, ts1, "tai_forward");
>>
>> Can we guard the new change with arm64 specific macro?
>
> Problem with this is that I'm not sure what other architectures could
> be affected. AFAICT from the test, what it cares about is that time is
> moving forwards rather than going backwards, so I thought GE is good
> enough for what it's testing for.

Yes, exactly. The time must not go backwards. Checking for GE is
fine. Thanks for fixing this.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/time_tai.c b/tools/testing/selftests/bpf/prog_tests/time_tai.c
index a31119823666..f45af1b0ef2c 100644
--- a/tools/testing/selftests/bpf/prog_tests/time_tai.c
+++ b/tools/testing/selftests/bpf/prog_tests/time_tai.c
@@ -56,7 +56,7 @@  void test_time_tai(void)
 	ASSERT_NEQ(ts2, 0, "tai_ts2");
 
 	/* TAI is moving forward only */
-	ASSERT_GT(ts2, ts1, "tai_forward");
+	ASSERT_GE(ts2, ts1, "tai_forward");
 
 	/* Check for future */
 	ret = clock_gettime(CLOCK_TAI, &now_tai);