On 5/9/2024 12:17 PM, Martin KaFai Lau wrote: > On 5/8/24 2:58 PM, Abhishek Chauhan wrote: >> With changes in the design to forward CLOCK_TAI in the skbuff >> framework, existing selftest framework needs modification >> to handle forwarding of UDP packets with CLOCK_TAI as clockid. > > The set lgtm. I have a few final nits on the test. > >> >> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@xxxxxxxxx/ >> Signed-off-by: Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx> >> --- >> Changes since v7 >> - Fixed issues in the ctx_rewrite.c >> with respect to dissembly in both >> .read and .write >> >> Changes since v6 >> - Moved all the selftest to another patch >> >> Changes since v1 - v5 >> - Patch was not present >> >> tools/include/uapi/linux/bpf.h | 15 ++++--- >> .../selftests/bpf/prog_tests/ctx_rewrite.c | 10 +++-- >> .../selftests/bpf/prog_tests/tc_redirect.c | 3 -- >> .../selftests/bpf/progs/test_tc_dtime.c | 39 +++++++++---------- >> 4 files changed, 34 insertions(+), 33 deletions(-) >> >> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h >> index 90706a47f6ff..25ea393cf084 100644 >> --- a/tools/include/uapi/linux/bpf.h >> +++ b/tools/include/uapi/linux/bpf.h > > nit. Please move this bpf.h sync changes to patch 2 where the uapi changes happen. > >> @@ -6207,12 +6207,17 @@ union { \ >> __u64 :64; \ >> } __attribute__((aligned(8))) >> +/* The enum used in skb->tstamp_type. It specifies the clock type >> + * of the time stored in the skb->tstamp. >> + */ >> enum { >> - BPF_SKB_TSTAMP_UNSPEC, >> - BPF_SKB_TSTAMP_DELIVERY_MONO, /* tstamp has mono delivery time */ >> - /* For any BPF_SKB_TSTAMP_* that the bpf prog cannot handle, >> - * the bpf prog should handle it like BPF_SKB_TSTAMP_UNSPEC >> - * and try to deduce it by ingress, egress or skb->sk->sk_clockid. >> + BPF_SKB_TSTAMP_UNSPEC = 0, /* DEPRECATED */ >> + BPF_SKB_TSTAMP_DELIVERY_MONO = 1, /* DEPRECATED */ >> + BPF_SKB_CLOCK_REALTIME = 0, >> + BPF_SKB_CLOCK_MONOTONIC = 1, >> + BPF_SKB_CLOCK_TAI = 2, >> + /* For any future BPF_SKB_CLOCK_* that the bpf prog cannot handle, >> + * the bpf prog can try to deduce it by ingress/egress/skb->sk->sk_clockid. >> */ >> }; >> diff --git a/tools/testing/selftests/bpf/prog_tests/ctx_rewrite.c b/tools/testing/selftests/bpf/prog_tests/ctx_rewrite.c >> index 3b7c57fe55a5..08b6391f2f56 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/ctx_rewrite.c >> +++ b/tools/testing/selftests/bpf/prog_tests/ctx_rewrite.c >> @@ -69,15 +69,17 @@ static struct test_case test_cases[] = { >> { >> N(SCHED_CLS, struct __sk_buff, tstamp), >> .read = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);" >> - "w11 &= 3;" >> - "if w11 != 0x3 goto pc+2;" >> + "if w11 & 0x4 goto pc+1;" >> + "goto pc+4;" >> + "if w11 & 0x3 goto pc+1;" >> + "goto pc+2;" >> "$dst = 0;" >> "goto pc+1;" >> "$dst = *(u64 *)($ctx + sk_buff::tstamp);", >> .write = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);" >> - "if w11 & 0x2 goto pc+1;" >> + "if w11 & 0x4 goto pc+1;" >> "goto pc+2;" >> - "w11 &= -2;" >> + "w11 &= -4;" >> "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;" >> "*(u64 *)($ctx + sk_buff::tstamp) = $src;", >> }, >> diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c >> index b1073d36d77a..327d51f59142 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c >> +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c >> @@ -890,9 +890,6 @@ static void test_udp_dtime(struct test_tc_dtime *skel, int family, bool bpf_fwd) >> ASSERT_EQ(dtimes[INGRESS_FWDNS_P100], 0, >> dtime_cnt_str(t, INGRESS_FWDNS_P100)); >> - /* non mono delivery time is not forwarded */ >> - ASSERT_EQ(dtimes[INGRESS_FWDNS_P101], 0, >> - dtime_cnt_str(t, INGRESS_FWDNS_P101)); >> for (i = EGRESS_FWDNS_P100; i < SET_DTIME; i++) >> ASSERT_GT(dtimes[i], 0, dtime_cnt_str(t, i)); >> diff --git a/tools/testing/selftests/bpf/progs/test_tc_dtime.c b/tools/testing/selftests/bpf/progs/test_tc_dtime.c >> index 74ec09f040b7..21f5be202e4b 100644 >> --- a/tools/testing/selftests/bpf/progs/test_tc_dtime.c >> +++ b/tools/testing/selftests/bpf/progs/test_tc_dtime.c >> @@ -222,13 +222,19 @@ int egress_host(struct __sk_buff *skb) >> return TC_ACT_OK; >> if (skb_proto(skb_type) == IPPROTO_TCP) { >> - if (skb->tstamp_type == BPF_SKB_TSTAMP_DELIVERY_MONO && >> + if (skb->tstamp_type == BPF_SKB_CLOCK_MONOTONIC && >> + skb->tstamp) >> + inc_dtimes(EGRESS_ENDHOST); >> + else >> + inc_errs(EGRESS_ENDHOST); >> + } else if (skb_proto(skb_type) == IPPROTO_UDP) { >> + if (skb->tstamp_type == BPF_SKB_CLOCK_TAI && >> skb->tstamp) >> inc_dtimes(EGRESS_ENDHOST); >> else >> inc_errs(EGRESS_ENDHOST); >> } else { >> - if (skb->tstamp_type == BPF_SKB_TSTAMP_UNSPEC && >> + if (skb->tstamp_type == BPF_SKB_CLOCK_REALTIME && >> skb->tstamp) > > Since the UDP+TAI can be handled properly in the above "else if" case now, I would like to further tighten the bolt on detecting the non-zero REALTIME skb->tstamp here since it should not happen at egress. Something like: > > } else { > if (skb->tstamp_type == BPF_SKB_CLOCK_REALTIME && > skb->tstamp) > inc_errs(EGRESS_ENDHOST); > } > > I ran the test (w or w/o the above inc_errs changes) in a loop and it consistently passes now. > > Other than the above small nits, in the next re-spin, please remove the RFC tag and you can carry my reviewed-by to all 3 patches. Thanks. > Noted! Thank you Martin and Willem for helping me with this series. And all the design discussion we had throughout the series. Appreciate all the comments from yourside. I will raise the last series with no RFC tag 1. carry Reviewed-by: 2. Fix all the above comments > Reviewed-by: Martin KaFai Lau <martin.lau@xxxxxxxxxx> > >> inc_dtimes(EGRESS_ENDHOST); >> else >
