[RFC PATCH v2 net-next 0/7] tcp: Make use of MSG_EOR in tcp_sendmsg

[RFC PATCH v2 net-next 0/7] tcp: Make use of MSG_EOR in tcp_sendmsg

[Martin KaFai Lau]

v2:
~ Rework based on the recent work
  "add TX timestamping via cmsg" by
  Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
~ This version takes the MSG_EOR bit as a signal of
  end-of-response-message and leave the selective
  timestamping job to the cmsg
~ Changes based on the v1 feedback (like avoid
  unlikely check in a loop and adding tcp_sendpage
  support)
~ The first 3 patches are bug fixes.  The fixes in this
  series depend on the newly introduced txstamp_ack in
  net-next.  I will make relevant patches against net after
  getting some feedback.
~ The test results are based on the recently posted net fix:
  "tcp: Fix SOF_TIMESTAMPING_TX_ACK when handling dup acks"
~ Due to the lacking cmsg support in packetdrill (or I just
  could not find it), a BPF prog is used to kprobe
  to sock_queue_err_skb() and print out the value of
  serr->ee.ee_data.  The BPF prog (run-able from bcc) is
  attached at the end.

Some of the following is stolen from a commit message of
the following patch to serve as a high level description of
the objective in this series:

This patchset allows the user process to use MSG_EOR during
tcp_sendmsg to tell the kernel that it is the last byte
of an application response message.

It is currently useful when the end-user has turned on any bit of the
SOF_TIMESTAMPING_TX_RECORD_MASK (either by setsockopt or cmsg).
The kernel will then mark the newly added tcb->eor_info bit so
that the shinfo->tskey will not be overwritten (i.e. lost) in
the later skb append/collapse operation.

Each skb can only track one tskey (which is the seq number of the
last byte of the message).   To allow tracking the last byte of
multiple response messages (e.g. HTTP2), this patch takes an
approach by not appending to the previous skb during tcp_sendmsg
if this previous skb's eor information (only shinfo->tskey for now)
will be overwritten.  A similar case also happens during
retransmission.

This approach avoids introducing another list to track the tskey.
The downside is that it will have less tso benefit and/or more
outgoing packets.  Practically, due to the amount of measurement
data generated, sampling is usually used in production. (i.e. not
every connection is tracked).

One of our use case is at the webserver.  The webserver tracks
the HTTP2 response latency by measuring when the webserver sends
the first byte to the socket till the TCP ACK of the last byte
is received.  In the cases where we don't have client side
measurement, measuring from the server side is the only option.
In the cases we have the client side measurement, the server side
data can also be used to justify/cross-check-with the client
side data.

The TCP PRR paper [1] also measures a similar metrics:
"The TCP latency of a HTTP response when the server sends the first
byte until it receives the acknowledgment (ACK) for the last byte."

[1] Proportional Rate Reduction for TCP:
http://static.googleusercontent.com/media/research.google.com/en//pubs/archive/37486.pdf

BPF prog used for testing:
~~~~~
#!/usr/bin/env python

from __future__ import print_function
from bcc import BPF

bpf_text = """
#include <uapi/linux/ptrace.h>
#include <net/sock.h>
#include <bcc/proto.h>
#include <linux/errqueue.h>

#ifdef memset
#undef memset
#endif

int trace_err_skb(struct pt_regs *ctx)
{
	struct sk_buff *skb = (struct sk_buff *)ctx->si;
	struct sock *sk = (struct sock *)ctx->di;
	struct sock_exterr_skb *serr;
	u32 ee_data = 0;

	if (!sk || !skb)
		return 0;

	serr = SKB_EXT_ERR(skb);
	bpf_probe_read(&ee_data, sizeof(ee_data), &serr->ee.ee_data);
	bpf_trace_printk("ee_data:%u\\n", ee_data);

	return 0;
};
"""

b = BPF(text=bpf_text)
b.attach_kprobe(event="sock_queue_err_skb", fn_name="trace_err_skb")
print("Attached to kprobe")
b.trace_print()

[RFC PATCH v2 net-next 1/7] tcp: Carry txstamp_ack in tcp_fragment_tstamp

[Martin KaFai Lau]

When a tcp skb is sliced into two smaller skbs (e.g. in
tcp_fragment() and tso_fragment()),  it does not carry
the txstamp_ack bit to the newly created skb if it is needed.
The end result is a timestamping event (SCM_TSTAMP_ACK) will
be missing from the sk->sk_error_queue.

This patch carries this bit to the new skb2 (if needed)
in tcp_fragment_tstamp().

BPF Output Before:
~~~~~~
<No output due to missing SCM_TSTAMP_ACK timestamp>

BPF Output After:
~~~~~~
<...>-2050  [000] d.s.   100.928763: : ee_data:14599

Packetdrill Script:
~~~~~~
+0 `sysctl -q -w net.ipv4.tcp_min_tso_segs=10`
+0 `sysctl -q -w net.ipv4.tcp_no_metrics_save=1`
+0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0

0.100 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7>
0.100 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
0.200 < . 1:1(0) ack 1 win 257
0.200 accept(3, ..., ...) = 4
+0 setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0

+0 setsockopt(4, SOL_SOCKET, 37, [2688], 4) = 0
0.200 write(4, ..., 14600) = 14600
+0 setsockopt(4, SOL_SOCKET, 37, [2176], 4) = 0

0.200 > . 1:7301(7300) ack 1
0.200 > P. 7301:14601(7300) ack 1

0.300 < . 1:1(0) ack 14601 win 257

0.300 close(4) = 0
0.300 > F. 14601:14601(0) ack 1
0.400 < F. 1:1(0) ack 16062 win 257
0.400 > . 14602:14602(0) ack 2

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_output.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 6451b83..0527ce9 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1123,6 +1123,8 @@ static void tcp_fragment_tstamp(struct sk_buff *skb, struct sk_buff *skb2)
 		shinfo->tx_flags &= ~tsflags;
 		shinfo2->tx_flags |= tsflags;
 		swap(shinfo->tskey, shinfo2->tskey);
+		TCP_SKB_CB(skb2)->txstamp_ack = TCP_SKB_CB(skb)->txstamp_ack;
+		TCP_SKB_CB(skb)->txstamp_ack = 0;
 	}
 }
 
-- 
2.5.1

Re: [RFC PATCH v2 net-next 1/7] tcp: Carry txstamp_ack in tcp_fragment_tstamp

[Soheil Hassas Yeganeh]

On Mon, Apr 18, 2016 at 6:46 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> When a tcp skb is sliced into two smaller skbs (e.g. in
> tcp_fragment() and tso_fragment()),  it does not carry
> the txstamp_ack bit to the newly created skb if it is needed.
> The end result is a timestamping event (SCM_TSTAMP_ACK) will
> be missing from the sk->sk_error_queue.
>
> This patch carries this bit to the new skb2 (if needed)
> in tcp_fragment_tstamp().
>
> BPF Output Before:
> ~~~~~~
> <No output due to missing SCM_TSTAMP_ACK timestamp>
>
> BPF Output After:
> ~~~~~~
> <...>-2050  [000] d.s.   100.928763: : ee_data:14599
>
> Packetdrill Script:
> ~~~~~~
> +0 `sysctl -q -w net.ipv4.tcp_min_tso_segs=10`
> +0 `sysctl -q -w net.ipv4.tcp_no_metrics_save=1`
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, ..., ...) = 0
> +0 listen(3, 1) = 0
>
> 0.100 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7>
> 0.100 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> 0.200 < . 1:1(0) ack 1 win 257
> 0.200 accept(3, ..., ...) = 4
> +0 setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0
>
> +0 setsockopt(4, SOL_SOCKET, 37, [2688], 4) = 0
> 0.200 write(4, ..., 14600) = 14600
> +0 setsockopt(4, SOL_SOCKET, 37, [2176], 4) = 0
>
> 0.200 > . 1:7301(7300) ack 1
> 0.200 > P. 7301:14601(7300) ack 1
>
> 0.300 < . 1:1(0) ack 14601 win 257
>
> 0.300 close(4) = 0
> 0.300 > F. 14601:14601(0) ack 1
> 0.400 < F. 1:1(0) ack 16062 win 257
> 0.400 > . 14602:14602(0) ack 2
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc:  Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> ---
>  net/ipv4/tcp_output.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 6451b83..0527ce9 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1123,6 +1123,8 @@ static void tcp_fragment_tstamp(struct sk_buff *skb, struct sk_buff *skb2)
>                 shinfo->tx_flags &= ~tsflags;
>                 shinfo2->tx_flags |= tsflags;
>                 swap(shinfo->tskey, shinfo2->tskey);
> +               TCP_SKB_CB(skb2)->txstamp_ack = TCP_SKB_CB(skb)->txstamp_ack;
> +               TCP_SKB_CB(skb)->txstamp_ack = 0;
>         }
>  }

