Re: [PATCH v4 net-next 1/8] net: Add skb->mono_delivery_time to distinguish mono delivery_time from (rcv) timestamp

From: Martin KaFai Lau <kafai@fb.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: <bpf@vger.kernel.org>, <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	David Miller <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, <kernel-team@fb.com>,
	Willem de Bruijn <willemb@google.com>
Subject: Re: [PATCH v4 net-next 1/8] net: Add skb->mono_delivery_time to distinguish mono delivery_time from (rcv) timestamp
Date: Tue, 15 Feb 2022 22:11:38 -0800	[thread overview]
Message-ID: <20220216061138.ndrb26i3erhnt6f7@kafai-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <1d03f759-3eea-0032-18fc-1f6fed2c14bc@iogearbox.net>

On Tue, Feb 15, 2022 at 09:49:07PM +0100, Daniel Borkmann wrote:
> On 2/11/22 8:12 AM, Martin KaFai Lau wrote:
> [...]
> > The current use case is to keep the TCP mono delivery_time (EDT) and
> > to be used with sch_fq.  A later patch will also allow tc-bpf@ingress
> > to read and change the mono delivery_time.
> > 
> > In the future, another bit (e.g. skb->user_delivery_time) can be added
> [...]
> > ---
> >   include/linux/skbuff.h                     | 13 +++++++++++++
> >   net/bridge/netfilter/nf_conntrack_bridge.c |  5 +++--
> >   net/ipv4/ip_output.c                       |  7 +++++--
> >   net/ipv4/tcp_output.c                      | 16 +++++++++-------
> >   net/ipv6/ip6_output.c                      |  5 +++--
> >   net/ipv6/netfilter.c                       |  5 +++--
> >   net/ipv6/tcp_ipv6.c                        |  2 +-
> >   7 files changed, 37 insertions(+), 16 deletions(-)
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index a5adbf6b51e8..32c793de3801 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -747,6 +747,10 @@ typedef unsigned char *sk_buff_data_t;
> >    *	@dst_pending_confirm: need to confirm neighbour
> >    *	@decrypted: Decrypted SKB
> >    *	@slow_gro: state present at GRO time, slower prepare step required
> > + *	@mono_delivery_time: When set, skb->tstamp has the
> > + *		delivery_time in mono clock base (i.e. EDT).  Otherwise, the
> > + *		skb->tstamp has the (rcv) timestamp at ingress and
> > + *		delivery_time at egress.
> >    *	@napi_id: id of the NAPI struct this skb came from
> >    *	@sender_cpu: (aka @napi_id) source CPU in XPS
> >    *	@secmark: security marking
> > @@ -917,6 +921,7 @@ struct sk_buff {
> >   	__u8			decrypted:1;
> >   #endif
> >   	__u8			slow_gro:1;
> > +	__u8			mono_delivery_time:1;
> 
> Don't you also need to extend sch_fq to treat any non-mono_delivery_time marked
> skb similar as if it hadn't been marked with a delivery time?
The current skb does not have clock base info, so the sch_fq does not depend
on it to work also.  fq_packet_beyond_horizon() should already be a good guard
on the clock base difference.  An extra check on the new mono_delivery_time does
not necessary add extra value on top.