Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <994c4c2e-63c1-40ab-a270-325c8a5f5f39@intel.com>
Date:   Wed, 29 Nov 2023 14:12:31 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v5 01/14] page_pool: make sure frag API fields
 don't span between cachelines
Content-Language: en-US
To:     Yunsheng Lin <linyunsheng@huawei.com>
CC:     Paul Menzel <pmenzel@molgen.mpg.de>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        <netdev@vger.kernel.org>, Alexander Duyck <alexanderduyck@fb.com>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        <linux-kernel@vger.kernel.org>, Eric Dumazet <edumazet@google.com>,
        Michal Kubiak <michal.kubiak@intel.com>,
        <intel-wired-lan@lists.osuosl.org>,
        David Christensen <drc@linux.vnet.ibm.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>
References: <20231124154732.1623518-1-aleksander.lobakin@intel.com>
 <20231124154732.1623518-2-aleksander.lobakin@intel.com>
 <9902d1c4-5e51-551a-3b66-c078c217c5ad@huawei.com>
 <5e6859d3-d3e7-44c1-acee-2c4ec568615d@intel.com>
 <bd35cf74-698f-e811-43be-af207a88fdc7@huawei.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <bd35cf74-698f-e811-43be-af207a88fdc7@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NkVXY1ZzdUtmUER5TVJFWEpIWGdNVC8vdHB3NWFXTmFnMml0YjdQaTFQajNP?=
 =?utf-8?B?blZVbGV6SXl4RmRJbG9uNk1KMldNNW9BNlBoQXBTSjBqM1VYUSs3UWtpYkZ0?=
 =?utf-8?B?QjhzemZMSFBUcldOM09HZEVkVlk3M0lFYkNrcHc4L1A4TVNuTWpnOFpxZG5S?=
 =?utf-8?B?N1ZZYS9yQi8rc3NqK2g4UUtYZmJZZ2N0OXFGckVOUmhNdGZrWGhzVkMzZlZ4?=
 =?utf-8?B?L3pvR0ttTHM4dlFORXFpSTYvcXZINEJZTzJraXVKdkRua2swUEtlK1d6L2lV?=
 =?utf-8?B?MnlWdUZISEoxSk84aENIU3JFbTJNZk93S0xqQlBKTGJQQm5lcE5LWGRCanYz?=
 =?utf-8?B?NzZBR29kVVkwVnRDZzJIaWg4eWRSRXBDSmhhS1cvTHlLREhGSmdBS29oWURK?=
 =?utf-8?B?c3Iwa2kyTEN3UDhKQTlyZGJUUzZoaGk4NkZzSExxUjRKTklhWkFCbEdYMEhx?=
 =?utf-8?B?UW1EU1hWK0IrTVphUjNOMUs2b0t4S2VDMUNuQm15aUFObUhWUFExMU5CbjNq?=
 =?utf-8?B?QU90SlJqL1AzOU40cVhPZlpsK2t3Zy92ZjY2ZnhSVzg4U1dUdFoya0NzTlRS?=
 =?utf-8?B?WE9vdklMdzV3ckdoMnk5cTQ2blRzOFVCNW9oL1lvT0pVOXA5ejNjT29hR20r?=
 =?utf-8?B?MDF3ekhPdnhvaUtJZ0Rjcm5lUU82Z0gwOHY4QkhjN0RyVElwc0VldVhkZlZm?=
 =?utf-8?B?dkVQRlI4Z3VQUUJxV0l1QVRuMmlyU1R1NnA0QVdlUElzTUJlM2dTTTZWbFJx?=
 =?utf-8?B?NU5iMEs1VnEwTzJEakFtcDJOTEVqVzJGaW42MzNIOTRhelVhZFBRdWdiWDJq?=
 =?utf-8?B?dC90d0RXRm5KbGlWQ2RWV00wVytaOGd1M3pFU1F5OVVLSEZjWmJmS1B3ckNP?=
 =?utf-8?B?azg0ajY3M3p6cHJPRFVXdVJIUVp5Uk9ySHhVcjJQakpYbzY3UURUa0pxc2tH?=
 =?utf-8?B?ZDBNSUR0eXgyYS9lMllON2dyRVg5cEVFWnhtRGhRVnU2ZGZyNGNoeVZxSHdt?=
 =?utf-8?B?ZFdiSmRhazF0UlZMU284MmYrdWg0WnFnM0J6RjB2azNkVjB5Qkt1L1hrWE1M?=
 =?utf-8?B?QTlKTjY3bDN3OVdMTXlDRGJwWHZ5Vkt1WkFXODFZVHNxWnpkNnc0M2c3bno5?=
 =?utf-8?B?bHJqTG1SbVIwYldCV3ZrQnc2dmcrdEVsRFlPSWYzVlpuNVdNN2RTWk5XZEF0?=
 =?utf-8?B?TVhMSXZGNmxydERRcytXZlRCbjAzU0xLUTg2MlRiZjRFVWtUOUNxQUJOa09I?=
 =?utf-8?B?aVpHbE5OZVFNL0hyUzNMVHV3em0vMS9iNnIwdFJFeFJXbnAyMWV2VEFpODBD?=
 =?utf-8?B?b1VQQlFab0EvOW4xNWlpL3EvampQSlErcTlIMjdUWVB6V2t2eDdKaUdpb1hj?=
 =?utf-8?B?YldZYmFLVmRvWlFWU2V5T0pOdGNlc0MrY2NGRXNTSmNLaDVJWVQ2NUlDdjA5?=
 =?utf-8?B?MHpmdHlqenVib0JMWGI0cCtIa1BxZ0lObHNQSWFGb0pIQmJqN2NMK014M2NJ?=
 =?utf-8?B?cmQwREx4QWRTQTUzam5kb2ZpZDdEREFPUWR5bzhOdzdrd3JhaUxlVWx0Rm0x?=
 =?utf-8?B?NHYzQUN0NWtRbFY0dDFya3F5UXl6QmNnMXQxanVjV25OYVRIVkhTamxLNy9D?=
 =?utf-8?B?TFdId2JQcm1Zb3h5bEQvK1g0ZnQ0czBHMEZDaUtwUUdGMGNYbG9RbTdRWXNB?=
 =?utf-8?B?Q3EzQTJERU5hVVNYREdvTGNpTFJWUGwyTEhBN0hkb1VjOXdwd0gzYVJDbkIr?=
 =?utf-8?B?NUdycnladHkrLzd4UUFwR1NTUGlNYmtyWjFRVDM2aUxCalNCaityMFM3bGFR?=
 =?utf-8?B?WG1qUlUyQU5DOVZMaFlULzZUaS9CVjEwZEZIU2U0YTdmNzN3SjF4NGJoNXRD?=
 =?utf-8?B?OXJwRnVveE5tak9heERReGtWZXNiSWVqRTNodkxnODZ3RlRDV2pIZ2FVMUdK?=
 =?utf-8?B?QmxrWnZFOG9pYWV2U3hOY0U5Skk3bTIwdkNnb081bkFRTDh6aWthWmJBN3FL?=
 =?utf-8?B?UFl1ek82WWVnZ0FBb0l6QVhzMUxBUVNZSDU2L0xYVkg4YmFQeER4U2VWU1o0?=
 =?utf-8?B?eGdyZTlnNFhTekw4N1M2WjZ4WmxJWTlQSjQweFlxVEJhcHBsbW8rZEoydW0w?=
 =?utf-8?B?R0U1YWZTUUFEa3VSZ01haHZib1EvcXVheHNJbUdnVFBCWGRsWlZGSHJqalpo?=
 =?utf-8?B?M2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ddb7a98-186e-4570-3188-08dbf0dcf167
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Nov 2023 13:13:14.4304
 (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: x3S+vCtDissSpYxXdl56g76ODMCl69UnOhxJXo5Lcp2QYZ6RjyYNbGpWYRjB5QojtTaaOs5nwbYvRwNlE0XAoJAV4pOkGFCbrlLDpZrtLvk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4832
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 29 Nov 2023 05:13:55 -0800 (PST)

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Wed, 29 Nov 2023 10:55:00 +0800

> On 2023/11/27 22:08, Alexander Lobakin wrote:
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>> Date: Sat, 25 Nov 2023 20:29:22 +0800
>>
>>> On 2023/11/24 23:47, Alexander Lobakin wrote:
>>>> After commit 5027ec19f104 ("net: page_pool: split the page_pool_params
>>>> into fast and slow") that made &page_pool contain only "hot" params at
>>>> the start, cacheline boundary chops frag API fields group in the middle
>>>> again.
>>>> To not bother with this each time fast params get expanded or shrunk,
>>>> let's just align them to `4 * sizeof(long)`, the closest upper pow-2 to
>>>> their actual size (2 longs + 2 ints). This ensures 16-byte alignment for
>>>> the 32-bit architectures and 32-byte alignment for the 64-bit ones,
>>>> excluding unnecessary false-sharing.
>>>>
>>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>>> ---
>>>>  include/net/page_pool/types.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
>>>> index e1bb92c192de..989d07b831fc 100644
>>>> --- a/include/net/page_pool/types.h
>>>> +++ b/include/net/page_pool/types.h
>>>> @@ -127,7 +127,7 @@ struct page_pool {
>>>>  
>>>>  	bool has_init_callback;
>>>
>>> It seems odd to have only a slow field between tow fast
>>> field group, isn't it better to move it to the end of
>>> page_pool or where is more appropriate?
>>
>> 1. There will be more in the subsequent patches.
>> 2. ::has_init_callback happens each new page allocation, it's not slow.
>>    Jakub did put it here for purpose.
>>
>>>
>>>>  
>>>> -	long frag_users;
>>>> +	long frag_users __aligned(4 * sizeof(long));
>>>
>>> If we need that, why not just use '____cacheline_aligned_in_smp'?
>>
>> It can be an overkill. We don't need a full cacheline, but only these
>> fields to stay within one, no matter whether they are in the beginning
>> of it or at the end.
> 
> I am still a little lost here, A comment explaining why using '4' in the
> above would be really helpful here.

The block is: 2 longs (users, frag pointer) and 2 ints (offset, cnt).
On 32-bit architectures, longs == ints, so that this effectively means 4
longs.
On 64-bit architectures, long is 8 bytes and int is 4, so that the value
becomes 2 * 8 + 2 * 4 = 24, but the alignment must be a pow-2. The
closest pow-2 to 24 is 32, which equals to 4 * 8 = 4 longs.
At the end, regardless of the architecture, the desired alignment would
end up as 4 * longs. As I wrote earlier, we could do something like

__aligned(roundup_pow_of_2(2 * sizeof(long) + 2 * sizeof(int)))

but doesn't that seem ugly as hell?

As I replied to Jakub, I'll add a comment in the code (so that you
wouldn't need refer to the Git history / commit message) in the next
version.

Thanks,
Olek