diff mbox series

[bpf-next,v12,10/12] bpf: add BPF_SOCK_OPS_TS_SND_CB callback

Message ID 20250218050125.73676-11-kerneljasonxing@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series net-timestamp: bpf extension to equip applications transparently | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 193 this patch: 193
netdev/build_tools success Errors and warnings before: 27 (+1) this patch: 27 (+1)
netdev/cc_maintainers warning 2 maintainers not CCed: horms@kernel.org ncardwell@google.com
netdev/build_clang success Errors and warnings before: 9362 this patch: 9362
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7019 this patch: 7019
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 32 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Jason Xing Feb. 18, 2025, 5:01 a.m. UTC 
This patch introduces a new callback in tcp_tx_timestamp() to correlate
tcp_sendmsg timestamp with timestamps from other tx timestamping
callbacks (e.g., SND/SW/ACK).

Without this patch, BPF program wouldn't know which timestamps belong
to which flow because of no socket lock protection. This new callback
is inserted in tcp_tx_timestamp() to address this issue because
tcp_tx_timestamp() still owns the same socket lock with
tcp_sendmsg_locked() in the meanwhile tcp_tx_timestamp() initializes
the timestamping related fields for the skb, especially tskey. The
tskey is the bridge to do the correlation.

For TCP, BPF program hooks the beginning of tcp_sendmsg_locked() and
then stores the sendmsg timestamp at the bpf_sk_storage, correlating
this timestamp with its tskey that are later used in other sending
timestamping callbacks.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 include/uapi/linux/bpf.h       | 5 +++++
 net/ipv4/tcp.c                 | 4 ++++
 tools/include/uapi/linux/bpf.h | 5 +++++
 3 files changed, 14 insertions(+)

Comments

