| Message ID | 20240504031331.2737365-4-quic_abchauha@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Replace mono_delivery_time with tstamp_type | expand |
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. > > Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/ > Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com> > --- > 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 > @@ -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..71940f4ef0fb 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;" Not an expert on this code, and I see that the existing code already has this below, but: isn't it odd and unnecessary to jump to an unconditional jump statement? > "$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 &= -3;" > "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;" > "*(u64 *)($ctx + sk_buff::tstamp) = $src;", > },
On 5/6/2024 12:04 PM, Willem de Bruijn wrote: > 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. >> >> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/ >> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com> >> --- >> 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 >> @@ -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..71940f4ef0fb 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;" > > Not an expert on this code, and I see that the existing code already > has this below, but: isn't it odd and unnecessary to jump to an > unconditional jump statement? > I am closely looking into your comment and i will evalute it(Martin can correct me if the jumps are correct or not as i am new to BPF as well) but i found out that JSET = "&" and not "==". So the above two ins has to change from - "if w11 == 0x4 goto pc+1;" ==>(needs to be corrected to) "if w11 & 0x4 goto pc+1;" "if w11 == 0x3 goto pc+1;" ==> (needs to be correct to) "if w11 & 0x3 goto pc+1;" >> "$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 &= -3;" Martin, Also i am not sure why the the dissembly complains because the value of SKB_TSTAMP_TYPE_MASK = 3 and we are negating it ~3 = -3. Can't match disassembly(left) with pattern(right): r11 = *(u8 *)(r1 +129) ; r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset) if w11 & 0x4 goto pc+1 ; if w11 & 0x4 goto pc+1 goto pc+2 ; goto pc+2 w11 &= -4 ; w11 &= -3 >> "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;" >> "*(u64 *)($ctx + sk_buff::tstamp) = $src;", >> },
On 5/6/2024 1:50 PM, Abhishek Chauhan (ABC) wrote: > > > On 5/6/2024 12:04 PM, Willem de Bruijn wrote: >> 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. >>> >>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/ >>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com> >>> --- >>> 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 >>> @@ -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..71940f4ef0fb 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;" >> >> Not an expert on this code, and I see that the existing code already >> has this below, but: isn't it odd and unnecessary to jump to an >> unconditional jump statement? >> > I am closely looking into your comment and i will evalute it(Martin can correct me > if the jumps are correct or not as i am new to BPF as well) but i found out that > JSET = "&" and not "==". So the above two ins has to change from - > > "if w11 == 0x4 goto pc+1;" ==>(needs to be corrected to) "if w11 & 0x4 goto pc+1;" > "if w11 == 0x3 goto pc+1;" ==> (needs to be correct to) "if w11 & 0x3 goto pc+1;" > > >>> "$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 &= -3;" > Martin, > Also i am not sure why the the dissembly complains because the value of SKB_TSTAMP_TYPE_MASK = 3 and we are > negating it ~3 = -3. > > Can't match disassembly(left) with pattern(right): > r11 = *(u8 *)(r1 +129) ; r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset) > if w11 & 0x4 goto pc+1 ; if w11 & 0x4 goto pc+1 > goto pc+2 ; goto pc+2 > w11 &= -4 ; w11 &= -3 > Martin, Please ignore this. It has to -4 and not -3. I figured it out. >>> "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;" >>> "*(u64 *)($ctx + sk_buff::tstamp) = $src;", >>> },
On 5/6/2024 1:54 PM, Abhishek Chauhan (ABC) wrote: > > > On 5/6/2024 1:50 PM, Abhishek Chauhan (ABC) wrote: >> >> >> On 5/6/2024 12:04 PM, Willem de Bruijn wrote: >>> 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. >>>> >>>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/ >>>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com> >>>> --- >>>> 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 >>>> @@ -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..71940f4ef0fb 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;" >>> >>> Not an expert on this code, and I see that the existing code already >>> has this below, but: isn't it odd and unnecessary to jump to an >>> unconditional jump statement? >>> >> I am closely looking into your comment and i will evalute it(Martin can correct me >> if the jumps are correct or not as i am new to BPF as well) but i found out that >> JSET = "&" and not "==". So the above two ins has to change from - >> >> "if w11 == 0x4 goto pc+1;" ==>(needs to be corrected to) "if w11 & 0x4 goto pc+1;" >> "if w11 == 0x3 goto pc+1;" ==> (needs to be correct to) "if w11 & 0x3 goto pc+1;" >> >> Willem, I looked at the jumps in the above code. They look correct to me. Martin can check too if i am doing anything wrong here other than the JSET "&". Ideally pc(program counter) points to the next instruction. "if w11 & 0x4 goto pc+1;" "goto pc+4;" [pc+0] "if w11 & 0x3 goto pc+1;" <== PC is going to be here [pc+1] "goto pc+2;" [pc+2] "$dst = 0;" [pc+3] "goto pc+1;" [pc+4] "$dst = *(u64 *)($ctx + sk_buff::tstamp);", <== This is where the code is intended to jump to for "goto pc+4;" >>>> "$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 &= -3;"
On 5/6/24 1:50 PM, Abhishek Chauhan (ABC) wrote: > > > On 5/6/2024 12:04 PM, Willem de Bruijn wrote: >> 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. >>> >>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/ >>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com> >>> --- >>> 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 >>> @@ -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..71940f4ef0fb 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;" >> >> Not an expert on this code, and I see that the existing code already >> has this below, but: isn't it odd and unnecessary to jump to an >> unconditional jump statement? >> > I am closely looking into your comment and i will evalute it(Martin can correct me > if the jumps are correct or not as i am new to BPF as well) but i found out that > JSET = "&" and not "==". So the above two ins has to change from - Yes, this should be bitwise "&" instead of "==". The bpf CI did report this: https://github.com/kernel-patches/bpf/actions/runs/8947652196/job/24579927178 Please monitor the bpf CI test result. Do you have issue running the test locally? > > "if w11 == 0x4 goto pc+1;" ==>(needs to be corrected to) "if w11 & 0x4 goto pc+1;" > "if w11 == 0x3 goto pc+1;" ==> (needs to be correct to) "if w11 & 0x3 goto pc+1;" > > >>> "$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 &= -3;" > Martin, > Also i am not sure why the the dissembly complains because the value of SKB_TSTAMP_TYPE_MASK = 3 and we are > negating it ~3 = -3. > > Can't match disassembly(left) with pattern(right): > r11 = *(u8 *)(r1 +129) ; r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset) > if w11 & 0x4 goto pc+1 ; if w11 & 0x4 goto pc+1 > goto pc+2 ; goto pc+2 > w11 &= -4 ; w11 &= -3 > >>> "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;" >>> "*(u64 *)($ctx + sk_buff::tstamp) = $src;", >>> },
On 5/6/2024 5:54 PM, Martin KaFai Lau wrote: > On 5/6/24 1:50 PM, Abhishek Chauhan (ABC) wrote: >> >> >> On 5/6/2024 12:04 PM, Willem de Bruijn wrote: >>> 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. >>>> >>>> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/ >>>> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com> >>>> --- >>>> 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 >>>> @@ -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..71940f4ef0fb 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;" >>> >>> Not an expert on this code, and I see that the existing code already >>> has this below, but: isn't it odd and unnecessary to jump to an >>> unconditional jump statement? >>> >> I am closely looking into your comment and i will evalute it(Martin can correct me >> if the jumps are correct or not as i am new to BPF as well) but i found out that >> JSET = "&" and not "==". So the above two ins has to change from - > > Yes, this should be bitwise "&" instead of "==". > > The bpf CI did report this: https://github.com/kernel-patches/bpf/actions/runs/8947652196/job/24579927178 > > Please monitor the bpf CI test result. > > Do you have issue running the test locally? > Yes, To be honest. I am facing compilation issues when i follow the documentation to Make BPF on latest kernel. This is slowing down my development with this patch. Very similar to the problem described here :- https://github.com/jsitnicki/ebpf-summit-2020/issues/1 local/mnt/workspace/kernel_master/linux-next/tools/testing/selftests/bpf/tools/build/bpftool/bootstrap/libbpf/include/bpf/bpf_core_read.h:379:26: note: expanded from macro '___arrow2' #define ___arrow2(a, b) a->b ~^ skeleton/pid_iter.bpf.c:19:9: note: forward declaration of 'struct bpf_link' struct bpf_link link; ^ skeleton/pid_iter.bpf.c:105:7: error: incomplete definition of type 'struct bpf_link' if (BPF_CORE_READ(link, type) == bpf_core_enum_value(enum bpf_link_type___local, ^~~~~~~~~~~~~~~~~~~~~~~~~ >> >> "if w11 == 0x4 goto pc+1;" ==>(needs to be corrected to) "if w11 & 0x4 goto pc+1;" >> "if w11 == 0x3 goto pc+1;" ==> (needs to be correct to) "if w11 & 0x3 goto pc+1;" >> >> >>>> "$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 &= -3;" >> Martin, >> Also i am not sure why the the dissembly complains because the value of SKB_TSTAMP_TYPE_MASK = 3 and we are >> negating it ~3 = -3. >> >> Can't match disassembly(left) with pattern(right): >> r11 = *(u8 *)(r1 +129) ; r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset) >> if w11 & 0x4 goto pc+1 ; if w11 & 0x4 goto pc+1 >> goto pc+2 ; goto pc+2 >> w11 &= -4 ; w11 &= -3 >> >>>> "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;" >>>> "*(u64 *)($ctx + sk_buff::tstamp) = $src;", >>>> }, >
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 @@ -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..71940f4ef0fb 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 &= -3;" "*(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) inc_dtimes(EGRESS_ENDHOST); else @@ -252,7 +258,7 @@ int ingress_host(struct __sk_buff *skb) if (!skb_type) return TC_ACT_OK; - if (skb->tstamp_type == BPF_SKB_TSTAMP_DELIVERY_MONO && + if (skb->tstamp_type == BPF_SKB_CLOCK_MONOTONIC && skb->tstamp == EGRESS_FWDNS_MAGIC) inc_dtimes(INGRESS_ENDHOST); else @@ -315,7 +321,6 @@ int egress_fwdns_prio100(struct __sk_buff *skb) SEC("tc") int ingress_fwdns_prio101(struct __sk_buff *skb) { - __u64 expected_dtime = EGRESS_ENDHOST_MAGIC; int skb_type; skb_type = skb_get_type(skb); @@ -323,29 +328,24 @@ int ingress_fwdns_prio101(struct __sk_buff *skb) /* Should have handled in prio100 */ return TC_ACT_SHOT; - if (skb_proto(skb_type) == IPPROTO_UDP) - expected_dtime = 0; - if (skb->tstamp_type) { if (fwdns_clear_dtime() || - skb->tstamp_type != BPF_SKB_TSTAMP_DELIVERY_MONO || - skb->tstamp != expected_dtime) + (skb->tstamp_type != BPF_SKB_CLOCK_MONOTONIC && + skb->tstamp_type != BPF_SKB_CLOCK_TAI) || + skb->tstamp != EGRESS_ENDHOST_MAGIC) inc_errs(INGRESS_FWDNS_P101); else inc_dtimes(INGRESS_FWDNS_P101); } else { - if (!fwdns_clear_dtime() && expected_dtime) + if (!fwdns_clear_dtime()) inc_errs(INGRESS_FWDNS_P101); } - if (skb->tstamp_type == BPF_SKB_TSTAMP_DELIVERY_MONO) { + if (skb->tstamp_type == BPF_SKB_CLOCK_MONOTONIC) { skb->tstamp = INGRESS_FWDNS_MAGIC; } else { if (bpf_skb_set_tstamp(skb, INGRESS_FWDNS_MAGIC, - BPF_SKB_TSTAMP_DELIVERY_MONO)) - inc_errs(SET_DTIME); - if (!bpf_skb_set_tstamp(skb, INGRESS_FWDNS_MAGIC, - BPF_SKB_TSTAMP_UNSPEC)) + BPF_SKB_CLOCK_MONOTONIC)) inc_errs(SET_DTIME); } @@ -370,7 +370,7 @@ int egress_fwdns_prio101(struct __sk_buff *skb) if (skb->tstamp_type) { if (fwdns_clear_dtime() || - skb->tstamp_type != BPF_SKB_TSTAMP_DELIVERY_MONO || + skb->tstamp_type != BPF_SKB_CLOCK_MONOTONIC || skb->tstamp != INGRESS_FWDNS_MAGIC) inc_errs(EGRESS_FWDNS_P101); else @@ -380,14 +380,11 @@ int egress_fwdns_prio101(struct __sk_buff *skb) inc_errs(EGRESS_FWDNS_P101); } - if (skb->tstamp_type == BPF_SKB_TSTAMP_DELIVERY_MONO) { + if (skb->tstamp_type == BPF_SKB_CLOCK_MONOTONIC) { skb->tstamp = EGRESS_FWDNS_MAGIC; } else { if (bpf_skb_set_tstamp(skb, EGRESS_FWDNS_MAGIC, - BPF_SKB_TSTAMP_DELIVERY_MONO)) - inc_errs(SET_DTIME); - if (!bpf_skb_set_tstamp(skb, INGRESS_FWDNS_MAGIC, - BPF_SKB_TSTAMP_UNSPEC)) + BPF_SKB_CLOCK_MONOTONIC)) inc_errs(SET_DTIME); }
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. Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@linux.dev/ Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com> --- 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(-)