Message-ID: <02363fef-0b5e-4b6c-bad9-db924db230b9@quicinc.com>
Date: Wed, 13 Mar 2024 13:59:04 -0700
From: "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Martin KaFai Lau
	<martin.lau@...ux.dev>
CC: <kernel@...cinc.com>, "David S. Miller" <davem@...emloft.net>,
        "Eric
 Dumazet" <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Andrew Halaney <ahalaney@...hat.com>,
        "Martin
 KaFai Lau" <martin.lau@...nel.org>, bpf <bpf@...r.kernel.org>,
        Daniel
 Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii
 Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH net-next v4] net: Re-use and set mono_delivery_time bit
 for userspace tstamp packets



On 3/13/2024 12:36 PM, Willem de Bruijn wrote:
> Martin KaFai Lau wrote:
>> On 3/13/24 1:52 AM, Willem de Bruijn wrote:
>>> Martin KaFai Lau wrote:
>>>> On 3/1/24 12:13 PM, Abhishek Chauhan wrote:
>>>>> Bridge driver today has no support to forward the userspace timestamp
>>>>> packets and ends up resetting the timestamp. ETF qdisc checks the
>>>>> packet coming from userspace and encounters to be 0 thereby dropping
>>>>> time sensitive packets. These changes will allow userspace timestamps
>>>>> packets to be forwarded from the bridge to NIC drivers.
>>>>>
>>>>> Setting the same bit (mono_delivery_time) to avoid dropping of
>>>>> userspace tstamp packets in the forwarding path.
>>>>>
>>>>> Existing functionality of mono_delivery_time remains unaltered here,
>>>>> instead just extended with userspace tstamp support for bridge
>>>>> forwarding path.
>>>>
>>>> The patch currently broke the bpf selftest test_tc_dtime:
>>>> https://github.com/kernel-patches/bpf/actions/runs/8242487344/job/22541746675
>>>>
>>>> In particular, there is a uapi field __sk_buff->tstamp_type which currently has
>>>> BPF_SKB_TSTAMP_DELIVERY_MONO to mean skb->tstamp has the MONO "delivery" time.
>>>> BPF_SKB_TSTAMP_UNSPEC means everything else (this could be a rx timestamp at
>>>> ingress or a delivery time set by user space).
>>>>
>>>> __sk_buff->tstamp_type depends on skb->mono_delivery_time which does not
>>>> necessarily mean mono after this patch. I thought about fixing it on the bpf
>>>> side such that reading __sk_buff->tstamp_type only returns
>>>> BPF_SKB_TSTAMP_DELIVERY_MONO when the skb->mono_delivery_time is set and skb->sk
>>>> is IPPROTO_TCP. However, it won't work because of bpf_skb_set_tstamp().
>>>>
>>>> There is a bpf helper, bpf_skb_set_tstamp(skb, tstamp,
>>>> BPF_SKB_TSTAMP_DELIVERY_MONO). This helper changes both the skb->tstamp and the
>>>> skb->mono_delivery_time. The expectation is this could change skb->tstamp in the
>>>> ingress skb and redirect to egress sch_fq. It could also set a mono time to
>>>> skb->tstamp where the udp sk->sk_clockid may not be necessary in mono and then
>>>> bpf_redirect to egress sch_fq. When bpf_skb_set_tstamp(skb, tstamp,
>>>> BPF_SKB_TSTAMP_DELIVERY_MONO) succeeds, reading __sk_buff->tstamp_type expects
>>>> BPF_SKB_TSTAMP_DELIVERY_MONO also.
>>>>
>>>> I ran out of idea to solve this uapi breakage.
>>>>
>>>> I am afraid it may need to go back to v1 idea and use another bit
>>>> (user_delivery_time) in the skb.
>>>
>>> Is the only conflict when bpf_skb_set_tstamp is called for an skb from
>>> a socket with sk_clockid set (and thus SO_TXTIME called)?
>>
>> Right, because skb->mono_delivery_time does not mean skb->tstamp is mono now and 
>> its interpretation depends on skb->sk->sk_clockid.
>>
>>> Interpreting skb->tstamp as mono if skb->mono_delivery_time is set and
>>> skb->sk is NULL is fine. This is the ingress to egress redirect case.
>>
>> skb->sk == NULL is fine. I tried something like this in 
>> bpf_convert_tstamp_type_read() for reading __sk_buff->tstamp_type:
>>
>> __sk_buff->tstamp_type is BPF_SKB_TSTAMP_DELIVERY_MONO when:
>>
>> 	skb->mono_delivery_time == 1 &&
>> 	(!skb->sk ||
>> 	 !sk_fullsock(skb->sk) /* tcp tw or req sk */ ||
>> 	 skb->sk->sk_protocol == IPPROTO_TCP)
>>
>> Not a small bpf instruction addition to bpf_convert_tstamp_type_read() but doable.
>>
>>>
>>> I don't see an immediate use for this BPF function on egress where it
>>> would overwrite an SO_TXTIME timestamp and now skb->tstamp is mono,
>>> but skb->sk != NULL and skb->sk->sk_clockid != CLOCK_MONOTONIC.
>>
>> The bpf prog may act as a traffic shaper that limits the bandwidth usage of all 
>> outgoing packets (tcp/udp/...) by setting the mono EDT in skb->tstamp before 
>> sending to the sch_fq.
>>
>> I currently also don't have a use case for skb->sk->sk_clockid != 
>> CLOCK_MONOTONIC. However, it is something that bpf_skb_set_tstamp() can do now 
>> before queuing to sch_fq.
>>
>> The container (in netns + veth) may use other sk_clockid/qdisc (e.g. sch_etf) 
>> setup and the non mono skb->tstamp is not cleared now during dev_forward_skb() 
>> between the veth pair.
>>
>>>
>>> Perhaps bpf_skb_set_tstamp() can just fail if another delivery time is
>>> already explicitly programmed?
>>
>> This will change the existing bpf_skb_set_tstamp() behavior, so probably not 
>> acceptable.
>>
>>>
>>>      skb->sk &&
>>>      sock_flag(sk, SOCK_TXTIME) &&
>>>      skb->sk->sk_clockid != CLOCK_MONOTONIC
>>
>>> Either that, or unset SOCK_TXTIME to make sk_clockid undefined and
>>> fall back on interpreting as monotonic.
>>
>> Change sk->sk_flags in tc bpf prog? hmm... I am not sure this will work well also.
>>
>> sock_valbool_flag(SOCK_TXTIME) should require a lock_sock() to make changes. The 
>> tc bpf prog cannot take the lock_sock, so bpf_skb_set_tstamp() currently only 
>> changes skb and does not change skb->sk.
>>
>> I think changing sock_valbool_flag(SOCK_TXTIME) will also have a new user space 
>> visible side effect. The sendmsg for cmsg with SCM_TXTIME will start failing 
>> from looking at __sock_cmsg_send().
>>
>> There may be a short period of disconnect between what is in sk->sk_flags and 
>> what is set in skb->tstamp. e.g. what if user space does setsockopt(SO_TXTIME) 
>> again after skb->tstamp is set by bpf. This could be considered a small glitch 
>> for some amount of skb(s) until the user space settled on setsockopt(SO_TXTIME).
>>
>> I think all this is crying for another bit in skb to mean user_delivery_time 
>> (skb->tstamp depends on skb->sk->sk_clockid) while mono_delivery_time is the 
>> mono time either set by kernel-tcp or bpf.
> 
> It does sound like the approach with least side effects.
> 
Martin and Willem , while we are discussing to use seperate bit per skb to check if its 
SO_TXTIME from userspace or (rcv) timestamp . I came across two flags used in skbuff
called in filter framework
@tc_at_ingress: used within tc_classify to distinguish in/egress
@from_ingress: packet was redirected from the ingress path

Since i believe the above testcase is based on redirecting from ingress to egress part
why cant we use these already existing flags ? Please advice 

I am not completely sure if we can use these flags to solve the bpf problem or not. 
but considering the problem statment put forth by Willem stating that we are just adding 
flags for every usecase and not keeping the design scalable for future needs. 

> If we're going to increase to two bits per skb, it's perhaps
> better to just encode the (selected supported) CLOCK_ type, rather
> than only supporting clockid through skb->sk->sk_clockid.
> 
> This BPF function is the analogue to SO_TXTIME. It is clearly
> extensible to additional BPF_SKB_TSTAMP_DELIVERY_.. types. To
> work with sch_etf, say.
> 
>> If we need to revert the 
>> mono_delivery_time reuse patch later, we will need to revert the netdev patch 
>> and the (to-be-made) bpf patch.
> 
> 
>  
> 
> 

Powered by blists - more mailing lists