Received-SPF: pass (google.com: domain of linux-kernel+bounces-211325-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <8aa33911-5e34-4a03-90de-81f42648ab5d@intel.com>
Date: Wed, 12 Jun 2024 12:07:05 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH iwl-next 01/12] libeth: add cacheline / struct alignment
 helpers
To: Jakub Kicinski <kuba@kernel.org>, Alexander Lobakin
	<aleksander.lobakin@intel.com>
CC: <intel-wired-lan@lists.osuosl.org>, Tony Nguyen
	<anthony.l.nguyen@intel.com>, "David S. Miller" <davem@davemloft.net>, "Eric
 Dumazet" <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, Mina Almasry
	<almasrymina@google.com>, <nex.sw.ncis.osdt.itp.upstreaming@intel.com>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240528134846.148890-1-aleksander.lobakin@intel.com>
 <20240528134846.148890-2-aleksander.lobakin@intel.com>
 <20240529183409.29a914c2@kernel.org>
Content-Language: en-US
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <20240529183409.29a914c2@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q3BVZDhNaU0wckl2S1phVW5JaUdHTXc4eXpvZmJZdHN5QkZjNi9waDhEOVl0?=
 =?utf-8?B?MmF3NTRrdjNrUDNJc3J4WFFhcFUyWTV0YzhVWjMyMjFlMUtrNlBsc08zOFhl?=
 =?utf-8?B?QnNaYWZCZ0pic1JjWmZsUjU4VEcwRWNQcVVuZXZ3WC9xTTJRZkgyd0VjcTM0?=
 =?utf-8?B?ZW42S3JWWjkvalBJTlVBRkwxS1BwN25NbEYzMGFqOVpKcVI4ZGhnYU1YalRT?=
 =?utf-8?B?WEdvbm4wQ3Y3M0I1VkNMQVV5anlFVHRTQ3ROUDl4R3A1ZU1abW1YTm1lcCtr?=
 =?utf-8?B?R21vTW5sT0FubTlxMkNzZmxrYm9NUWd0M0NrdkptU2FOVTVreFVZVkVIaDhs?=
 =?utf-8?B?VmVVTzNtcGFtRTFMd0lRVHk4eGQrSXNpYllxSTFYTWl4ODlMcDFESkNuVTY0?=
 =?utf-8?B?VXZjQmU3dm83V0QzdU5sYlBRVkpId2dXV3k1MSt5ZUIzUkp5Nndnc0NNMnJp?=
 =?utf-8?B?QVNNamZsYUNrcmVIMlIvT3gwNC9zbFpUeFlFSFFaOEVFVCswNXRiNTZUeUpy?=
 =?utf-8?B?TGF0S1Fia1NTR0xadE5TZWNVODBtUERKb2dBUFdZR2k5aXZ1NURmWUF5U2k0?=
 =?utf-8?B?dFZ4NnA4YVZZdG4yYnJjR3NpS3VmNy9vWi96YTB4bXpGTVNPbTBTRGdvU1Qy?=
 =?utf-8?B?bjIvTVh5elRtaUhMQURtWEZTdFFqNEowc2tkUitoTGRFd0p1WjRzOERQSXM3?=
 =?utf-8?B?cE9YWmNYTnpkVElLZVVvT2poN1ZPUHdMZjJSVmEzcWpqRkRDamwyWUdzUkZB?=
 =?utf-8?B?V0dNZ21rLy9lRkxCQVlRbUVwbm0wdmJnM0xZNlR6N2RsOXlmSUlSeU9Welhr?=
 =?utf-8?B?ejhVWFRhODVyYVR0Q1VwR1JzUndFREFPcFd0dFFScFhTWG1QWWt4eVZhMXVX?=
 =?utf-8?B?ZjRzNEJHVTBmZ3FTM0RuWUxZOTBhZFUzOHZ0YzlOcEJsQ2FFdmNNMEc3SWhS?=
 =?utf-8?B?SHdQdXJnK3lDRXI2bm9mSGM3RlRDWHlhRFNYd05KcXRYRXlqZG5JMGxtb3FG?=
 =?utf-8?B?ZXJ4NEREU2dqcStQMjNjbE1jZzRFZTdhUVZaWUs2eVJ4bUdlMGF6UVVIWmNv?=
 =?utf-8?B?Z2lWU1N1WEpFSWJtRE0xOXlkWnJJa1FmY3RXbUx6ejJYL2o3Zklxa1NGNjFI?=
 =?utf-8?B?bThYSXplM3dabFNUYXBEOGNFVVJlY1lFc0JXck1CejdoNkpvMXFKNlNtSTVG?=
 =?utf-8?B?aUJEYzhGL3RvRktjMWhXWTMxNFM0R0NGaTQrRFV6QTlMWWswY3RDOElnenl3?=
 =?utf-8?B?cU5MejVlQ1ZpWHA4ZXNLVWtRTDNRdWhHZEwvdlpQd1hpQU13eDZ2QVlEMHNp?=
 =?utf-8?B?S3paS1BQRFd3dEg2aGV0UTJ1Uy9zTEVOVzFZNFRnYWovRDUxN05Jd21qVEpM?=
 =?utf-8?B?VUtqL3BYTkluclpHWWtNUlBWMitxcVg0ZmRBRjNON2ovTlU2VGcxUVJZV1RC?=
 =?utf-8?B?eU9ML1RwdUhRdE5ZRmNYTm83YXhha2RHbkliY0p1RkVyeE9kZ29oSGdlbVA4?=
 =?utf-8?B?OFIwcVFpakZkT0hnZ251QloxaGRrY3lYWG9zTmhSajJBTkU1RXFYbFN2OVM3?=
 =?utf-8?B?aVNGTU1zVC8xc2wyaVBHVi9TS3E0eUpHWkNDaTRFVmVIbkluQTJCdnNiUFB6?=
 =?utf-8?B?dXc4N1hzTWROa0tWcXdjc25TMTgrN3BWSjFLVmM1SVQrNDIyd3dsTDJkNkVt?=
 =?utf-8?B?c2tBQ3lNQ2tNdzZqQUM3TVFXcXVFSHQrdUhONXQyTGVDdElWb1ZnNGRSYmpM?=
 =?utf-8?B?cTdUWkZTVXBkUzc0NGhDSW01TVE4aDRCUWNvVHBGbE81MjR1ZlhIemlpYzUw?=
 =?utf-8?B?dkQrK3orNmZRdTYxeDFsbUhoS2xhRm4yTDV0SWdyTWFpSk9scEUzZmx5SjRN?=
 =?utf-8?B?aHEzR01XdHFPSXJUSzRmVkFMVld6ZFFCL0hpVmJidmtNUkRNcTBOYWE3U1d3?=
 =?utf-8?B?bWFLZytDY1NsVWRtQ0grVU5wdG9ISUtWWlYrOWNDbHFuUjhoQUV3Y3R1VWNh?=
 =?utf-8?B?cTFQa1NuVlZ1SEc3S0pTT0k3YjRjZU0ycmZaR0RBMlNleTg3dDhuV2E1VzlF?=
 =?utf-8?B?bjA1ckZtTjlhZTJHakF0OGFIZ2Rkako4KzhkQTBaWUhZdStFTFZKK1M1MGhu?=
 =?utf-8?B?MGhxVTlNT3YxaUI2WEJhNEV4cFFLWm1SSERRSEdzRHNYc0ZWV2xsWHJqOS9i?=
 =?utf-8?B?Q3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a4669c87-3ea5-4c84-149e-08dc8ac76e7b
