Re: [PATCH 2/7] flow, flow_table: Pad flow table entries to 128 bytes, hash entries to 32 bits

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

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

On Tue, Jan 28, 2025 at 12:15:27AM +0100, Stefano Brivio wrote:
> ...to keep migration sane. Right now, the biggest struct in union flow
> is struct tcp_splice_conn with 120 bytes on x86_64, which should also
> have the biggest storage and alignment requirements of any
> architecture we might run on.

Necessary for the current "copy the entire table as a blob" approach.
As I've noted, I think that will be fragile, but we can revisit this
change when/if we figure out a different way to handle the table as a
whole.

> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  flow.h       | 18 ++++++++++++------
>  flow_table.h | 13 ++++++++++---
>  2 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/flow.h b/flow.h
> index 24ba3ef..8eb5964 100644
> --- a/flow.h
> +++ b/flow.h
> @@ -202,15 +202,21 @@ struct flow_common {
>  
>  /**
>   * struct flow_sidx - ID for one side of a specific flow
> - * @sidei:	Index of side referenced (0 or 1)
> - * @flowi:	Index of flow referenced
> + * @sidei:		Index of side referenced (0 or 1)
> + * @flowi:		Index of flow referenced
> + * @flow_sidx_storage:	Pad to 32 bits
>   */
>  typedef struct flow_sidx {
> -	unsigned	sidei :1;
> -	unsigned	flowi :FLOW_INDEX_BITS;
> +	union {
> +		struct {
> +			unsigned	sidei :1;
> +			unsigned	flowi :FLOW_INDEX_BITS;
> +		};
> +		uint32_t flow_sidx_storage;
> +	};
>  } flow_sidx_t;
> -static_assert(sizeof(flow_sidx_t) <= sizeof(uint32_t),
> -	      "flow_sidx_t must fit within 32 bits");
> +static_assert(sizeof(flow_sidx_t) == sizeof(uint32_t),
> +	      "flow_sidx_t must be 32-bit wide");
>  
>  #define FLOW_SIDX_NONE ((flow_sidx_t){ .flowi = FLOW_MAX })
>  
> diff --git a/flow_table.h b/flow_table.h
> index f15db53..007f4dd 100644
> --- a/flow_table.h
> +++ b/flow_table.h
> @@ -26,9 +26,13 @@ struct flow_free_cluster {
>  
>  /**
>   * union flow - Descriptor for a logical packet flow (e.g. connection)
> - * @f:		Fields common between all variants
> - * @tcp:	Fields for non-spliced TCP connections
> - * @tcp_splice:	Fields for spliced TCP connections
> + * @f:			Fields common between all variants
> + * @free:		Entry in a cluster of free entries
> + * @tcp:		Fields for non-spliced TCP connections
> + * @tcp_splice:		Fields for spliced TCP connections
> + * @ping:		Tracking for ping flows
> + * @udp:		Tracking for UDP flows
> + * @flow_storage:	Pad flow entries to 128 bytes to ease state migration
>  */
>  union flow {
>  	struct flow_common f;
> @@ -37,8 +41,11 @@ union flow {
>  	struct tcp_splice_conn tcp_splice;
>  	struct icmp_ping_flow ping;
>  	struct udp_flow udp;
> +	char flow_storage[128];
>  };
>  
> +static_assert(sizeof(union flow) == 128, "union flow should be 128-byte wide");
> +
>  /* Global Flow Table */
>  extern unsigned flow_first_free;
>  extern union flow flowtab[];

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