[bpf-next] bpf: add sacked flag in BPF_SOCK_OPS_RETRANS_CB

Commit Message

Philo Lu April 17, 2024, 12:46 p.m. UTC

Add TCP_SKB_CB(skb)->sacked as the 4th arg of sockops passed to bpf
program. Then we can get the retransmission efficiency by counting skbs
w/ and w/o TCPCB_EVER_RETRANS mark. And for this purpose, sacked
updating is moved after the BPF_SOCK_OPS_RETRANS_CB hook.

Signed-off-by: Philo Lu <[e-mail]>
---
 include/net/tcp.h              | 14 ++++++++++++++
 include/uapi/linux/bpf.h       |  2 ++
 net/ipv4/tcp_output.c          |  9 +++++----
 tools/include/uapi/linux/bpf.h |  2 ++
 4 files changed, 23 insertions(+), 4 deletions(-)

Comments

Eric Dumazet April 17, 2024, 1:11 p.m. UTC | #1

On Wed, Apr 17, 2024 at 2:46 PM Philo Lu <[e-mail]> wrote:
>
> Add TCP_SKB_CB(skb)->sacked as the 4th arg of sockops passed to bpf
> program. Then we can get the retransmission efficiency by counting skbs
> w/ and w/o TCPCB_EVER_RETRANS mark. And for this purpose, sacked
> updating is moved after the BPF_SOCK_OPS_RETRANS_CB hook.
>
> Signed-off-by: Philo Lu <[e-mail]>

This might be a naive question, but how the bpf program know what is the meaning
of each bit ?

Are they exposed already, and how future changes in TCP stack could
break old bpf programs ?

#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)

Martin KaFai Lau April 17, 2024, 10:48 p.m. UTC | #2

On 4/17/24 6:11 AM, Eric Dumazet wrote:
> On Wed, Apr 17, 2024 at 2:46 PM Philo Lu <[e-mail]> wrote:
>>
>> Add TCP_SKB_CB(skb)->sacked as the 4th arg of sockops passed to bpf
>> program. Then we can get the retransmission efficiency by counting skbs
>> w/ and w/o TCPCB_EVER_RETRANS mark. And for this purpose, sacked
>> updating is moved after the BPF_SOCK_OPS_RETRANS_CB hook.
>>
>> Signed-off-by: Philo Lu <[e-mail]>
> 
> This might be a naive question, but how the bpf program know what is the meaning
> of each bit ?
> 
> Are they exposed already, and how future changes in TCP stack could
> break old bpf programs ?
> 
> #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)

I think it is the best to use the trace_tcp_retransmit_skb() tracepoint instead.

iiuc the use case, moving the "TCP_SKB_CB(skb)->sacked |= TCPCB_EVER_RETRANS;" 
after the tracepoint should have similar effect.

If the TCPCB_* is moved to a enum, it will be included in the "vmlinux.h" that 
the bpf prog can use and no need to expose them in uapi.

Philo Lu April 18, 2024, 3:11 a.m. UTC | #3

On 2024/4/18 06:48, Martin KaFai Lau wrote:
> On 4/17/24 6:11 AM, Eric Dumazet wrote:
>> On Wed, Apr 17, 2024 at 2:46 PM Philo Lu <[e-mail]> wrote:
>>>
>>> Add TCP_SKB_CB(skb)->sacked as the 4th arg of sockops passed to bpf
>>> program. Then we can get the retransmission efficiency by counting skbs
>>> w/ and w/o TCPCB_EVER_RETRANS mark. And for this purpose, sacked
>>> updating is moved after the BPF_SOCK_OPS_RETRANS_CB hook.
>>>
>>> Signed-off-by: Philo Lu <[e-mail]>
>>
>> This might be a naive question, but how the bpf program know what is 
>> the meaning
>> of each bit ?
>>
>> Are they exposed already, and how future changes in TCP stack could
>> break old bpf programs ?
>>
>> #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)
> 
> I think it is the best to use the trace_tcp_retransmit_skb() tracepoint 
> instead.
> 
> iiuc the use case, moving the "TCP_SKB_CB(skb)->sacked |= 
> TCPCB_EVER_RETRANS;" after the tracepoint should have similar effect.

