netdev - Re: [RFC net-next 1/1] idpf: Don't hard code napi

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: <c32620a8-2497-432a-8958-b9b59b769498@intel.com>
Date: Mon, 30 Sep 2024 14:38:56 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Joe Damato <jdamato@...tly.com>
CC: <netdev@...r.kernel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "moderated list:INTEL
 ETHERNET DRIVERS" <intel-wired-lan@...ts.osuosl.org>, open list
	<linux-kernel@...r.kernel.org>
Subject: Re: [RFC net-next 1/1] idpf: Don't hard code napi_struct size

From: Alexander Lobakin <aleksander.lobakin@...el.com>
Date: Mon, 30 Sep 2024 14:33:45 +0200

> From: Joe Damato <jdamato@...tly.com>
> Date: Wed, 25 Sep 2024 18:00:17 +0000
> 
>> The sizeof(struct napi_struct) can change. Don't hardcode the size to
>> 400 bytes and instead use "sizeof(struct napi_struct)".
> 
> Just change this hardcode here when you submit your series.
> I use sizeof() here only for structures which size can change depending
> on .config, like ones containing spinlocks etc.
> If you just add one field, it's easy to adjust the size here.
> 
> Otherwise, these assertions lose their sense. They're used to make sure
> the structures are of *certain* *known* size, hardcoded inside them.
> sizeof()s mean nothing, they don't give you the idea of how the
> cachelines are organized after some code change.

struct dim depends on spinlock debug settings, lockdep etc., plenty of
different .config options which can change its size unpredictably.
cpumask_var_t directly depends on CONFIG_NR_CPUS, but it can also be a
pointer if CONFIG_CPUMASK_OFFSTACK. IOW its size can vary from 4 bytes
to several Kbs.

struct napi_struct doesn't have any such fields and doesn't depend on
the kernel configuration, that's why it's hardcoded.
Please don't change that, just adjust the hardcoded values when needed.
Otherwise, if anyone have objections with strong arguments, I'd just
remove these assertions completely.
It's a common thing that if we change some generic structure in the
kernel, we need to adjust some driver code.

> 
>>
>> While fixing this, also move other calculations into compile time
>> defines.
>>
>> Signed-off-by: Joe Damato <jdamato@...tly.com>
>> ---
>>  drivers/net/ethernet/intel/idpf/idpf_txrx.h | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
>> index f0537826f840..d5e904ddcb6e 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
>> @@ -437,9 +437,13 @@ struct idpf_q_vector {
>>  	cpumask_var_t affinity_mask;
>>  	__cacheline_group_end_aligned(cold);
>>  };
>> -libeth_cacheline_set_assert(struct idpf_q_vector, 112,
>> -			    424 + 2 * sizeof(struct dim),
>> -			    8 + sizeof(cpumask_var_t));
>> +
>> +#define IDPF_Q_VECTOR_RO_SZ (112)
>> +#define IDPF_Q_VECTOR_RW_SZ (sizeof(struct napi_struct) + 24 + \
>> +			     2 * sizeof(struct dim))
>> +#define IDPF_Q_VECTOR_COLD_SZ (8 + sizeof(cpumask_var_t))
>> +libeth_cacheline_set_assert(struct idpf_q_vector, IDPF_Q_VECTOR_RO_SZ,
>> +			    IDPF_Q_VECTOR_RW_SZ, IDPF_Q_VECTOR_COLD_SZ);
>>  
>>  struct idpf_rx_queue_stats {
>>  	u64_stats_t packets;

Thanks,
Olek