Received-SPF: pass (google.com: domain of linux-kernel+bounces-160526-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d05cbf22-f3e8-414a-a2e3-03e0b857eaca@oracle.com>
Date: Fri, 26 Apr 2024 10:57:31 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm/memory-failure: remove shake_page()
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, linmiaohe@huawei.com, jane.chu@oracle.com,
        nao.horiguchi@gmail.com, osalvador@suse.de
References: <20240426171511.122887-1-sidhartha.kumar@oracle.com>
 <ZivlrMAwRI6xJhc-@casper.infradead.org>
Content-Language: en-US
From: Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <ZivlrMAwRI6xJhc-@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?aDV2SktlSkh5aDRzZWNnZFcxcWYzaUhuWFBzblhZWTJ5cEZlQjF2c1pKWDQ2?=
 =?utf-8?B?dnBlbUFPN1BZZHBGSU5WQTRiZVpkK01pYmgzR1d2OWxwM3JGTEF6b2xMZEhI?=
 =?utf-8?B?S0RZZGk5RWdSMUFpd1hxSG5QK1dIWlZuZTFhYSs2cU1PSlNUWmhkSEpzODEx?=
 =?utf-8?B?S1ZYZ0JKVlJBVmJlTGR6QktBNTBJb3NVZ1p5N3MrZ1ZQdi9ETXRFMWpPc3Jk?=
 =?utf-8?B?Zmh5UVI1UTY5QkcrMTBkc3NVUnE0TGFaMHJwc29nMmo3VFJFOUFCOU05dFAv?=
 =?utf-8?B?aERURHV4MU9ITHh0Sm5wdkxJdThpR0srNlpnL1Bkb2tIWW9RWDc2ZUNJdDFu?=
 =?utf-8?B?L2NZa0NxK2daNmk0dkdTMzNLUFU2M3lrZGlYTnpSRmVOTk1YWE9oV0l0cGVF?=
 =?utf-8?B?cm15N0RaUy9McDlLZmpzcjNzWTdxTSs4ZW9zMUp5T2VkTXZOOVBDYzBBM0lW?=
 =?utf-8?B?cEQwREd3cERSdXlXS3R3elJDQVNKQlUrb2xMbjR1SDIwYXNteHlrcXp0YWkw?=
 =?utf-8?B?MFVKTDZsVTZIejBZUWNDZFZ5OVdOWjBveGs3Qmo1dVRoMThmb1BPY3lXRWFi?=
 =?utf-8?B?bytsUUxFamR5NkNKb3IrWDNxUzdFdTQ3UFVnaldHQmRUTEYzd1l6cHp4R2xX?=
 =?utf-8?B?M1AvbjdzWCtISkZwM2ZSeVZMM3dCc3QxeWtBbVB6ZVNuVmxwMklUV3owU1hz?=
 =?utf-8?B?S2IxOFZvYjRsVjU5V2Qwc0Y3S3o0S0pkd1l5aGNZSVd5T1BPdmdRSnRaVHFN?=
 =?utf-8?B?TmFBQS9lUkFjVHMzS2NUaHQ4WTBVVkticFZuTzhFY0p2T2M0QzJPaVAyMElN?=
 =?utf-8?B?eW5McjZCbWpHZEVnY1pwNEhRaUo2NjUvbE8vWGthT3BmcUdPaWVhSE10cmJ0?=
 =?utf-8?B?ejExMFY2RzUwcFVWU1UrOXd0M2dGdWo5OVJ2Y2Z1TkVDVjRBWW5peUJCQ1Zk?=
 =?utf-8?B?V0NiakZjbmd1TS9NMzJheGxhQmNYVytwVTE4TlZ3QTBNLzduTlhZb0NCa1A0?=
 =?utf-8?B?Y0xiSzE5QnFNM1NiNGlPZ0dBeDdjZEV1MWhRV1ZMbjc4WlhYbk5YME5hSm5q?=
 =?utf-8?B?L1BhekFaMGlKSlI1dy8zYnFkRlpYSGhCbnhlbTBielVqK0tIdTVjcjkvRUk4?=
 =?utf-8?B?MUptbGhUNUlSTGRwRFVvNjN1NWNWSExVVGJYN0xwSlp3M2l2VXBKNEdrUGgw?=
 =?utf-8?B?bytUeWdjL1FQYXVXQmhVODlxRjEvcVdhTnd6b01TeFg5bjV3c2k5Yk10Z1lt?=
 =?utf-8?B?TDJJUlh2aElJQkdadkVJdm5mb2ZOOWFrLzBrTGhaejEvRTVkc1RmTGRFb2VW?=
 =?utf-8?B?TDlQYXZoYnY3MFVDaUcwMnhTU3FEVnZsSmUvMEJqS2M3V3llWG9BeU55d0tY?=
 =?utf-8?B?OHlvNEpRVGl3VVNCWU83amVMbXJqemt5a3ZhclhZTUlMQ0VHWjkxR0dlMkw3?=
 =?utf-8?B?VXRDOXhhTjNNclM0ZS9HNUN2Smo0TWY5OHltR3pUd1daNG5CcmN2QWFYNnow?=
 =?utf-8?B?aXZmYXhyNnZkN0FhME5QNERvbW5LbjEzSURqbFpGbkNobjZ5MGNEQjllclEx?=
 =?utf-8?B?a3hzM0huN29QbHRzd3MzMmRhZ0x2SVFlb3dVN1hhRjhhQVY2VUg2c0MyaDYv?=
 =?utf-8?B?OEVNSGIwcUQrQTk4a3JFb1lPbjIvbEtXS2VXSVo5SFZpdjR6TFBIR0g0VWZs?=
 =?utf-8?B?cUR1c2QrdWJ3bzR0K2ZCbVhsWVpvbkY3cHZXSXdIaHJvelZCaUFSbUJ6b0Zt?=
 =?utf-8?B?aW5KU2tnQUJWNE02VVg4MUwwc3lNWWVONWMzU2J1a3VUTlNNT25jeEIrWXlV?=
 =?utf-8?B?OTNTL3l4bWNPKy82RUtSeGxSMUFmYitlejEraktITUhPVGVkWDV1TUNIclRS?=
 =?utf-8?B?RUQ4bW1MOWlpR2cweDRwaTFuNWRybElsVmxYeWgzNWpuRkNZK2wya3hPSk9z?=
 =?utf-8?B?UWpZamFZTlRCLzRsOHFoMWN6SjB5Szk5K0xkZkxOdmQ2ejRtOFhtekJGNG56?=
 =?utf-8?B?UEdDL0xZdCtFdVBwWVAvbytZdm02Um5BTGFjVnFlY1ZEbU42YkJ1MStrSC9y?=
 =?utf-8?B?WmZ4SGZadEVPLzlVS1hTb1B5VjI1aDBnQlNqaGx4bTFtd2gyYVJySEx4OXZq?=
 =?utf-8?B?RzVKSWduMTVPaFFwSGliQjVhMEdPd3FBMVlNcjh0ZUxPcXRHc3FLcStXTmR3?=
 =?utf-8?Q?BGFT4bH8EML68KMObpn1HaI=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	2zsVN+sJjbUW5dq5wWL9cYMwD5VHKS3/Vg9e+/gVB9I1DPNDwMaWy8zG+NfyJYM0neV6mCf+B5/rnFQfe6PR9Qz/GRrrwlf567azaym1BTe75YC8IrZ0RSsgsu1uOTn6Mh/tG4wzv1lU6B1svYMrcqvuWULsAAnwREUSoPfW6Ess0Zox6EJk4pPB65YHvwPJTGxOh/Mv07Ll/SFi+cNwRo0QdkmMUeUOpsix7g3FCohAw61gxzGSV6leEXiugTlz8wsd+E7vi/QVyNB/vTOrTNtRVfO+2nB2TMyykJSKn+p8zAu8Pom34vIK1e2Y9dbzecCQMgs5TF3AG/RilpYnlrzjzlequp6dA0oAOVstZnAxy2xcz/5DVP201XG8R1d30ZbLVXCYWHejq5Fir/hXhLYv6JkIG6jayb8B5OxIPd57ZwCSyW8/8sRGKg8j3qiyvtfOAaQp5/W/Voa+IPS/LM9GWELiNGJuo7xMs62vlC5KiCuX830kKj6kfin+NmGmlLt8tl4qO1vsVcG4WJtLEjkBj8MunfuF5VKT3QudFgRNiwxL2WONeFjDgpWJQdID6B1esmYZS2rdW8MQllpRoHSOzHmb2oi6QfivAzq+vO4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebdfebe2-bc12-4c05-42e4-08dc661a599d
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 17:57:34.0255
 (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: x4ILFnVfiXg/YzjCsTurY0s8OATp6aszmxtGc39QKC1eSU+NoIWqa6wvy1Xr8jaBaStqatfAIvHP5d8otVcxjW7Ca1OexNLj/t6knHho/pk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB7755
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-04-26_14,2024-04-26_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=999
 bulkscore=0 mlxscore=0 suspectscore=0 adultscore=0 malwarescore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404260121
X-Proofpoint-ORIG-GUID: yWwfYh0qEMJLbulcdTedWl7kB8QHnbIr
X-Proofpoint-GUID: yWwfYh0qEMJLbulcdTedWl7kB8QHnbIr

On 4/26/24 10:34 AM, Matthew Wilcox wrote:
> On Fri, Apr 26, 2024 at 10:15:11AM -0700, Sidhartha Kumar wrote:
>> Use a folio in get_any_page() to save 5 calls to compound head and
>> convert the last user of shake_page() to shake_folio(). This allows us
>> to remove the shake_page() definition.
> 
> So I didn't do this before because I wasn't convinced it was safe.
> We don't have a refcount on the folio, so the page might no longer
> be part of this folio by the time we get the refcount on the folio.
> 
> I'd really like to see some argumentation for why this is safe.

If I moved down the folio = page_folio() line to after we verify 
__get_hwpoison_page() has returned 1, which indicates the reference count was 
successfully incremented via foliO_try_get(), that means the folio conversion 
would happen after we have a refcount. In the case we don't call 
__get_hwpoison_page(), that means the MF_COUNT_INCREASED flag is set. This means 
the page has existing users so that path would be safe as well. So I think this 
is safe after moving page_folio() after __get_hwpoison_page().

Does that seem correct?

Thanks,
Sid


> 
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> ---
>>   mm/memory-failure.c | 20 ++++++++------------
>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 16ada4fb02b79..273f6fef29f25 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -385,11 +385,6 @@ void shake_folio(struct folio *folio)
>>   }
>>   EXPORT_SYMBOL_GPL(shake_folio);
>>   
>> -static void shake_page(struct page *page)
>> -{
>> -	shake_folio(page_folio(page));
>> -}
>> -
>>   static unsigned long dev_pagemap_mapping_shift(struct vm_area_struct *vma,
>>   		unsigned long address)
>>   {
>> @@ -1433,6 +1428,7 @@ static int get_any_page(struct page *p, unsigned long flags)
>>   {
>>   	int ret = 0, pass = 0;
>>   	bool count_increased = false;
>> +	struct folio *folio = page_folio(p);
>>   
>>   	if (flags & MF_COUNT_INCREASED)
>>   		count_increased = true;
>> @@ -1446,7 +1442,7 @@ static int get_any_page(struct page *p, unsigned long flags)
>>   				if (pass++ < 3)
>>   					goto try_again;
>>   				ret = -EBUSY;
>> -			} else if (!PageHuge(p) && !is_free_buddy_page(p)) {
>> +			} else if (!folio_test_hugetlb(folio) && !is_free_buddy_page(p)) {
>>   				/* We raced with put_page, retry. */
>>   				if (pass++ < 3)
>>   					goto try_again;
>> @@ -1459,7 +1455,7 @@ static int get_any_page(struct page *p, unsigned long flags)
>>   			 * page, retry.
>>   			 */
>>   			if (pass++ < 3) {
>> -				shake_page(p);
>> +				shake_folio(folio);
>>   				goto try_again;
>>   			}
>>   			ret = -EIO;
>> @@ -1467,7 +1463,7 @@ static int get_any_page(struct page *p, unsigned long flags)
>>   		}
>>   	}
>>   
>> -	if (PageHuge(p) || HWPoisonHandlable(p, flags)) {
>> +	if (folio_test_hugetlb(folio) || HWPoisonHandlable(p, flags)) {
>>   		ret = 1;
>>   	} else {
>>   		/*
>> @@ -1475,12 +1471,12 @@ static int get_any_page(struct page *p, unsigned long flags)
>>   		 * it into something we can handle.
>>   		 */
>>   		if (pass++ < 3) {
>> -			put_page(p);
>> -			shake_page(p);
>> +			folio_put(folio);
>> +			shake_folio(folio);
>>   			count_increased = false;
>>   			goto try_again;
>>   		}
>> -		put_page(p);
>> +		folio_put(folio);
>>   		ret = -EIO;
>>   	}
>>   out:
>> @@ -1643,7 +1639,7 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
>>   
>>   	/*
>>   	 * try_to_unmap() might put mlocked page in lru cache, so call
>> -	 * shake_page() again to ensure that it's flushed.
>> +	 * shake_folio() again to ensure that it's flushed.
>>   	 */
>>   	if (mlocked)
>>   		shake_folio(folio);
>> -- 
>> 2.44.0
>>
>