Received-SPF: pass (google.com: domain of linux-kernel+bounces-88555-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <77d3f074-8cb6-466b-ab31-a7b0bac659d0@intel.com>
Date: Fri, 1 Mar 2024 15:30:03 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] netdev: Use flexible array for trailing private bytes
Content-Language: en-US
To: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>, Kees Cook <keescook@chromium.org>,
	"David S. Miller" <davem@davemloft.net>, Paolo Abeni <pabeni@redhat.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>, "Gustavo A. R. Silva"
	<gustavoars@kernel.org>, <netdev@vger.kernel.org>,
	<linux-hardening@vger.kernel.org>, Simon Horman <horms@kernel.org>, "Jiri
 Pirko" <jiri@resnulli.us>, Daniel Borkmann <daniel@iogearbox.net>, Coco Li
	<lixiaoyan@google.com>, Amritha Nambiar <amritha.nambiar@intel.com>,
	<linux-kernel@vger.kernel.org>
References: <20240229213018.work.556-kees@kernel.org>
 <20240229225910.79e224cf@kernel.org>
 <CANn89iKeJGvhY0K=kLhhR39NVbaizP2UBk0Vk0r_XCe2XMBZHg@mail.gmail.com>
 <9050bdec-b34a-4133-8ba5-021dfd4b1c75@intel.com>
 <CANn89iLSLPn5PyXNQcA2HO0e5jGYvHKhTh_9_EMmfbTJKZPfbg@mail.gmail.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <CANn89iLSLPn5PyXNQcA2HO0e5jGYvHKhTh_9_EMmfbTJKZPfbg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RE9BMDVHZllOSUdaMVd1WWhpd2k3dStFZTlaY3h3aThsOEtEbzJNUTZVUEdi?=
 =?utf-8?B?aDZLeUhrZDNlazNEcXRreElDL2ZHb29BblhOaUlXZEt2ajZ5MWt5MUJybmhC?=
 =?utf-8?B?dnp6V2Z2L2dqSGVRdzFGSXBXUUhuTDZLMHkvN0MyQjFMVnlVb2lsRnQybEVt?=
 =?utf-8?B?VTQwdHk5YjVpa1VzNE5yY3FRNmY0RmozT3FjeDQvK0g0OVIvOXN3K1p0c1Zt?=
 =?utf-8?B?akZOSjRPNFo2bGo4aXF4WlAySFJwTjZ3a1cvZkpqVlpHUU1uZW5QMkdtSm1Y?=
 =?utf-8?B?dloyamdTdytkSVBZV3RJVzM1aURVZENNdmpBd1UyZXR3dG9aOFM2b1I5dmxV?=
 =?utf-8?B?ZEpPN2xoazRTYjRvUWhOMFMvaU5uaWpmMEdaUU1LQ3FpWnE4RzNNMnZPdmky?=
 =?utf-8?B?eFFRby9NVkNLMzExb0lQWmtoRmk5YTE1WjFtM3RMSjZ6L3NLSGdIL051NFZZ?=
 =?utf-8?B?SzN5U1R2TDB6VzlrTFdudnNOUVo5V1BGVUJ2Qi94MHpSNUZiZms2VjgyMGZj?=
 =?utf-8?B?RFdCUXZCTjlsNWkxN2RjTVRiRlZhMEZKZkxzdlNTOVZGT3JMSkZkMjZUMkVs?=
 =?utf-8?B?dVpKaXFubHR0KzdkeVlsVmxENVRpcWFmVWlRVzlacEtKakplNFdJbFlyOWhE?=
 =?utf-8?B?RFR4TmVSTlFUV0xvNG1Vbzd6UGMrS0xvUUZ6Q0Y2Qk5sbGN6a29jNk8vQlFD?=
 =?utf-8?B?YStZVHNPVnc5OElyNm5FbHhyeFppUDJORmtzYjNYKzFkU3pUOXJ3MzZmYnV2?=
 =?utf-8?B?cWtHRHBadFRGZWQ0aWkrK1VENXdDV25CV2IxclNySzNGY2tQZytrby83Szc2?=
 =?utf-8?B?SzFjM09XeUlBSGpxdzBFOEdkS0ZiMngwcjEvMmtHcG4vQ2Rza3BmaEZFOVdw?=
 =?utf-8?B?VXpaYXhZSGJMMG5rVUUrUGpYMFVIc0k2akFHZ3R0dUljUTdCcWNwWGlSVFIy?=
 =?utf-8?B?S2tkaFcwRVU5SEhaQXJQaC9oMkNHNUxtK0J3Qmpva0FaRytER3RBNTBhQW01?=
 =?utf-8?B?NlFZL3FybFZoazU0V0JUWTNOK2xDSjVyRDJjR2wwSElKdlhGTWpjMklodGp2?=
 =?utf-8?B?T3lRNFd6dW1MTmE4NGxOc2lIVm9jM2x3ejA1aHFlMFdlWEtocDUvSUs3ZVdS?=
 =?utf-8?B?UzVGaUN3YVp2aStZaWl6OEpucEk1aFJXUVljbHV5R3JCQVBIQUxGWHJEc0pH?=
 =?utf-8?B?d3lLTzl6SEgwQXdERlhWNlhlUFREY2xUc1Jiak84VUgvTUFBMmFWY2pDVUJk?=
 =?utf-8?B?WnR5cDczS2xpMlBPRHdJZzMvOTJmZlljeFhuNTg1WjQxVk84UGpnMm0reUZB?=
 =?utf-8?B?dm9icmZudGV5MDIycEVkN29EaUYxbjZxeUdaSGJMZ3Z5TWpKQXI4M0prQnM1?=
 =?utf-8?B?bjVJcUtTSjZHbWk3SnZWczkyRzlhSnRtV3hWYm1HQnkvTzRGNFgrci9GUnlz?=
 =?utf-8?B?OUQxWTdmdHZiaGd1V3ZYY1VmYmk3aVJrbjNaZGRUQUlFN2dXMWV3T2RhbVQr?=
 =?utf-8?B?b2FXamFLSmJNbXlLL3hNSXVKZ2l0MEROT3VnVUhPVjl0YXR5cFoyblVjM1M2?=
 =?utf-8?B?akRDRGZYaWs1Ujh6MklQQ0FlS3QwMmlCQXEvcUVpWDJyZXBZRlRiRUZ0NUl5?=
 =?utf-8?B?R2llZmlKTDJwQUVmb2wrbmZuRzFMV1VOTEx4OGtOaHFTYURzYmNKSVMzTGF4?=
 =?utf-8?B?dmowdFluVE5vbkphQm5Qa0hkTUZvbnppQUpVK2UxeTE1TlpQdis0aEh1bDV2?=
 =?utf-8?B?WkFaZmU3YkJLdGpQL3ZzWUdmc0MwczFaQVBhS051b1Fiam56dXJQTE56SHhL?=
 =?utf-8?B?dlYxcnJzcXl0MENCT1hvdWNQNjF2RG95OEtPbUM1eTNKQzF4bnVUZXp5ZG1Z?=
 =?utf-8?B?WnltOHlCWDV3aUwxTWhlNXpRamVtbzdTaEdnRVhkODU5UExVR09kYUVHcFZS?=
 =?utf-8?B?Q3NvSnE1dVMrajhWdGRONnM0UlR5WEZ6WCt0Z3Y3NjdtNXovOXlXN29KajVM?=
 =?utf-8?B?VkJnY3E0dTNyaS9RM29tc0ppLzJRQmZ1V3ZGYUFwc1FDdVdOanBPclhIMld5?=
 =?utf-8?B?azVJWVRzaTZSeGszZXdmdHZ0ZWlpZFEzQnlvMVBJT0E3bUNWdnhTczRMNnVx?=
 =?utf-8?B?YVY2RTlaUWdIcytPTE5MUy9Tb243c2R3NWsvekg5b0t2cDZMeTBmR09OTnRY?=
 =?utf-8?B?NHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 44bf8f3e-640d-4d6f-a703-08dc39fc2db8
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Mar 2024 14:30:44.3510
 (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: UTUTB0KyF4gF0gCZlzxmESGW+oN1OdyGKH2SytCPLVovecMyiasP/M+1igbb+B1KraO+ght9ZxD34MVNzP8b3TQB6KosuPmZjSq1YjmcTYo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7737
X-OriginatorOrg: intel.com

From: Eric Dumazet <edumazet@google.com>
Date: Fri, 1 Mar 2024 14:25:37 +0100

> On Fri, Mar 1, 2024 at 1:59 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Fri, 1 Mar 2024 09:03:55 +0100
>>
>>> On Fri, Mar 1, 2024 at 7:59 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>>>
>>>> On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
>>
>> Re WARN_ONCE() in netdev_priv(): netdev_priv() is VERY hot, I'm not sure
>> we want to add checks there. Maybe under CONFIG_DEBUG_NET?
>>
>>>
>>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>> index 118c40258d07..b476809d0bae 100644
>>>>> --- a/include/linux/netdevice.h
>>>>> +++ b/include/linux/netdevice.h
>>>>> @@ -1815,6 +1815,8 @@ enum netdev_stat_type {
>>>>>       NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
>>>>>  };
>>>>>
>>>>> +#define      NETDEV_ALIGN            32
>>>>
>>>> Unless someone knows what this is for it should go.
>>>> Align priv to cacheline size.
>>>
>>> +2
>>>
>>
>> Maybe
>>
>>> #define NETDEV_ALIGN    L1_CACHE_BYTES
>>
>> #define NETDEV_ALIGN    max(SMP_CACHE_BYTES, 32)
> 
> Why would we care if some arches have a very small SMP_CACHE_BYTES ?

Oh sorry, I thought %SMP_CACHE_BYTES is 1 when !SMP.
We can then just add ____cacheline_aligned to both struct net_device and
its ::priv flex array and that's it.

I like the idea of declaring priv explicitly rather than doing size +
ptr magic. But maybe we could just add this flex array to struct
net_device and avoid introducing a new structure.

> Bet it !
> 
> IMO nothing in networking mandates this minimal 32 byte alignment.
> 
>>
>> ?
>>
>> (or even max(1 << INTERNODE_CACHE_SHIFT, 32))
> 
> I do not think so.
> 
> INTERNODE_CACHE_SHIFT is a bit extreme on allyesconfig on x86 :/
> (with CONFIG_X86_VSMP=y)
> 
> 
>>
>>>
>>> or a general replacement of NETDEV_ALIGN....
>>>
>>>
>>
>> + I'd align both struct net_device AND its private space to
>> %NETDEV_ALIGN and remove this weird PTR_ALIGN. {k,v}malloc ensures
>> natural alignment of allocations for at least a couple years already
>> (IOW if struct net_device is aligned to 64, {k,v}malloc will *always*
>> return a 64-byte aligned address).
> 
> I think that with SLAB or SLOB in the past with some DEBUG options
> there was no such guarantee.
> 
> But this is probably no longer the case, and heavy DEBUG options these
> days (KASAN, KFENCE...)
> do not expect fast networking anyway.

Thanks,
Olek