by Abhishek Chauhan

[permalink] [raw]

Subject: Re: [RFC PATCH bpf-next v1 3/3] net: Add additional bit to support userspace timestamp type

On 4/10/2024 4:25 PM, Martin KaFai Lau wrote:
> On 4/10/24 1:25 PM, Abhishek Chauhan (ABC) wrote:
>>>> @@ -830,6 +833,9 @@ enum skb_tstamp_type {
>>>>    *        delivery_time in mono clock base (i.e. EDT). Otherwise, the
>>>>    *        skb->tstamp has the (rcv) timestamp at ingress and
>>>>    *        delivery_time at egress.
>>>> + *        delivery_time in mono clock base (i.e., EDT) or a clock base chosen
>>>> + *        by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
>>>> + *        ingress.
>>>>    *    @napi_id: id of the NAPI struct this skb came from
>>>>    *    @sender_cpu: (aka @napi_id) source CPU in XPS
>>>>    *    @alloc_cpu: CPU which did the skb allocation.
>>>> @@ -960,7 +966,7 @@ struct sk_buff {
>>>>       /* private: */
>>>>       __u8            __mono_tc_offset[0];
>>>>       /* public: */
>>>> -    __u8            tstamp_type:1;    /* See SKB_MONO_DELIVERY_TIME_MASK */
>>>> +    __u8            tstamp_type:2;    /* See SKB_MONO_DELIVERY_TIME_MASK */
>>>> #ifdef CONFIG_NET_XGRESS
>>>>       __u8            tc_at_ingress:1;    /* See TC_AT_INGRESS_MASK */
>
> The above "tstamp_type:2" change shifted the tc_at_ingress bit.
> TC_AT_INGRESS_MASK needs to be adjusted.
>
>>>>       __u8            tc_skip_classify:1;
>>>
>>> With pahole, does this have an effect on sk_buff layout?
>>>
>> I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
>> changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC.
>> That being said i am actually trying to understand/learn BPF instructions to know things better.
>> I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK
>>
>>
>> #ifdef __BIG_ENDIAN_BITFIELD
>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 7) //Suspecting changes here too
>> #define TC_AT_INGRESS_MASK        (1 << 6) // and here
>> #else
>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 0)
>> #define TC_AT_INGRESS_MASK        (1 << 1) (this might have to change to 1<<2 )
>
> This should be (1 << 2) now. Similar adjustment for the big endian.
>
>> #endif
>> #define SKB_BF_MONO_TC_OFFSET        offsetof(struct sk_buff, __mono_tc_offset)
>>
>> Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c
>
> ctx_rewrite.c tests the bpf ctx rewrite code. In this particular case, it tests
> the bpf_convert_tstamp_read() and bpf_convert_tstamp_write() generate the
> correct bpf instructions.
> e.g. "w11 &= 3;" is testing the following in bpf_convert_tstamp_read():
>         *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
>                      TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);
>
> The existing "TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK" is 0x3
> and it should become 0x5 if my hand counts correctly.
>

so the changes will be as follows (Martin correct me if am wrong)

//w11 is checked againt 0x5 (Binary = 101)
N(SCHED_CLS, struct __sk_buff, tstamp),
.read = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
"w11 &= 5;" <== here
"if w11 != 0x5 goto pc+2;" <==here
"$dst = 0;"
"goto pc+1;"
"$dst = *(u64 *)($ctx + sk_buff::tstamp);",

//w11 is checked againt 0x4 (100)
.write = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
"if w11 & 0x4 goto pc+1;" <== here
"goto pc+2;"
"w11 &= -4;" <==here
"*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;"
"*(u64 *)($ctx + sk_buff::tstamp) = $src;",

> The patch set cannot be applied to the bpf-next:
> https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
> , so bpf CI cannot run to reproduce the issue.
>

2024-04-12 21:18:17

by Abhishek Chauhan

[permalink] [raw]

Subject: Re: [RFC PATCH bpf-next v1 3/3] net: Add additional bit to support userspace timestamp type

