Re: [PATCH bpf-next v12 01/12] bpf: add networking timestamping support to bpf_get/setsockopt()

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

> > Can you find a hole further down to place this in, or at least a spot
> > that does not result in 7b of wasted space (in the hotpath cacheline
> > groups of all places).
> 
> There is one place where I can simply insert the flag.
> 
> The diff patch on top of this series is:
> diff --git a/include/net/sock.h b/include/net/sock.h
> index e85d6fb3a2ba..9fa27693fb02 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -446,8 +446,6 @@ 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))
> -       u8                      sk_bpf_cb_flags;
>         __cacheline_group_end(sock_write_rxtx);
> 
>         __cacheline_group_begin(sock_write_tx);
> @@ -528,6 +526,8 @@ struct sock {
>         u8                      sk_txtime_deadline_mode : 1,
>                                 sk_txtime_report_errors : 1,
>                                 sk_txtime_unused : 6;
> +#define SK_BPF_CB_FLAG_TEST(SK, FLAG) ((SK)->sk_bpf_cb_flags & (FLAG))
> +       u8                      sk_bpf_cb_flags;
> 
>         void                    *sk_user_data;
>  #ifdef CONFIG_SECURITY
> 
> 
> 1) before applying the whole series:
> ...
>         /* --- cacheline 10 boundary (640 bytes) --- */
>         ktime_t                    sk_stamp;             /* 0x280   0x8 */
>         int                        sk_disconnects;       /* 0x288   0x4 */
>         u8                         sk_txrehash;          /* 0x28c   0x1 */
>         u8                         sk_clockid;           /* 0x28d   0x1 */
>         u8                         sk_txtime_deadline_mode:1; /* 0x28e: 0 0x1 */
>         u8                         sk_txtime_report_errors:1; /*
> 0x28e:0x1 0x1 */
>         u8                         sk_txtime_unused:6;   /* 0x28e:0x2 0x1 */
> 
>         /* XXX 1 byte hole, try to pack */
> 
>         void *                     sk_user_data;         /* 0x290   0x8 */
>         void *                     sk_security;          /* 0x298   0x8 */
>         struct sock_cgroup_data    sk_cgrp_data;         /* 0x2a0  0x10 */
> ...
> /* sum members: 773, holes: 1, sum holes: 1 */
> 
> 
> 2) after applying the series with the above diff patch:
> ...
>         /* --- cacheline 10 boundary (640 bytes) --- */
>         ktime_t                    sk_stamp;             /* 0x280   0x8 */
>         int                        sk_disconnects;       /* 0x288   0x4 */
>         u8                         sk_txrehash;          /* 0x28c   0x1 */
>         u8                         sk_clockid;           /* 0x28d   0x1 */
>         u8                         sk_txtime_deadline_mode:1; /* 0x28e: 0 0x1 */
>         u8                         sk_txtime_report_errors:1; /*
> 0x28e:0x1 0x1 */
>         u8                         sk_txtime_unused:6;   /* 0x28e:0x2 0x1 */
>         u8                         sk_bpf_cb_flags;      /* 0x28f   0x1 */
>         void *                     sk_user_data;         /* 0x290
> 0x8 */
>         void *                     sk_security;          /* 0x298   0x8 */
>         struct sock_cgroup_data    sk_cgrp_data;         /* 0x2a0  0x10 */
> ...
> /* sum members: 774 */
> 
> It turns out that the new sk_bpf_cb_flags fills the hole exactly. The
> new field and some of its nearby fields are quite similar because they
> are only/nearly written during the creation or setsockopt phase.
> 
> I think now it's a good place to insert the new flag?

Thanks. This seems fine to me.