Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e48185cb-3057-e778-75c4-d266a249088b@intel.com>
Date:   Tue, 18 Jul 2023 15:50:08 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH RFC net-next v2 2/7] net: page_pool: place frag_* fields
 in one cacheline
Content-Language: en-US
To:     Jesper Dangaard Brouer <jbrouer@redhat.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, <brouer@redhat.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        Yunsheng Lin <linyunsheng@huawei.com>,
        Alexander Duyck <alexanderduyck@fb.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        "Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230714170853.866018-1-aleksander.lobakin@intel.com>
 <20230714170853.866018-3-aleksander.lobakin@intel.com>
 <ac97825d-6a27-f121-4cee-9d2ee0934ce6@redhat.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <ac97825d-6a27-f121-4cee-9d2ee0934ce6@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R0hST2tWV0tteHdNSW5tTTJndC9QYXlSWEtXb3JERkZOcGxTdHhEUUdteTZj?=
 =?utf-8?B?RENVcjZNeFNOQVBmRGZZOXM1SnVtdkZNbEdqU3J0N0NaNitHWldkbHZhR3BN?=
 =?utf-8?B?cDJEdVZVNWt6NGF3Vmpqc0YwK2pPdG1iK1dTaC9IVzNLa2NBNEtTM09jT1VG?=
 =?utf-8?B?cjFZMXRTUHdPYVlvSTlLTWJwV1RmeVBvZ2Z6MTk0RHQ0L21HQUZIZ2hqZGY0?=
 =?utf-8?B?RkRlRWtUeHpscHJQL1lFKzBpZGpveW5CN0NjcFR6TXJtV052eFhKTjRWSWxH?=
 =?utf-8?B?VDFwOWVxNDIwSVpwRzRBNHFBN2dRRU5LQ0x5WEtqMXhqVjVqdld0V2oyRlFa?=
 =?utf-8?B?bDBBVHY2djJmN2tveTFQQVZaSlRHMHFxOXBUTUtxWFBXdlBRMkppUE90ZmI2?=
 =?utf-8?B?R3lEQVI4UTJnVk9JSU5qeVY4a00wMkR5TzZBTG1ncEdNVE4wMEVZWWlxTXZk?=
 =?utf-8?B?cC85TUx4WExKeloybzdmMHg0dnNHVno0NkxLK0RkekJvMEZONGtNdHJtVUM2?=
 =?utf-8?B?M3ZTMm9GTzVzOTYwU2lQRWh5ZithNlJxdzVPM2RmRHpHWFBHS3lXR1JGblpp?=
 =?utf-8?B?NWlpbk1ZVUZsS29lZlNWZGpwY1J6RFgzRkVRMTNBckVsQ1FxekVHR3lxRUw4?=
 =?utf-8?B?d0VUcE53c3crRWVNVmNXRittOWlleXNyK1FwNlJQNGJEbUp0QkY4amFxZDgv?=
 =?utf-8?B?M0xuWWR1REFXdXBrNFgxcVFwRkV1U0JZQ295aUpvYmJGS1JmYjJBNHBwMHkv?=
 =?utf-8?B?L0FUQWpJL1NOcHlqMks5NC9Ua3VWYXU2dFloRHlieUE1Y0tYTzFBbzB4MEha?=
 =?utf-8?B?ckJ1UU5sZWdYdWdJV3A5ZktlVEZrUnNjWExLR0pmWk9kTWNvK2ltcExSemhx?=
 =?utf-8?B?Y2NZdFEwQWk1TU44ckdRYXpqelB6RVI5RS9KZjhGUHRzTm9WYU9OdFR6cHk1?=
 =?utf-8?B?T3FQaUNKRGlFNHRxUVdmVk53T1YxQmxUemtVZy9WaDhtZ09laDNIRGt4d3RD?=
 =?utf-8?B?dm9aa3QwVHE0UHBvb1hFdVdwY0tTYWsvYzVyNUpPUGR3WWt1U28rRjc4U1ZL?=
 =?utf-8?B?WG1UUnkyQTZYYmwrM0Zpa2x4ek02d1N1NmhRdDM2Q3N2aHQ5MEdQUFJGUTlP?=
 =?utf-8?B?N2pSOWg2UWN4VFFNNm5HM3BGTmkrWUM5cjFOcWlyb01mdHNKc3gyb2gzcWdE?=
 =?utf-8?B?RXY5TVZETmp0OUtMOWNUOVljMmNTK1VBeEtEbW1hZmdycE5pQnNONnAwZ2JG?=
 =?utf-8?B?c2FLZGQ4cVlDc3B4RXY4ZzFWZ3AvaS96eVFSblhvVlZlMGMxMUttQU1pTHYz?=
 =?utf-8?B?czFrL1hSNjR3N3BaL3VFRzdLTHdWbmxWd0RIRjR3NzJGV042NkNyRmVnRnAy?=
 =?utf-8?B?WUMzM29DUmpnN2NRVUo3T0JFQUtXQktoSTk4SmZlMmU3bVRjYWNBTEdGVHFh?=
 =?utf-8?B?WmM2UGhCdERxYjRwRXVhSU1RU01RejdScGg2UmNOWFlWemtpN25xUkx3ZGxi?=
 =?utf-8?B?S0w2b0Z3ZW9VMHhqOXF6T25DWnczWDRCNDViaDNVMENsb2pmSzZIRUY4blUv?=
 =?utf-8?B?dit1QWJDVmtCTDBiQWhROXRjSW5YMUUxdFBIK1crSFNUaS9wWENqK0JCSk9Z?=
 =?utf-8?B?UHZvTWsveURHN0NJaWZuY1d1T0gzMmhYTy9jZXk2Z1VEMHdOczY4TWJUYTIw?=
 =?utf-8?B?cEZkSWhhcFZXRnU3bCtQWUVlQVgxZjdFSk5ZMTVqQzZ1amV3czFHL2J2c0hs?=
 =?utf-8?B?TTIvQk15eWQ4UXU3b2pTR3cycFNFU3ZjVUJySXBFQzhubnZZLzNnOGd1Snkv?=
 =?utf-8?B?L0xHT245TUZqQmQ5bm9GK2pIYWJ2bXg5dTBFamxiZWdLUkpnU2FTTnFPNTZV?=
 =?utf-8?B?eVVyVlZVZm5GemZmZ1p5eDBrcEVIcE44U0RrbjFObStiK3NqbmE4SmdpZ1hq?=
 =?utf-8?B?MmVGcjBBaEdPM2J2WjVxUnIvbGZNU25JazBHRFQzMWNMcktRY3JLdEk0NGNK?=
 =?utf-8?B?U0x3RTU2SFdOOGNhNjlIU0dVblFJWTAzdU9tNkNBVnR4TDZyazgycG5xR3NR?=
 =?utf-8?B?QmdINGEyVitzOThNU3N0VC9ydkxxaDRnTmZ0ZHpzc0RET2Z5S29GTm5pUmpY?=
 =?utf-8?B?ZmdYby9QMmFLNTlXLzZHYU1kZkxRTjl0c1lVcmt4cmQxbXlGdFgzaGhDOHAy?=
 =?utf-8?B?dkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 60a6e1bf-865d-4551-c9ad-08db8796225f
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jul 2023 13:51:49.6798
 (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: 3kR/SoL4DEW3isnsVhAcSWK2HbW+BjAjgfnf7ANa1dPK1ByjvEUiQYec3q/7FWary4BRKsZxnpC4oI1g7Os6RuYw7YU6IxvXcRijoHfcvFU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5551
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Jesper Dangaard Brouer <jbrouer@redhat.com>
Date: Fri, 14 Jul 2023 20:37:39 +0200

> 
> 
> On 14/07/2023 19.08, Alexander Lobakin wrote:
>> On x86_64, frag_* fields of struct page_pool are scattered across two
>> cachelines despite the summary size of 24 bytes. The last field,
>> ::frag_users, is pushed out to the next one, sharing it with
>> ::alloc_stats.
>> All three fields are used in pretty much the same places. There are some
>> holes and cold members to move around. Move frag_* one block up, placing
>> them right after &page_pool_params perfectly at the beginning of CL2.
>> This doesn't do any meaningful to the second block, as those are some
>> destroy-path cold structures, and doesn't do anything to ::alloc_stats,
>> which still starts at 200-byte offset, 8 bytes after CL3 (still fitting
>> into 1 cacheline).
>> On my setup, this yields 1-2% of Mpps when using PP frags actively.
>> When it comes to 32-bit architectures with 32-byte CL: &page_pool_params
>> plus ::pad is 44 bytes, the block taken care of is 16 bytes within one
>> CL, so there should be at least no regressions from the actual change.
>>
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>> ---
>>   include/net/page_pool.h | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>> index 829dc1f8ba6b..212d72b5cfec 100644
>> --- a/include/net/page_pool.h
>> +++ b/include/net/page_pool.h
>> @@ -130,16 +130,16 @@ static inline u64
>> *page_pool_ethtool_stats_get(u64 *data, void *stats)
>>   struct page_pool {
>>       struct page_pool_params p;
>>   +    long frag_users;
>> +    struct page *frag_page;
>> +    unsigned int frag_offset;
>> +    u32 pages_state_hold_cnt;
> 
> I think this is okay, but I want to highlight that:
>  - pages_state_hold_cnt and pages_state_release_cnt
> need to be kept on separate cache-lines.

They're pretty far away from each other. I moved hold_cnt here as well
to keep it stacked with frag_offset and avoid introducing 32-bit holes.

> 
> 
>> +
>>       struct delayed_work release_dw;
>>       void (*disconnect)(void *);
>>       unsigned long defer_start;
>>       unsigned long defer_warn;
>>   -    u32 pages_state_hold_cnt;
>> -    unsigned int frag_offset;
>> -    struct page *frag_page;
>> -    long frag_users;
>> -
>>   #ifdef CONFIG_PAGE_POOL_STATS
>>       /* these stats are incremented while in softirq context */
>>       struct page_pool_alloc_stats alloc_stats;
> 

Thanks,
Olek