Received-SPF: pass (google.com: domain of linux-kernel+bounces-160641-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <47614f5d-0942-4b2c-a51a-451f6bc9c802@oracle.com>
Date: Fri, 26 Apr 2024 12:52:59 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm/memory-failure: remove shake_page()
To: Matthew Wilcox <willy@infradead.org>,
        Sidhartha Kumar <sidhartha.kumar@oracle.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, linmiaohe@huawei.com,
        nao.horiguchi@gmail.com, osalvador@suse.de
References: <20240426171511.122887-1-sidhartha.kumar@oracle.com>
 <ZivlrMAwRI6xJhc-@casper.infradead.org>
 <d05cbf22-f3e8-414a-a2e3-03e0b857eaca@oracle.com>
 <ZivyC3vqa2BIBoMj@casper.infradead.org>
 <c40cfd0b-f045-4887-a955-fee7e0392cf1@oracle.com>
 <Ziv67sGBi02YCYZ5@casper.infradead.org>
Content-Language: en-US
From: Jane Chu <jane.chu@oracle.com>
In-Reply-To: <Ziv67sGBi02YCYZ5@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?QXh0aUxMNzFwcjBJYm1sVHRaeG8waUhoM25CeWdCZk5POUNOZVFqMWZrWEVo?=
 =?utf-8?B?Z0huZWV1dklIdmd3TXhYWGVHS3Vyak51ak84U3hkS3ZpWFpXVHI1QlFVMGIr?=
 =?utf-8?B?Wm9hTFlwS2dKekM0YzNLNVVrSzd6YUYxclFqdERrQ2N6ZXpyVy9IMUpEN0RR?=
 =?utf-8?B?RzVEMDhrdERWTFFoSitiK09UV2h5N1hIc3F1ZHEwNHFHQUR6Ty92eW13UTNM?=
 =?utf-8?B?U3pReTJrMysvSmd2V1FpT2tpOWdaelNjUGg1bzQrTE1CWlVKbjhxSndRU0pj?=
 =?utf-8?B?ZVByaWZReW1RTHE4dWJNelJ3eEl6SkovUGJ3c0F2NVZtQUFFbkhwcnJlK3JZ?=
 =?utf-8?B?Z0N4THFZRytNcWI1TE00R3Z0SkRkNWxKalRaU1l4MzgrVm1LRUI1dEc1L3M5?=
 =?utf-8?B?bHFYcm1EMXpLckJqYm41U3FzR0ZoOE1QdkV6L3ZsUlZWaVZRbzlrL3hYVm9r?=
 =?utf-8?B?RlR0NWU1RmllcmlEUUt4Rk1iSktuaGtyZTlIV3hkYXJwZnNPcStwNWM1aE5O?=
 =?utf-8?B?NlA1ME9oMSt6K0FiRWg3SXQ3MC8rQjVpUWNXS0hCWFhLdUlySjN2QkQvVkN3?=
 =?utf-8?B?cVV0SENtdGhyRXBPRjRlUFUrejAwbEJrZUlxTldFQlExaUtMU0wyWkNMcHpM?=
 =?utf-8?B?aFdBVDZGTllQMkw4QmpVS0FaVW8zdDhuZVowVGVQQzhCK3JOUmVraDZSQkEx?=
 =?utf-8?B?ZUhhc2poTmg1MDllTHBYS3Z0SnZBWktXS2VLdUg2N1Z1VGlwbjlkdllpamh4?=
 =?utf-8?B?VnNEVXVqWkJoYzNKUkFPVVFxTFNQbEhKSG56Z0QvNkorbEU1QUtGRTZvdTNI?=
 =?utf-8?B?OVhwcmM3ck9ONkpUcUtVend2QU9GWlVzZWtFTVdEYjk4QUV5Sjl5d3UrSDJ0?=
 =?utf-8?B?cjk0eUZhcXA3SFB4ME5PNEVJenFucG50am45Z2NmTXlBb0k3TXBmY2E4Sll4?=
 =?utf-8?B?RDBORHJZS2dPcGx1T0ZvM2E5dk9FVUJnUWZlaTc4NmpJRDNmWk1abHFZT0tm?=
 =?utf-8?B?YVpPbGpsT2h3S0ZwMkV5V25oaU05RktoeTZ1YStpdkNFU1dvOUNPdmc0MmlN?=
 =?utf-8?B?bnI1TFZseFBSb1l1T0dkeFV0QldFUEt1eTBsRWhPc1lLTzkrUUt2MVllalFt?=
 =?utf-8?B?cit2YU5BZURoNmtNRlZyMVYxalQxODFXci8zS1pQUWdjd3A5bGtBWTY5TFpZ?=
 =?utf-8?B?VEphUkY0Y2IzTFBBaW13cllUYVVWRDFOWnI3bW5DU2RRU1h4cmc1ZVZoZ09L?=
 =?utf-8?B?aEdzc0FUcEkvRW9xekFBZloyb2ZmWWozUlUwVlkyay9XQmV4RFJvaFRrdzNF?=
 =?utf-8?B?UHNYb0F6cC9XNkxaOWlIYmVUa2ttR2tiUStocmJ2QkZSUU5nbENJQUEyOVQw?=
 =?utf-8?B?aUc2cTRkYjZnSlg4azk5ZTRpcmRzemNqYUZvbGRLT2xCc3BZdHJSRlN5VVdH?=
 =?utf-8?B?dG9VTExFVDJaZ0NTVmVaSkZnTHZIaG9wR3ZDcTR0dDB2aUw5NkloT2RQdS9p?=
 =?utf-8?B?bUxFQ0dLQ2NIaUl3TCtFZUdQT1hXSmJVTHFlV3V1WGZlRjlRdlR0dWZVTHEw?=
 =?utf-8?B?WGhDakozZzhEMXlZdjNtdDgwekwvZ2tIcUNUa2VPMmxqc2RrbHI0MGFhWmZS?=
 =?utf-8?B?eEVmc1EvcEhHcmlOTWtEMjlHS2twaE54Q2xOamtRenZzSVVPcnF2cVNYQVFq?=
 =?utf-8?B?N1VXdG1YUXdNazJFTHpOb0o3RDdGWm9GN2N0SW5oM1pDb3YxNlFSN3g5NGdw?=
 =?utf-8?B?S0p0ZS95T2Y3bXdsd3YzSTJvREVQNXgxYUdhWkkrY2NLZE96SHpiTmVrRk1N?=
 =?utf-8?B?ZFYvRTArR2NSNHRUMG50T25XOUcvelBrN0F2QkJndVBHYlUwYUQ5THhIMVMx?=
 =?utf-8?B?VFpVRWdabWpweUcrZm4wTHlETFFEeDlTTjRJWTBLMGNQdmxISzNuQUFJbld2?=
 =?utf-8?B?cTJtNzBnRWR1a3oyM3kraHpzSXU3WVRxM3k0VWNNOU9Cc3BFSWxXeFRkdUNB?=
 =?utf-8?B?UGdRTHdwZXNocVlYVDRjQ1p6Ynh5VE5VdXZEdlF2Sk8yNVRYQWt6OG10Z1pV?=
 =?utf-8?B?SHA2anFqL2VBK0xlWTlFL0h3T3ZtTFhxUENmaUo3YWhQYzBadXhDbWgvcUMz?=
 =?utf-8?Q?cQluyVYinGwQv+Weo5c33dBWw?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ITfTKQY3+QZ6MQlAaYNaozYh9vAVVPNTh4nO7onTvMUyVBk7LwCb6/bNCVON6swya0Mh9YKu29gHVutp3Hd4JsMiB1CK2g/5ZFwuL3aPn10EoU4rTv7WdVs9LFDEu/7p9An3qQ2WrWOjOgjhASQsfmGq3DsNOeIbeRwo8XehZgMevsK1q4/FGPonfgcPJ92j0Dc3KJMcxyVUzXtZoWX45cPmCdvjhWEo5K473OuWqx+kbAQaxytIKvexsV3Gvf4E8XsZejApFmgRn31HF+I4tiEII4LRMpuwsutx6BwzSZdYozd5mUQCn23MZ5RAwBT6T4romld9apJ2n51wu2MxrzWaMjCaz6CK+MtNEra+lMk3O6WC5/ma669cTD8YEdOpZi9jIufXadUcXLg3EWomOqyNyVQL/CTcMjKOcrL4gBjyFWE7edT42ua6phJsfCQssLVoaGkk9TX5ti+5+aANoBSNmcW5BzrgIbXuMn/NDL06Q3wrb9JIMRBGWorKMKMSmFt5hkgpAaVIC9Dt7w1BV5/WGsKXQVV8kthhS+FZYuGdkgfLTiy90joYkknbj0DzIwC5vIOYYEcDtgDrbwToEagBZw0SQmW4jZcoJwbfJwA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0cd41a01-43e2-493f-41a5-08dc662a7e40
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4429.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 19:53:07.6183
 (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: zGyxYWlw3M6VQjBFASiohWGevEtX0RhPYXm/G/jTkSU2E+Q+5+BH0xls8X4aGWDaaXxNvBLuXXT4CQDHudRT9g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYYPR10MB7565
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_16,2024-04-26_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 spamscore=0
 bulkscore=0 suspectscore=0 mlxlogscore=999 adultscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2404260136
X-Proofpoint-ORIG-GUID: 5fhhDN32QHV7h4tSYEBiyNcq6lLFfyx5
X-Proofpoint-GUID: 5fhhDN32QHV7h4tSYEBiyNcq6lLFfyx5

On 4/26/2024 12:05 PM, Matthew Wilcox wrote:

> On Fri, Apr 26, 2024 at 11:53:01AM -0700, Sidhartha Kumar wrote:
>> On 4/26/24 11:27 AM, Matthew Wilcox wrote:
>>> On Fri, Apr 26, 2024 at 10:57:31AM -0700, Sidhartha Kumar wrote:
>>>> 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().
>>> See if you can find a hole in this chain of reasoning ...
>>>
>>> memory_failure()
>>>           p = pfn_to_online_page(pfn);
>>>           res = try_memory_failure_hugetlb(pfn, flags, &hugetlb);
>>> (not a hugetlb)
>>>           if (TestSetPageHWPoison(p)) {
>>> (not already poisoned)
>>>           if (!(flags & MF_COUNT_INCREASED)) {
>>>                   res = get_hwpoison_page(p, flags);
>>>
>>> get_hwpoison_page()
>>>                   ret = get_any_page(p, flags);
>>>
>>> get_any_page()
>>> 	folio = page_folio(page)
>> That would be unsafe, the safe way would be if we moved page_folio() after
>> the call to __get_hw_poison() in get_any_page() and there would still be one
>> remaining user of shake_page() that we can't convert. A safe version of this
>> patch would result in a removal of one use of PageHuge() and two uses of
>> put_page(), would that be worth submitting?
>>
>> get_any_page()
>> 	if(__get_hwpoison_page())
>> 		folio = page_folio() /* folio_try_get() returned 1, safe */
> I think we should convert __get_hwpoison_page() to return either the folio
> or an ERR_PTR or NULL.  Also, I think we should delete the "cannot catch
> tail" part and just loop in __get_hwpoison_page() until we do catch it.
> See try_get_folio() in mm/gup.c for inspiration (although you can't use
> it exactly because that code knows that the page is mapped into a page
> table, so has a refcount).
>
> But that's just an immediate assessment; you might find a reason that
> doesn't work.
Besides, in a possible hugetlb demote scenario, it seems to me that we 
should retry
get_hwpoison_hugetlb_folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/get_hwpoison_hugetlb_folio>() 
instead of falling thru to folio_try_get().

staticint__get_hwpoison_page 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/__get_hwpoison_page>(structpage 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page>*page 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page>,unsignedlongflags)
{
structfolio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/folio>*folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/folio>=page_folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page_folio>(page 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page>);
intret=0;
bool <https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/bool>hugetlb 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/hugetlb>=false 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/false>;

ret=get_hwpoison_hugetlb_folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/get_hwpoison_hugetlb_folio>(folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/folio>,&hugetlb 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/hugetlb>,false 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/false>);
if(hugetlb <https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/hugetlb>){
/* Make sure hugetlb demotion did not happen from under us. */
if(folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/folio>==page_folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page_folio>(page 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page>))
returnret;
if(ret>0){   <---------  folio changes due to demote
folio_put 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/folio_put>(folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/folio>);
folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/folio>=page_folio 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page_folio>(page 
<https://elixir.bootlin.com/linux/v6.9-rc5/C/ident/page>);


thanks,
-jane