Willem de Bruijn Feb. 18, 2025, 2:24 p.m. UTC | #1 
Jason Xing wrote:
> This patch introduces a new callback in tcp_tx_timestamp() to correlate
> tcp_sendmsg timestamp with timestamps from other tx timestamping
> callbacks (e.g., SND/SW/ACK).
> 
> Without this patch, BPF program wouldn't know which timestamps belong
> to which flow because of no socket lock protection. This new callback
> is inserted in tcp_tx_timestamp() to address this issue because
> tcp_tx_timestamp() still owns the same socket lock with
> tcp_sendmsg_locked() in the meanwhile tcp_tx_timestamp() initializes
> the timestamping related fields for the skb, especially tskey. The
> tskey is the bridge to do the correlation.
> 
> For TCP, BPF program hooks the beginning of tcp_sendmsg_locked() and
> then stores the sendmsg timestamp at the bpf_sk_storage, correlating
> this timestamp with its tskey that are later used in other sending
> timestamping callbacks.
> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>
Willem de Bruijn Feb. 20, 2025, 2:55 a.m. UTC | #2 
Jason Xing wrote:
> This patch introduces a new callback in tcp_tx_timestamp() to correlate
> tcp_sendmsg timestamp with timestamps from other tx timestamping
> callbacks (e.g., SND/SW/ACK).
> 
> Without this patch, BPF program wouldn't know which timestamps belong
> to which flow because of no socket lock protection. This new callback
> is inserted in tcp_tx_timestamp() to address this issue because
> tcp_tx_timestamp() still owns the same socket lock with
> tcp_sendmsg_locked() in the meanwhile tcp_tx_timestamp() initializes
> the timestamping related fields for the skb, especially tskey. The
> tskey is the bridge to do the correlation.
> 
> For TCP, BPF program hooks the beginning of tcp_sendmsg_locked() and
> then stores the sendmsg timestamp at the bpf_sk_storage, correlating
> this timestamp with its tskey that are later used in other sending
> timestamping callbacks.
> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>  include/uapi/linux/bpf.h       | 5 +++++
>  net/ipv4/tcp.c                 | 4 ++++
>  tools/include/uapi/linux/bpf.h | 5 +++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 9355d617767f..86fca729fbd8 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7052,6 +7052,11 @@ enum {
>  					 * when SK_BPF_CB_TX_TIMESTAMPING
>  					 * feature is on.
>  					 */
> +	BPF_SOCK_OPS_TS_SND_CB,		/* Called when every sendmsg syscall
> +					 * is triggered. It's used to correlate
> +					 * sendmsg timestamp with corresponding
> +					 * tskey.
> +					 */
>  };
>  
>  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 12b9c4f9c151..4b9739cd3bc5 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -492,6 +492,10 @@ static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
>  		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
>  			shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
>  	}
> +
> +	if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
> +	    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING) && skb)
> +		bpf_skops_tx_timestamping(sk, skb, BPF_SOCK_OPS_TS_SND_CB);
>  }
>  
>  static bool tcp_stream_is_readable(struct sock *sk, int target)
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index d3e2988b3b4c..2739ee0154a0 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -7042,6 +7042,11 @@ enum {
>  					 * when SK_BPF_CB_TX_TIMESTAMPING
>  					 * feature is on.
>  					 */
> +	BPF_SOCK_OPS_TS_SND_CB,		/* Called when every sendmsg syscall
> +					 * is triggered. It's used to correlate
> +					 * sendmsg timestamp with corresponding
> +					 * tskey.
> +					 */

Feel free to decline this late in the review process, but a bit more
bikeshedding..

Can we spell out TSTAMP instead of TS in these definitions? Within
the context of this series it is self-explanatory, but when reading
kernel code the meaning of a two letter acronym is not that clear.

And instead of SND can we use SENDMSG or something like that?
SND here confused me as the software timestamp is SCM_TSTAMP_SND.

For instance:

    BPF_SOCK_OPS_TSTAMP_SENDMSG_CB,
    BPF_SOCK_OPS_TSTAMP_SCHED_CB,
    BPF_SOCK_OPS_TSTAMP_SND_SW_CB,
    BPF_SOCK_OPS_TSTAMP_SND_HW_CB,
   (BPF_SOCK_OPS_TSTAMP_TX_COMPLETION_CB,)
    BPF_SOCK_OPS_TSTAMP_ACK_CB,

(not sure what the OPT in OPT_CB added).
Jason Xing Feb. 20, 2025, 3:15 a.m. UTC | #3 
On Thu, Feb 20, 2025 at 10:55 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > This patch introduces a new callback in tcp_tx_timestamp() to correlate
> > tcp_sendmsg timestamp with timestamps from other tx timestamping
> > callbacks (e.g., SND/SW/ACK).
> >
> > Without this patch, BPF program wouldn't know which timestamps belong
> > to which flow because of no socket lock protection. This new callback
> > is inserted in tcp_tx_timestamp() to address this issue because
> > tcp_tx_timestamp() still owns the same socket lock with
> > tcp_sendmsg_locked() in the meanwhile tcp_tx_timestamp() initializes
> > the timestamping related fields for the skb, especially tskey. The
> > tskey is the bridge to do the correlation.
> >
> > For TCP, BPF program hooks the beginning of tcp_sendmsg_locked() and
> > then stores the sendmsg timestamp at the bpf_sk_storage, correlating
> > this timestamp with its tskey that are later used in other sending
> > timestamping callbacks.
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > ---
> >  include/uapi/linux/bpf.h       | 5 +++++
> >  net/ipv4/tcp.c                 | 4 ++++
> >  tools/include/uapi/linux/bpf.h | 5 +++++
> >  3 files changed, 14 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 9355d617767f..86fca729fbd8 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -7052,6 +7052,11 @@ enum {
> >                                        * when SK_BPF_CB_TX_TIMESTAMPING
> >                                        * feature is on.
> >                                        */
> > +     BPF_SOCK_OPS_TS_SND_CB,         /* Called when every sendmsg syscall
> > +                                      * is triggered. It's used to correlate
> > +                                      * sendmsg timestamp with corresponding
> > +                                      * tskey.
> > +                                      */
> >  };
> >
> >  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 12b9c4f9c151..4b9739cd3bc5 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -492,6 +492,10 @@ static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
> >               if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
> >                       shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> >       }
> > +
> > +     if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
> > +         SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING) && skb)
> > +             bpf_skops_tx_timestamping(sk, skb, BPF_SOCK_OPS_TS_SND_CB);
> >  }
> >
> >  static bool tcp_stream_is_readable(struct sock *sk, int target)
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index d3e2988b3b4c..2739ee0154a0 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -7042,6 +7042,11 @@ enum {
> >                                        * when SK_BPF_CB_TX_TIMESTAMPING
> >                                        * feature is on.
> >                                        */
> > +     BPF_SOCK_OPS_TS_SND_CB,         /* Called when every sendmsg syscall
> > +                                      * is triggered. It's used to correlate
> > +                                      * sendmsg timestamp with corresponding
> > +                                      * tskey.
> > +                                      */
>
> Feel free to decline this late in the review process, but a bit more
> bikeshedding..
>
> Can we spell out TSTAMP instead of TS in these definitions? Within
> the context of this series it is self-explanatory, but when reading
> kernel code the meaning of a two letter acronym is not that clear.

Even though I feel reluctant to change across the whole series because
if so, I will adjust in many places. Of course, you're right about the
new name being clearer :)