X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8102.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jun 2024 10:07:13.8216
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JCWhs2hAFheFY7TyvPJ+CLBbK9/ITAkzaJ0oHcYsJpSWJ1tlBFgBd5/pf9A0hhH0sbk/ZUP1mUqhi/xIDHAFB/X44lpXb9Y0bVGRyI5FqKI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7528
X-OriginatorOrg: intel.com

On 5/30/24 03:34, Jakub Kicinski wrote:
> On Tue, 28 May 2024 15:48:35 +0200 Alexander Lobakin wrote:
>> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
>> index 95a59ac78f82..d0cf9a2d82de 100755
>> --- a/scripts/kernel-doc
>> +++ b/scripts/kernel-doc
>> @@ -1155,6 +1155,7 @@ sub dump_struct($$) {
>>           $members =~ s/\bstruct_group_attr\s*\(([^,]*,){2}/STRUCT_GROUP(/gos;
>>           $members =~ s/\bstruct_group_tagged\s*\(([^,]*),([^,]*),/struct $1 $2; STRUCT_GROUP(/gos;
>>           $members =~ s/\b__struct_group\s*\(([^,]*,){3}/STRUCT_GROUP(/gos;
>> +        $members =~ s/\blibeth_cacheline_group\s*\(([^,]*,)/struct { } $1; STRUCT_GROUP(/gos;
>>           $members =~ s/\bSTRUCT_GROUP(\(((?:(?>[^)(]+)|(?1))*)\))[^;]*;/$2/gos;
>>   
>>           my $args = qr{([^,)]+)};
> 
> Having per-driver grouping defines is a no-go.

[1]

> Do you need the defines in the first place?

this patch was a tough one for me too, but I see the idea promising

> Are you sure the assert you're adding are not going to explode
> on some weird arch? Honestly, patch 5 feels like a little too
> much for a driver..
> 

definitively some of the patch 5 should be added here as doc/example,
but it would be even better to simplify a bit

--

I think that "mark this struct as explicit split into cachelines" is
a hard hard C problem in general, especially in the kernel context, 
*but* I think that this could be simplified for your use case - split
into exactly 3 (possibly empty) sections: mostly-Read, RW, COLD?

Given that it will be a generic solution (would fix the [1] above),
and be also easier to use, like:

  CACHELINE_STRUCT_GROUP(idpf_q_vector,
	CACHELINE_STRUCT_GROUP_RD(/* read mostly */
		struct idpf_vport *vport;
		u16 num_rxq;
		u16 num_txq;
		u16 num_bufq;
		u16 num_complq;
		struct idpf_rx_queue **rx;
		struct idpf_tx_queue **tx;
		struct idpf_buf_queue **bufq;
		struct idpf_compl_queue **complq;
		struct idpf_intr_reg intr_reg;
	),
	CACHELINE_STRUCT_GROUP_RW(
		struct napi_struct napi;
		u16 total_events;
		struct dim tx_dim;
		u16 tx_itr_value;
		bool tx_intr_mode;
		u32 tx_itr_idx;
		struct dim rx_dim;
		u16 rx_itr_value;
		bool rx_intr_mode;
		u32 rx_itr_idx;
	),
	CACHELINE_STRUCT_GROUP_COLD(
		u16 v_idx;
		cpumask_var_t affinity_mask;
	)
);

Note that those three inner macros have distinct meaningful names not to
have this working, but to aid human reader, then checkpatch/check-kdoc.
Technically could be all the same CACHELINE_GROUP().

I'm not sure if (at most) 3 cacheline groups are fine for the general
case, but it would be best to have just one variant of the
CACHELINE_STRUCT_GROUP(), perhaps as a vararg.