On Wed, Feb 19, 2025 at 5:55 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 2/18/25 6:22 AM, Willem de Bruijn wrote:
> > Jason Xing wrote:
> >> The new SK_BPF_CB_FLAGS and new SK_BPF_CB_TX_TIMESTAMPING are
> >> added to bpf_get/setsockopt. The later patches will implement the
> >> BPF networking timestamping. The BPF program will use
> >> bpf_setsockopt(SK_BPF_CB_FLAGS, SK_BPF_CB_TX_TIMESTAMPING) to
> >> enable the BPF networking timestamping on a socket.
> >>
> >> Signed-off-by: Jason Xing <kerneljasonxing@xxxxxxxxx>
> >> ---
> >> include/net/sock.h | 3 +++
> >> include/uapi/linux/bpf.h | 8 ++++++++
> >> net/core/filter.c | 23 +++++++++++++++++++++++
> >> tools/include/uapi/linux/bpf.h | 1 +
> >> 4 files changed, 35 insertions(+)
> >>
> >> diff --git a/include/net/sock.h b/include/net/sock.h
> >> index 8036b3b79cd8..7916982343c6 100644
> >> --- a/include/net/sock.h
> >> +++ b/include/net/sock.h
> >> @@ -303,6 +303,7 @@ struct sk_filter;
> >> * @sk_stamp: time stamp of last packet received
> >> * @sk_stamp_seq: lock for accessing sk_stamp on 32 bit architectures only
> >> * @sk_tsflags: SO_TIMESTAMPING flags
> >> + * @sk_bpf_cb_flags: used in bpf_setsockopt()
> >> * @sk_use_task_frag: allow sk_page_frag() to use current->task_frag.
> >> * Sockets that can be used under memory reclaim should
> >> * set this to false.
> >> @@ -445,6 +446,8 @@ struct sock {
> >> u32 sk_reserved_mem;
> >> int sk_forward_alloc;
> >> u32 sk_tsflags;
> >> +#define SK_BPF_CB_FLAG_TEST(SK, FLAG) ((SK)->sk_bpf_cb_flags & (FLAG))
> >> + u32 sk_bpf_cb_flags;
> >> __cacheline_group_end(sock_write_rxtx);
> >
> > So far only one bit is defined. Does this have to be a 32-bit field in
> > every socket?
>
> iirc, I think there were multiple callback (cb) flags/bits in the earlier
> revisions, but it had been simplified to one bit in the later revisions.
>
> It's an internal implementation detail. We can reuse some free bits from another
> variable for now. Probably get a bit from sk_tsflags? SOCKCM_FLAG_TS_OPT_ID uses
> BIT(31). Maybe a new SK_TS_FLAG_BPF_TX that uses BIT(30)? I don't have a strong
> preference on the name.
>
> When the BPF program calls bpf_setsockopt(SK_BPF_CB_FLAGS,
> SK_BPF_CB_TX_TIMESTAMPING), the kernel will set/test the BIT(30) of sk_tsflags.
>
> We can wait until there are more socket-level cb flags in the future (e.g., more
> SK_BPF_CB_XXX will be needed) before adding a dedicated int field in the sock.
Sorry, I still preferred the way we've discussed already:
1) Introducing a new field sk_bpf_cb_flags extends the use for bpf
timestamping, more than SK_BPF_CB_TX_TIMESTAMPING one flag. I think
SK_BPF_CB_RX_TIMESTAMPING is also needed in the next move. And more
subfeatures (like bpf extension for OPT_ID) will use it. It gives us a
separate way to do more things based on this bpf timestamping.
2) sk_bpf_cb_flags provides a way to let the socket-level use new
features for bpf while now we only have a tcp_sock-level, namely,
bpf_sock_ops_cb_flags. It's obviously good for others.
It's the first move to open the gate for socket-level usage for BPF,
just like how TCP_BPF_SOCK_OPS_CB_FLAGS works in sol_tcp_sockopt(). So
I hope we will not abandon this good approach :(
Now I wonder if I should use the u8 sk_bpf_cb_flags in V13 or just
keep it as-is? Either way is fine with me :) bpf_sock_ops_cb_flags
uses u8 as an example, thus I think we prefer the former?
Thanks,
Jason
