Re: [PATCH iwl-next v2 01/14] cache: add __cacheline_group_{begin,end}_aligned() (+ couple more)

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 6/20/24 15:53, Alexander Lobakin wrote:
> __cacheline_group_begin(), unfortunately, doesn't align the group
> anyhow. If it is wanted, then you need to do something like
> 
> __cacheline_group_begin(grp) __aligned(ALIGN)
> 
> which isn't really convenient nor compact.
> Add the _aligned() counterparts to align the groups automatically to
> either the specified alignment (optional) or ``SMP_CACHE_BYTES``.
> Note that the actual struct layout will then be (on x64 with 64-byte CL):
> 
> struct x {
> 	u32 y;				// offset 0, size 4, padding 56
> 	__cacheline_group_begin__grp;	// offset 64, size 0
> 	u32 z;				// offset 64, size 4, padding 4
> 	__cacheline_group_end__grp;	// offset 72, size 0
> 	__cacheline_group_pad__grp;	// offset 72, size 0, padding 56
> 	u32 w;				// offset 128
> };
> 
> The end marker is aligned to long, so that you can assert the struct
> size more strictly, but the offset of the next field in the structure
> will be aligned to the group alignment, so that the next field won't
> fall into the group it's not intended to.
> 
> Add __LARGEST_ALIGN definition and LARGEST_ALIGN() macro.
> __LARGEST_ALIGN is the value to which the compilers align fields when
> __aligned_largest is specified. Sometimes, it might be needed to get
> this value outside of variable definitions. LARGEST_ALIGN() is macro
> which just aligns a value to __LARGEST_ALIGN.
> Also add SMP_CACHE_ALIGN(), similar to L1_CACHE_ALIGN(), but using
> ``SMP_CACHE_BYTES`` instead of ``L1_CACHE_BYTES`` as the former
> also accounts L2, needed in some cases.
> 
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>   include/linux/cache.h | 59 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
> 

[...]

> +/**
> + * __cacheline_group_begin_aligned - declare an aligned group start
> + * @GROUP: name of the group
> + * @...: optional group alignment

didn't know that you could document "..." :)

> + *
> + * The following block inside a struct:
> + *
> + *	__cacheline_group_begin_aligned(grp);
> + *	field a;
> + *	field b;
> + *	__cacheline_group_end_aligned(grp);
> + *
> + * will always be aligned to either the specified alignment or
> + * ``SMP_CACHE_BYTES``.
> + */
> +#define __cacheline_group_begin_aligned(GROUP, ...)		\
> +	__cacheline_group_begin(GROUP)				\
> +	__aligned((__VA_ARGS__ + 0) ? : SMP_CACHE_BYTES)

nice trick :) +0

> +
> +/**
> + * __cacheline_group_end_aligned - declare an aligned group end
> + * @GROUP: name of the group
> + * @...: optional alignment (same as was in __cacheline_group_begin_aligned())
> + *
> + * Note that the end marker is aligned to sizeof(long) to allow more precise
> + * size assertion. It also declares a padding at the end to avoid next field
> + * falling into this cacheline.
> + */
> +#define __cacheline_group_end_aligned(GROUP, ...)		\
> +	__cacheline_group_end(GROUP) __aligned(sizeof(long));	\
> +	struct { } __cacheline_group_pad__##GROUP		\
> +	__aligned((__VA_ARGS__ + 0) ? : SMP_CACHE_BYTES)
> +
>   #ifndef CACHELINE_ASSERT_GROUP_MEMBER
>   #define CACHELINE_ASSERT_GROUP_MEMBER(TYPE, GROUP, MEMBER) \
>   	BUILD_BUG_ON(!(offsetof(TYPE, MEMBER) >= \

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>