On Tue, Feb 4, 2025 at 9:16 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 1/28/25 12:46 AM, Jason Xing wrote:
> > Introduce the callback to correlate tcp_sendmsg timestamp with other
> > points, like SND/SW/ACK. We can let bpf trace the beginning of
> > tcp_sendmsg_locked() and fetch the socket addr, so that in
>
> Instead of "fetch the socket addr...", should be "store the sendmsg timestamp at
> the bpf_sk_storage ...".
I will revise it. Thanks.
>
> > tcp_tx_timestamp() we can correlate the tskey with the socket addr.
>
>
> > It is accurate since they are under the protect of socket lock.
> > More details can be found in the selftest.
>
> The selftest uses the bpf_sk_storage to store the sendmsg timestamp at
> fentry/tcp_sendmsg_locked and retrieves it back at tcp_tx_timestamp (i.e.
> BPF_SOCK_OPS_TS_SND_CB added in this patch).
>
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@xxxxxxxxx>
> > ---
> > include/uapi/linux/bpf.h | 7 +++++++
> > net/ipv4/tcp.c | 1 +
> > tools/include/uapi/linux/bpf.h | 7 +++++++
> > 3 files changed, 15 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 800122a8abe5..accb3b314fff 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -7052,6 +7052,13 @@ enum {
> > * when SK_BPF_CB_TX_TIMESTAMPING
> > * feature is on.
> > */
> > + BPF_SOCK_OPS_TS_SND_CB, /* Called when every sendmsg syscall
> > + * is triggered. For TCP, it stays
> > + * in the last send process to
> > + * correlate with tcp_sendmsg timestamp
> > + * with other timestamping callbacks,
> > + * like SND/SW/ACK.
>
> Do you have a chance to look at how this will work at UDP?
Sure, I feel like it could not be useful for UDP. Well, things get
strange because I did write a long paragraph about this thing which
apparently disappeared...
I manage to find what I wrote:
For UDP type, BPF_SOCK_OPS_TS_SND_CB may be not suitable because
there are two sending process, 1) lockless path, 2) lock path, which
should be handled carefully later. For the former, even though it's
unlikely multiple threads access the socket to call sendmsg at the
same time, I think we'd better not correlate it like what we do to the
TCP case because of the lack of sock lock protection. Considering SND_CB is
uapi flag, I think we don't need to forcely add the 'TCP_' prefix in
case we need to use it someday.
And one more thing is I'd like to use the v5[1] method in the next round
to introduce a new tskey_bpf which is good for UDP type. The new field
will not conflict with the tskey in shared info which is generated
by sk->sk_tskey in __ip_append_data(). It hardly works if both features
(so_timestamping and its bpf extension) exists at the same time. Users
could get confused because sometimes they fetch the tskey from skb,
sometimes they don't, especially when we have cmsg feature to turn it on/
off per sendmsg. A standalone tskey for bpf extension will be needed.
With this tskey_bpf, we can easily correlate the timestamp in sendmsg
syscall with other tx points(SND/SW/ACK...).
[1]: https://lore.kernel.org/all/20250112113748.73504-14-kerneljasonxing@xxxxxxxxx/
If possible, we can leave this question until the UDP support series
shows up. I will figure out a better solution :)
In conclusion, it probably won't be used by the UDP type. It's uAPI
flag so I consider the compatibility reason.
