Received-SPF: pass (google.com: domain of linux-kernel+bounces-102541-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 13 Mar 2024 17:40:53 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: "Abhishek Chauhan (ABC)" <quic_abchauha@quicinc.com>, 
 Martin KaFai Lau <martin.lau@linux.dev>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: kernel@quicinc.com, 
 "David S. Miller" <davem@davemloft.net>, 
 Eric Dumazet <edumazet@google.com>, 
 Jakub Kicinski <kuba@kernel.org>, 
 Paolo Abeni <pabeni@redhat.com>, 
 netdev@vger.kernel.org, 
 linux-kernel@vger.kernel.org, 
 Andrew Halaney <ahalaney@redhat.com>, 
 Martin KaFai Lau <martin.lau@kernel.org>, 
 bpf <bpf@vger.kernel.org>, 
 Daniel Borkmann <daniel@iogearbox.net>, 
 Alexei Starovoitov <ast@kernel.org>, 
 Andrii Nakryiko <andrii@kernel.org>
Message-ID: <65f21d65820fc_3d934129463@willemb.c.googlers.com.notmuch>
In-Reply-To: <e5da91bc-5827-4347-ab38-36c92ae2dfa2@quicinc.com>
References: <20240301201348.2815102-1-quic_abchauha@quicinc.com>
 <2a4cb416-5d95-459d-8c1c-3fb225240363@linux.dev>
 <65f16946cd33e_344ff1294fc@willemb.c.googlers.com.notmuch>
 <28282905-065a-4233-a0a2-53aa9b85f381@linux.dev>
 <65f2004e65802_3d1e792943e@willemb.c.googlers.com.notmuch>
 <0dff8f05-e18d-47c8-9f19-351c44ea8624@linux.dev>
 <e5da91bc-5827-4347-ab38-36c92ae2dfa2@quicinc.com>
Subject: Re: [PATCH net-next v4] net: Re-use and set mono_delivery_time bit
 for userspace tstamp packets
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

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 time=
stamp
> >>>>>> packets and ends up resetting the timestamp. ETF qdisc checks th=
e
> >>>>>> packet coming from userspace and encounters to be 0 thereby drop=
ping
> >>>>>> time sensitive packets. These changes will allow userspace times=
tamps
> >>>>>> 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 h=
ere,
> >>>>>> 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 "de=
livery" time.
> >>>>> BPF_SKB_TSTAMP_UNSPEC means everything else (this could be a rx t=
imestamp at
> >>>>> ingress or a delivery time set by user space).
> >>>>>
> >>>>> __sk_buff->tstamp_type depends on skb->mono_delivery_time which d=
oes 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_tst=
amp().
> >>>>>
> >>>>> 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 mo=
no 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, tstam=
p,
> >>>>> 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 ca=
se.
> >>>
> >>> skb->sk =3D=3D 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:
> >>>
> >>> =C2=A0=C2=A0=C2=A0=C2=A0skb->mono_delivery_time =3D=3D 1 &&
> >>> =C2=A0=C2=A0=C2=A0=C2=A0(!skb->sk ||
> >>> =C2=A0=C2=A0=C2=A0=C2=A0 !sk_fullsock(skb->sk) /* tcp tw or req sk =
*/ ||
> >>> =C2=A0=C2=A0=C2=A0=C2=A0 skb->sk->sk_protocol =3D=3D IPPROTO_TCP)
> >>>
> >>> Not a small bpf instruction addition to bpf_convert_tstamp_type_rea=
d() 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 !=3D NULL and skb->sk->sk_clockid !=3D 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->tsta=
mp before
> >>> sending to the sch_fq.
> >>>
> >>> I currently also don't have a use case for skb->sk->sk_clockid !=3D=

> >>> 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_fo=
rward_skb()
> >>> between the veth pair.
> >>>
> >>>>
> >>>> Perhaps bpf_skb_set_tstamp() can just fail if another delivery tim=
e is
> >>>> already explicitly programmed?
> >>>
> >>> This will change the existing bpf_skb_set_tstamp() behavior, so pro=
bably not
> >>> acceptable.
> >>>
> >>>>
> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 skb->sk &&
> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sock_flag(sk, SOCK_TXTIME) &&
> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 skb->sk->sk_clockid !=3D CLOCK_MONO=
TONIC
> >>>
> >>>> 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() curr=
ently only
> >>> changes skb and does not change skb->sk.
> >>>
> >>> I think changing sock_valbool_flag(SOCK_TXTIME) will also have a ne=
w user space
> >>> visible side effect. The sendmsg for cmsg with SCM_TXTIME will star=
t 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 s=
mall glitch
> >>> for some amount of skb(s) until the user space settled on setsockop=
t(SO_TXTIME).
> >>>
> >>> I think all this is crying for another bit in skb to mean user_deli=
very_time
> >>> (skb->tstamp depends on skb->sk->sk_clockid) while mono_delivery_ti=
me 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?), o=
nly 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->tstam=
p, BPF_SKB_TSTAMP_DELIVERY_ can be extended to match it. It will be easie=
r if the values in the skb bits is the same as the BPF_SKB_TSTAMP_DELIVER=
Y_*.
> > =

> > The bpf_convert_tstamp_*() and the bpf_skb_set_tstamp helper will nee=
d 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 apporac=
h, do you want to revert the current patch first and then start from clea=
n?
> > =

> Yes , I think since we have concluded the two bit .(Thanks for discussi=
on 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 f=
or 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_...

So indeed we will need a user_delivery_time bit and use that to look
at sk_clockid.=