Thanks for the fixes! I was going to submit similar patches. :-)

Could you please submit the timestamping patches separately as non RFCs? Thanks!

> --
> 2.5.1
>

Re: [RFC PATCH v2 net-next 1/7] tcp: Carry txstamp_ack in tcp_fragment_tstamp

[Martin KaFai Lau]

On Tue, Apr 19, 2016 at 01:21:04AM -0400, Soheil Hassas Yeganeh wrote:
> Could you please submit the timestamping patches separately as non RFCs? Thanks!
Agree.  I will re-spin.

Re: [RFC PATCH v2 net-next 1/7] tcp: Carry txstamp_ack in tcp_fragment_tstamp

[Soheil Hassas Yeganeh]

On Tue, Apr 19, 2016 at 1:39 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Tue, Apr 19, 2016 at 01:21:04AM -0400, Soheil Hassas Yeganeh wrote:
>> Could you please submit the timestamping patches separately as non RFCs? Thanks!
> Agree.  I will re-spin.

Great, thank you very much!

[RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Martin KaFai Lau]

If two skbs are merged/collapsed during retransmission, the current
logic does not merge the tx_flags, tskey and txstamp_ack.  The end
result is the SCM_TSTAMP_ACK timestamp could be missing for a
packet that the end-user has specifically turned on
SOF_TIMESTAMPING_TX_ACK (e.g. by cmsg).

The patch:
1. Merge the tx_flags and txstamp_ack
2. Overwrite the tskey with the later skb (next_skb)

BPF Output Before:
~~~~~~
<no-output-due-to-missing-tstamp-event>

BPF Output After:
~~~~~~
packetdrill-2092  [001] d.s.   453.998486: : ee_data:1459

Packetdrill Script:
~~~~~~
+0 `sysctl -q -w net.ipv4.tcp_min_tso_segs=10`
+0 `sysctl -q -w net.ipv4.tcp_no_metrics_save=1`
+0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0

0.100 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7>
0.100 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
0.200 < . 1:1(0) ack 1 win 257
0.200 accept(3, ..., ...) = 4
+0 setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0

0.200 write(4, ..., 730) = 730
+0 setsockopt(4, SOL_SOCKET, 37, [2688], 4) = 0
0.200 write(4, ..., 730) = 730
+0 setsockopt(4, SOL_SOCKET, 37, [2176], 4) = 0
0.200 write(4, ..., 11680) = 11680

0.200 > P. 1:731(730) ack 1
0.200 > P. 731:1461(730) ack 1
0.200 > . 1461:8761(7300) ack 1
0.200 > P. 8761:13141(4380) ack 1

0.300 < . 1:1(0) ack 1 win 257 <sack 1461:2921,nop,nop>
0.300 < . 1:1(0) ack 1 win 257 <sack 1461:4381,nop,nop>
0.300 < . 1:1(0) ack 1 win 257 <sack 1461:5841,nop,nop>
0.300 > P. 1:1461(1460) ack 1
0.400 < . 1:1(0) ack 13141 win 257

0.400 close(4) = 0
0.400 > F. 13141:13141(0) ack 1
0.500 < F. 1:1(0) ack 13142 win 257
0.500 > . 13142:13142(0) ack 2

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_output.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 0527ce9..889ed96 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2443,6 +2443,22 @@ u32 __tcp_select_window(struct sock *sk)
 	return window;
 }
 
+static void tcp_skb_collapse_tstamp(struct sk_buff *skb,
+				    const struct sk_buff *next_skb)
+{
+	const struct skb_shared_info *next_shinfo = skb_shinfo(next_skb);
+
+	if (unlikely(next_shinfo->tx_flags & SKBTX_ANY_TSTAMP)) {
+		struct skb_shared_info *shinfo = skb_shinfo(skb);
+		u8 tsflags = next_shinfo->tx_flags & SKBTX_ANY_TSTAMP;
+
+		shinfo->tx_flags |= tsflags;
+		shinfo->tskey = next_shinfo->tskey;
+		TCP_SKB_CB(skb)->txstamp_ack =
+			!!(shinfo->tx_flags & SKBTX_ACK_TSTAMP);
+	}
+}
+
 /* Collapses two adjacent SKB's during retransmission. */
 static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
 {
@@ -2486,6 +2502,8 @@ static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
 
 	tcp_adjust_pcount(sk, next_skb, tcp_skb_pcount(next_skb));
 
+	tcp_skb_collapse_tstamp(skb, next_skb);
+
 	sk_wmem_free_skb(sk, next_skb);
 }
 
-- 
2.5.1

Re: [RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Soheil Hassas Yeganeh]

On Mon, Apr 18, 2016 at 6:46 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> If two skbs are merged/collapsed during retransmission, the current
> logic does not merge the tx_flags, tskey and txstamp_ack.  The end
> result is the SCM_TSTAMP_ACK timestamp could be missing for a
> packet that the end-user has specifically turned on
> SOF_TIMESTAMPING_TX_ACK (e.g. by cmsg).
>
> The patch:
> 1. Merge the tx_flags and txstamp_ack
> 2. Overwrite the tskey with the later skb (next_skb)
>
> BPF Output Before:
> ~~~~~~
> <no-output-due-to-missing-tstamp-event>
>
> BPF Output After:
> ~~~~~~
> packetdrill-2092  [001] d.s.   453.998486: : ee_data:1459
>
> Packetdrill Script:
> ~~~~~~
> +0 `sysctl -q -w net.ipv4.tcp_min_tso_segs=10`
> +0 `sysctl -q -w net.ipv4.tcp_no_metrics_save=1`
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, ..., ...) = 0
> +0 listen(3, 1) = 0
>
> 0.100 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7>
> 0.100 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> 0.200 < . 1:1(0) ack 1 win 257
> 0.200 accept(3, ..., ...) = 4
> +0 setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0
>
> 0.200 write(4, ..., 730) = 730
> +0 setsockopt(4, SOL_SOCKET, 37, [2688], 4) = 0
> 0.200 write(4, ..., 730) = 730
> +0 setsockopt(4, SOL_SOCKET, 37, [2176], 4) = 0
> 0.200 write(4, ..., 11680) = 11680
>
> 0.200 > P. 1:731(730) ack 1
> 0.200 > P. 731:1461(730) ack 1
> 0.200 > . 1461:8761(7300) ack 1
> 0.200 > P. 8761:13141(4380) ack 1
>
> 0.300 < . 1:1(0) ack 1 win 257 <sack 1461:2921,nop,nop>
> 0.300 < . 1:1(0) ack 1 win 257 <sack 1461:4381,nop,nop>
> 0.300 < . 1:1(0) ack 1 win 257 <sack 1461:5841,nop,nop>
> 0.300 > P. 1:1461(1460) ack 1
> 0.400 < . 1:1(0) ack 13141 win 257
>
> 0.400 close(4) = 0
> 0.400 > F. 13141:13141(0) ack 1
> 0.500 < F. 1:1(0) ack 13142 win 257
> 0.500 > . 13142:13142(0) ack 2
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>

Cc:  Soheil Hassas Yeganeh <soheil@google.com>

> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> ---
>  net/ipv4/tcp_output.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 0527ce9..889ed96 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2443,6 +2443,22 @@ u32 __tcp_select_window(struct sock *sk)
>         return window;
>  }
>
> +static void tcp_skb_collapse_tstamp(struct sk_buff *skb,
> +                                   const struct sk_buff *next_skb)
> +{
> +       const struct skb_shared_info *next_shinfo = skb_shinfo(next_skb);
> +
> +       if (unlikely(next_shinfo->tx_flags & SKBTX_ANY_TSTAMP)) {
> +               struct skb_shared_info *shinfo = skb_shinfo(skb);
> +               u8 tsflags = next_shinfo->tx_flags & SKBTX_ANY_TSTAMP;

nit: maybe move this local variable out of the if block?

      tsflags = ...
      if (unlikely(tsflags)) { ... }

> +
> +               shinfo->tx_flags |= tsflags;
> +               shinfo->tskey = next_shinfo->tskey;
> +               TCP_SKB_CB(skb)->txstamp_ack =
> +                       !!(shinfo->tx_flags & SKBTX_ACK_TSTAMP);

Maybe we can skip a conditional jump here (because of !!), by simply
using the cached bit in next_skb:
TCP_SKB_CB(skb)->txstamp_ack = TCP_SKB_CB(next_skb)->txstamp_ack;

> +       }
> +}
> +
>  /* Collapses two adjacent SKB's during retransmission. */
>  static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
>  {
> @@ -2486,6 +2502,8 @@ static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
>
>         tcp_adjust_pcount(sk, next_skb, tcp_skb_pcount(next_skb));
>
> +       tcp_skb_collapse_tstamp(skb, next_skb);
> +
>         sk_wmem_free_skb(sk, next_skb);
>  }

Really nice fixes! thanks.

> --
> 2.5.1
>

Re: [RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Martin KaFai Lau]

On Tue, Apr 19, 2016 at 01:32:14AM -0400, Soheil Hassas Yeganeh wrote:
> > +               TCP_SKB_CB(skb)->txstamp_ack =
> > +                       !!(shinfo->tx_flags & SKBTX_ACK_TSTAMP);
>
> Maybe we can skip a conditional jump here (because of !!), by simply
> using the cached bit in next_skb:
> TCP_SKB_CB(skb)->txstamp_ack = TCP_SKB_CB(next_skb)->txstamp_ack;
Recall the tx_flags are merged/combined (and so should be the txstamp_ack).
Would there be a case that TCP_SKB_CB(skb)->txstamp_ack is 1 and
TCP_SKB_CB(next_skb)->txstamp_ack is 0?

I can change it like the following which may help in showing the intention:
if (TCP_SKB_CB(next_skb)->txstamp_ack)
	TCP_SKB_CB(skb)->txstamp_ack = 1;

A bit off topic, I feel like the SKBTX_ACK_TSTAMP and txstamp_ack are sort
of redundant but I have not look into the details yet, so not completely
sure.  It wwould be a separate cleanup patch if it is the case.

Re: [RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Eric Dumazet]

On Tue, Apr 19, 2016 at 10:28 AM, Martin KaFai Lau <kafai@fb.com> wrote:

> A bit off topic, I feel like the SKBTX_ACK_TSTAMP and txstamp_ack are sort
> of redundant but I have not look into the details yet, so not completely
> sure.  It wwould be a separate cleanup patch if it is the case.

Please read 6b084928baac562ed61866f540a96120e9c9ddb7 changelog ;)

A cache line miss avoidance is critical

Re: [RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Martin KaFai Lau]

