Message-ID: <65f2c81fc7988_3ee61729465@willemb.c.googlers.com.notmuch>
Date: Thu, 14 Mar 2024 05:49:19 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
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

Martin KaFai Lau wrote:
> On 3/13/24 2:40 PM, Willem de Bruijn wrote:
> > Abhishek Chauhan (ABC) wrote:
> >>
> >>
> >> On 3/13/2024 2:01 PM, Martin KaFai Lau wrote:
> >>> On 3/13/24 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.
> >>>>
> >>>> 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.
> >>>
> >>> Good idea. May be starting with mono and tai (Abishek's use case?), only forward these two clocks and reset the skb->tstamp for others.
> >>>
> >>>>
> >>>> 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.
> >>>
> >>> Yes, if there are bits in skb to describe the clock in the skb->tstamp, BPF_SKB_TSTAMP_DELIVERY_ can be extended to match it. It will be easier if the values in the skb bits is the same as the BPF_SKB_TSTAMP_DELIVERY_*.
> >>>
> >>> The bpf_convert_tstamp_*() and the bpf_skb_set_tstamp helper will need changes to include the consideration of these two bits. I think we have mostly settled with the approach (thanks for the discussion!). Abhishek, not sure how much can be reused from this patch for the two bits apporach, do you want to revert the current patch first and then start from clean?
> >>>
> >> Yes , I think since we have concluded the two bit .(Thanks for discussion again, Martin and Willem)
> >>
> >> Here is what i will do from myside
> >> 1. Revert the v4 patch :-  net: Re-use and set mono_delivery_time bit for userspace tstamp packets
> >> 2. Keep mono_delivery_time as ease
> >> 3. Add user_delivery_time as a new bitfield
> >> 4. Add BPF_SKB_TSTAMP_DELIVERY_TAI in the bpf.h for etf support
> >> 5. do not reset the time if either mono_delivery_time or user_delivery_time is set.
> >>
> >> Let me know if i have covered all the design details or add if i missed anything.
> > 
> > Thanks for revising this.
> > 
> > No need to add the BPF part here.
> > 
> > I was mistaken that we can encode the clock_id in two skb bits.
> > SO_TXTIME allows essentially all CLOCK_...
> 
> The two bits could potentially only encode the delivery time that is allowed to 
> be forwarded without reset. 0 could mean refering back to sk_clockid and don't 
> forward. The final consumer of the forwarded skb->tstamp is the qdisc which 
> currently only has mono and tai.

So the followinng meaning of bit pair
{ skb->mono_delivery_time, skb->user_delivery_time } ?
 
- { 0, 0 } legacy skb->tstamp: realtime on rx
- { 1, 0 } skb->tstamp is mono: existing behavior of mono_delivery_time bit
- { 0, 1 } skb->tstamp is tai: analogous to mono case
- { 1, 1 } skb->tstamp defined by skb->sk->sk_clockid

> The concern is more on future extension to 
> allow more clock type to be forwarded?

Right. Not because I have an immediate use for them. Only because
SO_TXTIME already allows configuring other values for sk_clockid.
  
> I don't have a use case for having BPF_SKB_TSTAMP_DELIVERY_TAI but curious on 
> how other clock types are used now.
> 
> Thanks.
> 
> > 
> > So indeed we will need a user_delivery_time bit and use that to look
> > at sk_clockid.
> 

Powered by blists - more mailing lists