Vadim Fedorenko wrote:

On 06/09/2024 17:33, Willem de Bruijn wrote:

quoted

Willem de Bruijn wrote:

quoted

Vadim Fedorenko wrote:

quoted

On 05/09/2024 17:39, Willem de Bruijn wrote:

quoted

Vadim Fedorenko wrote:

quoted

On 04/09/2024 12:31, Vadim Fedorenko wrote:

quoted

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 <redacted>
---
    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.

Yes, you are right, the problem is in a different place.

__skb_complete_tx_timestamp receives skb with shinfo->tskey equal to
provided by cmsg. But for TCP sockets it unconditionally adjusts ee_data
value:

	if (sk_is_tcp(sk))
		serr->ee.ee_data -= atomic_read(&sk->sk_tskey);

It happens because of assumption that for TCP sockets shinfo::tskey will
have sequence number and the logic has to recalculate it back to the
bytes sent. The same logic exists in all TCP TX timestamping functions
(mentioned in the previous mail) and may trigger some unexpected
behavior. To fix the issue we have to provide some kind of signal that
tskey value is provided from user-space and shouldn't be changed. And we
have to have it somewhere in skb info. Again, tx_flags looks like the
best candidate, but it's impossible to use. I'm thinking of using
special flag in tcp_skb_cb - gonna test more, but open for other
suggestions.

Ai, that is tricky. tx_flags is full/scarce indeed.

CB does not persist as the skb transitions between layers.

Though specifically for TCP, it is possible to look up the fast
clone on the rtx queue, whose tcp_skb_cb will be unperturbed. But
the tcb currently does not have this data either.

It will work fine for software timestamps, but we cannot do the same
trick in case of HW timestamps, right?

I'm not really advocating it. The only user of this trick that I can
find is skb_still_in_host_queue, through skb_fclone_busy.

That said, it would also work for hardware, as the SKB_FCLONE_ORIG
remains on the rtx queue. In the common case. But skb_fclone_busy
points out edge cases, such as if a driver call skb_orphan..