On Tue, Apr 19, 2016 at 10:35:52AM -0700, Eric Dumazet wrote:
> On Tue, Apr 19, 2016 at 10:28 AM, Martin KaFai Lau <kafai@fb.com> wrote:
>
> > A bit off topic, I feel like the SKBTX_ACK_TSTAMP and txstamp_ack are sort
> > of redundant but I have not look into the details yet, so not completely
> > sure.  It wwould be a separate cleanup patch if it is the case.
>
> Please read 6b084928baac562ed61866f540a96120e9c9ddb7 changelog ;)
>
> A cache line miss avoidance is critical
I looked at the patch but I probably am missing something :(
Is checking txstamp_ack alone enough and SKBTX_ACK_TSTAMP is not needed
since they are always set together?

Re: [RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Soheil Hassas Yeganeh]

On Tue, Apr 19, 2016 at 2:18 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Tue, Apr 19, 2016 at 10:35:52AM -0700, Eric Dumazet wrote:
>> On Tue, Apr 19, 2016 at 10:28 AM, Martin KaFai Lau <kafai@fb.com> wrote:
>>
>> > A bit off topic, I feel like the SKBTX_ACK_TSTAMP and txstamp_ack are sort
>> > of redundant but I have not look into the details yet, so not completely
>> > sure.  It wwould be a separate cleanup patch if it is the case.
>>
>> Please read 6b084928baac562ed61866f540a96120e9c9ddb7 changelog ;)
>>
>> A cache line miss avoidance is critical
> I looked at the patch but I probably am missing something :(
> Is checking txstamp_ack alone enough and SKBTX_ACK_TSTAMP is not needed
> since they are always set together?

That's right, the check on "(shinfo->tx_flags & SKBTX_ACK_TSTAMP)" in
tcp_ack_tstamp() is redundant and I had a patch prepared to remove it.
But I thought it's better to wait for
https://patchwork.ozlabs.org/patch/611938/ to be merged first.

Feel free to remove it in your patches, if you'd prefer that.

Re: [RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Willem de Bruijn]

On Tue, Apr 19, 2016 at 2:24 PM, Soheil Hassas Yeganeh
<soheil@google.com> wrote:
> On Tue, Apr 19, 2016 at 2:18 PM, Martin KaFai Lau <kafai@fb.com> wrote:
>> On Tue, Apr 19, 2016 at 10:35:52AM -0700, Eric Dumazet wrote:
>>> On Tue, Apr 19, 2016 at 10:28 AM, Martin KaFai Lau <kafai@fb.com> wrote:
>>>
>>> > A bit off topic, I feel like the SKBTX_ACK_TSTAMP and txstamp_ack are sort
>>> > of redundant but I have not look into the details yet, so not completely
>>> > sure.  It wwould be a separate cleanup patch if it is the case.

Yes, with the introduction of txstamp_ack, SKBTX_ACK_TSTAMP is completely
redundant.

>>>
>>> Please read 6b084928baac562ed61866f540a96120e9c9ddb7 changelog ;)
>>>
>>> A cache line miss avoidance is critical
>> I looked at the patch but I probably am missing something :(
>> Is checking txstamp_ack alone enough and SKBTX_ACK_TSTAMP is not needed
>> since they are always set together?
>
> That's right, the check on "(shinfo->tx_flags & SKBTX_ACK_TSTAMP)" in
> tcp_ack_tstamp() is redundant and I had a patch prepared to remove it.

You can even remove the flag completely and

-               tcb->txstamp_ack = !!(shinfo->tx_flags & SKBTX_ACK_TSTAMP);
+               if (tsflags & SOF_TIMESTAMPING_TX_ACK)
+                       tcb->txstamp_ack = 1;

> But I thought it's better to wait for
> https://patchwork.ozlabs.org/patch/611938/ to be merged first.
>
> Feel free to remove it in your patches, if you'd prefer that.

Re: [RFC PATCH v2 net-next 2/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_collapse_retrans

[Soheil Hassas Yeganeh]

On Tue, Apr 19, 2016 at 1:28 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Tue, Apr 19, 2016 at 01:32:14AM -0400, Soheil Hassas Yeganeh wrote:
>> > +               TCP_SKB_CB(skb)->txstamp_ack =
>> > +                       !!(shinfo->tx_flags & SKBTX_ACK_TSTAMP);
>>
>> Maybe we can skip a conditional jump here (because of !!), by simply
>> using the cached bit in next_skb:
>> TCP_SKB_CB(skb)->txstamp_ack = TCP_SKB_CB(next_skb)->txstamp_ack;
> Recall the tx_flags are merged/combined (and so should be the txstamp_ack).

Oh sure, sorry, I missed an "or":

TCP_SKB_CB(skb)->txstamp_ack |= TCP_SKB_CB(next_skb)->txstamp_ack;

> Would there be a case that TCP_SKB_CB(skb)->txstamp_ack is 1 and
> TCP_SKB_CB(next_skb)->txstamp_ack is 0?
>
> I can change it like the following which may help in showing the intention:
> if (TCP_SKB_CB(next_skb)->txstamp_ack)
>         TCP_SKB_CB(skb)->txstamp_ack = 1;
>
> A bit off topic, I feel like the SKBTX_ACK_TSTAMP and txstamp_ack are sort
> of redundant but I have not look into the details yet, so not completely
> sure.  It wwould be a separate cleanup patch if it is the case.

As Eric mentioned, this is needed to avoid a cache-line miss in
accessing the shared info.

[RFC PATCH v2 net-next 3/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_shifted_skb

[Martin KaFai Lau]

After receiving sacks, tcp_shifted_skb() will collapse
skbs if possible.  tx_flags/tskey/txstamp_ack also has
to be merged in this case.

This patch resues the tcp_skb_collapse_tstamp() to handle
them.

BPF Output Before:
~~~~~
<no-output-due-to-missing-tstamp-event>

BPF Output After:
~~~~~
<...>-2024  [007] d.s.    88.644374: : ee_data:14599

Packetdrill Script:
~~~~~
+0 `sysctl -q -w net.ipv4.tcp_min_tso_segs=10`
+0 `sysctl -q -w net.ipv4.tcp_no_metrics_save=1`
+0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0

0.100 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7>
0.100 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
0.200 < . 1:1(0) ack 1 win 257
0.200 accept(3, ..., ...) = 4
+0 setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0

0.200 write(4, ..., 1460) = 1460
+0 setsockopt(4, SOL_SOCKET, 37, [2688], 4) = 0
0.200 write(4, ..., 13140) = 13140
+0 setsockopt(4, SOL_SOCKET, 37, [2176], 4) = 0

0.200 > P. 1:1461(1460) ack 1
0.200 > . 1461:8761(7300) ack 1
0.200 > P. 8761:14601(5840) ack 1

0.300 < . 1:1(0) ack 1 win 257 <sack 1461:14601,nop,nop>
0.300 > P. 1:1461(1460) ack 1
0.400 < . 1:1(0) ack 14601 win 257

0.400 close(4) = 0
0.400 > F. 14601:14601(0) ack 1
0.500 < F. 1:1(0) ack 14602 win 257
0.500 > . 14602:14602(0) ack 2

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 include/net/tcp.h     | 2 ++
 net/ipv4/tcp_input.c  | 1 +
 net/ipv4/tcp_output.c | 4 ++--
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index fd40f8c..c0ef054 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -557,6 +557,8 @@ void tcp_send_ack(struct sock *sk);
 void tcp_send_delayed_ack(struct sock *sk);
 void tcp_send_loss_probe(struct sock *sk);
 bool tcp_schedule_loss_probe(struct sock *sk);
+void tcp_skb_collapse_tstamp(struct sk_buff *skb,
+			     const struct sk_buff *next_skb);
 
 /* tcp_input.c */
 void tcp_resume_early_retransmit(struct sock *sk);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5e45a9c..75e8336 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1309,6 +1309,7 @@ static bool tcp_shifted_skb(struct sock *sk, struct sk_buff *skb,
 	if (skb == tcp_highest_sack(sk))
 		tcp_advance_highest_sack(sk, skb);
 
+	tcp_skb_collapse_tstamp(prev, skb);
 	tcp_unlink_write_queue(skb, sk);
 	sk_wmem_free_skb(sk, skb);
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 889ed96..d21a78f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2443,8 +2443,8 @@ u32 __tcp_select_window(struct sock *sk)
 	return window;
 }
 
-static void tcp_skb_collapse_tstamp(struct sk_buff *skb,
-				    const struct sk_buff *next_skb)
+void tcp_skb_collapse_tstamp(struct sk_buff *skb,
+			     const struct sk_buff *next_skb)
 {
 	const struct skb_shared_info *next_shinfo = skb_shinfo(next_skb);
 
-- 
2.5.1

Re: [RFC PATCH v2 net-next 3/7] tcp: Merge tx_flags/tskey/txstamp_ack in tcp_shifted_skb

[Soheil Hassas Yeganeh]

On Mon, Apr 18, 2016 at 6:46 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> After receiving sacks, tcp_shifted_skb() will collapse
> skbs if possible.  tx_flags/tskey/txstamp_ack also has
> to be merged in this case.
>
> This patch resues the tcp_skb_collapse_tstamp() to handle
> them.
>
> BPF Output Before:
> ~~~~~
> <no-output-due-to-missing-tstamp-event>
>
> BPF Output After:
> ~~~~~
> <...>-2024  [007] d.s.    88.644374: : ee_data:14599
>
> Packetdrill Script:
> ~~~~~
> +0 `sysctl -q -w net.ipv4.tcp_min_tso_segs=10`
> +0 `sysctl -q -w net.ipv4.tcp_no_metrics_save=1`
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, ..., ...) = 0
> +0 listen(3, 1) = 0
>
> 0.100 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7>
> 0.100 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> 0.200 < . 1:1(0) ack 1 win 257
> 0.200 accept(3, ..., ...) = 4
> +0 setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0
>
> 0.200 write(4, ..., 1460) = 1460
> +0 setsockopt(4, SOL_SOCKET, 37, [2688], 4) = 0
> 0.200 write(4, ..., 13140) = 13140
> +0 setsockopt(4, SOL_SOCKET, 37, [2176], 4) = 0
>
> 0.200 > P. 1:1461(1460) ack 1
> 0.200 > . 1461:8761(7300) ack 1
> 0.200 > P. 8761:14601(5840) ack 1
>
> 0.300 < . 1:1(0) ack 1 win 257 <sack 1461:14601,nop,nop>
> 0.300 > P. 1:1461(1460) ack 1
> 0.400 < . 1:1(0) ack 14601 win 257
>
> 0.400 close(4) = 0
> 0.400 > F. 14601:14601(0) ack 1
> 0.500 < F. 1:1(0) ack 14602 win 257
> 0.500 > . 14602:14602(0) ack 2
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> ---
>  include/net/tcp.h     | 2 ++
>  net/ipv4/tcp_input.c  | 1 +
>  net/ipv4/tcp_output.c | 4 ++--
>  3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index fd40f8c..c0ef054 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -557,6 +557,8 @@ void tcp_send_ack(struct sock *sk);
>  void tcp_send_delayed_ack(struct sock *sk);
>  void tcp_send_loss_probe(struct sock *sk);
>  bool tcp_schedule_loss_probe(struct sock *sk);
> +void tcp_skb_collapse_tstamp(struct sk_buff *skb,
> +                            const struct sk_buff *next_skb);
>
>  /* tcp_input.c */
>  void tcp_resume_early_retransmit(struct sock *sk);
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 5e45a9c..75e8336 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -1309,6 +1309,7 @@ static bool tcp_shifted_skb(struct sock *sk, struct sk_buff *skb,
>         if (skb == tcp_highest_sack(sk))
>                 tcp_advance_highest_sack(sk, skb);
>
> +       tcp_skb_collapse_tstamp(prev, skb);
>         tcp_unlink_write_queue(skb, sk);
>         sk_wmem_free_skb(sk, skb);
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 889ed96..d21a78f 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2443,8 +2443,8 @@ u32 __tcp_select_window(struct sock *sk)
>         return window;
>  }
>
> -static void tcp_skb_collapse_tstamp(struct sk_buff *skb,
> -                                   const struct sk_buff *next_skb)
> +void tcp_skb_collapse_tstamp(struct sk_buff *skb,
> +                            const struct sk_buff *next_skb)
>  {
>         const struct skb_shared_info *next_shinfo = skb_shinfo(next_skb);
>

nice, thanks for the fix!

> --
> 2.5.1
>

[RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Martin KaFai Lau]

This patch allows the user process to use MSG_EOR during
tcp_sendmsg to tell the kernel that it is the last byte
of an application response message.

It is currently useful when the end-user has turned on any bit of the
SOF_TIMESTAMPING_TX_RECORD_MASK (either by setsockopt or cmsg).
The kernel will then mark the newly added tcb->eor_info bit so
that the shinfo->tskey will not be overwritten (i.e. lost) in
the later skb append/collapse operation.

With selective SOF_TIMESTAMPING_TX_ACK (by cmsg) and MSG_EOR (this
patch), the user application can specially tell which outgoing byte
it wants to track its ACK and ask the kernel not to lose this
tracking info in the later skb append/collapse action.

This patch handles the append case in tcp_sendmsg.  The later
patches will handle the collapse during retransmission and
skb slicing in tcp_fragment()/tso_fragment().

One of our use case is at the webserver.  The webserver tracks
the HTTP2 response latency by measuring when the webserver sends
the first byte to the socket till the TCP ACK of the last byte
is received.  In the cases where we don't have client side
measurement, measuring from the server side is the only option.
In the cases we have the client side measurement, the server side
data can also be used to justify/cross-check-with the client
side data.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 include/net/tcp.h |  5 ++++-
 net/ipv4/tcp.c    | 21 +++++++++++++++++----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index c0ef054..f3c5dcb 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -762,7 +762,10 @@ struct tcp_skb_cb {
 
 	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
 	__u8		txstamp_ack:1,	/* Record TX timestamp for ack? */
-			unused:7;
+			eor_info:1,	/* Any EOR marked info that prevents
+					 * skbs from merging.
+					 */
+			unused:6;
 	__u32		ack_seq;	/* Sequence number ACK'd	*/
 	union {
 		struct inet_skb_parm	h4;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4d73858..2918f42 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -428,15 +428,18 @@ void tcp_init_sock(struct sock *sk)
 }
 EXPORT_SYMBOL(tcp_init_sock);
 
-static void tcp_tx_timestamp(struct sock *sk, u16 tsflags, struct sk_buff *skb)
+static void tcp_tx_timestamp(struct sock *sk, u16 tsflags, struct sk_buff *skb,
+			     int flags)
 {
 	if (sk->sk_tsflags || tsflags) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 		struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
 
 		sock_tx_timestamp(sk, tsflags, &shinfo->tx_flags);
-		if (shinfo->tx_flags & SKBTX_ANY_TSTAMP)
+		if (shinfo->tx_flags & SKBTX_ANY_TSTAMP) {
 			shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
+			tcb->eor_info = !!(flags & MSG_EOR);
+		}
 		tcb->txstamp_ack = !!(shinfo->tx_flags & SKBTX_ACK_TSTAMP);
 	}
 }
@@ -874,6 +877,13 @@ static int tcp_send_mss(struct sock *sk, int *size_goal, int flags)
 	return mss_now;
 }
 
+static bool tcp_sendmsg_noappend(struct sock *sk, u16 tx_tsflags)
+{
+	return unlikely((tx_tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) &&
+			tcp_send_head(sk) &&
+			TCP_SKB_CB(tcp_write_queue_tail(sk))->eor_info);
+}
+
 static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 				size_t size, int flags)
 {
@@ -959,7 +969,7 @@ new_segment:
 		offset += copy;
 		size -= copy;
 		if (!size) {
-			tcp_tx_timestamp(sk, sk->sk_tsflags, skb);
+			tcp_tx_timestamp(sk, sk->sk_tsflags, skb, 0);
 			goto out;
 		}
 
@@ -1145,6 +1155,9 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 
 	sg = !!(sk->sk_route_caps & NETIF_F_SG);
 
+	if (tcp_sendmsg_noappend(sk, sockc.tsflags))
+		goto new_segment;
+
 	while (msg_data_left(msg)) {
 		int copy = 0;
 		int max = size_goal;
@@ -1249,7 +1262,7 @@ new_segment:
 
 		copied += copy;
 		if (!msg_data_left(msg)) {
-			tcp_tx_timestamp(sk, sockc.tsflags, skb);
+			tcp_tx_timestamp(sk, sockc.tsflags, skb, flags);
 			goto out;
 		}
 
-- 
2.5.1

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Eric Dumazet]

On Mon, 2016-04-18 at 15:46 -0700, Martin KaFai Lau wrote:
> This patch allows the user process to use MSG_EOR during
> tcp_sendmsg to tell the kernel that it is the last byte
> of an application response message.
> 
> It is currently useful when the end-user has turned on any bit of the
> SOF_TIMESTAMPING_TX_RECORD_MASK (either by setsockopt or cmsg).
> The kernel will then mark the newly added tcb->eor_info bit so
> that the shinfo->tskey will not be overwritten (i.e. lost) in
> the later skb append/collapse operation.
> 
> With selective SOF_TIMESTAMPING_TX_ACK (by cmsg) and MSG_EOR (this
> patch), the user application can specially tell which outgoing byte
> it wants to track its ACK and ask the kernel not to lose this
> tracking info in the later skb append/collapse action.
> 
> This patch handles the append case in tcp_sendmsg.  The later
> patches will handle the collapse during retransmission and
> skb slicing in tcp_fragment()/tso_fragment().
> 
> One of our use case is at the webserver.  The webserver tracks
> the HTTP2 response latency by measuring when the webserver sends
> the first byte to the socket till the TCP ACK of the last byte
> is received.  In the cases where we don't have client side
> measurement, measuring from the server side is the only option.
> In the cases we have the client side measurement, the server side
> data can also be used to justify/cross-check-with the client
> side data.
> 
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> ---

MSG_EOR should not depend on SKBTX_ANY_TSTAMP

Really, simply using send(fd, ..., len, MSG_EOR) should instruct TCP to
mark the cooked skb as a non candidate for future coalescing.

netperf could then get an option to set this MSG_EOR ;)

I believe Soheil was working on such simple alternative ?

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[kafai]

On Mon, Apr 18, 2016 at 04:18:13PM -0700, Eric Dumazet wrote:
> On Mon, 2016-04-18 at 15:46 -0700, Martin KaFai Lau wrote:
> > This patch allows the user process to use MSG_EOR during
> > tcp_sendmsg to tell the kernel that it is the last byte
> > of an application response message.
> >
> > It is currently useful when the end-user has turned on any bit of the
> > SOF_TIMESTAMPING_TX_RECORD_MASK (either by setsockopt or cmsg).
> > The kernel will then mark the newly added tcb->eor_info bit so
> > that the shinfo->tskey will not be overwritten (i.e. lost) in
> > the later skb append/collapse operation.
> >
> > With selective SOF_TIMESTAMPING_TX_ACK (by cmsg) and MSG_EOR (this
> > patch), the user application can specially tell which outgoing byte
> > it wants to track its ACK and ask the kernel not to lose this
> > tracking info in the later skb append/collapse action.
> >
> > This patch handles the append case in tcp_sendmsg.  The later
> > patches will handle the collapse during retransmission and
> > skb slicing in tcp_fragment()/tso_fragment().
> >
> > One of our use case is at the webserver.  The webserver tracks
> > the HTTP2 response latency by measuring when the webserver sends
> > the first byte to the socket till the TCP ACK of the last byte
> > is received.  In the cases where we don't have client side
> > measurement, measuring from the server side is the only option.
> > In the cases we have the client side measurement, the server side
> > data can also be used to justify/cross-check-with the client
> > side data.
> >
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Neal Cardwell <ncardwell@google.com>
> > Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
> > Cc: Willem de Bruijn <willemb@google.com>
> > Cc: Yuchung Cheng <ycheng@google.com>
> > ---
>
> MSG_EOR should not depend on SKBTX_ANY_TSTAMP
>
> Really, simply using send(fd, ..., len, MSG_EOR) should instruct TCP to
> mark the cooked skb as a non candidate for future coalescing.
It was one of my earlier local attempt.  There are cases that coalescing
will not lose the tskey, so I trashed it.

If we mark eor only based on MSG_EOR, we can still do checks on
prev_skb's tskey and next_skb's tskey before coalescing two skbs
or
you meant simply don't coalesce if the prev_skb has eor marked?

>
> netperf could then get an option to set this MSG_EOR ;)
Not sure how it is related.  Can you share how netperf can
benefit from MSG_EOR in TCP tests without any of the
SOF_TIMESTAMPING_TX_RECORD_MASK.

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Eric Dumazet]

