[PATCH net-next 0/2] tcp: update TCPCB_EVER_RETRANS after trace_tcp_retransmit_skb()

[PATCH net-next 0/2] tcp: update TCPCB_EVER_RETRANS after trace_tcp_retransmit_skb()

[Philo Lu]

Move TCPCB_EVER_RETRANS updating after the trace_tcp_retransmit_skb()
in __tcp_retransmit_skb(), and then we are aware of whether the skb has
ever been retransmitted in this tracepoint. This can be used, e.g., to get
retransmission efficiency by counting skbs w/ and w/o TCPCB_EVER_RETRANS
(through bpf tracing programs).

For this purpose, TCPCB_EVER_RETRANS is also needed to be exposed to bpf.
Previously, the flags are defined as macros in struct tcp_skb_cb. I moved them
out into a new enum, and then they can be accessed with vmlinux.h.

We have discussed to achieve this with BPF_SOCK_OPS in [0], and using
tracepoint is thought to be a better solution.

[0]
https://lore.kernel.org/all/20240417124622.35333-1-lulie@linux.alibaba.com/

Philo Lu (2):
  tcp: move tcp_skb_cb->sacked flags to enum
  tcp: update sacked after tracepoint in __tcp_retransmit_skb

 include/net/tcp.h     | 22 +++++++++++++---------
 net/ipv4/tcp_output.c | 11 ++++++-----
 2 files changed, 19 insertions(+), 14 deletions(-)

--
2.32.0.3.g01195cf9f

[PATCH net-next 1/2] tcp: move tcp_skb_cb->sacked flags to enum

[Philo Lu]

Move the flag definitions for tcp_skb_cb->sacked into a new enum named
tcp_skb_cb_sacked_flags, then we can get access to them in bpf via
vmlinux.h, e.g., in tracepoints.

This patch does not change any existing functionality.

Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
---
 include/net/tcp.h | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index b935e1ae4caf8..ffc9371fe9dea 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -928,6 +928,19 @@ static inline u32 tcp_rsk_tsval(const struct tcp_request_sock *treq)
 
 #define TCPHDR_SYN_ECN	(TCPHDR_SYN | TCPHDR_ECE | TCPHDR_CWR)
 
+/* State flags for sacked in struct tcp_skb_cb */
+enum tcp_skb_cb_sacked_flags {
+	TCPCB_SACKED_ACKED	= (1 << 0),	/* SKB ACK'd by a SACK block	*/
+	TCPCB_SACKED_RETRANS	= (1 << 1),	/* SKB retransmitted		*/
+	TCPCB_LOST		= (1 << 2),	/* SKB is lost			*/
+	TCPCB_TAGBITS		= (TCPCB_SACKED_ACKED | TCPCB_SACKED_RETRANS |
+				   TCPCB_LOST),	/* All tag bits			*/
+	TCPCB_REPAIRED		= (1 << 4),	/* SKB repaired (no skb_mstamp_ns)	*/
+	TCPCB_EVER_RETRANS	= (1 << 7),	/* Ever retransmitted frame	*/
+	TCPCB_RETRANS		= (TCPCB_SACKED_RETRANS | TCPCB_EVER_RETRANS |
+				   TCPCB_REPAIRED),
+};
+
 /* This is what the send packet queuing engine uses to pass
  * TCP per-packet control information to the transmission code.
  * We also store the host-order sequence numbers in here too.
@@ -950,15 +963,6 @@ struct tcp_skb_cb {
 	__u8		tcp_flags;	/* TCP header flags. (tcp[13])	*/
 
 	__u8		sacked;		/* State flags for SACK.	*/
-#define TCPCB_SACKED_ACKED	0x01	/* SKB ACK'd by a SACK block	*/
-#define TCPCB_SACKED_RETRANS	0x02	/* SKB retransmitted		*/
-#define TCPCB_LOST		0x04	/* SKB is lost			*/
-#define TCPCB_TAGBITS		0x07	/* All tag bits			*/
-#define TCPCB_REPAIRED		0x10	/* SKB repaired (no skb_mstamp_ns)	*/
-#define TCPCB_EVER_RETRANS	0x80	/* Ever retransmitted frame	*/
-#define TCPCB_RETRANS		(TCPCB_SACKED_RETRANS|TCPCB_EVER_RETRANS| \
-				TCPCB_REPAIRED)
-
 	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
 	__u8		txstamp_ack:1,	/* Record TX timestamp for ack? */
 			eor:1,		/* Is skb MSG_EOR marked? */
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net-next 1/2] tcp: move tcp_skb_cb->sacked flags to enum