>
> And instead of SND can we use SENDMSG or something like that?
> SND here confused me as the software timestamp is SCM_TSTAMP_SND.

I'm not sure about this. For TCP, it's not implemented in the
tcp_sendmsg_locked but tcp_tx_timestamp. Well, I have no strong
preference.

You can make the final call :)

Thanks,
Jason

>
> For instance:
>
>     BPF_SOCK_OPS_TSTAMP_SENDMSG_CB,
>     BPF_SOCK_OPS_TSTAMP_SCHED_CB,
>     BPF_SOCK_OPS_TSTAMP_SND_SW_CB,
>     BPF_SOCK_OPS_TSTAMP_SND_HW_CB,
>    (BPF_SOCK_OPS_TSTAMP_TX_COMPLETION_CB,)
>     BPF_SOCK_OPS_TSTAMP_ACK_CB,
>
> (not sure what the OPT in OPT_CB added).
>
>
Jason Xing Feb. 20, 2025, 4:31 a.m. UTC | #4 
On Thu, Feb 20, 2025 at 11:15 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 10:55 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > This patch introduces a new callback in tcp_tx_timestamp() to correlate
> > > tcp_sendmsg timestamp with timestamps from other tx timestamping
> > > callbacks (e.g., SND/SW/ACK).
> > >
> > > Without this patch, BPF program wouldn't know which timestamps belong
> > > to which flow because of no socket lock protection. This new callback
> > > is inserted in tcp_tx_timestamp() to address this issue because
> > > tcp_tx_timestamp() still owns the same socket lock with
> > > tcp_sendmsg_locked() in the meanwhile tcp_tx_timestamp() initializes
> > > the timestamping related fields for the skb, especially tskey. The
> > > tskey is the bridge to do the correlation.
> > >
> > > For TCP, BPF program hooks the beginning of tcp_sendmsg_locked() and
> > > then stores the sendmsg timestamp at the bpf_sk_storage, correlating
> > > this timestamp with its tskey that are later used in other sending
> > > timestamping callbacks.
> > >
> > > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > > ---
> > >  include/uapi/linux/bpf.h       | 5 +++++
> > >  net/ipv4/tcp.c                 | 4 ++++
> > >  tools/include/uapi/linux/bpf.h | 5 +++++
> > >  3 files changed, 14 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 9355d617767f..86fca729fbd8 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -7052,6 +7052,11 @@ enum {
> > >                                        * when SK_BPF_CB_TX_TIMESTAMPING
> > >                                        * feature is on.
> > >                                        */
> > > +     BPF_SOCK_OPS_TS_SND_CB,         /* Called when every sendmsg syscall
> > > +                                      * is triggered. It's used to correlate
> > > +                                      * sendmsg timestamp with corresponding
> > > +                                      * tskey.
> > > +                                      */
> > >  };
> > >
> > >  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index 12b9c4f9c151..4b9739cd3bc5 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -492,6 +492,10 @@ static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
> > >               if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
> > >                       shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > >       }
> > > +
> > > +     if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
> > > +         SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING) && skb)
> > > +             bpf_skops_tx_timestamping(sk, skb, BPF_SOCK_OPS_TS_SND_CB);
> > >  }
> > >
> > >  static bool tcp_stream_is_readable(struct sock *sk, int target)
> > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > index d3e2988b3b4c..2739ee0154a0 100644
> > > --- a/tools/include/uapi/linux/bpf.h
> > > +++ b/tools/include/uapi/linux/bpf.h
> > > @@ -7042,6 +7042,11 @@ enum {
> > >                                        * when SK_BPF_CB_TX_TIMESTAMPING
> > >                                        * feature is on.
> > >                                        */
> > > +     BPF_SOCK_OPS_TS_SND_CB,         /* Called when every sendmsg syscall
> > > +                                      * is triggered. It's used to correlate
> > > +                                      * sendmsg timestamp with corresponding
> > > +                                      * tskey.
> > > +                                      */
> >
> > Feel free to decline this late in the review process, but a bit more
> > bikeshedding..
> >
> > Can we spell out TSTAMP instead of TS in these definitions? Within
> > the context of this series it is self-explanatory, but when reading
> > kernel code the meaning of a two letter acronym is not that clear.
>
> Even though I feel reluctant to change across the whole series because
> if so, I will adjust in many places. Of course, you're right about the
> new name being clearer :)
>
> >
> > And instead of SND can we use SENDMSG or something like that?
> > SND here confused me as the software timestamp is SCM_TSTAMP_SND.
>
> I'm not sure about this. For TCP, it's not implemented in the
> tcp_sendmsg_locked but tcp_tx_timestamp. Well, I have no strong
> preference.
>
> You can make the final call :)

