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

[Philo Lu <lulie@linux.alibaba.com>]

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 <lulie@linux.alibaba.com> 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 <lulie@linux.alibaba.com>
>>
>> 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.