Re: [PATCH bpf-next] bpf: add sock_ops callbacks for data send/recv/acked events

[Martin KaFai Lau <martin.lau@linux.dev>]

On 11/29/23 2:05 AM, Philo Lu wrote:
> 
> On 2023/11/29 08:33, Martin KaFai Lau wrote:
>> On 11/23/23 4:37 AM, Philo Lu wrote:
>>> Sorry, I forgot to cc the maintainers.
>>>
>>> On 2023/11/23 11:07, Philo Lu wrote:
>>>> Add 3 sock_ops operators, namely BPF_SOCK_OPS_DATA_SEND_CB,
>>>> BPF_SOCK_OPS_DATA_RECV_CB, and BPF_SOCK_OPS_DATA_ACKED_CB. A flag
>>>> BPF_SOCK_OPS_DATA_EVENT_CB_FLAG is provided to minimize the performance
>>>> impact. The flag must be explicitly set to enable these callbacks.
>>>>
>>>> If the flag is enabled, bpf sock_ops program will be called every time a
>>>> tcp data packet is sent, received, and acked.
>>>> BPF_SOCK_OPS_DATA_SEND_CB: call bpf after a data packet is sent.
>>>> BPF_SOCK_OPS_DATA_RECV_CB: call bpf after a data packet is receviced.
>>>> BPF_SOCK_OPS_DATA_ACKED_CB: call bpf after a valid ack packet is
>>>> processed (some sent data are ackknowledged).
>>>>
>>>> We use these callbacks for fine-grained tcp monitoring, which collects
>>>> and analyses every tcp request/response event information. The whole
>>>> system has been described in SIGMOD'18 (see
>>>> https://dl.acm.org/doi/pdf/10.1145/3183713.3190659 for details). To
>>>> achieve this with bpf, we require hooks for data events that call
>>>> sock_ops bpf (1) when any data packet is sent/received/acked, and (2)
>>>> after critical tcp state variables have been updated (e.g., snd_una,
>>>> snd_nxt, rcv_nxt). However, existing sock_ops operators cannot meet our
>>>> requirements.
>>>>
>>>> Besides, these hooks also help to debug tcp when data send/recv/acked.
>>
>> This all sounds like a tracing use case. Why tracepoint is not used instead?
> 
> Yes, our use case is pure tracing. We add hooks to sockops because we also use
> other ops like BPF_SOCK_OPS_STATE_CB. Thus, sockops seems a natural solution
> for us.

There is also an existing trace_inet_sock_set_state() tracepoint for tracking 
the state change. There are other existing tracepoints in 
include/trace/events/tcp.h for tcp perf monitoring/analysis purpose (e.g. 
trace_tcp_retransmit_skb). All it needs is read-only access to sk and the 
purpose is for tcp perf monitoring/analysis. If a hook is needed here 
(cgroup-bpf or tracepoint), I would think it is better to supplement the 
existing tcp tracepoints which were also added to do tcp monitoring.

I suspect the fexit bpf prog may also work because the fexit bpf prog is called 
after the traced kernel function is called. However, the kernel functions may 
get inlined and the tracepoint will still be needed. May be the netdev 
maintainer can chime in here regarding the tracepoint additions.

> 
> We can also use tracepoint (with sockops) instead. So we think which to use
> depends on your opinions. Many thanks.
> 
>