On Sat, Jan 22, 2022 at 10:32:16AM -0500, Willem de Bruijn wrote: > On Fri, Jan 21, 2022 at 2:30 AM Martin KaFai Lau <kafai@xxxxxx> wrote: > > > > skb->tstamp was first used as the (rcv) timestamp in real time clock base. > > The major usage is to report it to the user (e.g. SO_TIMESTAMP). > > > > Later, skb->tstamp is also set as the (future) delivery_time (e.g. EDT in TCP) > > during egress and used by the qdisc (e.g. sch_fq) to make decision on when > > the skb can be passed to the dev. > > > > Currently, there is no way to tell skb->tstamp having the (rcv) timestamp > > or the delivery_time, so it is always reset to 0 whenever forwarded > > between egress and ingress. > > > > While it makes sense to always clear the (rcv) timestamp in skb->tstamp > > to avoid confusing sch_fq that expects the delivery_time, it is a > > performance issue [0] to clear the delivery_time if the skb finally > > egress to a fq@phy-dev. For example, when forwarding from egress to > > ingress and then finally back to egress: > > > > tcp-sender => veth@netns => veth@hostns => fq@eth0@hostns > > ^ ^ > > reset rest > > > > [0] (slide 22): https://linuxplumbersconf.org/event/11/contributions/953/attachments/867/1658/LPC_2021_BPF_Datapath_Extensions.pdf > > > > This patch adds one bit skb->mono_delivery_time to flag the skb->tstamp > > is storing the mono delivery_time instead of the (rcv) timestamp. > > > > The current use case is to keep the TCP mono delivery_time (EDT) and > > to be used with sch_fq. The later patch will also allow tc-bpf to read > > and change the mono delivery_time. > > > > In the future, another bit (e.g. skb->user_delivery_time) can be added > > for the SCM_TXTIME where the clock base is tracked by sk->sk_clockid. > > > > [ This patch is a prep work. The following patch will > > get the other parts of the stack ready first. Then another patch > > after that will finally set the skb->mono_delivery_time. ] > > > > skb_set_delivery_time() function is added. It is used by the tcp_output.c > > and during ip[6] fragmentation to assign the delivery_time to > > the skb->tstamp and also set the skb->mono_delivery_time. > > > > Signed-off-by: Martin KaFai Lau <kafai@xxxxxx> > > --- > > include/linux/skbuff.h | 13 +++++++++++++ > > net/bridge/netfilter/nf_conntrack_bridge.c | 5 +++-- > > net/ipv4/ip_output.c | 5 +++-- > > net/ipv4/tcp_output.c | 16 +++++++++------- > > net/ipv6/ip6_output.c | 5 +++-- > > net/ipv6/netfilter.c | 5 +++-- > > 6 files changed, 34 insertions(+), 15 deletions(-) > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index bf11e1fbd69b..b9e20187242a 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -720,6 +720,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->tstamap has the > > tstamp > > > + * 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 > > @@ -890,6 +894,7 @@ struct sk_buff { > > __u8 decrypted:1; > > #endif > > __u8 slow_gro:1; > > + __u8 mono_delivery_time:1; > > This bit fills a hole, does not change sk_buff size, right? > > > > > #ifdef CONFIG_NET_SCHED > > __u16 tc_index; /* traffic control index */ > > @@ -3903,6 +3908,14 @@ static inline ktime_t net_invalid_timestamp(void) > > return 0; > > } > > > > +static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt, > > + bool mono) > > +{ > > + skb->tstamp = kt; > > + /* Setting mono_delivery_time will be enabled later */ > > + /* skb->mono_delivery_time = kt && mono; */ > > +} > > + > > static inline u8 skb_metadata_len(const struct sk_buff *skb) > > { > > return skb_shinfo(skb)->meta_len; > > diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c > > index fdbed3158555..ebfb2a5c59e4 100644 > > --- a/net/bridge/netfilter/nf_conntrack_bridge.c > > +++ b/net/bridge/netfilter/nf_conntrack_bridge.c > > @@ -32,6 +32,7 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk, > > struct sk_buff *)) > > { > > int frag_max_size = BR_INPUT_SKB_CB(skb)->frag_max_size; > > + bool mono_delivery_time = skb->mono_delivery_time; > > This use of a local variable is just a style choice, not needed for > correctness, correct? It is needed. The current code also saves the very first skb->tstamp (two lines below): > > unsigned int hlen, ll_rs, mtu; > > ktime_t tstamp = skb->tstamp; My understanding is all frags reuse the first (/original) skb tstamp info. The frags output() is one-by-one, meaning when sending the later frags, the first skb has already been output()-ed, so the first skb tstamp info needs to be saved in local variables.