Good idea. This does also achieve this goal. So it would be like:
```
-TCP_SKB_CB(skb)->sacked |= TCPCB_EVER_RETRANS;

  if (likely(!err)) {
  	trace_tcp_retransmit_skb(sk, skb);
  } else if (err != -EBUSY) {
  	NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPRETRANSFAIL, segs);
  }

+TCP_SKB_CB(skb)->sacked |= TCPCB_EVER_RETRANS;
  return err;
```

> 
> If the TCPCB_* is moved to a enum, it will be included in the 
> "vmlinux.h" that the bpf prog can use and no need to expose them in uapi.

This is okay for me. Though I'm not sure if moving to enum brings any 
unexpected side effect?

BTW, need we concern about those that use trace_tcp_retransmit_skb to 
check TCPCB_EVER_RETRANS before?

Thanks.

Patch

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 6ae35199d3b3..7defe67183c9 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2660,6 +2660,14 @@  static inline int tcp_call_bpf_3arg(struct sock *sk, int op, u32 arg1, u32 arg2,
 	return tcp_call_bpf(sk, op, 3, args);
 }
 
+static inline int tcp_call_bpf_4arg(struct sock *sk, int op, u32 arg1, u32 arg2,
+				    u32 arg3, u32 arg4)
+{
+	u32 args[4] = {arg1, arg2, arg3, arg4};
+
+	return tcp_call_bpf(sk, op, 4, args);
+}
+
 #else
 static inline int tcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args)
 {
@@ -2677,6 +2685,12 @@  static inline int tcp_call_bpf_3arg(struct sock *sk, int op, u32 arg1, u32 arg2,
 	return -EPERM;
 }
 
+static inline int tcp_call_bpf_4arg(struct sock *sk, int op, u32 arg1, u32 arg2,
+				    u32 arg3, u32 arg4)
+{
+	return -EPERM;
+}
+
 #endif
 
 static inline u32 tcp_timeout_init(struct sock *sk)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index cee0a7915c08..df6bb9a62e0b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6938,6 +6938,8 @@  enum {
 					 * Arg2: # segments
 					 * Arg3: return value of
 					 *       tcp_transmit_skb (0 => success)
+					 * Arg4: TCP_SKB_CB(skb)->sacked before
+					 *       TCPCB_EVER_RETRANS marking
 					 */
 	BPF_SOCK_OPS_STATE_CB,		/* Called when TCP changes state.
 					 * Arg1: old_state
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e3167ad96567..370e6cee6794 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3387,15 +3387,16 @@  int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
 		err = tcp_transmit_skb(sk, skb, 1, GFP_ATOMIC);
 	}
 
+	if (BPF_SOCK_OPS_TEST_FLAG(tp, BPF_SOCK_OPS_RETRANS_CB_FLAG))
+		tcp_call_bpf_4arg(sk, BPF_SOCK_OPS_RETRANS_CB,
+				  TCP_SKB_CB(skb)->seq, segs, err,
+				  TCP_SKB_CB(skb)->sacked);
+
 	/* 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);
-
 	if (likely(!err)) {
 		trace_tcp_retransmit_skb(sk, skb);
 	} else if (err != -EBUSY) {
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index cee0a7915c08..df6bb9a62e0b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6938,6 +6938,8 @@  enum {
 					 * Arg2: # segments
 					 * Arg3: return value of
 					 *       tcp_transmit_skb (0 => success)
+					 * Arg4: TCP_SKB_CB(skb)->sacked before
+					 *       TCPCB_EVER_RETRANS marking
 					 */
 	BPF_SOCK_OPS_STATE_CB,		/* Called when TCP changes state.
 					 * Arg1: old_state