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: <4824666a-baa6-f380-5e79-3e13303abf8d@oracle.com>
Date:   Fri, 12 May 2023 11:04:03 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] mm, compaction: Skip all pinned pages during scan
Content-Language: en-US
To:     Matthew Wilcox <willy@infradead.org>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Steve Sistare <steven.sistare@oracle.com>
References: <20230511165516.77957-1-khalid.aziz@oracle.com>
 <ZF5g3p+FFc4Gkw69@casper.infradead.org>
From:   Khalid Aziz <khalid.aziz@oracle.com>
In-Reply-To: <ZF5g3p+FFc4Gkw69@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?M1dtZmhwN3pHbmZZczRKRHA3dys5NnNzVHpXUXEzbnJCQ2djYXRlaGwyUnpQ?=
 =?utf-8?B?S2Y0TFZDK2RmTkxtRUpXSnBpSUl1VXRFWG1SeSthdVcvSGE3RnkwS3pjQ1hX?=
 =?utf-8?B?cUpQend1UjlSU3FBVzZDc3d2aXJsOEd3OWJpcnFaOGtPS3FwL0tnTDRvazFO?=
 =?utf-8?B?TGRNUEhFdi9Ba2ZxWHF5Q21OVGlod1ZieHpaV2hYTEwreVFOM1I2RHNZMzJy?=
 =?utf-8?B?U0FpY2pIY1cyMS95b0tocFJTdG1iMzZGR0RtUnR2ZmlsbTRlcjExSWRzcXlZ?=
 =?utf-8?B?M2xJM1B6bENkZHVYTVYwUGJrL1hpNHY3eUlEeWtULzlVTVJtVzd6MFZTdnp4?=
 =?utf-8?B?WjNHcHdsV2YyNW04QldNTFBCQUJnTWl1Y1FISFJJaHFJRSttRjhTbkpKdTdt?=
 =?utf-8?B?enJEU08ydG5FeUczV0RtNWtac0pTZm5hdmNWZ2daVnBSQUN0WklkWWMra3Bo?=
 =?utf-8?B?c1F4Z3MzRzF0VnhGbHZWMzVLQWtaeWdQeThDZm1id3U0bUVlLzRSOUhvQmVm?=
 =?utf-8?B?dVlTQ1ByYjUzZVVWV2IyUFgwcjhIZ3ZGK1V2czY5eDF0NVREUUNiWElpc2Fq?=
 =?utf-8?B?bmlxK2JLSkQxTjNxNGJLWTF2Y0htYXh6RVl0eTlKSGw1QU5sWjVDc3dJV3Ft?=
 =?utf-8?B?RlEreDlWSWRUQ2hTY09mZCtXYngvM1FwSzdtckNlSWxzK2Q3N3M3bk9ScWMw?=
 =?utf-8?B?QTB3eGJrY1A2Z01DZFZzNWFTT0x1SlpCZE5tS1Z0dnUveUgwNUVOeWl5NEUy?=
 =?utf-8?B?MTZRTUdqdGhldWJPV2R3a2NyV25WM3d0RzI0VXBQaTUyNlIyTi9jblZFSHJr?=
 =?utf-8?B?VS9WaHhDVjRMOXNGTGZrcmRSVkJtTmxUM1JQS3VvWEN2WGVNaGNFYmo2elBR?=
 =?utf-8?B?N3RKRGg5T0h0cllXbzdNV1F4RDcyS2Q5N3BTTHVUNjkrUmFhRjhPUG1uUE5K?=
 =?utf-8?B?ZlRjTzBBd2tBQmc2bUVyT2g1YnZHSzJJNjBpQ2FCTFlaU1pYZi9NNzh1dk1q?=
 =?utf-8?B?QkppTzhSTjR1MGhzOXlLRmhtWXc4aW96YXNFdmpONEV5QTdjcTRnRjVDdnpK?=
 =?utf-8?B?TVd0OFZ3TTRmZHJiL0cyZTh6VmNEelZ3UDJYUWVaSUtzQmdSRytkV1dxMWJm?=
 =?utf-8?B?bjNUMk91TGh0VmxkYmhQeTlkWHRicGVyZ1U5eG5FQ3k2d1d6RXJkSGVSaTR5?=
 =?utf-8?B?MWtjVkx0bE1jVS9GZWZKREc1UGYwbUhJWWQ1Q0QwTm9FY0NYM0NvdlRyZk5v?=
 =?utf-8?B?TU1hTUgvWXlEQXhpaElDc1lycUJKVWdYVENYNkpDUG13YTNtdHNYSDFMZi9r?=
 =?utf-8?B?ZkhYTTJ2WjdZb3JmVUNza1h6cmQ3QnNaR3FsaldaRlJSZllteUZXZVl5Z2Fv?=
 =?utf-8?B?SG90dnhLN21VS1BLWTYvaFE3QnB1a0gxZEJEeFlTeE9rQ1hLMTl6bmVYY1ZS?=
 =?utf-8?B?QUZ1S2NWM093ODBzcTRZVDJaMHUzRVJwNTAvN3dYZHBJV1BOdmFFcFRVaHBC?=
 =?utf-8?B?VkxQc2Nkdkg1OTFmZkFUcGh5V1NNb1ZWV2ZDanVwNFJhVVhLTW84Z1d2VzR4?=
 =?utf-8?B?TTE5a3ZydElENTNmcGZzeG5KRklLaXhoT2Y4ZVZpTjhza2c4MEZ1VnpMd2d2?=
 =?utf-8?B?M1d5OWZWZ1ZyNytlZlZZdTgvZGRUTlV6UTRHZzM4YUFza29UekQxbEFKcjZC?=
 =?utf-8?B?ZXd0eHJjL25TTFErUm1TeUhCWU1wRUdaK01LQWNrMzJnY2p2dGtBNUtoNE1s?=
 =?utf-8?B?YmR4TTdZUUhWaGlRL1NwVnhRK2Q1THZYOUhJSEFHaFlZQWlPRnh2U21EcXZ0?=
 =?utf-8?B?dUxqZThucDZuOEl1V2VyOEk3Zm1rNEdTYlJsT1QrNk5USndtTENZNUJKa1ZE?=
 =?utf-8?B?Szl4cEtWZE1WaUJBZlhaN0E0ekdYanpIU3F1T0I3aGJNQll4RWtvdTErWGN6?=
 =?utf-8?B?Y0RrSWdINFNHKzlQQkRhekJ4dlNKQWVoUC9rdTNZWE1rdEdlQ0Y3ZFJadUZx?=
 =?utf-8?B?ZkIwNEZ2OHhWWW1FdTlhMmpkdkl1cHdTNUQrbkkwWXdlS3dDL2ZrMHlpeWFN?=
 =?utf-8?B?c1RHUGwwaUh4cGlpYnNmQkx5UnlBMloyeG1KSUVjSGpldWtWcU44WE0yT04r?=
 =?utf-8?Q?RqJFrC07x5gj+OL86+3kPmr3F?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: NXSZ8wCyYiotwmaA+rtF5xgoiXMRy5th54iHKV8BPEqURgs5i8gmhOFt8XNNqRB0WUv3BRhKD/YvWSvcWWLiNpcZ3thSAY6e4as6AeTLbUBniFJGNqKHA5gzQzLMiE+zWTW1nVpzS2G/ZEhs/so87KiSufGQiGZS1uyYwX54lXghFmVOZ4WQh4U82lryBsmUCFOQpbRNTFjv36Kw6CXcWwqhGrUaBQFr19OccHdT/GIiZgMZw+flAryRfnV8sHG4uWdIPuEd5tA+BBz5zFoF0u3TjuEAV4iPvRDI/ar9/QaIrLnevK9TM1Q1KGdASJ6eC7WkSgI9jWtatWk2GpyMhK7mukHOIUTFSvsy3nx/Gv877Y+hrsHcSBs3pPQc1/GzABtQs3ajt4g78tIvBO6JeFSA7aZS9nljxp9KHVCI9eIQ2ouiXjL7tDciMnTTw44Ye9kPmvR/+pUE81yWDpoeu0fv1jsj5Y3qTndlCDmcuCInny62Nx6bAtoBR2vsPsCOcABr9JtxuXIo8lcmQZEO1B0v2FYxJU8rmPT0IVfXGywDPGp67QH60J//HbWMOv022e9plK2NggX96Q3D7Z672bhbuZV3C3j6/wWcQqWRfVdzLRNYmP7EnfozE+gYo+pe+YFxwgxVN5EyxhLYPxsjQ+cEfOoR7dREr0C1cLyWCIn2yBObk7EAF4hVtirxcY/vSkZpZXyRrvwF4PGeCYIr6MxuhsMPT9haNmQmM+5A0wfHTAOxif3pnpaE8LW19oVhPSlNbTx3uzsQO9BBTaxpzDOVsCeHI74I/IKYOmjv10EBLvo/T0CjLohikbIZuzFhobpDCG0y5cYMbtoHh8dv0GBfnP3NxYATj4T4HvWoaoArNKWMhP0di5qaSjsABLU5
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 55f4eed9-a390-451c-ce18-08db530ae4db
X-MS-Exchange-CrossTenant-AuthSource: CH3PR10MB6810.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 May 2023 17:04:06.3191
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ScQzZuWK4ggBVxG/xT3qATPZi6CMt6usmzpnHs0yit8OsQv1QHTA+YmFcfSpEm51442Mc8OLjThWkiJbTyXHlg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6391
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-12_10,2023-05-05_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=967 malwarescore=0
 adultscore=0 mlxscore=0 bulkscore=0 suspectscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2304280000 definitions=main-2305120142
