From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-170.mta1.migadu.com (out-170.mta1.migadu.com [95.215.58.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F367C3D523E for ; Wed, 13 May 2026 06:51:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.170 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778655075; cv=none; b=PCTrZxegjMH8J9ghUcHKXPEUHp5S3HgFlczDkWarrNJl9QHrwvftE6wKVFUsW1cEqZj2pIphgHhlsJ5VpWBVq+CS/MtjnMg0eRXRRon3L8lwKctbsFD2QKuNqrHJG438a+3hmrxrftEBr+KwU3t3zV1CFKjAOXlLiYb5EK6ijrY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778655075; c=relaxed/simple; bh=rIf8SB1hzKrAo6YFUj3MCv1EQ8vBQ8bFBL5BXYOzWbQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=dmLeYMCOZz5Ysk2Tj0J+/l1Hd0Dm1iSj/aNN8cirvhw04fDfE1D0BjhtDaaIaoxasNdSGPCl1R3FvO1fA9EEdPam8Zb+miDn4AqSZxVXxxQfD0wJhSpteCVWk6SJAkUtoPiwxpp6FqVGa0jWpO1aU1wJj/R9h1vmly+tKVyZpwE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=NikSNfLX; arc=none smtp.client-ip=95.215.58.170 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="NikSNfLX" Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1778655070; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ljaFWWwsq2EiNVrMSOLQz4qax9kr/k7rePcEqZBtHwE=; b=NikSNfLXZbTPU9WgKh/NocBWEiUXLWGUl/CB2u/Vp65cRn25HiKUBv31q/MJ8nw90Nujo2 5EeBANeuVldlIOGLLmQZkLSwaeQHO4PzPBYZ3wpJPjxp71L6PRftoUIpoHY0JPtcYfBMwp Z6/az2eq4+PVLvS0MA2eGxT4zBklIOU= Date: Wed, 13 May 2026 14:50:56 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [RESEND PATCH] mm/memory_failure: use bool for hugetlb indicator in try_memory_failure_hugetlb To: Oscar Salvador Cc: Miaohe Lin , Andrew Morton , Ye Liu , Naoya Horiguchi , linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20260513024853.65566-1-ye.liu@linux.dev> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Ye Liu In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT 在 2026/5/13 11:36, Oscar Salvador 写道: > On Wed, May 13, 2026 at 10:48:52AM +0800, Ye Liu wrote: >> From: Ye Liu >> >> The hugetlb indicator in try_memory_failure_hugetlb is a Boolean >> flag, but was declared and assigned as int/0/1. Convert to `bool` >> and `true`/`false` for clarity and type safety. >> >> - try_memory_failure_hugetlb(unsigned long pfn, int flags, bool *hugetlb) >> - testcase path in memory_failure(): bool hugetlb = false >> - clear semantic conversion in MF_HUGETLB_NON_HUGEPAGE >> - preserve behavior (no functional change) >> >> Signed-off-by: Ye Liu >> Acked-by: Miaohe Lin >> --- >> mm/memory-failure.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 866c4428ac7e..f164fc5959f0 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -2032,7 +2032,7 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags, >> * -EHWPOISON - folio or exact page already poisoned >> * -EFAULT - kill_accessing_process finds current->mm null >> */ >> -static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb) >> +static int try_memory_failure_hugetlb(unsigned long pfn, int flags, bool *hugetlb) >> { >> int res, rv; >> struct page *p = pfn_to_page(pfn); >> @@ -2040,12 +2040,12 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb >> unsigned long page_flags; >> bool migratable_cleared = false; >> >> - *hugetlb = 1; >> + *hugetlb = true; >> retry: >> res = get_huge_page_for_hwpoison(pfn, flags, &migratable_cleared); >> switch (res) { >> case MF_HUGETLB_NON_HUGEPAGE: /* fallback to normal page handling */ >> - *hugetlb = 0; >> + *hugetlb = false; > > Do we really need this boolean though? > Why do not simply return ENOENT when we find a non-hugetlb page, and then the caller > knows that if we get ENOENT, it can proceed with normal page handling? > > I might be missing something, but is not the following more cleaer? > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index ee42d4361309..44f388df5731 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -2026,13 +2026,14 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags, > * So some of prechecks for hwpoison (pinning, and testing/setting > * PageHWPoison) should be done in single hugetlb_lock range. > * Returns: > - * 0 - not hugetlb, or recovered > + * 0 - recovered > + * -ENOENT - no hugetlb page > * -EBUSY - not recovered > * -EOPNOTSUPP - hwpoison_filter'ed > * -EHWPOISON - folio or exact page already poisoned > * -EFAULT - kill_accessing_process finds current->mm null > */ > -static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb) > +static int try_memory_failure_hugetlb(unsigned long pfn, int flags) > { > int res, rv; > struct page *p = pfn_to_page(pfn); > @@ -2040,13 +2041,11 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb > unsigned long page_flags; > bool migratable_cleared = false; > > - *hugetlb = 1; > retry: > res = get_huge_page_for_hwpoison(pfn, flags, &migratable_cleared); > switch (res) { > case MF_HUGETLB_NON_HUGEPAGE: /* fallback to normal page handling */ > - *hugetlb = 0; > - return 0; > + return -ENOENT; > case MF_HUGETLB_RETRY: > if (!(flags & MF_NO_RETRY)) { > flags |= MF_NO_RETRY; > @@ -2107,7 +2106,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb > } > > #else > -static inline int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb) > +static inline int try_memory_failure_hugetlb(unsigned long pfn, int flags) > { > return 0; > } > @@ -2386,8 +2385,11 @@ int memory_failure(unsigned long pfn, int flags) > } > > try_again: > - res = try_memory_failure_hugetlb(pfn, flags, &hugetlb); > - if (hugetlb) > + res = try_memory_failure_hugetlb(pfn, flags); > + /* > + * -ENOENT means the page we found is not hugetlb, so proceed with normal page handling > + */ > + if (res != -ENOENT) > goto unlock_mutex; > > if (TestSetPageHWPoison(p)) { > > Hi Oscar, Good point. Using -ENOENT to distinguish "not a hugetlb page" from "hugetlb handled" is indeed cleaner than carrying an extra output parameter. One thing to note: the #else stub when CONFIG_HUGETLB_PAGE is not set currently does: return 0; which with your change would mean "hugetlb handled, skip normal path" instead of the intended "not hugetlb, proceed with normal handling". It should be changed to: return -ENOENT; Otherwise the non-hugetlb config would silently skip all normal page failure processing. -- Thanks, Ye Liu