Abhishek Chauhan (ABC) wrote: > > > On 4/13/2024 11:54 AM, Willem de Bruijn wrote: > > Abhishek Chauhan wrote: > >> mono_delivery_time was added to check if skb->tstamp has delivery > >> time in mono clock base (i.e. EDT) otherwise skb->tstamp has > >> timestamp in ingress and delivery_time at egress. > >> > >> Renaming the bitfield from mono_delivery_time to tstamp_type is for > >> extensibilty for other timestamps such as userspace timestamp > >> (i.e. SO_TXTIME) set via sock opts. > >> > >> As we are renaming the mono_delivery_time to tstamp_type, it makes > >> sense to start assigning tstamp_type based out if enum defined as > >> part of this commit > >> > >> Earlier we used bool arg flag to check if the tstamp is mono in > >> function skb_set_delivery_time, Now the signature of the functions > >> accepts enum to distinguish between mono and real time > >> > >> 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. > >> > >> In future tstamp_type:1 can be extended to support userspace timestamp > >> by increasing the bitfield. > >> > >> Link: https://lore.kernel.org/netdev/bc037db4-58bb-4861-ac31-a361a93841d3@xxxxxxxxx/ > >> Signed-off-by: Abhishek Chauhan <quic_abchauha@xxxxxxxxxxx> > >> --- > >> Changes since v2 > >> - Minor changes to commit subject > >> > >> Changes since v1 > >> - Squashed the two commits into one as mentioned by Willem. > >> - Introduced switch in skb_set_delivery_time. > >> - Renamed and removed directionality aspects w.r.t tstamp_type > >> as mentioned by Willem. > >> > >> include/linux/skbuff.h | 33 +++++++++++++++------- > >> include/net/inet_frag.h | 4 +-- > >> net/bridge/netfilter/nf_conntrack_bridge.c | 6 ++-- > >> net/core/dev.c | 2 +- > >> net/core/filter.c | 8 +++--- > >> net/ipv4/inet_fragment.c | 2 +- > >> net/ipv4/ip_fragment.c | 2 +- > >> net/ipv4/ip_output.c | 8 +++--- > >> net/ipv4/tcp_output.c | 14 ++++----- > >> net/ipv6/ip6_output.c | 6 ++-- > >> net/ipv6/netfilter.c | 6 ++-- > >> net/ipv6/netfilter/nf_conntrack_reasm.c | 2 +- > >> net/ipv6/reassembly.c | 2 +- > >> net/ipv6/tcp_ipv6.c | 2 +- > >> net/sched/act_bpf.c | 4 +-- > >> net/sched/cls_bpf.c | 4 +-- > >> 16 files changed, 59 insertions(+), 46 deletions(-) > >> > >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > >> index 7135a3e94afd..a83a2120b57f 100644 > >> --- a/include/linux/skbuff.h > >> +++ b/include/linux/skbuff.h > >> @@ -702,6 +702,11 @@ typedef unsigned int sk_buff_data_t; > >> typedef unsigned char *sk_buff_data_t; > >> #endif > >> > >> +enum skb_tstamp_type { > >> + CLOCK_REAL = 0, /* Time base is realtime */ > >> + CLOCK_MONO = 1, /* Time base is Monotonic */ > >> +}; > > > > Minor: inconsistent capitalization > > > I will fix this. > > >> + > >> /** > >> * DOC: Basic sk_buff geometry > >> * > >> @@ -819,7 +824,7 @@ 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 > >> + * @tstamp_type: 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. > >> @@ -950,7 +955,7 @@ struct sk_buff { > >> /* private: */ > >> __u8 __mono_tc_offset[0]; > >> /* public: */ > >> - __u8 mono_delivery_time:1; /* See SKB_MONO_DELIVERY_TIME_MASK */ > >> + __u8 tstamp_type:1; /* See SKB_MONO_DELIVERY_TIME_MASK */ > > > > Also remove reference to MONO_DELIVERY_TIME_MASK, or instead refer to > > skb_tstamp_type. > > > >> #ifdef CONFIG_NET_XGRESS > >> __u8 tc_at_ingress:1; /* See TC_AT_INGRESS_MASK */ > >> __u8 tc_skip_classify:1; > >> @@ -4237,7 +4242,7 @@ static inline void skb_get_new_timestampns(const struct sk_buff *skb, > >> static inline void __net_timestamp(struct sk_buff *skb) > >> { > >> skb->tstamp = ktime_get_real(); > >> - skb->mono_delivery_time = 0; > >> + skb->tstamp_type = CLOCK_REAL; > >> } > >> > >> static inline ktime_t net_timedelta(ktime_t t) > >> @@ -4246,10 +4251,18 @@ static inline ktime_t net_timedelta(ktime_t t) > >> } > >> > >> static inline void skb_set_delivery_time(struct sk_buff *skb, ktime_t kt, > >> - bool mono) > >> + u8 tstamp_type) > >> { > >> skb->tstamp = kt; > >> - skb->mono_delivery_time = kt && mono; > >> + > >> + switch (tstamp_type) { > >> + case CLOCK_REAL: > >> + skb->tstamp_type = CLOCK_REAL; > >> + break; > >> + case CLOCK_MONO: > >> + skb->tstamp_type = kt && tstamp_type; > >> + break; > >> + } > > > > Technically this leaves the tstamp_type undefined if (skb, 0, CLOCK_REAL) > Do you think i should be checking for valid value of tstamp before setting the tstamp_type ? Only then set it. A kt of 0 is interpreted as resetting the type. That should probably be maintained. For SO_TIMESTAMPING, a mono delivery time of 0 does have some meaning. In __sock_recv_timestamp: /* Race occurred between timestamp enabling and packet receiving. Fill in the current time for now. */ if (need_software_tstamp && skb->tstamp == 0) { __net_timestamp(skb); false_tstamp = 1; }
