Abhishek Chauhan (ABC) wrote: > > > On 5/6/2024 11:51 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 on enum defined > >> in 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 tstamp_type to distinguish between mono and real time. > >> > >> Also skb_set_delivery_type_by_clockid is a new function which accepts > >> clockid to determine the tstamp_type. > >> > >> 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 v5 > >> - Avoided using garble function names as mentioned by > >> Willem. > >> - Implemented a conversion function stead of duplicating > >> the same logic as mentioned by Willem. > >> - Fixed indentation problems and minor documentation issues > >> which mentions tstamp_type as a whole instead of bitfield > >> notations. (Mentioned both by Willem and Martin) > >> > >> Changes since v4 > >> - Introduce new function to directly delivery_time and > >> another to set tstamp_type based on clockid. > >> - Removed un-necessary comments in skbuff.h as > >> enums were obvious and understood. > >> > >> Changes since v3 > >> - Fixed inconsistent capitalization in skbuff.h > >> - remove reference to MONO_DELIVERY_TIME_MASK in skbuff.h > >> and point it to skb_tstamp_type now. > >> - Explicitely setting SKB_CLOCK_MONO if valid transmit_time > >> ip_send_unicast_reply > >> - Keeping skb_tstamp inline with skb_clear_tstamp. > >> - skb_set_delivery_time checks if timstamp is 0 and > >> sets the tstamp_type to SKB_CLOCK_REAL. > >> - Above comments are given by Willem > >> - Found out that skbuff.h has access to uapi/linux/time.h > >> So now instead of using CLOCK_REAL/CLOCK_MONO > >> i am checking actual clockid_t directly to set tstamp_type > >> example:- CLOCK_REALTIME/CLOCK_MONOTONIC > >> - Compilation error fixed in > >> net/ieee802154/6lowpan/reassembly.c > >> > >> 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 | 53 ++++++++++++++++------ > >> include/net/inet_frag.h | 4 +- > >> net/bridge/netfilter/nf_conntrack_bridge.c | 6 +-- > >> net/core/dev.c | 2 +- > >> net/core/filter.c | 10 ++-- > >> net/ieee802154/6lowpan/reassembly.c | 2 +- > >> net/ipv4/inet_fragment.c | 2 +- > >> net/ipv4/ip_fragment.c | 2 +- > >> net/ipv4/ip_output.c | 9 ++-- > >> net/ipv4/tcp_output.c | 16 +++---- > >> 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 +- > >> 17 files changed, 80 insertions(+), 52 deletions(-) > >> > >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > >> index 1c2902eaebd3..de3915e2bfdb 100644 > >> --- a/include/linux/skbuff.h > >> +++ b/include/linux/skbuff.h > >> @@ -706,6 +706,11 @@ typedef unsigned int sk_buff_data_t; > >> typedef unsigned char *sk_buff_data_t; > >> #endif > >> > >> +enum skb_tstamp_type { > >> + SKB_CLOCK_REALTIME, > >> + SKB_CLOCK_MONOTONIC, > >> +}; > >> + > >> /** > >> * DOC: Basic sk_buff geometry > >> * > >> @@ -823,10 +828,9 @@ 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. > >> + * @tstamp_type: When set, skb->tstamp has the > >> + * delivery_time in mono clock base Otherwise, the > >> + * timestamp is considered real clock base. > > > > Missing period. More importantly, no longer conditional. It always > > captures the type of skb->tstamp. > > > I think i should move the patchset 2 documentation to this patch itself. > > @tstamp_type: When set, skb->tstamp has the > + * delivery_time clock base of skb->tstamp. > >> --- a/net/ipv4/tcp_output.c > >> +++ b/net/ipv4/tcp_output.c > >> @@ -1301,7 +1301,7 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, > >> tp = tcp_sk(sk); > >> prior_wstamp = tp->tcp_wstamp_ns; > >> tp->tcp_wstamp_ns = max(tp->tcp_wstamp_ns, tp->tcp_clock_cache); > >> - skb_set_delivery_time(skb, tp->tcp_wstamp_ns, true); > >> + skb_set_delivery_type_by_clockid(skb, tp->tcp_wstamp_ns, CLOCK_MONOTONIC); > >> if (clone_it) { > >> oskb = skb; > >> > >> @@ -1655,7 +1655,7 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue, > >> > >> skb_split(skb, buff, len); > >> > >> - skb_set_delivery_time(buff, skb->tstamp, true); > >> + skb_set_delivery_type_by_clockid(buff, skb->tstamp, CLOCK_MONOTONIC); > >> tcp_fragment_tstamp(skb, buff); > > > > All these hardcoded monotonic calls in TCP can be the shorter version > > > > skb_set_delivery_type(.., SKB_CLOCK_MONOTONIC); > I think i should directly call skb_set_delivery_time if i know that TCP always uses Monotonic clock base, > rather than calling the wrapper api which does nothing but switch and then calls skb_set_delivery_Time. > > Makes sense. I will make the changes in v7 . Thanks, +1 on both
