On 4/30/2024 1:26 PM, Willem de Bruijn wrote: > Abhishek Chauhan (ABC) wrote: >> >> >> On 4/26/2024 4:50 PM, Martin KaFai Lau wrote: >>> On 4/26/24 11:46 AM, Abhishek Chauhan (ABC) wrote: >>>>>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c >>>>>> index 591226dcde26..f195b31d6e75 100644 >>>>>> --- a/net/ipv4/ip_output.c >>>>>> +++ b/net/ipv4/ip_output.c >>>>>> @@ -1457,7 +1457,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk, >>>>>> skb->priority = (cork->tos != -1) ? cork->priority: READ_ONCE(sk->sk_priority); >>>>>> skb->mark = cork->mark; >>>>>> - skb->tstamp = cork->transmit_time; >>>>>> + skb_set_tstamp_type_frm_clkid(skb, cork->transmit_time, sk->sk_clockid); >>>>> hmm... I think this will break for tcp. This sequence in particular: > > Good catch, thanks! > >>>>> >>>>> tcp_v4_timewait_ack() >>>>> tcp_v4_send_ack() >>>>> ip_send_unicast_reply() >>>>> ip_push_pending_frames() >>>>> ip_finish_skb() >>>>> __ip_make_skb() >>>>> /* sk_clockid is REAL but cork->transmit_time should be in mono */ >>>>> skb_set_tstamp_type_frm_clkid(skb, cork->transmit_time, sk->sk_clockid);; >>>>> >>>>> I think I hit it from time to time when running the test in this patch set. >>>>> >>>> do you think i need to check for protocol type here . since tcp uses Mono and the rest according to the new design is based on >>>> sk->sk_clockid >>>> if (iph->protocol == IPPROTO_TCP) >>>> skb_set_tstamp_type_frm_clkid(skb, cork->transmit_time, CLOCK_MONOTONIC); >>>> else >>>> skb_set_tstamp_type_frm_clkid(skb, cork->transmit_time, sk->sk_clockid); >>> >>> Looks ok. iph->protocol is from sk->sk_protocol. I would defer to Willem input here. >>> >>> There is at least one more place that needs this protocol check, __ip6_make_skb(). >> >> Sounds good. I will wait for Willem to comment here. > > This would be sk_is_tcp(sk). > > I think we want to avoid special casing if we can. Note the if. > > If TCP always uses monotonic, we could consider initializing > sk_clockid to CLOCK_MONONOTIC in tcp_init_sock. > > I guess TCP logic currently entirely ignores sk_clockid. If we are to > start using this, then setsocktop SO_TXTIME must explicitly fail or > ignore for TCP sockets, or silently skip the write. > > All of that is more complexity. Than is maybe warranted for this one > case. So no objections from me to special casing using sk_is_tcp(sk) > either. > > Thanks Willem and Martin for all the support and reviews. I will take care of this in the next RFC patch - adding the sk_is_tcp check and setting tstamp_type to mono for tcp > >
