Re: [RFC PATCH v2 01/16] mm,hwpoison: cleanup unused PageHuge() check

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
To: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>,
	"mike.kravetz\@oracle.com" <mike.kravetz@oracle.com>,
	"linux-mm\@kvack.org" <linux-mm@kvack.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2 01/16] mm,hwpoison: cleanup unused PageHuge() check
Date: Tue, 12 Nov 2019 17:52:58 +0530	[thread overview]
Message-ID: <87d0dxs2ql.fsf@linux.ibm.com> (raw)
In-Reply-To: <20191021070046.GA8782@hori.linux.bs1.fc.nec.co.jp>

Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> writes:

> On Fri, Oct 18, 2019 at 01:48:32PM +0200, Michal Hocko wrote:
>> On Thu 17-10-19 16:21:08, Oscar Salvador wrote:
>> > From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> > 
>> > Drop the PageHuge check since memory_failure forks into memory_failure_hugetlb()
>> > for hugetlb pages.
>> > 
>> > Signed-off-by: Oscar Salvador <osalvador@suse.de>
>> > Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> 
>> s-o-b chain is reversed.
>> 
>> The code is a bit confusing. Doesn't this check aim for THP?
>
> No, PageHuge() is false for thp, so this if branch is just dead code.

memory_failure()
{

	if (PageTransHuge(hpage)) {
		lock_page(p);
		if (!PageAnon(p) || unlikely(split_huge_page(p))) {
			unlock_page(p);
			if (!PageAnon(p))
				pr_err("Memory failure: %#lx: non anonymous thp\n",
					pfn);
			else
				pr_err("Memory failure: %#lx: thp split failed\n",
					pfn);
			if (TestClearPageHWPoison(p))
				num_poisoned_pages_dec();
			put_hwpoison_page(p);
			return -EBUSY;
		}
		unlock_page(p);
		VM_BUG_ON_PAGE(!page_count(p), p);
		hpage = compound_head(p);
	}

}

Do we need that hpage = compund_head(p) conversion there? We should just
be able to say hpage = p, or even better after this change use p
directly instead of hpage in the code following?

-aneesh