On Mon, 2016-04-18 at 16:43 -0700, kafai@fb.com wrote:

> >
> > netperf could then get an option to set this MSG_EOR ;)
> Not sure how it is related.  Can you share how netperf can
> benefit from MSG_EOR in TCP tests without any of the
> SOF_TIMESTAMPING_TX_RECORD_MASK.

Simply setting MSG_EOR would be orthogonal to other timestamping stuff.

Maybe the application does not _want_ to be notified when skb is sent or
acknowledged, but would like some kind of "tcpdump awareness" or
something about burst control, who knows...

It should only be a request from user space to ask TCP to not aggregate
stuff on future sendpage()/sendmsg() on the skb carrying this new flag.

We already have other flags to ask for timestamping stuff, and they
could be used at the same time.

If the stack needs to be changed to properly fragment skb (or
aggregating them at retransmit), this is a separate concern.

Note that you do not need to automatically assert MSG_BOR (Begin of
Request) : MSG_EOR should really control the fact that last byte sent
marks the skb as being a non candidate for aggregation.

This would keep tcp_sendmsg() reasonnably fast.
Your tcp_sendmsg_noappend() is quite expensive :(

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Martin KaFai Lau]

On Mon, Apr 18, 2016 at 05:06:57PM -0700, Eric Dumazet wrote:
> It should only be a request from user space to ask TCP to not aggregate
> stuff on future sendpage()/sendmsg() on the skb carrying this new flag.
>
How about something like this.  Please advise if tcp_sendmsg_noappend can
be simpler.

