Received-SPF: pass (google.com: domain of linux-kernel+bounces-160588-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <c40cfd0b-f045-4887-a955-fee7e0392cf1@oracle.com>
Date: Fri, 26 Apr 2024 11:53:01 -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>
 <d05cbf22-f3e8-414a-a2e3-03e0b857eaca@oracle.com>
 <ZivyC3vqa2BIBoMj@casper.infradead.org>
Content-Language: en-US
From: Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <ZivyC3vqa2BIBoMj@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?S093YTU5UU9nWGJZMzJCb2ZPallORVdkb1luWWpjOWdwMmxWb00rNjBVYk42?=
 =?utf-8?B?aDNoSXkvby9BQzJjV2sxZytDKy9sMlJuVGUzWGNZK3MxN0ZBUnoxMnZKMkc4?=
 =?utf-8?B?YmZoeEJISkdyaGt2bC90Qk80WWxqSVFWR2R5dENvem11aFg1Y0Y2ckY5Njdi?=
 =?utf-8?B?TWIzQnRKVnJEdXUwMDFXdmY0T3JpVVlseHJaSUhJOHJwR3IzK0FDdHNEVThv?=
 =?utf-8?B?bGN1dHdvOU56RE43a25uMHVMTWNtWXVGUmhPNnRHdFh2OTI2VnAwQTBldUlD?=
 =?utf-8?B?azYraCtoVVIyWUNRSnR4V0N4RDJ0TFg0OFh6czZDL3pvT1crSUg1bGZoTmU5?=
 =?utf-8?B?N1Q3WmpsaFQ0RWlGSGdiRlBTQVBaeVVTZFk0VSs0UHhoMi9vMlVzYVZTZURD?=
 =?utf-8?B?bW9vT1dpUVV5UTdoa25Kd0xBSjV2Wk9MTXhjTTg0WXhJTVdSSUJwWnpGM0tZ?=
 =?utf-8?B?UDJ6bWZVYUZYRjJuMWNqZ2NDc3hJSTZqcG4xM1N4dUJvdnZMSVBPSUlXSUdy?=
 =?utf-8?B?VXFHREplaDJlSi9yYk1pRUNISFQ0YUNnQU5lazlDVjIrN1l3RTFSQWZyVGpB?=
 =?utf-8?B?ZXh3WktXUW10REQycnBiUXJaRisvdlp6WXlYVmJXczVaclRYK011WGErL1E4?=
 =?utf-8?B?aUIwTUZtbjZFaWZ5VHhMTXZEbG9kWnpSM2R4djBEYlNTYkJ2WG5sZUVjV24y?=
 =?utf-8?B?ZW1iZVVVZzBrTmFiR0RrWUVhRlFacGhOWENOQjRrKzZSUitqMnc1Q2RiYUpP?=
 =?utf-8?B?ZmJZRkZObkhmdGNkcmt4dGZtU0FzU0ttYWpIVTVINjE0YkY3UXBrVG5Ub0lV?=
 =?utf-8?B?MnhxWU5pSC8wWlY3R0dSbzBtMXBhT0lLWEFPL2dFN3J6eGNyejhQVU4xWE9L?=
 =?utf-8?B?YWpZdG1aOEJqTmxwS1BjOG5RS2QxdXBjMEtjeDY4cFo1eFp1V0NkOGhJUFVV?=
 =?utf-8?B?b216QjI0UFlHSDd5ZVR1TXBTNHdKQVFBVllpT2hkNVhYaVZLNVlJZWxpdGRH?=
 =?utf-8?B?TThGVFA5QjVidUFOQWtZcjFxcmdXTDdUaUZNL0x1cmpIU0J6SDZDdU9XZzRm?=
 =?utf-8?B?UnMzdUpTenNPQWxmT1RCNmdCeE9XN0FRVnNGTUIyMUdDenFmK3BoMEhrMzg5?=
 =?utf-8?B?OWwzUWpFenp6WUducDk1QVhoK25iWEhaNTB2Qm9DSW1NUEExSmw5NjFiRjFp?=
 =?utf-8?B?MnpTZlp2S20yTnRReW8xQ25qb3VmUUdBVHl4alphOFc4NUR1R3g0Q1YwRGxv?=
 =?utf-8?B?d1ZZNFBncWt2VTB2UkpXbEh3clA3SXByM0FtelVEZmUweU5PNi8ra1BxYUZM?=
 =?utf-8?B?QlQrUnJkVzRiUkxCRHlBK2wyZkdLMVEyK2w2SnVVMGY0UkFiMDRnQi9WUWYw?=
 =?utf-8?B?cjAycXBLQ2Ewa043Vzd0YnpKNE1HQVRMQXU3YXNoQkI5M1ZuajJjUC84N2Rs?=
 =?utf-8?B?eTBWUG5vZm5PU09mVzliYzB2aXdRWGo0bVgycnNpdnJzTy9xUlFpYm0rZ2ZS?=
 =?utf-8?B?VWxTWEtORSsyUmt3OE5sbVlPY0JISkZDMmtudmR5UmdzT3ZMcE9lQURSSGxU?=
 =?utf-8?B?TTN4Mm1oM2VWdWlESjVSbFFUODQrb0krZlR5SU44SXNzdVd1ekhITURKRDVq?=
 =?utf-8?B?N3lvaGwwR1JIVE1JOVk5L1ZOTXZJOGl2bnBKWC85S09UNU41QkRKWFEzYlZR?=
 =?utf-8?B?bXlXZVNqV1c0RXRCajlzN2l1NUM5MnFIWG9SVFVsd1pDMWc1bWhmUjBDK3hP?=
 =?utf-8?B?REFBeDdwV1lDNUhGLytpL2haT0pOKzRYclVYMHhnQVVsQkpLTmJSTXNReTU0?=
 =?utf-8?B?bW44TlRtTlFSSUR2QnFsQ0R3aEphcmNieE15ajdNcEphN2tPR1oxSldWOGtB?=
 =?utf-8?B?eGhUakQvNy9BRWkzSHlDbCtLVktJcVgvZjBRUTJoaGk0YitTQlRtMUx2NEIz?=
 =?utf-8?B?dThBa2k3dlFUVlRsTDRDeWwreTBrbUVBNCs1ODhQNmZXeXFUZHhkSCs2T205?=
 =?utf-8?B?K3NkZHVaSUswWTk0czRFUUpZNDVYREJ1TGFWWDk3eW9wSWpVTFJLQ0RUOVhz?=
 =?utf-8?B?RVRKeE5xWDF2RjNGZG55U1lrZ0NjK3I0MU1IZnN2dXRUdzNPOHh2djc2TWt2?=
 =?utf-8?B?RFV6ODFaSEFzclZxT21Ramowd3ZWRlZ1WVFHN2l6RUo0YndCMkhVcnpCbGJR?=
 =?utf-8?Q?Q+3Pr4IURo/1ItRzkBH7BeU=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	jGTsKYyTBnJIBehVKH1exudaYRqRCdwcYRmeW4EjCYi0FqMTDVmGRmfqx3TYvExUCp3gMyIi6S6z4ZEPBc179OLpuibgM7aGAp3W8iPFDFwX4ZtRyj7Y2T2XO1+uk1CvW6Ja2COwQecBC8naLjvHmh5tHPbvnz9uJMUyggCX4O1xPVk/cd7m9mAnlFkPNUCEsyrXvhzLvQ70PupgsM/vBH7mBmaVNa2VZPzq6V1sHewZBQh/+DYcuaDtazSKKIM5HjRQCJqQHnntBjGBA3uV0BexK1S47UrriI2noAuLuwlTjTQraoEjKq3MTwEwBxgSPVQx59t0qvZp9jOQ4diruoyTJtUMowgpulwhAuQeFGiZFTOo3BNWSq24b96MqLyBp8mweCHFFSsYRHzr7/WUCGdg0WyYlmPVAQdzIHwuZUoU/++dDcbV/JRzOVycjZ4l28809OBaOuQfFtj+ZNVV6JTc1uLzOr321Q2wAZwGcg3WmFDMdbZMz8ZtHLAYZsQdRPxKB1S99p8FX8Z2958aSnolw1cfbBWIH2FB3MkB61Zwfz+hmfD3FkTAdacdGb6IrBWQfjBL43MytrDY4PXjxkbvqj+4vmJ4Fz7BuQiqHT4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fc22077a-9358-44a4-d410-08dc66221abd
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 18:53:04.5085
 (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: 2tOCW0QvDnkY5gkdxdJLp5LclHyHK5n8Ss4ZdBzCLt9ZLr3VoagvxVD6iI24xZg7YL9twf27c+cDUu1roM3vpHkanFqKqp43g1+SMyODmEg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB6869
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 phishscore=0 mlxlogscore=982
 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-2404260129
X-Proofpoint-GUID: qo9WyMshD4RD0p5JkdzNJXx_MP_i2Q5m
X-Proofpoint-ORIG-GUID: qo9WyMshD4RD0p5JkdzNJXx_MP_i2Q5m

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 */

> 
> Because we don't have a reference on the folio at this point (how could
> we?), the folio might be split, and now we have a pointer to a folio
> which no longer contains the page (assuming we had a hwerror in what
> was a tail page at this time).