[PATCH] tcp: add tsval and tsecr to TCP_INFO

[PATCH] tcp: add tsval and tsecr to TCP_INFO

[William Dauchy]

tsval and tsecr are useful in some cases to diagnose TCP issues from the
sender point of view where unexplained RTT values are seen. Getting the
the timestamps from both ends will help understand those issues more
easily.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
---
 include/uapi/linux/tcp.h | 3 +++
 net/ipv4/tcp.c           | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 81e697978e8b..fecd4d0f177c 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -276,6 +276,9 @@ struct tcp_info {
 	__u32	tcpi_snd_wnd;	     /* peer's advertised receive window after
 				      * scaling (bytes)
 				      */
+
+	__u32	tcpi_tsval;          /* Time stamp value */
+	__u32	tcpi_tsecr;          /* Time stamp echo reply */
 };
 
 /* netlink attributes types for SCM_TIMESTAMPING_OPT_STATS */
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 79c325a07ba5..7d0968df99c9 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3229,8 +3229,11 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
 	info->tcpi_probes = icsk->icsk_probes_out;
 	info->tcpi_backoff = icsk->icsk_backoff;
 
-	if (tp->rx_opt.tstamp_ok)
+	if (tp->rx_opt.tstamp_ok) {
 		info->tcpi_options |= TCPI_OPT_TIMESTAMPS;
+		info->tcpi_tsval = tp->rx_opt.rcv_tsval;
+		info->tcpi_tsecr = tp->rx_opt.rcv_tsecr;
+	}
 	if (tcp_is_sack(tp))
 		info->tcpi_options |= TCPI_OPT_SACK;
 	if (tp->rx_opt.wscale_ok) {
-- 
2.23.0

Re: [PATCH] tcp: add tsval and tsecr to TCP_INFO

[Eric Dumazet]

On 10/2/19 3:10 PM, William Dauchy wrote:
> tsval and tsecr are useful in some cases to diagnose TCP issues from the
> sender point of view where unexplained RTT values are seen. Getting the
> the timestamps from both ends will help understand those issues more
> easily.
> 

Reporting the last recorded values is really not good,
a packet capture will give you all this information in a non
racy way.

Re: [PATCH] tcp: add tsval and tsecr to TCP_INFO

[William Dauchy]

Hello Eric,

On Thu, Oct 3, 2019 at 12:33 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On 10/2/19 3:10 PM, William Dauchy wrote:
> Reporting the last recorded values is really not good,
> a packet capture will give you all this information in a non
> racy way.

Thank you for your quick answer.
In my use case I use it on a http server where I tag my requests with
such informations coming from tcp, which later helps to diagnose some
issues and create some useful metrics to give me a general signal.
Does it still sound like an invalid use case?

Best regards,
-- 
William

Re: [PATCH] tcp: add tsval and tsecr to TCP_INFO

[Eric Dumazet]

On 10/2/19 3:54 PM, William Dauchy wrote:
> Hello Eric,
> 
> On Thu, Oct 3, 2019 at 12:33 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On 10/2/19 3:10 PM, William Dauchy wrote:
>> Reporting the last recorded values is really not good,
>> a packet capture will give you all this information in a non
>> racy way.
> 
> Thank you for your quick answer.
> In my use case I use it on a http server where I tag my requests with
> such informations coming from tcp, which later helps to diagnose some
> issues and create some useful metrics to give me a general signal.
> Does it still sound like an invalid use case?

I would rather use a new getsockopt() to fetch this specific data,
instead of making TCP_INFO bigger for everyone :/

ss command can dump millions of sockets in busy hosts, we need to be
careful of TCP_INFO size.

Re: [PATCH] tcp: add tsval and tsecr to TCP_INFO

[William Dauchy]

On Thu, Oct 3, 2019 at 1:14 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> I would rather use a new getsockopt() to fetch this specific data,
> instead of making TCP_INFO bigger for everyone :/
>
> ss command can dump millions of sockets in busy hosts, we need to be
> careful of TCP_INFO size.

Thanks Eric for your advices. I will see how I can come back with a
second proposition.
-- 
William

[PATCH v2] tcp: add timestamp options fetcher

[William Dauchy]

tsval and tsecr are useful in some cases to diagnose TCP issues from the
sender point of view where unexplained RTT values are seen. Getting the
the timestamps from both ends will help understand those issues more
easily.
It can be mostly use in some specific cases, e.g a http server where
requests are tagged with such informations, which later helps to
diagnose some issues and create some useful metrics to give a general
signal.

Signed-off-by: William Dauchy <wdauchy@gmail.com>

---

Changes in v2:
- change from tcp_info to a new getsockopt() to avoid making tcp_info
  bigger for everyone
---
 include/uapi/linux/tcp.h |  6 ++++++
 net/ipv4/tcp.c           | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 81e697978e8b..2a9685216aef 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -128,12 +128,18 @@ enum {
 #define TCP_CM_INQ		TCP_INQ
 
 #define TCP_TX_DELAY		37	/* delay outgoing packets by XX usec */
+#define TCP_TIMESTAMP_OPT	38	/* timestamps option with tsval and tsecr values */
 
 
 #define TCP_REPAIR_ON		1
 #define TCP_REPAIR_OFF		0
 #define TCP_REPAIR_OFF_NO_WP	-1	/* Turn off without window probes */
 
+struct tcp_timestamp_opt {
+	__u32	tcp_tsval;
+	__u32	tcp_tsecr;
+};
+
 struct tcp_repair_opt {
 	__u32	opt_code;
 	__u32	opt_val;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 42187a3b82f4..b9c34a5477dd 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3309,6 +3309,21 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
 }
 EXPORT_SYMBOL_GPL(tcp_get_info);
 
+/* Return timestamps option of tcp endpoint. */
+static void tcp_get_tsopt(struct sock *sk, struct tcp_timestamp_opt *tsopt)
+{
+	const struct tcp_sock *tp = tcp_sk(sk);
+
+	memset(tsopt, 0, sizeof(*tsopt));
+	if (sk->sk_type != SOCK_STREAM)
+		return;
+
+	if (tp->rx_opt.tstamp_ok) {
+		tsopt->tcp_tsval = tp->rx_opt.rcv_tsval;
+		tsopt->tcp_tsecr = tp->rx_opt.rcv_tsecr;
+	}
+}
+
 static size_t tcp_opt_stats_get_size(void)
 {
 	return
@@ -3668,6 +3683,21 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
 		return err;
 	}
 #endif
+	case TCP_TIMESTAMP_OPT: {
+		struct tcp_timestamp_opt tsopt;
+
+		if (get_user(len, optlen))
+			return -EFAULT;
+
+		tcp_get_tsopt(sk, &tsopt);
+
+		len = min_t(unsigned int, len, sizeof(tsopt));
+		if (put_user(len, optlen))
+			return -EFAULT;
+		if (copy_to_user(optval, &tsopt, len))
+			return -EFAULT;
+		return 0;
+	}
 	default:
 		return -ENOPROTOOPT;
 	}
-- 
2.23.0

Re: [PATCH v2] tcp: add timestamp options fetcher

[Eric Dumazet]

On 10/26/19 11:45 AM, William Dauchy wrote:
> tsval and tsecr are useful in some cases to diagnose TCP issues from the
> sender point of view where unexplained RTT values are seen. Getting the
> the timestamps from both ends will help understand those issues more
> easily.
> It can be mostly use in some specific cases, e.g a http server where
> requests are tagged with such informations, which later helps to
> diagnose some issues and create some useful metrics to give a general
> signal.
> 

William, I am sorry but you do not describe what is supposed to be returned
in the structure.

Is it something updated for every incoming packet, every outgoing packet ?

What about out of order packets ?

Is the tcp_tsval our view of the tcp timestamps, or the value from the remote peer ?

What time unit is used for the values ?

What happens if TCP receives a packet that is discarded ? (for example by PAWS check)

tcp_parse_aligned_timestamp() would have updated tp->rx_opt.rcv_tsval and tp->rx_opt.rcv_tsecr
with garbage.

This means tp->rx_opt.rcv_tsval and tp->rx_opt.rcv_tsecr and really some working space
that could contain garbage. We could even imagine that a future version of TCP
no longer uses stable storage in the tcp socket for these, but some temporary room in the stack.

You really need to put more thinking into this, because otherwise every possible user
will have to dig the answers into linux kernel source.

In short, I really believe this tsval/tsecr thing is very hard to define and would
add more complexity in TCP just to make sure this is correctly implemented
and wont regress in the future.

You will have to convince us that this is a super useful feature, maybe publishing research results.