Received-SPF: pass (google.com: domain of linux-kernel+bounces-187950-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <9d217b1a-c11f-4fd7-ae81-c704268da2ab@oracle.com>
Date: Thu, 23 May 2024 13:01:50 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 5/5] mm/memory-failure: send SIGBUS in the event of thp
 split fail
To: Miaohe Lin <linmiaohe@huawei.com>, nao.horiguchi@gmail.com,
        akpm@linux-foundation.org, osalvador@suse.de, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20240521235429.2368017-1-jane.chu@oracle.com>
 <20240521235429.2368017-6-jane.chu@oracle.com>
 <c95a0336-6755-03a4-5c09-273d538e139e@huawei.com>
Content-Language: en-US
From: Jane Chu <jane.chu@oracle.com>
In-Reply-To: <c95a0336-6755-03a4-5c09-273d538e139e@huawei.com>
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?Mk9xbEM1ZHNhRXBXZFlSMWxJekVtZFhlb1BRaDR4SU1GeC8ydlJEZmJZbldC?=
 =?utf-8?B?K3BheXd2c05OWmQ2a3ZpY1hKWjhXVzA1UzV1SWtIMzRPMVZ0elVFQ3FjWDdU?=
 =?utf-8?B?MWNhT3BjdklubHNGVjZBczdkYUNLaTJnM2tZZlp2NFdLeGN4bjFidWdXU0FX?=
 =?utf-8?B?SzNMYitEdjlMZGlKWUNza1VWeU5qbmZ1NWprK0JmcERoLzBJSG9zaGl6Ynp5?=
 =?utf-8?B?c1VoSkh5QlUzRnJYVVFod1N4eFYxSDBVaFZnYjJXYjlreTg4TFA4U2Z5Q01O?=
 =?utf-8?B?RDg1VXR0V3gvQXh1YmQrYmRtbGN4cVVEK3RRUEdCSC9zbW5kNTBZKzd6SFBR?=
 =?utf-8?B?UmxxWGFuOENRUDB5WENHM3pqdnd4VHQ2TStLSEdOL0ZmTmhDdFFPYkpPa0NS?=
 =?utf-8?B?bTFyK2c2SXd1UGFYSXFLd2VIcmxRSWN6VkFLcWd6ejZIeGVLK1lmeDVSZlEy?=
 =?utf-8?B?QXd4YWhtMG5rWnZCWVV5WHNPaHN1TTRlcUtYNDBhM2RzRUFpdnhBbTJSNEJE?=
 =?utf-8?B?Zk4va1M1MEVyTm5QeFR5VFNYQkpxOUhhTVd1cjVYYW8xeWcrNXl3eEFxUGVq?=
 =?utf-8?B?cnlIbWtJNkw0K1owanZTZGs4LzVDM2dNVW12WnZyMTB0MHRPYnhjOUdYQTZD?=
 =?utf-8?B?ZzFEbUhlcUl2TGlVa1ZGKzliTTFEbU9sU2pjUmdGR1ZBQnFGMXZ3Y0Zla3N1?=
 =?utf-8?B?NnkycW13U1ZoOUc5b25yejJ5Wnh2WE5IbnlEclQwSFcwSUszK250ekZ1cStq?=
 =?utf-8?B?ZjdNN0hqZ3ZIMy9ieTZFempIQWd4U2l2OTJjM0ZHRVNWSEdUZmdIQkhiZG9V?=
 =?utf-8?B?cWE4a1R1UzV0Ukd6VHRzbjBmQkhqa2J1ZE5NeXo2MTZqTlNBd0JkZTQzTjRV?=
 =?utf-8?B?SnlMc2lXOGFhQUwvaUFOcmgyN1BGOWtKWUtOeG5TVEl6WmVWclRFZVhyb0p5?=
 =?utf-8?B?aVFkTnhxM1V4QStyMUJtRnEvek45ck4vTWV4TmtzZk9xNzFscEI3RnJVdDJY?=
 =?utf-8?B?QXkxTVA1UG1UKzJQSVRrWUIzVkh6eUhueFJHb1dWckcxUzBmWWhNYjI1Q0dy?=
 =?utf-8?B?czVsM01seVdlTU9RZU8vRmJGWlltR3VYMGpkdlhZZmhBTkJyZUxmSWVreDY5?=
 =?utf-8?B?Nk9xd1VBYUpBNGZBcmppUVdCZUZjeVdHanpjVkFVcGNFM3ZnQjd0TExDUDRu?=
 =?utf-8?B?OVZjOG1aYzYrQm5saG01Y1d2R09HMXpRc2xJdlVTSkxMYUx1M0ZzOGlYQ0dZ?=
 =?utf-8?B?OHhscitsNVhieXUzQnZTdy90YVhmek9KcWxPOFRCWlFXaDdrTHVreGtDeWhw?=
 =?utf-8?B?NEREQ2hqWFI4WE8wbXFCOTBWMW9mUW5LTmtuWWdhdlRDUHZYU2lyZ3NJU21r?=
 =?utf-8?B?ams0NElIYzdEL2dia2FDVmRsSUxPcHU5NlhKZkRTalBOTklRZ01KcWZCell0?=
 =?utf-8?B?cDN2WEVRTC9XYkthMjlVaTZ0UGM0QVorU0dKa3ZIUTVQaFJVTk1PME42aVZD?=
 =?utf-8?B?c2dqcmdrT29tSlpXYmhUYUR0WitSSEhsUTN6ZWcvMERxZm9mbWpKSnNtK3ZF?=
 =?utf-8?B?akhCVXZtUVhqK25pTmorWWhweEVHTGhiUThCK1ExeFdZUmtCcTZIejlRc0x3?=
 =?utf-8?B?N2szSGZxSzk4NGZtZ1JIQWlCT3VsOHNTVlg3VjM1ZFZOaVpJdm5xN2hsakIr?=
 =?utf-8?B?RWFsSldPOFZFRG9sSm84TnlqUXpDbTRrdjlQUzEyV3BpQ2tKZm0zd1ptUHls?=
 =?utf-8?B?MlpOVkc4cXZIeW9Ra0NiM0dpTHVXRmJseC9MbVhkaUkwaENIT0ZkV1N2bG02?=
 =?utf-8?B?a0JyREhxV2drSVdVVXhBdEgrOElmUzJSdTJrT3huMDREWENjelg4YXpyU29O?=
 =?utf-8?B?eU8xa2N5am1mZkx5WTVuZHJWeVdtd1pSR3BHZHBqUXFtbU5LWU16SE51ZlBy?=
 =?utf-8?B?akYzd3dhbWM4TlRMbG9uMjhUVXhIYVl6ZDNsQ0xvc0thZWRoZHZoOEZNZ2hm?=
 =?utf-8?B?MGFVcnFMTWpvZmtWd2NBY1lKWkI5N1dKVkpHd21LVDdhVFE5dHV4UWFUQnA4?=
 =?utf-8?B?bHh4c3M3WG1FVDdaUFd5WmdoNnZzbHBjSG9DN1QyRmJ5T1JHeWQ3NCtpblRP?=
 =?utf-8?Q?LNTslnrwy4acER7LPfROKOXUm?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ja5L/IcAG+tvkL9BjkaXDkdxMcIF5OjZsJBa9q3+GtIcZGmj+0xuOlw1uwKTyIppFtf9wDh8+EzudvxUgIxSf8pskf3kdIGGESaB8ryWtaPDzaqstuy1F7e352RYxliv2MfJH/i3CI17ti0Gr2ddFiC1OLIr0deJ8ffQCmvxB03tzq3Mi8dq8osL6PuP0SLjma0maE5Y61Kkd1FXcxCGtxIvhfZrL1+0EXH5WovBi1x3NV+QscMlFi1cirIvlnNsFRF2EWcbRNXN3Vt7HWkyN64qgMDjO2p/FdYTIzUzndHKxnySRKbLiq2u61088di41+v5QeTryUiWE4IINckGx+4Mi0qiQkX3XqlTdxJCcGOroS78FIYYNFiY8SJM1Jo/JChf8YGMkIaKzk/wiM/KK+0D3pRu7ldwI9fmWS+aUHdC9MURPnD3a/AwQJJdxrswjydQiuJHgZfOYHbjRkkONou15t+cMDhYW6+f2Wm/NIe/q89jkkD8o/yLkNO0EqDoWDmxd06naO/APKQrh4o1f5JyXjIV7OnbUZMkE+liioGVz9zEY9KdicBtEZAoTd+2FDmOy0gHZw37jrgQFoDEP1TDc3Fh+T+kyjvey74Jg/Y=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fda850dd-b24a-4563-4eb6-08dc7b633195
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4429.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 May 2024 20:01:54.5237
 (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: 9QRhk5ZKrUpD3MUqpgUV5+SlnmFWOSS5Ws2w2W/RYRozRsA9mE5LzUS4Hhon6jewfdvYshmb5rnRJzmvYoxB8w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB6149
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-23_11,2024-05-23_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0
 mlxlogscore=999 bulkscore=0 spamscore=0 malwarescore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405230138
X-Proofpoint-ORIG-GUID: f4BWqgr6GTnX8Xa2qbNGvtizObR-nNiM
X-Proofpoint-GUID: f4BWqgr6GTnX8Xa2qbNGvtizObR-nNiM

On 5/22/2024 8:02 PM, Miaohe Lin wrote:

> On 2024/5/22 7:54, Jane Chu wrote:
>> While handling hwpoison in a THP page, it is possible that
>> try_to_split_thp_page() fails. For example, when the THP page has
>> been RDMA pinned. At this point, the kernel cannot isolate the
>> poisoned THP page, all it could do is to send a SIGBUS to the user
>> process with meaningful payload to give user-level recovery a chance.
>>
> Thanks for your patch.
>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>> ---
>>   mm/memory-failure.c | 35 ++++++++++++++++++++++++++++++-----
>>   1 file changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 794196951a04..a14d56e66902 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1706,7 +1706,12 @@ static int identify_page_state(unsigned long pfn, struct page *p,
>>   	return page_action(ps, p, pfn);
>>   }
>>   
>> -static int try_to_split_thp_page(struct page *page)
>> +/*
>> + * When 'release' is 'false', it means that if thp split has failed,
>> + * there is still more to do, hence the page refcount we took earlier
>> + * is still needed.
>> + */
>> +static int try_to_split_thp_page(struct page *page, bool release)
>>   {
>>   	int ret;
>>   
>> @@ -1714,7 +1719,7 @@ static int try_to_split_thp_page(struct page *page)
>>   	ret = split_huge_page(page);
>>   	unlock_page(page);
>>   
>> -	if (unlikely(ret))
>> +	if (ret && release)
>>   		put_page(page);
> Is "unlikely" still needed?
I'd say not, because this code is not on performance sensitive code path.
>>   
>>   	return ret;
>> @@ -2187,6 +2192,24 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>   	return rc;
>>   }
>>   
>> +/*
>> + * The calling condition is as such: thp split failed, page might have
>> + * been RDMA pinned, not much can be done for recovery.
>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>> + * application has a chance to recover. Also, application processes'
>> + * election for MCE early killed will be honored.
>> + */
>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>> +				struct folio *folio)
>> +{
>> +	LIST_HEAD(tokill);
>> +
>> +	collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>> +	kill_procs(&tokill, true, pfn, flags);
>> +
>> +	return -EHWPOISON;
>> +}
>> +
>>   /**
>>    * memory_failure - Handle memory failure of a page.
>>    * @pfn: Page Number of the corrupted page
>> @@ -2328,8 +2351,10 @@ int memory_failure(unsigned long pfn, int flags)
>>   		 * page is a valid handlable page.
>>   		 */
>>   		folio_set_has_hwpoisoned(folio);
>> -		if (try_to_split_thp_page(p) < 0) {
>> -			res = action_result(pfn, MF_MSG_UNSPLIT_THP, MF_IGNORED);
>> +		if (try_to_split_thp_page(p, false) < 0) {
>> +			res = kill_procs_now(p, pfn, flags, folio);
> No strong opinion but we might remove the return value of kill_procs_now as
> it always return -EHWPOISON? We could simply set res to -EHWPOISON here.
I like that, will change.
>
> Besides from above possible nits, this patch looks good to me.
> Acked-by: Miaohe Lin <linmiaohe@huawei.com>
> Thanks.

Thank!

-jane

> .
>