Re: [PATCH net-next] netfilter: Reorder fields in 'struct nf_conntrack_expect'

[Simon Horman]

On Mon, May 08, 2023 at 06:53:14PM +0200, Christophe JAILLET wrote:
> Group some variables based on their sizes to reduce holes.
> On x86_64, this shrinks the size of 'struct nf_conntrack_expect' from 264
> to 256 bytes.
> 
> This structure deserve a dedicated cache, so reducing its size looks nice.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>

Thanks, this much nicer to me.

I slightly concerned that there may be implications for
fields that were on the same cacheline now being on different cachelines.
But only very slightly.

Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>

> ---
> Using pahole
> 
> Before:
> ======
> struct nf_conntrack_expect {
> 	struct hlist_node          lnode;                /*     0    16 */
> 	struct hlist_node          hnode;                /*    16    16 */
> 	struct nf_conntrack_tuple  tuple;                /*    32    40 */
> 	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
> 	struct nf_conntrack_tuple_mask mask;             /*    72    20 */
> 
> 	/* XXX 4 bytes hole, try to pack */
> 
> 	void                       (*expectfn)(struct nf_conn *, struct nf_conntrack_expect *); /*    96     8 */
> 	struct nf_conntrack_helper * helper;             /*   104     8 */
> 	struct nf_conn *           master;               /*   112     8 */
> 	struct timer_list          timeout;              /*   120    88 */
> 	/* --- cacheline 3 boundary (192 bytes) was 16 bytes ago --- */
> 	refcount_t                 use;                  /*   208     4 */
> 	unsigned int               flags;                /*   212     4 */
> 	unsigned int               class;                /*   216     4 */
> 	union nf_inet_addr         saved_addr;           /*   220    16 */
> 	union nf_conntrack_man_proto saved_proto;        /*   236     2 */
> 
> 	/* XXX 2 bytes hole, try to pack */
> 
> 	enum ip_conntrack_dir      dir;                  /*   240     4 */
> 
> 	/* XXX 4 bytes hole, try to pack */
> 
> 	struct callback_head       rcu __attribute__((__aligned__(8))); /*   248    16 */
> 
> 	/* size: 264, cachelines: 5, members: 15 */
> 	/* sum members: 254, holes: 3, sum holes: 10 */
> 	/* forced alignments: 1, forced holes: 1, sum forced holes: 4 */
> 	/* last cacheline: 8 bytes */
> } __attribute__((__aligned__(8)));
> 
> 
> After:
> =====
> struct nf_conntrack_expect {
> 	struct hlist_node          lnode;                /*     0    16 */
> 	struct hlist_node          hnode;                /*    16    16 */
> 	struct nf_conntrack_tuple  tuple;                /*    32    40 */
> 	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
> 	struct nf_conntrack_tuple_mask mask;             /*    72    20 */
> 	refcount_t                 use;                  /*    92     4 */
> 	unsigned int               flags;                /*    96     4 */
> 	unsigned int               class;                /*   100     4 */
> 	void                       (*expectfn)(struct nf_conn *, struct nf_conntrack_expect *); /*   104     8 */
> 	struct nf_conntrack_helper * helper;             /*   112     8 */
> 	struct nf_conn *           master;               /*   120     8 */
> 	/* --- cacheline 2 boundary (128 bytes) --- */
> 	struct timer_list          timeout;              /*   128    88 */
> 	/* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */
> 	union nf_inet_addr         saved_addr;           /*   216    16 */
> 	union nf_conntrack_man_proto saved_proto;        /*   232     2 */
> 
> 	/* XXX 2 bytes hole, try to pack */
> 
> 	enum ip_conntrack_dir      dir;                  /*   236     4 */
> 	struct callback_head       rcu __attribute__((__aligned__(8))); /*   240    16 */
> 
> 	/* size: 256, cachelines: 4, members: 15 */
> 	/* sum members: 254, holes: 1, sum holes: 2 */
> 	/* forced alignments: 1 */
> } __attribute__((__aligned__(8)));
> ---
>  include/net/netfilter/nf_conntrack_expect.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_expect.h b/include/net/netfilter/nf_conntrack_expect.h
> index 0855b60fba17..cf0d81be5a96 100644
> --- a/include/net/netfilter/nf_conntrack_expect.h
> +++ b/include/net/netfilter/nf_conntrack_expect.h
> @@ -26,6 +26,15 @@ struct nf_conntrack_expect {
>  	struct nf_conntrack_tuple tuple;
>  	struct nf_conntrack_tuple_mask mask;
>  
> +	/* Usage count. */
> +	refcount_t use;
> +
> +	/* Flags */
> +	unsigned int flags;
> +
> +	/* Expectation class */
> +	unsigned int class;
> +
>  	/* Function to call after setup and insertion */
>  	void (*expectfn)(struct nf_conn *new,
>  			 struct nf_conntrack_expect *this);
> @@ -39,15 +48,6 @@ struct nf_conntrack_expect {
>  	/* Timer function; deletes the expectation. */
>  	struct timer_list timeout;
>  
> -	/* Usage count. */
> -	refcount_t use;
> -
> -	/* Flags */
> -	unsigned int flags;
> -
> -	/* Expectation class */
> -	unsigned int class;
> -
>  #if IS_ENABLED(CONFIG_NF_NAT)
>  	union nf_inet_addr saved_addr;
>  	/* This is the original per-proto part, used to map the
> -- 
> 2.34.1
> 
>