Received-SPF: pass (google.com: domain of linux-kernel+bounces-212247-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 12 Jun 2024 13:55:36 -0700
From: Jakub Kicinski <kuba@kernel.org>
To: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: Alexander Lobakin <aleksander.lobakin@intel.com>,
 <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>
Subject: Re: [PATCH iwl-next 01/12] libeth: add cacheline / struct alignment
 helpers
Message-ID: <20240612135536.08c2eb34@kernel.org>
In-Reply-To: <8aa33911-5e34-4a03-90de-81f42648ab5d@intel.com>
References: <20240528134846.148890-1-aleksander.lobakin@intel.com>
	<20240528134846.148890-2-aleksander.lobakin@intel.com>
	<20240529183409.29a914c2@kernel.org>
	<8aa33911-5e34-4a03-90de-81f42648ab5d@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 12 Jun 2024 12:07:05 +0200 Przemek Kitszel wrote:
> 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.

I almost want to CC Linus on this because I think it's mostly about
personal preferences. I dislike the struct_group()-style macros. They
don't scale (imagine having to define two partially overlapping groups)
and don't look like C to my eyes. Kees really had to do this for his
memory safety work because we need to communicate a "real struct" type
to the compiler, but if you're just doing this so fail the build and
make the developer stop to think - it's not worth the ugliness.

Can we not extend __cacheline_group_begin() and __cacheline_group_end()
-style markings?