Received-SPF: pass (google.com: domain of linux-kernel+bounces-184085-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <78a04731-d585-4fdf-8af6-e9acac18b2d5@oracle.com>
Date: Mon, 20 May 2024 11:48:45 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 5/5] mm/memory-failure: send SIGBUS in the event of thp
 split fail
To: Oscar Salvador <osalvador@suse.de>
Cc: linmiaohe@huawei.com, nao.horiguchi@gmail.com, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20240510062602.901510-1-jane.chu@oracle.com>
 <20240510062602.901510-6-jane.chu@oracle.com>
 <ZkYARVW2cOZcsFYB@localhost.localdomain>
Content-Language: en-US
From: Jane Chu <jane.chu@oracle.com>
In-Reply-To: <ZkYARVW2cOZcsFYB@localhost.localdomain>
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?ZjRJZjUzZys2bDY1cWRqY0FpZWVCRXNHSXdVUVNjNy9rR0EvYmJpYXl5aDBK?=
 =?utf-8?B?cXNkblczMklmZnF6TmN1OUxkeVVQbVRlb2UydmVVYkx2SkxTS3doQlY1by9J?=
 =?utf-8?B?YXhaUi9YNzVQd25oV1FXeUNYdHJXY0JOQlgzSWpvWXlDY0MxYmRsT3NHcHpj?=
 =?utf-8?B?K0xwalFCcW1YRGNRY1hqdFNkR2gvbGNuWm9IRVB6TTBiTHl2OFYwdEEzWkJO?=
 =?utf-8?B?VjEyeTdMMzFaaERCZGlhNkFlUk5obVVLd0VzVXRVWXZPcitROFlxWldYeE5O?=
 =?utf-8?B?ZU42Y1FnYXA2aTdFRVp6VUZ6b213aURDYiswNmkwdm9IcHZDNEhBL0N3MW5T?=
 =?utf-8?B?aHNuNVprRUlWOE8vS3ExN0MvNFJOOCtwNzFDeWNiNVB0NDF1cWFZbkdoeFFV?=
 =?utf-8?B?eHZkSHgvUnRLd0VGN2RCQXdzV3MwUjBycEZvd3B3R3BZcjE2cG1ycDg0OXlF?=
 =?utf-8?B?dlVDUi9vL0txTHZENmE3OW1ISlhPMFZUMnA1ZGVON2k1VWU4TVJWdW1IQXlp?=
 =?utf-8?B?K1ZVMXBiajhNZktFWVp4VFc3M1BKMlF4UWd0bktSZElHOEFmTWd0dEJwL29u?=
 =?utf-8?B?YVQ3dk5Fdm1oaHVLWEdBQmhPOG9DQ3hhK3lUVHJqeTIzK3RpVWIvZWpUZVFm?=
 =?utf-8?B?VDQ1MzR2ZE4zdWJVMW5jbXNlWTE5cUxOeXhwN3ZOaDZ5ZngvOHY1djdvcVJr?=
 =?utf-8?B?RU9IZXJzMXJ6V2xKTTJ2Z1cvSEdpdFptbWtUblYrWFFWdUEvQWlBZ2IzMFdz?=
 =?utf-8?B?NCtFMU9iMnk0R25COVAzN0lrWTJwWlY5eXJ5ak12T3BwTmVVWWNWbkJ5cmp4?=
 =?utf-8?B?UUpzdHRoUVBlNjRrQUVDaTU4ZGxYeVVzS1V3NkxkaFRTZUdGdGREOHcwcEpV?=
 =?utf-8?B?YW5Kb0Q5REJDSGpEdmFUbHRhMnJ5SHBOQlk3dTZxVUpxUDgrbzJQL2xwUDRY?=
 =?utf-8?B?WVVEWGkwTE0wNXA2VE5HTEZIYkFYTFVpN0Raa1N6VkZNYjFBNzZ3bEhINiti?=
 =?utf-8?B?WWxaU0lXalJ1djFQdlJIbE5UcGM0Wmp2cE42Z3lwYjdTcHlmdGROL2plKzhZ?=
 =?utf-8?B?TGJDQWpVVU9XRGNOYmJDZnhtdFdXcnBNbkc1TDFoQnhVK2YzZjZCTG9LRmFJ?=
 =?utf-8?B?TEVSSFhTY0VjeHNuS2pvWTA2MllkTnU5S2x5VytneC9NM3JsWG50MGx2dHJj?=
 =?utf-8?B?WFBQU3ExYjFWZU4xRzlsbkdLVUgyMEZyS0xxV3p2Q1VqYUFpY1A0dWUwbjRM?=
 =?utf-8?B?QlFWN3VRbXFEVC8va0Q5TC9JRndobUFzc2VwWnFjSkFiSDhaaGNIWTVORm1L?=
 =?utf-8?B?Z0JKdjhLT3RrY2NyRVdQUHNSWHR1RVk2aVg1d3k4V0xMVTZreDN1ckNweUNp?=
 =?utf-8?B?L3B2c2x6d245NkYrQ0F3ZlR4cVFPM0Z6VzdGbjJ1YmNvclgyR1JoVGZ5RzEw?=
 =?utf-8?B?aGRocmo1SVNIK3E5SU1JWTZzbVhCS1hmM1F1OStraExZd3Bxd1Vub3Uwb3dD?=
 =?utf-8?B?Sm1TZkRiM3Fwa3d3YWQwRllIOU9zbURwMEV4N04wNEtyUlM1TDNhVS9GOEJj?=
 =?utf-8?B?akJBTkE3TEUyNVJsWnd2RDdSNUs2S3o0L0RlenRGdjdTSnQ2RENlNzh3aC9w?=
 =?utf-8?B?MWdDUE5BU0FoREFLSHdUUVlLZDU0cHVSeUcyWWs2L2xzRHd1UXNXY2lxeXdY?=
 =?utf-8?B?dXdyRGM1eDNtTGtnMHZsREd6NXlIOHRuaHhnM2ZFNFI4enF4VU1qK3dIV09H?=
 =?utf-8?B?NWZQeDR3ZExXdUxzWFZCbjBNdmQ2eXM5YW9saXJNYkJ4VUUzMEJUNER4dTg5?=
 =?utf-8?B?Qmp1TFhZa3RpN0N0TXhlUjJaMjdVNDZWR1J1YUJJdmZMWG8wT1M2NXNJWUdr?=
 =?utf-8?B?NUc2SnpQeXhwckxpM2kySjVYemN6R3hLcUV2dEsvTjFWcnJhYmQvS3JjK0hU?=
 =?utf-8?B?TVJpelJBVTRieHZybVcvelNSeW5QY3V6RDdJcWkycXFyNThnOGhzclpNeWJY?=
 =?utf-8?B?WFhIZCtsVlZranZRSmx4akJLTDdHenBKdGtXVTJxelNPUTJaUXJBWE1PSW1C?=
 =?utf-8?B?RVNCSE5tdXFaNEpaMlE5OTRxQ2dxT2tCY0t5STBGN29yK0RScDU0akJSYWp4?=
 =?utf-8?Q?B6vTcCHwuAX/WiA2re0+8Owt2?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	0W0GdkzBw3BKq1aSEQslXQ65BmbY+gkLSAyFhy2DyslpCoMhY3baKIHrQMm0WDk/SVXeVtAq3amvhrpohaAGS9vMNnDGpVMv7+ayvQejOdWSNHgjjMcozfNJ7KnHj6MyHaCIMPMcF34JR0SMtDoAzCU6bUkcvWhp9fBC6Zmh+vVEbS8ytCPnh/kL8YKFoifHgrDmix6jXWnl0Ref1JdsKOpkuAze/TClb4CPzBC58svdvoVkzDgZreQTSBpX2LwCfQH+vG6soq1QklGdld5XsJ3ew+QhmsS9JASyjbP/we4itrgpsuzd3tIKcXb5kYbNoXye4Q0Nh1CP1WUdvQhYeT38mujOCEJn7vdsRM/b/RqGPtVE8TqzoSXwg6FOoZMtMTdyj5tEG/Zk14AKcNWTt1gmCzIVlti6807PaB6wG9zdJgwrds/svB138RbfizL9O3H4PCnHithbrgFjD4yFhOdho1ARW6UseDjB5oi2lX7Bz/dVSdNeNagsswdASQRoyktyP0QUClmVVATUuz0M2DuS03+Zmdv6SHpBxP4HE4z/EwDBMOb7kuA08gotuxihZJl2qC6Nza8w+nqLn1o3QnVdQ284tpqOd64vpg4BE4c=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dc7ca02a-0def-4475-49fe-08dc78fd7b6b
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4429.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 May 2024 18:48:47.3266
 (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: EauZV/7L5MY0YKtZRJmJ1Mn4nwoxQLuzSVZ0MOxNQ4DhG32x3vbW0eRMTXAo1gQ0LHXEJ+LMbT5qjCvacy4jpg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB4154
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-20_09,2024-05-17_03,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0
 suspectscore=0 spamscore=0 mlxscore=0 adultscore=0 mlxlogscore=999
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405200150
X-Proofpoint-ORIG-GUID: vzFevCgC2AuQutiSsqVmjsONVw9QShtN
X-Proofpoint-GUID: vzFevCgC2AuQutiSsqVmjsONVw9QShtN

On 5/16/2024 5:47 AM, Oscar Salvador wrote:

> On Fri, May 10, 2024 at 12:26:02AM -0600, Jane Chu wrote:
>> When handle hwpoison in a RDMA longterm pinned thp page,
>> try_to_split_thp_page() will fail. And at this point, there is
>> little else the kernel could do except sending a SIGBUS to
>> the user process, thus give it a chance to recover.
> Well, it does need to be a RDMA longterm pinned, right?
> Anything holding an extra refcount can already make us bite the dust, so
> I would not make it that specific.

How about let me just mention RDMA longterm pin as one of the use cases?

To be honest, it is the only known case to me, and not all FOLL_LONGTERM 
pin

lead to THP split failure.

>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>> ---
>>   mm/memory-failure.c | 31 ++++++++++++++++++++++++++-----
>>   1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 2fa884d8b5a3..15bb1c0c42e8 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1697,7 +1697,7 @@ 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)
>> +static int try_to_split_thp_page(struct page *page, bool release)
>>   {
>>   	int ret;
>>   
>> @@ -1705,7 +1705,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);
> I would document whhen and when not we can release the page.
> E.g: we cannot release it if there are still processes mapping the thp.
Sure.
>
>> +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;
> You are returning -EHWPOISON here,

Yes, indeed.

>> +}
>> +
>>   /**
>>    * memory_failure - Handle memory failure of a page.
>>    * @pfn: Page Number of the corrupted page
>> @@ -2313,8 +2331,11 @@ 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) {
>> +			pr_err("%#lx: thp split failed\n", pfn);
>> +			res = kill_procs_now(p, pfn, flags, folio);
>> +			put_page(p);
>> +			res = action_result(pfn, MF_MSG_UNSPLIT_THP, MF_FAILED);
> just to overwrite it here with action_result(). Which one do we need?
> I think we would need -EBUSY here, right? So I would drop the retcode
> from kill_procs_now.

The overwrite was wrong, it should return -EHWPOISON to indicate to the 
caller (such as kill_me_maybe)

that no further action against the process is needed since the m-f() 
handler has killed the process.

>
> Also, do we want the extra pr_err() here.
> action_result() will already provide us the pfn and the
> action_page_types which will be "unsplit thp". Is not that clear enough?
>
> I would drop that.

Agreed, will drop the extra print.

thanks!

-jane

>
>