netdev - Re: [PATCH iwl-next v2 2/9] ice: use cacheline groups for ice_rx

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] [day] [month] [year] [list]

Message-ID: <8f2640b7-0b11-47c9-a43b-870f4613dd69@intel.com>
Date: Fri, 7 Nov 2025 01:55:09 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Simon Horman <horms@...nel.org>
CC: Aleksandr Loktionov <aleksandr.loktionov@...el.com>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-next v2 2/9] ice: use cacheline groups for ice_rx_ring
 structure

On 11/6/2025 9:23 AM, Simon Horman wrote:
> On Wed, Nov 05, 2025 at 01:06:34PM -0800, Jacob Keller wrote:
>> @@ -298,10 +302,22 @@ struct ice_rx_ring {
>>  #define ICE_RX_FLAGS_MULTIDEV		BIT(3)
>>  #define ICE_RX_FLAGS_RING_GCS		BIT(4)
>>  	u8 flags;
>> -	/* CL5 - 5th cacheline starts here */
>> +	__cacheline_group_end_aligned(cl4);
>> +
>> +	__cacheline_group_begin_aligned(cl5);
>>  	struct xdp_rxq_info xdp_rxq;
>> +	__cacheline_group_end_aligned(cl5);
>>  } ____cacheline_internodealigned_in_smp;
>>  
>> +static inline void ice_rx_ring_struct_check(void)
>> +{
>> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl1, 64);
>> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl2, 64);
>> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl3, 64);
>> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl4, 64);
>> +	CACHELINE_ASSERT_GROUP_SIZE(struct ice_rx_ring, cl5, 64);
> 
> Hi Jacob,
> 
> Unfortunately the last line results in a build failure on ARM (32-bit)
> with allmodconfig. It seems that in that case the size of the group is
> 128 bytes.
> 
Ok. I got a cross compilation working which actually built with 128-byte
cache size. Here's the relevant part of the structure layout:

>         /* --- cacheline 8 boundary (512 bytes) --- */
>         struct {
>         } __cacheline_group_pad__cl4 __attribute__((__aligned__(128)));              /*   512     0 */
>         __u8                       __cacheline_group_begin__cl5[0] __attribute__((__aligned__(128))); /*   512     0 */
>         struct xdp_rxq_info        xdp_rxq __attribute__((__aligned__(128))); /*   512   128 */
> 

The xdp_rxq_info structure is aligned to 128 bytes, due to the
____cacheline_aligned attribute.

>         /* XXX last struct has 104 bytes of padding */
> 
>         /* --- cacheline 10 boundary (640 bytes) --- */
>         __u8                       __cacheline_group_end__cl5[0] __attribute__((__aligned__(4))); /*   640     0 */

Due to the padding, this results in the group end being placed after the
104 bytes of padding. This is because the member struct has the padding,
rather than the ice_rx_ring structure.

>         struct {
>         } __cacheline_group_pad__cl5 __attribute__((__aligned__(128)));              /*   640     0 */
> 
>         /* size: 640, cachelines: 10, members: 48 */
>         /* sum members: 263, holes: 6, sum holes: 377 */
>         /* paddings: 1, sum paddings: 104 */
>         /* forced alignments: 18, forced holes: 5, sum forced holes: 373 */
> } __attribute__((__aligned__(128)));

I'm not sure what the most ideal solution here is. I think the simplest
is to use SMP_CACHE_BYTES for the last cacheline size assertion, since
this one will always scale to the size of the cacheline due to the
__cacheline_aligned attribute.

Does that seem reasonable? I guess I could make them all
SMP_CATCH_BYTES.. but I feel like that makes them a bit less clear. The
other groups don't have such varying sizes as the
CACHELINE_ASSERT_GROUP_SIZE doesn't include the padding added by the
cacheline_group_end, and using SMP_CACHE_BYTES would mean new members
could pass on 128-byte cache size systems even if we exceeded the
intended limit of 64bytes. Then again, on 128-byte cache size systems
the structure does grow to 640 bytes due to the padding which is keeping
the cache groups on dedicated cache lines.. We could drop the assertions
entirely too I suppose...

Another option is to remove the cacheline group from around the
xdp_rxq_info struct entirely, since it will get a separate cacheline due
to cacheline_aligned...

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)