linux-kernel - Re: [PATCH iwl-next v2 01/14] cache: add __cacheline_group_{begin,end}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <38875747-d728-4784-b8da-057d999e1fac@intel.com>
Date: Thu, 20 Jun 2024 17:15:53 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, "Paolo
 Abeni" <pabeni@...hat.com>, Jacob Keller <jacob.e.keller@...el.com>, "Mina
 Almasry" <almasrymina@...gle.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@...el.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH iwl-next v2 01/14] cache: add
 __cacheline_group_{begin,end}_aligned() (+ couple more)

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@...el.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@...el.com>