Re: [PATCH net-next v3 2/4] net_tstamp: add SCM_TS_OPT_ID for TCP sockets

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Vadim Fedorenko wrote:
> On 04/09/2024 12:31, Vadim Fedorenko wrote:
> > TCP sockets have different flow for providing timestamp OPT_ID value.
> > Adjust the code to support SCM_TS_OPT_ID option for TCP sockets.
> > 
> > Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> > ---
> >   net/ipv4/tcp.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 8a5680b4e786..5553a8aeee80 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -474,9 +474,10 @@ void tcp_init_sock(struct sock *sk)
> >   }
> >   EXPORT_SYMBOL(tcp_init_sock);
> >   
> > -static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
> > +static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
> >   {
> >   	struct sk_buff *skb = tcp_write_queue_tail(sk);
> > +	u32 tsflags = sockc->tsflags;
> >   
> >   	if (tsflags && skb) {
> >   		struct skb_shared_info *shinfo = skb_shinfo(skb);
> > @@ -485,8 +486,12 @@ static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
> >   		sock_tx_timestamp(sk, tsflags, &shinfo->tx_flags);
> >   		if (tsflags & SOF_TIMESTAMPING_TX_ACK)
> >   			tcb->txstamp_ack = 1;
> > -		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
> > -			shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > +		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) {
> > +			if (tsflags & SOCKCM_FLAG_TS_OPT_ID)
> > +				shinfo->tskey = sockc->ts_opt_id;
> > +			else
> > +				shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > +		}
> >   	}
> >   }
> >   
> > @@ -1318,7 +1323,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
> >   
> >   out:
> >   	if (copied) {
> > -		tcp_tx_timestamp(sk, sockc.tsflags);
> > +		tcp_tx_timestamp(sk, &sockc);
> >   		tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
> >   	}
> >   out_nopush:
> 
> Hi Willem,
> 
> Unfortunately, these changes are not enough to enable custom OPT_ID for
> TCP sockets. There are some functions which rewrite shinfo->tskey in TCP
> flow:
> 
> tcp_skb_collapse_tstamp()
> tcp_fragment_tstamp()
> tcp_gso_tstamp()
> 
> I believe the last one breaks tests, but the problem is that there is no
> easy way to provide the flag of constant tskey to it. Only
> shinfo::tx_flags are available at the caller side and we have already
> discussed that we shouldn't use the last bit of this field.
> 
> So, how should we deal with the problem? Or is it better to postpone
> support for TCP sockets in this case?

Are you sure that this is a problem. These functions pass on the
skb_shinfo(skb)->ts_key from one skb to another.

As long as tcp_tx_timestamp sets the skb_shinfo(skb)->ts_key of the
original skb correctly, either from the cmsg or sk_tskey, then I don't
immediate see how this passing on from one skb to another would break
the intent.