X-Proofpoint-ORIG-GUID: VleQX1Q4C0xJ6UVnrwMR6rRRap13wztX
X-Proofpoint-GUID: VleQX1Q4C0xJ6UVnrwMR6rRRap13wztX
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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

Hi Matthew,

Thanks for the review.

On 5/12/23 09:53, Matthew Wilcox wrote:
> On Thu, May 11, 2023 at 10:55:16AM -0600, Khalid Aziz wrote:
>> +/*
>> + * Check if this base page should be skipped from isolation because
>> + * it is pinned. This function is called for regular pages only, and not
>> + * for THP or hugetlbfs pages. This code is inspired by similar code
>> + * in migrate_vma_check_page(), can_split_folio() and
>> + * folio_migrate_mapping()
>> + */
>> +static inline bool is_pinned_page(struct page *page)
> 
> ... yet another reminder this file hasn't been converted to folios :-(
> This part is particularly hard because we don't have a refcount on the
> page yet, so it may be allocated or freed while we're looking at it
> which means we can't use folios _here_ because the Tail flag may get
> set which would cause the folio code to drop BUGs all over us.
> 
>> +{
>> +	unsigned long extra_refs;
>> +
>> +	/* anonymous page can have extra ref from page cache */
>> +	if (page_mapping(page))
> 
> We already did the work of calling page_mapping() in the caller.
> Probably best to pass it in here.

That makes sense. I will change that.

> 
>> +		extra_refs = 1 + page_has_private(page);
> 
> page_has_private() is wrong.  That's for determining if we need to call
> the release function.  Filesystems don't increment the refcount when
> they set PG_private_2.  This should just be PagePrivate().

I will fix that.

> 
>> +	else
>> +		extra_refs = PageSwapCache(page) ? 1 : 0;
>> +
>> +	/*
>> +	 * This is an admittedly racy check but good enough to determine
>> +	 * if a page should be isolated
>> +	 */
>> +	if ((page_count(page) - extra_refs) > page_mapcount(page))
> 
> page_count() includes a hidden call to compound_head(); you probably
> meant page_ref_count() here.

You are right.

> 
>>   		/*
>> -		 * Migration will fail if an anonymous page is pinned in memory,
>> -		 * so avoid taking lru_lock and isolating it unnecessarily in an
>> -		 * admittedly racy check.
>> +		 * Migration will fail if a page is pinned in memory,
>> +		 * so avoid taking lru_lock and isolating it unnecessarily
>>   		 */
>>   		mapping = page_mapping(page);
>> -		if (!mapping && (page_count(page) - 1) > total_mapcount(page))
>> +		if (is_pinned_page(page))
> 
> "pinned" now has two meanings when applied to pages, alas.  Better to
> say "If there are extra references to this page beyond those from the
> page/swap cache and page tables".
> 
> So it's probably also unwise to call it is_pinned_page().  Maybe
> 
> 		if (page_extra_refcounts(page)) ?

I like that better. I will make these modifications.

Thanks,
Khalid