On 4/10/2024 4:39 PM, Abhishek Chauhan (ABC) wrote:
>
>
> On 4/10/2024 4:25 PM, Martin KaFai Lau wrote:
>> On 4/10/24 1:25 PM, Abhishek Chauhan (ABC) wrote:
>>>>> @@ -830,6 +833,9 @@ enum skb_tstamp_type {
>>>>>    *        delivery_time in mono clock base (i.e. EDT). Otherwise, the
>>>>>    *        skb->tstamp has the (rcv) timestamp at ingress and
>>>>>    *        delivery_time at egress.
>>>>> + *        delivery_time in mono clock base (i.e., EDT) or a clock base chosen
>>>>> + *        by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
>>>>> + *        ingress.
>>>>>    *    @napi_id: id of the NAPI struct this skb came from
>>>>>    *    @sender_cpu: (aka @napi_id) source CPU in XPS
>>>>>    *    @alloc_cpu: CPU which did the skb allocation.
>>>>> @@ -960,7 +966,7 @@ struct sk_buff {
>>>>>       /* private: */
>>>>>       __u8            __mono_tc_offset[0];
>>>>>       /* public: */
>>>>> -    __u8            tstamp_type:1;    /* See SKB_MONO_DELIVERY_TIME_MASK */
>>>>> +    __u8            tstamp_type:2;    /* See SKB_MONO_DELIVERY_TIME_MASK */
>>>>> #ifdef CONFIG_NET_XGRESS
>>>>>       __u8            tc_at_ingress:1;    /* See TC_AT_INGRESS_MASK */
>>
>> The above "tstamp_type:2" change shifted the tc_at_ingress bit.
>> TC_AT_INGRESS_MASK needs to be adjusted.
>>
>>>>>       __u8            tc_skip_classify:1;
>>>>
>>>> With pahole, does this have an effect on sk_buff layout?
>>>>
>>> I think it does and it also impacts BPF testing. Hence in my cover letter i have mentioned that these
>>> changes will impact BPF. My level of expertise is very limited to BPF hence the reason for RFC.
>>> That being said i am actually trying to understand/learn BPF instructions to know things better.
>>> I think we need to also change the offset SKB_MONO_DELIVERY_TIME_MASK and TC_AT_INGRESS_MASK
>>>
>>>
>>> #ifdef __BIG_ENDIAN_BITFIELD
>>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 7) //Suspecting changes here too
>>> #define TC_AT_INGRESS_MASK        (1 << 6) // and here
>>> #else
>>> #define SKB_MONO_DELIVERY_TIME_MASK    (1 << 0)
>>> #define TC_AT_INGRESS_MASK        (1 << 1) (this might have to change to 1<<2 )
>>
>> This should be (1 << 2) now. Similar adjustment for the big endian.
>>
>>> #endif
>>> #define SKB_BF_MONO_TC_OFFSET        offsetof(struct sk_buff, __mono_tc_offset)
>>>
>>> Also i suspect i change in /selftests/bpf/prog_tests/ctx_rewrite.c
>>
>> ctx_rewrite.c tests the bpf ctx rewrite code. In this particular case, it tests
>> the bpf_convert_tstamp_read() and bpf_convert_tstamp_write() generate the
>> correct bpf instructions.
>> e.g. "w11 &= 3;" is testing the following in bpf_convert_tstamp_read():
>>         *insn++ = BPF_ALU32_IMM(BPF_AND, tmp_reg,
>>                      TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK);
>>
>> The existing "TC_AT_INGRESS_MASK | SKB_MONO_DELIVERY_TIME_MASK" is 0x3
>> and it should become 0x5 if my hand counts correctly.
>>
>
> so the changes will be as follows (Martin correct me if am wrong)
>
> //w11 is checked againt 0x5 (Binary = 101)
> N(SCHED_CLS, struct __sk_buff, tstamp),
> .read = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
> "w11 &= 5;" <== here
> "if w11 != 0x5 goto pc+2;" <==here
> "$dst = 0;"
> "goto pc+1;"
> "$dst = *(u64 *)($ctx + sk_buff::tstamp);",
>
> //w11 is checked againt 0x4 (100)
> .write = "r11 = *(u8 *)($ctx + sk_buff::__mono_tc_offset);"
> "if w11 & 0x4 goto pc+1;" <== here
> "goto pc+2;"
> "w11 &= -4;" <==here
> "*(u8 *)($ctx + sk_buff::__mono_tc_offset) = r11;"
> "*(u64 *)($ctx + sk_buff::tstamp) = $src;",
>
>
Martin and Willem,
After the above changes, patchset v3 of these changes passed BPF test cases . Looks like we are good to go with final review now. If you have any further comments
Thank you for all the comments and design discussion that we had as part of this patch set series.

Testing :-
1. https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
2. https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

>> The patch set cannot be applied to the bpf-next:
>> https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
>> , so bpf CI cannot run to reproduce the issue.
>>