diff --git a/include/net/tcp.h b/include/net/tcp.h
index c0ef054..ac31798 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -762,7 +762,8 @@ struct tcp_skb_cb {

 	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
 	__u8		txstamp_ack:1,	/* Record TX timestamp for ack? */
-			unused:7;
+			eor:1,		/* Is skb MSG_EOR marked */
+			unused:6;
 	__u32		ack_seq;	/* Sequence number ACK'd	*/
 	union {
 		struct inet_skb_parm	h4;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4d73858..12772be 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -874,6 +874,13 @@ static int tcp_send_mss(struct sock *sk, int *size_goal, int flags)
 	return mss_now;
 }

+static bool tcp_sendmsg_noappend(const struct sock *sk)
+{
+	const struct sk_buff *skb = tcp_write_queue_tail(sk);
+
+	return (!skb || TCP_SKB_CB(skb)->eor);
+}
+
 static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 				size_t size, int flags)
 {
@@ -903,6 +910,9 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 	if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
 		goto out_err;

+	if (tcp_sendmsg_noappend(sk))
+		goto new_segment;
+
 	while (size > 0) {
 		struct sk_buff *skb = tcp_write_queue_tail(sk);
 		int copy, i;
@@ -960,6 +970,7 @@ new_segment:
 		size -= copy;
 		if (!size) {
 			tcp_tx_timestamp(sk, sk->sk_tsflags, skb);
+			TCP_SKB_CB(skb)->eor = !!(flags & MSG_EOR);
 			goto out;
 		}

@@ -1145,6 +1156,9 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)

 	sg = !!(sk->sk_route_caps & NETIF_F_SG);

+	if (tcp_sendmsg_noappend(sk))
+		goto new_segment;
+
 	while (msg_data_left(msg)) {
 		int copy = 0;
 		int max = size_goal;
@@ -1250,6 +1264,7 @@ new_segment:
 		copied += copy;
 		if (!msg_data_left(msg)) {
 			tcp_tx_timestamp(sk, sockc.tsflags, skb);
+			TCP_SKB_CB(skb)->eor = !!(flags & MSG_EOR);
 			goto out;
 		}

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Eric Dumazet]

On Mon, 2016-04-18 at 19:27 -0700, Martin KaFai Lau wrote:
> On Mon, Apr 18, 2016 at 05:06:57PM -0700, Eric Dumazet wrote:
> > It should only be a request from user space to ask TCP to not aggregate
> > stuff on future sendpage()/sendmsg() on the skb carrying this new flag.
> >
> How about something like this.  Please advise if tcp_sendmsg_noappend can
> be simpler.
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index c0ef054..ac31798 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -762,7 +762,8 @@ struct tcp_skb_cb {
> 
>  	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
>  	__u8		txstamp_ack:1,	/* Record TX timestamp for ack? */
> -			unused:7;
> +			eor:1,		/* Is skb MSG_EOR marked */
> +			unused:6;
>  	__u32		ack_seq;	/* Sequence number ACK'd	*/
>  	union {
>  		struct inet_skb_parm	h4;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 4d73858..12772be 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -874,6 +874,13 @@ static int tcp_send_mss(struct sock *sk, int *size_goal, int flags)
>  	return mss_now;
>  }
> 
> +static bool tcp_sendmsg_noappend(const struct sock *sk)
> +{
> +	const struct sk_buff *skb = tcp_write_queue_tail(sk);
> +
> +	return (!skb || TCP_SKB_CB(skb)->eor);
> +}
> +
>  static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>  				size_t size, int flags)
>  {
> @@ -903,6 +910,9 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>  	if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
>  		goto out_err;
> 
> +	if (tcp_sendmsg_noappend(sk))
> +		goto new_segment;
> +
>  	while (size > 0) {
>  		struct sk_buff *skb = tcp_write_queue_tail(sk);
>  		int copy, i;
> @@ -960,6 +970,7 @@ new_segment:
>  		size -= copy;
>  		if (!size) {
>  			tcp_tx_timestamp(sk, sk->sk_tsflags, skb);
> +			TCP_SKB_CB(skb)->eor = !!(flags & MSG_EOR);
>  			goto out;
>  		}
> 
> @@ -1145,6 +1156,9 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> 
>  	sg = !!(sk->sk_route_caps & NETIF_F_SG);
> 
> +	if (tcp_sendmsg_noappend(sk))
> +		goto new_segment;
> +
>  	while (msg_data_left(msg)) {
>  		int copy = 0;
>  		int max = size_goal;
> @@ -1250,6 +1264,7 @@ new_segment:
>  		copied += copy;
>  		if (!msg_data_left(msg)) {
>  			tcp_tx_timestamp(sk, sockc.tsflags, skb);
> +			TCP_SKB_CB(skb)->eor = !!(flags & MSG_EOR);
>  			goto out;
>  		}

I believe it is slightly wrong (to do the goto new_segment if there is
no data to send)

I would instead use this fast path, doing the test _when_ we already
have an skb to test for.

diff --git a/include/net/tcp.h b/include/net/tcp.h
index fd40f8c64d5f..015851634195 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -760,7 +760,8 @@ struct tcp_skb_cb {
 
 	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
 	__u8		txstamp_ack:1,	/* Record TX timestamp for ack? */
-			unused:7;
+			eor:1,		/* Is skb MSG_EOR marked */
+			unused:6;
 	__u32		ack_seq;	/* Sequence number ACK'd	*/
 	union {
 		struct inet_skb_parm	h4;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4d73858991af..1944c19885ab 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -908,7 +908,9 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 		int copy, i;
 		bool can_coalesce;
 
-		if (!tcp_send_head(sk) || (copy = size_goal - skb->len) <= 0) {
+		if (!tcp_send_head(sk) ||
+		    (copy = size_goal - skb->len) <= 0 ||
+		    TCP_SKB_CB(skb)->eor) {
 new_segment:
 			if (!sk_stream_memory_free(sk))
 				goto wait_for_sndbuf;
@@ -960,6 +962,7 @@ new_segment:
 		size -= copy;
 		if (!size) {
 			tcp_tx_timestamp(sk, sk->sk_tsflags, skb);
+			TCP_SKB_CB(skb)->eor = !!(flags & MSG_EOR);
 			goto out;
 		}
 
@@ -1156,7 +1159,7 @@ int tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 			copy = max - skb->len;
 		}
 
-		if (copy <= 0) {
+		if (copy <= 0 || TCP_SKB_CB(skb)->eor) {
 new_segment:
 			/* Allocate new segment. If the interface is SG,
 			 * allocate skb fitting to single page.
@@ -1250,6 +1253,7 @@ new_segment:
 		copied += copy;
 		if (!msg_data_left(msg)) {
 			tcp_tx_timestamp(sk, sockc.tsflags, skb);
+			TCP_SKB_CB(skb)->eor = !!(flags & MSG_EOR);
 			goto out;
 		}
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 6451b83d81e9..acfbff81ef47 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1171,6 +1171,8 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
 	TCP_SKB_CB(skb)->tcp_flags = flags & ~(TCPHDR_FIN | TCPHDR_PSH);
 	TCP_SKB_CB(buff)->tcp_flags = flags;
 	TCP_SKB_CB(buff)->sacked = TCP_SKB_CB(skb)->sacked;
+	TCP_SKB_CB(buff)->eor = TCP_SKB_CB(skb)->eor;
+	TCP_SKB_CB(skb)->eor = 0;
 
 	if (!skb_shinfo(skb)->nr_frags && skb->ip_summed != CHECKSUM_PARTIAL) {
 		/* Copy and checksum data tail into the new buffer. */
@@ -1730,6 +1732,8 @@ static int tso_fragment(struct sock *sk, struct sk_buff *skb, unsigned int len,
 
 	/* This packet was never sent out yet, so no SACK bits. */
 	TCP_SKB_CB(buff)->sacked = 0;
+	TCP_SKB_CB(buff)->eor = TCP_SKB_CB(skb)->eor;
+	TCP_SKB_CB(skb)->eor = 0;
 
 	buff->ip_summed = skb->ip_summed = CHECKSUM_PARTIAL;
 	skb_split(skb, buff, len);
@@ -2471,6 +2475,7 @@ static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
 
 	/* Merge over control information. This moves PSH/FIN etc. over */
 	TCP_SKB_CB(skb)->tcp_flags |= TCP_SKB_CB(next_skb)->tcp_flags;
+	TCP_SKB_CB(skb)->eor = TCP_SKB_CB(next_skb)->eor;
 
 	/* All done, get rid of second SKB and account for it so
 	 * packet counting does not break.
@@ -2502,7 +2507,8 @@ static bool tcp_can_collapse(const struct sock *sk, const struct sk_buff *skb)
 	/* Some heurestics for collapsing over SACK'd could be invented */
 	if (TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_ACKED)
 		return false;
-
+	if (TCP_SKB_CB(skb)->eor)
+		return false;
 	return true;
 }
 

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Martin KaFai Lau]

On Mon, Apr 18, 2016 at 07:50:41PM -0700, Eric Dumazet wrote:
> I believe it is slightly wrong (to do the goto new_segment if there is
> no data to send)
Aha. Thanks for pointing it out.

>
> I would instead use this fast path, doing the test _when_ we already
> have an skb to test for.
The v1 was doing a check in the loop but the feedback was, instead
of doing this unlikely(test) repeatedly in the loop, do it before
entering the loop and do a goto new_segment if needed.

I agree that doing it in the loop is easier to follow/read
and checking TCP_SKB_CB(skb)->eor is cheaper than my v1.
I will respin with your suggestion.

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 6451b83d81e9..acfbff81ef47 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1171,6 +1171,8 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
>  	TCP_SKB_CB(skb)->tcp_flags = flags & ~(TCPHDR_FIN | TCPHDR_PSH);
>  	TCP_SKB_CB(buff)->tcp_flags = flags;
>  	TCP_SKB_CB(buff)->sacked = TCP_SKB_CB(skb)->sacked;
> +	TCP_SKB_CB(buff)->eor = TCP_SKB_CB(skb)->eor;
> +	TCP_SKB_CB(skb)->eor = 0;
>
>  	if (!skb_shinfo(skb)->nr_frags && skb->ip_summed != CHECKSUM_PARTIAL) {
>  		/* Copy and checksum data tail into the new buffer. */
> @@ -1730,6 +1732,8 @@ static int tso_fragment(struct sock *sk, struct sk_buff *skb, unsigned int len,
>
>  	/* This packet was never sent out yet, so no SACK bits. */
>  	TCP_SKB_CB(buff)->sacked = 0;
> +	TCP_SKB_CB(buff)->eor = TCP_SKB_CB(skb)->eor;
> +	TCP_SKB_CB(skb)->eor = 0;
>
>  	buff->ip_summed = skb->ip_summed = CHECKSUM_PARTIAL;
>  	skb_split(skb, buff, len);
> @@ -2471,6 +2475,7 @@ static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
>
>  	/* Merge over control information. This moves PSH/FIN etc. over */
>  	TCP_SKB_CB(skb)->tcp_flags |= TCP_SKB_CB(next_skb)->tcp_flags;
> +	TCP_SKB_CB(skb)->eor = TCP_SKB_CB(next_skb)->eor;
>
>  	/* All done, get rid of second SKB and account for it so
>  	 * packet counting does not break.
> @@ -2502,7 +2507,8 @@ static bool tcp_can_collapse(const struct sock *sk, const struct sk_buff *skb)
>  	/* Some heurestics for collapsing over SACK'd could be invented */
>  	if (TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_ACKED)
>  		return false;
> -
> +	if (TCP_SKB_CB(skb)->eor)
> +		return false;
>  	return true;
>  }
Thanks for this diff.  It confirms that I probably understand your last
suggestion correctly.  I also have similar diff for the sacks handling.

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Eric Dumazet]

On Mon, 2016-04-18 at 20:18 -0700, Martin KaFai Lau wrote:
> On Mon, Apr 18, 2016 at 07:50:41PM -0700, Eric Dumazet wrote:
> > I believe it is slightly wrong (to do the goto new_segment if there is
> > no data to send)
> Aha. Thanks for pointing it out.
> 
> >
> > I would instead use this fast path, doing the test _when_ we already
> > have an skb to test for.
> The v1 was doing a check in the loop but the feedback was, instead
> of doing this unlikely(test) repeatedly in the loop, do it before
> entering the loop and do a goto new_segment if needed.
> 
> I agree that doing it in the loop is easier to follow/read
> and checking TCP_SKB_CB(skb)->eor is cheaper than my v1.
> I will respin with your suggestion.

I do not remember the suggestion. It is better to do the test in the
loop, as you could have multiple threads doing a sendmsg() on the same
socket, and eventually sleeping in sk_stream_wait_memory()

A bit convoluted, but who knows what can be done by some applications ;)

RE: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[David Laight]

From: Eric Dumazet
> Sent: 19 April 2016 00:18
...
> MSG_EOR should not depend on SKBTX_ANY_TSTAMP
> 
> Really, simply using send(fd, ..., len, MSG_EOR) should instruct TCP to
> mark the cooked skb as a non candidate for future coalescing.

Isn't that very similar to the inverse of MSG_MORE?
Or a send with Nagle disabled?

	David

Re: [RFC PATCH v2 net-next 4/7] tcp: Make use of MSG_EOR flag in tcp_sendmsg

[Eric Dumazet]

On Tue, 2016-04-19 at 09:47 +0000, David Laight wrote:
> From: Eric Dumazet
> > Sent: 19 April 2016 00:18
> ...
> > MSG_EOR should not depend on SKBTX_ANY_TSTAMP
> > 
> > Really, simply using send(fd, ..., len, MSG_EOR) should instruct TCP to
> > mark the cooked skb as a non candidate for future coalescing.
> 
> Isn't that very similar to the inverse of MSG_MORE?

Yes. But not specifying MSG_MORE does not mean we want to force send
tiny packets. TCP is allowed to aggregate since it is a stream protocol,
since it reduces overhead.

> Or a send with Nagle disabled?

No.  tcp_sendmsg() still can coalesce/aggregate data to the last packet
in write queue, if it was not yet sent, regardless of Nagle.

If you enable or disable Nagle, following command will only send first
packets with 724 bytes, but following ones will be very big.

netperf -t TCP_STREAM -- -m 724

05:16:53.707778 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [S], seq 711876787, win 29200, options [mss 1460,sackOK,TS val 320048472 ecr 0,nop,wscale 7], length 0
05:16:53.707908 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [S.], seq 2592965917, ack 711876788, win 28960, options [mss 1460,sackOK,TS val 23660532 ecr 320048472,nop,wscale 7], length 0
05:16:53.708001 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [.], ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 0
05:16:53.708045 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 1:725, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 724
05:16:53.708053 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 725, win 238, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708083 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 725:2173, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 1448
05:16:53.708097 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 2173, win 261, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708094 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 2173:3621, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 1448
05:16:53.708107 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 3621, win 283, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708132 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 3621:6517, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 2896
05:16:53.708133 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 6517:7965, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 1448
05:16:53.708148 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 6517, win 329, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708152 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 7965, win 351, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708157 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 7965:10861, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 2896
05:16:53.708165 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 10861, win 396, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708168 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 10861:12309, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 1448
05:16:53.708177 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 12309, win 419, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708220 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [.], seq 12309:16653, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 4344
05:16:53.708240 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 16653, win 487, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708335 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 16653:28237, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 11584
05:16:53.708378 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 28237, win 668, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708453 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 28237:42717, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 14480
05:16:53.708496 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 42717, win 894, options [nop,nop,TS val 23660532 ecr 320048473], length 0
05:16:53.708537 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [.], seq 42717:49957, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 7240
05:16:53.708585 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 49957:57197, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 7240
05:16:53.708641 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 49957, win 1007, options [nop,nop,TS val 23660533 ecr 320048473], length 0
05:16:53.708651 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 57197, win 1120, options [nop,nop,TS val 23660533 ecr 320048473], length 0
05:16:53.708806 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 57197:83261, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660532], length 26064
05:16:53.708836 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 83261, win 1528, options [nop,nop,TS val 23660533 ecr 320048473], length 0
05:16:53.708944 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [.], seq 83261:97741, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660533], length 14480
05:16:53.708967 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 97741, win 1754, options [nop,nop,TS val 23660533 ecr 320048473], length 0
05:16:53.709005 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [.], seq 97741:104981, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660533], length 7240
05:16:53.709036 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 104981:109325, ack 1, win 229, options [nop,nop,TS val 320048473 ecr 23660533], length 4344
05:16:53.709058 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 104981, win 1867, options [nop,nop,TS val 23660533 ecr 320048473], length 0
05:16:53.709065 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 109325, win 1924, options [nop,nop,TS val 23660533 ecr 320048473], length 0
05:16:53.709251 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 109325:135389, ack 1, win 229, options [nop,nop,TS val 320048474 ecr 23660533], length 26064
05:16:53.709332 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [.], seq 135389:141181, ack 1, win 229, options [nop,nop,TS val 320048474 ecr 23660533], length 5792
05:16:53.709423 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 135389, win 1924, options [nop,nop,TS val 23660533 ecr 320048474], length 0
05:16:53.709434 IP 10.246.7.151.42681 > 10.246.7.133.37427: Flags [.], ack 141181, win 1924, options [nop,nop,TS val 23660533 ecr 320048474], length 0
05:16:53.709730 IP 10.246.7.133.37427 > 10.246.7.151.42681: Flags [P.], seq 141181:191861, ack 1, win 229, options [nop,nop,TS val 320048474 ecr 23660533], length 50680

Using MSG_EOR on the send(fd, buffer, 724, MSG_EOR) would force TCP to
send tiny datagrams and not big GSO/TSO packets.

[RFC PATCH v2 net-next 5/7] tcp: Make use of MSG_EOR in tcp_sendpage

[Martin KaFai Lau]

It reuses the tcp_sendmsg_noappend() to decide if a new_segment
is needed before entering the loop.  More checks could be added
later for the tcp_sendpage case to decide if a new_segment is
needed immediately.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 2918f42..6bb33b8 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -913,6 +913,9 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
 	if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
 		goto out_err;
 
+	if (tcp_sendmsg_noappend(sk, sk->sk_tsflags))
+		goto new_segment;
+
 	while (size > 0) {
 		struct sk_buff *skb = tcp_write_queue_tail(sk);
 		int copy, i;
@@ -969,7 +972,7 @@ new_segment:
 		offset += copy;
 		size -= copy;
 		if (!size) {
-			tcp_tx_timestamp(sk, sk->sk_tsflags, skb, 0);
+			tcp_tx_timestamp(sk, sk->sk_tsflags, skb, flags);
 			goto out;
 		}
 
-- 
2.5.1

[RFC PATCH v2 net-next 6/7] tcp: Carry eor_info in tcp_fragment_tstamp() and tcp_skb_collapse_tstamp()

[Martin KaFai Lau]

Like txstamp_ack bit, if needed, the eor_info bit should also be carried
to the new skb2 when splitting a skb
or
to the prev skb from the next_skb when collapsing skbs.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_output.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index d21a78f..e71336c 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1125,6 +1125,8 @@ static void tcp_fragment_tstamp(struct sk_buff *skb, struct sk_buff *skb2)
 		swap(shinfo->tskey, shinfo2->tskey);
 		TCP_SKB_CB(skb2)->txstamp_ack = TCP_SKB_CB(skb)->txstamp_ack;
 		TCP_SKB_CB(skb)->txstamp_ack = 0;
+		TCP_SKB_CB(skb2)->eor_info = TCP_SKB_CB(skb)->eor_info;
+		TCP_SKB_CB(skb)->eor_info = 0;
 	}
 }
 
@@ -2456,6 +2458,8 @@ void tcp_skb_collapse_tstamp(struct sk_buff *skb,
 		shinfo->tskey = next_shinfo->tskey;
 		TCP_SKB_CB(skb)->txstamp_ack =
 			!!(shinfo->tx_flags & SKBTX_ACK_TSTAMP);
+		if (TCP_SKB_CB(next_skb)->eor_info)
+			TCP_SKB_CB(skb)->eor_info = 1;
 	}
 }
 
-- 
2.5.1

[RFC PATCH v2 net-next 7/7] tcp: Avoid losing eor_info when collapsing skbs

[Martin KaFai Lau]

When collapsing skbs during tcp_collapse_retrans and tcp_shift_skb_data,
this patch is to avoid collapsing to a prev skb that has eor_info mark
and the next_skb also has the tskey set (i.e. the prev skb will lose
a eor marked tskey info).

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 include/net/tcp.h     | 6 ++++++
 net/ipv4/tcp_input.c  | 3 +++
 net/ipv4/tcp_output.c | 7 +++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index f3c5dcb..56a287a 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -812,6 +812,12 @@ static inline int tcp_skb_mss(const struct sk_buff *skb)
 	return TCP_SKB_CB(skb)->tcp_gso_size;
 }
 
+static inline bool  tcp_skb_can_collapse_eor(const struct sk_buff *skb,
+					     const struct sk_buff *next_skb)
+{
+	return !(TCP_SKB_CB(skb)->eor_info && skb_shinfo(next_skb)->tskey);
+}
+
 /* Events passed to congestion control interface */
 enum tcp_ca_event {
 	CA_EVENT_TX_START,	/* first transmit when no packets in flight */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 75e8336..e60922d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1368,6 +1368,9 @@ static struct sk_buff *tcp_shift_skb_data(struct sock *sk, struct sk_buff *skb,
 	if ((TCP_SKB_CB(prev)->sacked & TCPCB_TAGBITS) != TCPCB_SACKED_ACKED)
 		goto fallback;
 
+	if (!tcp_skb_can_collapse_eor(prev, skb))
+		goto fallback;
+
 	in_sack = !after(start_seq, TCP_SKB_CB(skb)->seq) &&
 		  !before(end_seq, TCP_SKB_CB(skb)->end_seq);
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e71336c..1ae21f1 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2512,7 +2512,8 @@ static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
 }
 
 /* Check if coalescing SKBs is legal. */
-static bool tcp_can_collapse(const struct sock *sk, const struct sk_buff *skb)
+static bool tcp_can_collapse(const struct sock *sk, const struct sk_buff *to,
+			     const struct sk_buff *skb)
 {
 	if (tcp_skb_pcount(skb) > 1)
 		return false;
@@ -2526,6 +2527,8 @@ static bool tcp_can_collapse(const struct sock *sk, const struct sk_buff *skb)
 	/* Some heurestics for collapsing over SACK'd could be invented */
 	if (TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_ACKED)
 		return false;
+	if (!tcp_skb_can_collapse_eor(to, skb))
+		return false;
 
 	return true;
 }
@@ -2546,7 +2549,7 @@ static void tcp_retrans_try_collapse(struct sock *sk, struct sk_buff *to,
 		return;
 
 	tcp_for_write_queue_from_safe(skb, tmp, sk) {
-		if (!tcp_can_collapse(sk, skb))
+		if (!tcp_can_collapse(sk, to, skb))
 			break;
 
 		space -= skb->len;
-- 
2.5.1