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;"
"$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);
}
--
2.25.1Abhishek 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/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;",
>>>> },
>
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;"
© 2016 - 2025 Red Hat, Inc.