[Eric Dumazet]

On Sun, Apr 21, 2024 at 6:20 AM Philo Lu <lulie@linux.alibaba.com> wrote:
>
> Move the flag definitions for tcp_skb_cb->sacked into a new enum named
> tcp_skb_cb_sacked_flags, then we can get access to them in bpf via
> vmlinux.h, e.g., in tracepoints.
>
> This patch does not change any existing functionality.
>
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>

Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH net-next 2/2] tcp: update sacked after tracepoint in __tcp_retransmit_skb

[Philo Lu]

Marking TCP_SKB_CB(skb)->sacked with TCPCB_EVER_RETRANS after the
traceopint (trace_tcp_retransmit_skb), then we can get the
retransmission efficiency by counting skbs w/ and w/o TCPCB_EVER_RETRANS
mark in this tracepoint.

We have discussed to achieve this with BPF_SOCK_OPS in [0], and using
tracepoint is thought to be a better solution.

[0]
https://lore.kernel.org/all/20240417124622.35333-1-lulie@linux.alibaba.com/

Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
---
 net/ipv4/tcp_output.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 61119d42b0fd2..e19e74e005c1b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3390,11 +3390,6 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
 		err = tcp_transmit_skb(sk, skb, 1, GFP_ATOMIC);
 	}
 
-	/* To avoid taking spuriously low RTT samples based on a timestamp
-	 * for a transmit that never happened, always mark EVER_RETRANS
-	 */
-	TCP_SKB_CB(skb)->sacked |= TCPCB_EVER_RETRANS;
-
 	if (BPF_SOCK_OPS_TEST_FLAG(tp, BPF_SOCK_OPS_RETRANS_CB_FLAG))
 		tcp_call_bpf_3arg(sk, BPF_SOCK_OPS_RETRANS_CB,
 				  TCP_SKB_CB(skb)->seq, segs, err);
@@ -3404,6 +3399,12 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
 	} else if (err != -EBUSY) {
 		NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPRETRANSFAIL, segs);
 	}
+
+	/* To avoid taking spuriously low RTT samples based on a timestamp
+	 * for a transmit that never happened, always mark EVER_RETRANS
+	 */
+	TCP_SKB_CB(skb)->sacked |= TCPCB_EVER_RETRANS;
+
 	return err;
 }
 
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net-next 2/2] tcp: update sacked after tracepoint in __tcp_retransmit_skb

[Eric Dumazet]

On Sun, Apr 21, 2024 at 6:20 AM Philo Lu <lulie@linux.alibaba.com> wrote:
>
> Marking TCP_SKB_CB(skb)->sacked with TCPCB_EVER_RETRANS after the
> traceopint (trace_tcp_retransmit_skb), then we can get the
> retransmission efficiency by counting skbs w/ and w/o TCPCB_EVER_RETRANS
> mark in this tracepoint.
>
> We have discussed to achieve this with BPF_SOCK_OPS in [0], and using
> tracepoint is thought to be a better solution.
>
> [0]
> https://lore.kernel.org/all/20240417124622.35333-1-lulie@linux.alibaba.com/
>
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 0/2] tcp: update TCPCB_EVER_RETRANS after trace_tcp_retransmit_skb()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Sun, 21 Apr 2024 12:20:07 +0800 you wrote:
> Move TCPCB_EVER_RETRANS updating after the trace_tcp_retransmit_skb()
> in __tcp_retransmit_skb(), and then we are aware of whether the skb has
> ever been retransmitted in this tracepoint. This can be used, e.g., to get
> retransmission efficiency by counting skbs w/ and w/o TCPCB_EVER_RETRANS
> (through bpf tracing programs).
> 
> For this purpose, TCPCB_EVER_RETRANS is also needed to be exposed to bpf.
> Previously, the flags are defined as macros in struct tcp_skb_cb. I moved them
> out into a new enum, and then they can be accessed with vmlinux.h.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] tcp: move tcp_skb_cb->sacked flags to enum
    https://git.kernel.org/netdev/net-next/c/14b5fb2145ca
  - [net-next,2/2] tcp: update sacked after tracepoint in __tcp_retransmit_skb
    https://git.kernel.org/netdev/net-next/c/2bf90a57f0e6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html