Re: [PATCH] udp_flow: Save 8 bytes in struct udp_flow on 64-bit architectures

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4468 bytes --]

On Tue, Apr 08, 2025 at 07:56:24AM +0200, Stefano Brivio wrote:
> Shuffle the fields just added by commits a7775e9550fa ("udp: support
> traceroute in direction tap-socket") and 9725e7988837 ("udp_flow:
> Don't discard packets that arrive between bind() and connect()").
> 
> On x86_64, as reported by pahole(1), before:
> 
> struct udp_flow {
>         struct flow_common         f;                    /*     0    76 */
>         /* --- cacheline 1 boundary (64 bytes) was 12 bytes ago --- */
>         _Bool                      closed:1;             /*    76: 0  1 */
> 
>         /* XXX 7 bits hole, try to pack */
> 
>         _Bool                      flush0;               /*    77     1 */
>         _Bool                      flush1:1;             /*    78: 0  1 */
> 
>         /* XXX 7 bits hole, try to pack */
>         /* XXX 1 byte hole, try to pack */
> 
>         time_t                     ts;                   /*    80     8 */
>         int                        s[2];                 /*    88     8 */
>         uint8_t                    ttl[2];               /*    96     2 */
> 
>         /* size: 104, cachelines: 2, members: 7 */
>         /* sum members: 95, holes: 1, sum holes: 1 */
>         /* sum bitfield members: 2 bits, bit holes: 2, sum bit holes: 14 bits */
>         /* padding: 6 */
>         /* last cacheline: 40 bytes */
> };
> 
> and after:
> 
> struct udp_flow {
>         struct flow_common         f;                    /*     0    76 */
>         /* --- cacheline 1 boundary (64 bytes) was 12 bytes ago --- */
>         uint8_t                    ttl[2];               /*    76     2 */
>         _Bool                      closed:1;             /*    78: 0  1 */
>         _Bool                      flush0:1;             /*    78: 1  1 */
>         _Bool                      flush1:1;             /*    78: 2  1 */
> 
>         /* XXX 5 bits hole, try to pack */
>         /* XXX 1 byte hole, try to pack */
> 
>         time_t                     ts;                   /*    80     8 */
>         int                        s[2];                 /*    88     8 */
> 
>         /* size: 96, cachelines: 2, members: 7 */
>         /* sum members: 94, holes: 1, sum holes: 1 */
>         /* sum bitfield members: 3 bits, bit holes: 1, sum bit holes: 5 bits */
>         /* last cacheline: 32 bytes */
> };
> 
> It doesn't matter much because anyway the typical storage for struct
> udp_flow is given by union flow:
> 
> union flow {
>         struct flow_common         f;                  /*     0    76 */
>         struct flow_free_cluster   free;               /*     0    84 */
>         struct tcp_tap_conn        tcp;                /*     0   120 */
>         struct tcp_splice_conn     tcp_splice;         /*     0   120 */
>         struct icmp_ping_flow      ping;               /*     0    96 */
>         struct udp_flow            udp;                /*     0    96 */
> };
> 
> but it still improves data locality somewhat, so let me fix this up
> now that commits are fresh.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  udp_flow.h | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/udp_flow.h b/udp_flow.h
> index 90d3b29..e289122 100644
> --- a/udp_flow.h
> +++ b/udp_flow.h
> @@ -10,22 +10,25 @@
>  /**
>   * struct udp_flow - Descriptor for a flow of UDP packets
>   * @f:		Generic flow information
> + * @ttl:	TTL or hop_limit for both sides
>   * @closed:	Flow is already closed
>   * @flush0:	@s[0] may have datagrams queued for other flows
>   * @flush1:	@s[1] may have datagrams queued for other flows
>   * @ts:		Activity timestamp
>   * @s:		Socket fd (or -1) for each side of the flow
> - * @ttl:	TTL or hop_limit for both sides
>   */
>  struct udp_flow {
>  	/* Must be first element */
>  	struct flow_common f;
>  
> -	bool closed :1;
> -	bool flush0, flush1 :1;
> +	uint8_t ttl[SIDES];
> +
> +	bool	closed	:1,
> +		flush0	:1,
> +		flush1	:1;
> +
>  	time_t ts;
>  	int s[SIDES];
> -	uint8_t ttl[SIDES];
>  };
>  
>  struct udp_flow *udp_at_sidx(flow_sidx_t sidx);

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]