Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <c1b13519-88c4-3e43-eedf-9b848877ade4@oracle.com>
Date:   Mon, 31 Jan 2022 11:24:45 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH] mm/hwpoison: Check the subpage, not the head page
Content-Language: en-US
To:     Matthew Wilcox <willy@infradead.org>,
        =?UTF-8?B?SE9SSUdVQ0hJIE5BT1lBKOWggOWPoyDnm7TkuZ8p?= 
        <naoya.horiguchi@nec.com>
Cc:     David Rientjes <rientjes@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20220130013042.1906881-1-willy@infradead.org>
 <dcfa1ee5-1512-5e49-92c2-4a33ab59080@google.com>
 <Yfb/rVg1HYHkKBgO@casper.infradead.org>
 <20220131054432.GA856839@hori.linux.bs1.fc.nec.co.jp>
 <YffjC9+JsrZB5ekr@casper.infradead.org>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <YffjC9+JsrZB5ekr@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MWhwK3lWM2ZxUWNZaTdVR3h0UllyRE92WDhLQ3hCbDZzRW45eHY1LzZ5NGdy?=
 =?utf-8?B?M0VjU3l2ejhYWDhsVGg4MitrVmNUazZoNUVUVm83V0VaRVNZTkw4dGJ6Mjk2?=
 =?utf-8?B?Vkg2S3BDaWpQZUk4Y3d0cytFMXdScURpRUV6c0JqQ0pVN0pKT3F1L3ZreDFm?=
 =?utf-8?B?Z0FKa1NmTE9nMTkyRDBjcFJIbUd0S01xNHB2RnlvSWxLL3prWG5sNkhVdUhC?=
 =?utf-8?B?VTFwZlkzZlJUQ0kvOEd2Ky9DbTQvWlZ0Ym16VW5RNFFwSmowbS9KQVE4dTRy?=
 =?utf-8?B?TGJmYmVIMEFNUnlpcW55L1VCbEdIQkpOdllKYXRyM09RK1RPZEhtakNEa3Iy?=
 =?utf-8?B?SzU0Z0g5d1QzM1ltbzkzcXhsV2p0aGJNWEp6N0NEbzdrVDlpS2daRzl0V2lm?=
 =?utf-8?B?Q0EyQ1gvOG9XV1FXZ3hvRFN0MlcyZTBTWG1GRDV3bUE5SlpYbnZZVUVYNklp?=
 =?utf-8?B?UlRJNlY4UHEwUGdMN1JHM3VtdXhKUER3RDZNVzhTL2JYaE9zRC9UV28xWHg0?=
 =?utf-8?B?RjAyYmtFbzZjL2pkSFkvcER0SlpFc1ZIMGRuVjJZR0hBdis0Tll5TnBSWHFO?=
 =?utf-8?B?a3ZxNjFrTmpyV3dqVFlRcCt3NllwUUVYYUl1aVFIbk9BbU11WWwrWlA4Yzgw?=
 =?utf-8?B?ejgvM1ArbWFidlpJVmRMaTNObTJQczZDL2NZTVgxeGw3LzVkdnMwVFpBeVJK?=
 =?utf-8?B?cWdYK1NZN3ZvRHIvWkxoblBBWE1heWdKY2hiTE5nbFlJbW1LMnVzOHpTejI4?=
 =?utf-8?B?djNnemVDRG90cFBkQkdBVkQxV1lXK2E0R1JIS2U2UTNKQ3ZKZWVRMHBFOG4w?=
 =?utf-8?B?Ym41TytqQ0tLMGozWEVEWmZrRFJqeDhrMEcwRzZocFVhamE2T3AvUEhobXFw?=
 =?utf-8?B?VVF6MmVTQVhBSm9zR2pJQ29sS3hTS1Q2UW5qUGZYdTdVUTZkdVBtWmVUeTA2?=
 =?utf-8?B?RVozdHlDcUlXL2ZYMW5KQ0kwbHY2YmtTczA2bHYvMGV1UzUxVkM4Z0UrSitR?=
 =?utf-8?B?Rk16MlNsL1oyZmd3TmhTeE5UekFJRTBCSUxNZE9LaUprS2g2QmJHU1pnWXNa?=
 =?utf-8?B?V3dLdVVMM2hzdEhNeFlkYXlvdUFjK0V6Q0RqaEZ1aUkzMXIvWEpVRlFaczQr?=
 =?utf-8?B?Mm9ZbHRBSk12M3JCdWJuand1ajRvVDdNcTlQNlR4MGU5cTRQQkJrR2tvZyt0?=
 =?utf-8?B?Tk9menFhWkRJbVFJdTR0TlRHdVVDYjNHNmFNNGtJc3ZUdWdGa0toMWtvV0pT?=
 =?utf-8?B?bzFJYXp2MktKWEE1Y3NTdUYrOG9RcWxNZ21Lamo2OHFiWVhDU3hpSkRaMzQx?=
 =?utf-8?B?WGFHdzdra3ZXNUdmSFFOY3ZPM24rSDlBcGpLSG5wTWpWeGZlUkd0Z3dEVmZm?=
 =?utf-8?B?dCtsczJhcDZlcFY5bzJ2ZmxUdVI3Ync5aFpDdkZNakJIT1Noa3FkaUNGTllV?=
 =?utf-8?B?S1pTMDR4VXYrTzEwZENCYndXRjM5MklXN2ZkekJVUzZWVjFHT0d6WGNLelJD?=
 =?utf-8?B?T0tEN3hES2trWEVtWXB2RDk2MEtJVlREeW1OaUlOU2FObTN0WC95RlUwNWRD?=
 =?utf-8?B?Z2JTdTdGdnVnK29UZHFIQlRPaE51QkFaZ01ibTZwUDRLYURoQXd4NFhoYks5?=
 =?utf-8?B?U1lLc3NMVVJqcnRDREJmUmNZLzRXb3RzcFBUOXZUYkhibGtGOW84VE10bHd6?=
 =?utf-8?B?UjhtbDRtZGF0RS8rdldTYWovaGd6QVJqU1pNSlZaUXZkWVF5WTg3Ry80UVli?=
 =?utf-8?B?T3VnbHBueFpYdHhCMmtzY3dFWU13REh4Z3FUelA1dGI5WUFGd0d4MVRwdFgr?=
 =?utf-8?B?aThPcUZaeE95NExZdGRqcmQvSis3VFRWOG9zV1BvT3lqcFBLeVBMbGp0QUhw?=
 =?utf-8?B?YkVJSzRSSGwvR2Z3WURvQWtHckZoNlFZTnFyclNSdUJSUHlhL0hDWUs1UTY1?=
 =?utf-8?B?UTNIcmZ6cHpUN0dJank2OEt2QTVoRnB5d3ZvSHBYSVhtY2l3VVZIaTRra1lL?=
 =?utf-8?B?NTZ1RUZPTTExWmRVUDArb2Y1QmtvZ1hmUENQUE5LNkhiR1Zqb2Q2YVhaMlBM?=
 =?utf-8?B?ZGthaGRSdHo0Yytvb1F1VnhxN2VvOW1aZGJGeDNQcDV2Um9xWGxHck9SS0FL?=
 =?utf-8?B?RmdzYU1nUVVKR0U3SUY3dkgzV05iSk5pZTV4d3BwbkVpWHJ5NzNqNm9vb1Ev?=
 =?utf-8?Q?Q0eLEhbNYzdKPdODfQH8/yE=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: db56176e-a98e-45c3-3000-08d9e4ef57f8
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2022 19:24:47.5241
 (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: 7DGWBOYCb+Z5kgveV83usODUHuX0b9GlZ4DJ+CUeFcQBM3fHjG7knSYyC3er5qBUPTjGoeE20dtUEdLKmP3R/Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3467
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10244 signatures=673430
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0
 suspectscore=0 malwarescore=0 bulkscore=0 mlxscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2201310124
X-Proofpoint-ORIG-GUID: tHF1hs74Ncfc8_wLCf7Rm8kKYbvQCxrW
X-Proofpoint-GUID: tHF1hs74Ncfc8_wLCf7Rm8kKYbvQCxrW
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/31/22 05:24, Matthew Wilcox wrote:
> On Mon, Jan 31, 2022 at 05:44:35AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
>> On Sun, Jan 30, 2022 at 09:14:21PM +0000, Matthew Wilcox wrote:
>>> On Sun, Jan 30, 2022 at 12:58:17PM -0800, David Rientjes wrote:
>>>> On Sun, 30 Jan 2022, Matthew Wilcox (Oracle) wrote:
>>>>
>>>>> Hardware poison is tracked on a per-page basis, not on the head page.
>>>>>
>>>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>>>>> ---
>>>>>  mm/rmap.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>> index 6a1e8c7f6213..09b08888120e 100644
>>>>> --- a/mm/rmap.c
>>>>> +++ b/mm/rmap.c
>>>>> @@ -1553,7 +1553,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>>>>  		/* Update high watermark before we lower rss */
>>>>>  		update_hiwater_rss(mm);
>>>>>  
>>>>> -		if (PageHWPoison(page) && !(flags & TTU_IGNORE_HWPOISON)) {
>>>>> +		if (PageHWPoison(subpage) && !(flags & TTU_IGNORE_HWPOISON)) {
>>>>>  			pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
>>>>>  			if (PageHuge(page)) {
>>>>>  				hugetlb_count_sub(compound_nr(page), mm);
>>>>> @@ -1873,7 +1873,7 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
>>>>>  			 * memory are supported.
>>>>>  			 */
>>>>>  			subpage = page;
>>>>> -		} else if (PageHWPoison(page)) {
>>>>> +		} else if (PageHWPoison(subpage)) {
>>>>>  			pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
>>>>>  			if (PageHuge(page)) {
>>>>>  				hugetlb_count_sub(compound_nr(page), mm);
>>>>
>>>> This looks correct.  Correct me if I'm wrong that this is for consistency 
>>>> and cleanup and that there is no bug being fixed by this, however.
>>>
>>> Oh, no, I think there's a real bug here.  It's just that we're looking
>>> at an uncommon & hence rarely-tested scenario -- a memory fault in the
>>> middle of a THP (in mainline; obviously it'll be a little more common
>>> with arbitrary sized folios).  I don't do HWPoison testing myself, so
>>> this was by inspection and not from testing.  A scenario where things
>>> would go wrong is a memory error on a non-head-page would go unnoticed
>>> when migrating or unmapping.  Contrariwise, if there's a hardware error
>>> on a head page, all the subpages get treated as poisoned, even though
>>> they shouldn't be.
>>
>> Thank you for reporting.  As you point out, the current check does not
>> handle thp properly.  The reason of checking head page here is to handle
>> hwpoisoned hugetlb (which has PG_hwpoison on the head page even if the error
>> is on any of tail page).  So I think that the proper fix is to add a helper
>> function to check page type (normal, thp, or hugetlb) as well as PageHWPoison.
> 
> I think this handles HugeTLB pages correctly:
> 
>                 subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte);
> 
> As I understand the HugeTLB code, pvmw.pte refers to the head page, not
> the subpage (unlike a PTE-mapped THP page, where it would refer to the
> precise page).  But I cheerfully admit that the intricacies of the
> HugeTLB code are not something I'm an expert on.

Your understanding is correct.  Here is the comment for the routine which
sets pvmw.pte.

 * For HugeTLB pages, @pvmw->pte is set to the relevant page table entry
 * regardless of which page table level the page is mapped at. @pvmw->pmd is
 * NULL.

Just another thought.  With new hugetlb vmmemmap optimizations, it is not
possible to set poison on hugetlb tail pages until after allocating struct
pages.
-- 
Mike Kravetz