From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 50DF13A9630 for ; Sat, 11 Apr 2026 16:05:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775923562; cv=none; b=cAVOQKILjlTwAo3J6FRvNuX8oIGAkYsuXnBPwr/a2g1dU1H24xlIKdCYtimTLwhzGZ2ZwgcJDR2VWND9jP8Ll2HGuXSKUqUxSTIyc6LNSgVCGBEIHbs0vajeKQj+wRrjkc2uZyyPskFMhuLzDH5kUIPqJ596TJQNUHrJXsEUkPM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775923562; c=relaxed/simple; bh=YgGoVGTSUFJQ2wjFuJc6O6sq01Ubnj51Q2u2XY0BNqM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=axQbUyigi/GI5d6BReaOSYxVcPfcgcVBglPDKstjS8rRWvFm35jQLNJ4ui1tn115XRrkNwKlldn8miLPZWYrvPh7mkLrj30XNmRnB0UxWbkJOhx9l/RXUtoIE/w1ryYD7eFHFiIuqBkrdelKSA5jEFm7EX5eA2p/fvvRKyV8iW8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b=l81/NQEP; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=arm.com header.i=@arm.com header.b="l81/NQEP" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3F8DC1AED; Sat, 11 Apr 2026 09:05:52 -0700 (PDT) Received: from [10.163.141.179] (unknown [10.163.141.179]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E860A3F641; Sat, 11 Apr 2026 09:05:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1775923557; bh=YgGoVGTSUFJQ2wjFuJc6O6sq01Ubnj51Q2u2XY0BNqM=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=l81/NQEP3YyzwME1xAv3gfRk3cZAmVh8Z3BO7/U06YFd+LFGAzZ828zTKUqISKPQc Z/QLwB5+otpl5v3ME0lGnnBF+Z2cLZeahS58c5qJ1x2AvPPgvAgI8NtF2jB9jA2byl 1HGDDBsJbjAED6Wo5Fhv/vKC+J5b32N6aBu4mOvU= Message-ID: Date: Sat, 11 Apr 2026 21:35:41 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/9] mm/rmap: refactor hugetlb pte clearing in try_to_unmap_one To: Barry Song Cc: akpm@linux-foundation.org, david@kernel.org, hughd@google.com, chrisl@kernel.org, ljs@kernel.org, Liam.Howlett@oracle.com, vbabka@kernel.org, rppt@kernel.org, surenb@google.com, mhocko@suse.com, kasong@tencent.com, qi.zheng@linux.dev, shakeel.butt@linux.dev, axelrasmussen@google.com, yuanchu@google.com, weixugc@google.com, riel@surriel.com, harry@kernel.org, jannh@google.com, pfalcato@suse.de, baolin.wang@linux.alibaba.com, shikemeng@huaweicloud.com, nphamcs@gmail.com, bhe@redhat.com, youngjun.park@lge.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, ryan.roberts@arm.com, anshuman.khandual@arm.com References: <20260410103204.120409-1-dev.jain@arm.com> <20260410103204.120409-3-dev.jain@arm.com> Content-Language: en-US From: Dev Jain In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 11/04/26 2:25 pm, Barry Song wrote: > On Fri, Apr 10, 2026 at 6:32 PM Dev Jain wrote: >> >> Simplify the code by refactoring the folio_test_hugetlb() branch into >> a new function. >> >> No functional change is intended. >> >> Signed-off-by: Dev Jain >> --- >> mm/rmap.c | 116 +++++++++++++++++++++++++++++++----------------------- >> 1 file changed, 67 insertions(+), 49 deletions(-) >> >> diff --git a/mm/rmap.c b/mm/rmap.c >> index 62a8c912fd788..a9c43e2f6e695 100644 >> --- a/mm/rmap.c >> +++ b/mm/rmap.c >> @@ -1978,6 +1978,67 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio, >> FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY); >> } >> >> +static inline bool unmap_hugetlb_folio(struct vm_area_struct *vma, >> + struct folio *folio, struct page_vma_mapped_walk *pvmw, >> + struct page *page, enum ttu_flags flags, pte_t *pteval, >> + struct mmu_notifier_range *range, bool *walk_done) >> +{ > > Can we add a comment before the function explaining what > the return value means? Yes I can add that. > >> + /* >> + * The try_to_unmap() is only passed a hugetlb page >> + * in the case where the hugetlb page is poisoned. >> + */ >> + VM_WARN_ON_PAGE(!PageHWPoison(page), page); >> + /* >> + * huge_pmd_unshare may unmap an entire PMD page. >> + * There is no way of knowing exactly which PMDs may >> + * be cached for this mm, so we must flush them all. >> + * start/end were already adjusted above to cover this >> + * range. >> + */ >> + flush_cache_range(vma, range->start, range->end); >> + >> + /* >> + * To call huge_pmd_unshare, i_mmap_rwsem must be >> + * held in write mode. Caller needs to explicitly >> + * do this outside rmap routines. >> + * >> + * We also must hold hugetlb vma_lock in write mode. >> + * Lock order dictates acquiring vma_lock BEFORE >> + * i_mmap_rwsem. We can only try lock here and fail >> + * if unsuccessful. >> + */ >> + if (!folio_test_anon(folio)) { >> + struct mmu_gather tlb; >> + >> + VM_WARN_ON(!(flags & TTU_RMAP_LOCKED)); >> + if (!hugetlb_vma_trylock_write(vma)) { >> + *walk_done = true; >> + return false; >> + } >> + > > Sometimes I feel walk_done is misleading, since walk_done with > ret = false actually means walk_abort. I'll rename this to exit_walk, so it doesn't collide with the label names. > > So another option is to make this function return a tristate: > WALK_DONE, WALK_ABORT, WALK_CONT. Then we could drop the > walk_done argument entirely. I thought a lot about how to refactor try_to_unmap_one() as a whole, and couldn't come up with a good solution. There are these conditions: 1. ret = false => page_vma_mapped_walk_done(), break 2. ret not decided, "continue" 3. ret = true a) exit the while loop naturally b) exit prematurely -> page_vma_mapped_walk_done(), break I had thought about the refactoring method to have an enum for all conditions. So we can refactor bits of code, return an enum, but we will still retain ugliness like if (ret == WALK_DONE) goto walk_done; if (ret == WALK_ABORT) goto walk_abort; if (ret == WALK_CONTINUE) continue; This seemed more of a forced-refactoring to me, IMHO doesn't reduce the complexity of the function at all. I don't have a clever solution to get rid of all the label jumping, so I refactored what I could. > > Thanks > Barry