After taking a break, I feel full of energy and I will update them all
as you requested :)

Thanks,
Jason
Willem de Bruijn Feb. 20, 2025, 3:28 p.m. UTC | #5 
Jason Xing wrote:
> On Thu, Feb 20, 2025 at 11:15 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 10:55 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > This patch introduces a new callback in tcp_tx_timestamp() to correlate
> > > > tcp_sendmsg timestamp with timestamps from other tx timestamping
> > > > callbacks (e.g., SND/SW/ACK).
> > > >
> > > > Without this patch, BPF program wouldn't know which timestamps belong
> > > > to which flow because of no socket lock protection. This new callback
> > > > is inserted in tcp_tx_timestamp() to address this issue because
> > > > tcp_tx_timestamp() still owns the same socket lock with
> > > > tcp_sendmsg_locked() in the meanwhile tcp_tx_timestamp() initializes
> > > > the timestamping related fields for the skb, especially tskey. The
> > > > tskey is the bridge to do the correlation.
> > > >
> > > > For TCP, BPF program hooks the beginning of tcp_sendmsg_locked() and
> > > > then stores the sendmsg timestamp at the bpf_sk_storage, correlating
> > > > this timestamp with its tskey that are later used in other sending
> > > > timestamping callbacks.
> > > >
> > > > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 5 +++++
> > > >  net/ipv4/tcp.c                 | 4 ++++
> > > >  tools/include/uapi/linux/bpf.h | 5 +++++
> > > >  3 files changed, 14 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index 9355d617767f..86fca729fbd8 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -7052,6 +7052,11 @@ enum {
> > > >                                        * when SK_BPF_CB_TX_TIMESTAMPING
> > > >                                        * feature is on.
> > > >                                        */
> > > > +     BPF_SOCK_OPS_TS_SND_CB,         /* Called when every sendmsg syscall
> > > > +                                      * is triggered. It's used to correlate
> > > > +                                      * sendmsg timestamp with corresponding
> > > > +                                      * tskey.
> > > > +                                      */
> > > >  };
> > > >
> > > >  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > index 12b9c4f9c151..4b9739cd3bc5 100644
> > > > --- a/net/ipv4/tcp.c
> > > > +++ b/net/ipv4/tcp.c
> > > > @@ -492,6 +492,10 @@ static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
> > > >               if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
> > > >                       shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > > >       }
> > > > +
> > > > +     if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
> > > > +         SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING) && skb)
> > > > +             bpf_skops_tx_timestamping(sk, skb, BPF_SOCK_OPS_TS_SND_CB);
> > > >  }
> > > >
> > > >  static bool tcp_stream_is_readable(struct sock *sk, int target)
> > > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > > index d3e2988b3b4c..2739ee0154a0 100644
> > > > --- a/tools/include/uapi/linux/bpf.h
> > > > +++ b/tools/include/uapi/linux/bpf.h
> > > > @@ -7042,6 +7042,11 @@ enum {
> > > >                                        * when SK_BPF_CB_TX_TIMESTAMPING
> > > >                                        * feature is on.
> > > >                                        */
> > > > +     BPF_SOCK_OPS_TS_SND_CB,         /* Called when every sendmsg syscall
> > > > +                                      * is triggered. It's used to correlate
> > > > +                                      * sendmsg timestamp with corresponding
> > > > +                                      * tskey.
> > > > +                                      */
> > >
> > > Feel free to decline this late in the review process, but a bit more
> > > bikeshedding..
> > >
> > > Can we spell out TSTAMP instead of TS in these definitions? Within
> > > the context of this series it is self-explanatory, but when reading
> > > kernel code the meaning of a two letter acronym is not that clear.
> >
> > Even though I feel reluctant to change across the whole series because
> > if so, I will adjust in many places. Of course, you're right about the
> > new name being clearer :)
> >
> > >
> > > And instead of SND can we use SENDMSG or something like that?
> > > SND here confused me as the software timestamp is SCM_TSTAMP_SND.
> >
> > I'm not sure about this. For TCP, it's not implemented in the
> > tcp_sendmsg_locked but tcp_tx_timestamp. Well, I have no strong
> > preference.
> >
> > You can make the final call :)
> 
> After taking a break, I feel full of energy and I will update them all
> as you requested :)

Awesome, thank you.
diff mbox series

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9355d617767f..86fca729fbd8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7052,6 +7052,11 @@  enum {
 					 * when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_SND_CB,		/* Called when every sendmsg syscall
+					 * is triggered. It's used to correlate
+					 * sendmsg timestamp with corresponding
+					 * tskey.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 12b9c4f9c151..4b9739cd3bc5 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -492,6 +492,10 @@  static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
 		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
 			shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
 	}
+
+	if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
+	    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING) && skb)
+		bpf_skops_tx_timestamping(sk, skb, BPF_SOCK_OPS_TS_SND_CB);
 }
 
 static bool tcp_stream_is_readable(struct sock *sk, int target)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index d3e2988b3b4c..2739ee0154a0 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7042,6 +7042,11 @@  enum {
 					 * when SK_BPF_CB_TX_TIMESTAMPING
 					 * feature is on.
 					 */
+	BPF_SOCK_OPS_TS_SND_CB,		/* Called when every sendmsg syscall
+					 * is triggered. It's used to correlate
+					 * sendmsg timestamp with corresponding
+					 * tskey.
+					 */
 };
 
 /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect