Message-ID: <67b542b9c4e3d_1692112944@willemb.c.googlers.com.notmuch>
Date: Tue, 18 Feb 2025 21:32:25 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 willemb@...gle.com, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 eddyz87@...il.com, 
 song@...nel.org, 
 yonghong.song@...ux.dev, 
 john.fastabend@...il.com, 
 kpsingh@...nel.org, 
 sdf@...ichev.me, 
 haoluo@...gle.com, 
 jolsa@...nel.org, 
 shuah@...nel.org, 
 ykolal@...com, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v12 01/12] bpf: add networking timestamping
 support to bpf_get/setsockopt()

Jason Xing wrote:
> On Wed, Feb 19, 2025 at 5:55 AM Martin KaFai Lau <martin.lau@...ux.dev> 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@...il.com>
> > >> ---
> > >>   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:

Adding fields to structs in the hot path is a tragedy of the commons.

Every developer focuses on their specific workload and pet feature,
while imposing a cost on everyone else.

We have a duty to be frugal and mitigate this cost where possible.
Especially for a feature that is likely to be used sparingly.

> 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,

Can you give a short list of bits that you could see being used, to
get an idea of the count. In my mind this is a very short list, not
worth reserving 32 bits for. But you might have more developed plans.

> 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?

If it fits in a u8 and that in practice also results in less memory
and cache pressure (i.e., does not just add a 24b hole), then it is a
net improvement.

Powered by blists - more mailing lists