From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 E90B93A544A for ; Tue, 31 Mar 2026 19:59:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774987166; cv=none; b=ekewFI3uUEh2BCdsufLXBjbCA3Q7td8FfMyiuQxKt0SL1F7AIk+N0JukkZSdRCfC/AJFiJMRtbBpQsugD0S0yj7pfuqlaRKCpRdbsf3lpXWQR9iXyU7ep/WPlgflTZge9viID39DMppq3REm7jnjSm0SbuaAc+kYZ6V0YH/cW7o= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774987166; c=relaxed/simple; bh=LxmMKzf7AC255UJDZa3vjLah4nTY2GHIEayHmzfCNyU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=k/GX2ZnsGXq8drcz9nYE9AGFdUBR/pnPFUeQka00INvuapMhoTo358b7iJjmOYmx9tJFnQDDx/a4p4B75V0Z0EQNInX79FTxmHWvQQeOiwoevMI8U3X13IRRirHpuujTeMZjKOn/2wmOVErcwBb5+PmDvTbYFF3Z4+pB5Ox+Mwg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=k1aYxgRT; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="k1aYxgRT" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C3B49C19423; Tue, 31 Mar 2026 19:59:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1774987163; bh=LxmMKzf7AC255UJDZa3vjLah4nTY2GHIEayHmzfCNyU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=k1aYxgRT++QNPITYjQqA9vhZIEjuCZ3w0zUX+YIrWF49Sp7bpST99vgbN0FXofsVO rmdnAY7cE6sWxVOqqwmQfeIeuA0RxMh30mvaSNl/SWtevqKTuivs9QSMUZuf3Rty+1 jehYrIJUWJp1p9SaQpGHqjWhW2MJimRdJmDrjvDETPny9EhbdJaxUEVFGgmeA3fN2+ w0BG5lPfdtOYdLmzppxja4R8N61QcuNsQDJjdoqmIJJjcHl1szACPumNndVdJdp+NJ 6LN0veNm1+9GKkJxYhaTjaJsMkU8lxMygzfOmIkUjxajvMSCZnolgTeNaNkBSwiPFA ufR6Q2lmV+xIA== Date: Tue, 31 Mar 2026 20:59:10 +0100 From: "Lorenzo Stoakes (Oracle)" To: Nico Pache Cc: "David Hildenbrand (Arm)" , linux-kernel@vger.kernel.org, linux-mm@kvack.org, aarcange@redhat.com, akpm@linux-foundation.org, anshuman.khandual@arm.com, apopple@nvidia.com, baohua@kernel.org, baolin.wang@linux.alibaba.com, byungchul@sk.com, catalin.marinas@arm.com, cl@gentwo.org, corbet@lwn.net, dave.hansen@linux.intel.com, dev.jain@arm.com, gourry@gourry.net, hannes@cmpxchg.org, hughd@google.com, jackmanb@google.com, jack@suse.cz, jannh@google.com, jglisse@google.com, joshua.hahnjy@gmail.com, kas@kernel.org, lance.yang@linux.dev, Liam.Howlett@oracle.com, lorenzo.stoakes@oracle.com, mathieu.desnoyers@efficios.com, matthew.brost@intel.com, mhiramat@kernel.org, mhocko@suse.com, peterx@redhat.com, pfalcato@suse.de, rakie.kim@sk.com, raquini@redhat.com, rdunlap@infradead.org, richard.weiyang@gmail.com, rientjes@google.com, rostedt@goodmis.org, rppt@kernel.org, ryan.roberts@arm.com, shivankg@amd.com, sunnanyong@huawei.com, surenb@google.com, thomas.hellstrom@linux.intel.com, tiwai@suse.de, usamaarif642@gmail.com, vbabka@suse.cz, vishal.moola@gmail.com, wangkefeng.wang@huawei.com, will@kernel.org, willy@infradead.org, yang@os.amperecomputing.com, ying.huang@linux.alibaba.com, ziy@nvidia.com, zokeefe@google.com Subject: Re: [PATCH mm-unstable v4 5/5] mm/khugepaged: unify khugepaged and madv_collapse with collapse_single_pmd() Message-ID: References: <20260325114022.444081-1-npache@redhat.com> <20260325114022.444081-6-npache@redhat.com> <7760c811-e100-4d40-9217-0813c28314be@lucifer.local> <0223d45b-e8b4-49a7-882b-477250d0c14d@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Tue, Mar 31, 2026 at 01:46:02PM -0600, Nico Pache wrote: > On Tue, Mar 31, 2026 at 8:13 AM David Hildenbrand (Arm) > wrote: > > > > >> + if (*lock_dropped) { > > >> cond_resched(); > > >> mmap_read_lock(mm); > > >> - mmap_locked = true; > > >> + *lock_dropped = false; > > > > > > So this is the bug. 'lock_dropped' needs to record if the lock was _ever_ > > > dropped, not if it is _currently_ dropped. > > > > Ah, well spotted. I thought we discussed that at some point during > > review ... > > Yes I think we've discussed this before, and IIRC the outcome was, "We > weren't sure why this semantic was in place, or if we truly needed to > track if it was dropped at any point, or simply if it is currently > dropped.". The code is rather confusing, and changed mid-flight during > my series. > > Lorenzo asked me to unify this semantic across the two functions to > further simplify readability, but it looks like we indeed needed that > extra tracking. > > > Cheers, > -- Nico > > > > > Thanks for tackling this! > > > > [...] > > > > > > > > ----8<---- > > > From a4dfc7718a15035449f344a0bc7f58e449366405 Mon Sep 17 00:00:00 2001 > > > From: "Lorenzo Stoakes (Oracle)" > > > Date: Tue, 31 Mar 2026 13:11:18 +0100 > > > Subject: [PATCH] mm/khugepaged: fix issue with tracking lock > > > > > > We are incorrectly treating lock_dropped to track both whether the lock is > > > currently held and whether or not the lock was ever dropped. > > > > > > Update this change to account for this. > > > > > > Signed-off-by: Lorenzo Stoakes (Oracle) > > > --- > > > mm/khugepaged.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > > index d21348b85a59..b8452dbdb043 100644 > > > --- a/mm/khugepaged.c > > > +++ b/mm/khugepaged.c > > > @@ -2828,6 +2828,7 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start, > > > unsigned long hstart, hend, addr; > > > enum scan_result last_fail = SCAN_FAIL; > > > int thps = 0; > > > + bool mmap_unlocked = false; > > > > > > BUG_ON(vma->vm_start > start); > > > BUG_ON(vma->vm_end < end); > > > @@ -2850,10 +2851,11 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start, > > > for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) { > > > enum scan_result result = SCAN_FAIL; > > > > > > - if (*lock_dropped) { > > > + if (mmap_unlocked) { > > > cond_resched(); > > > mmap_read_lock(mm); > > > - *lock_dropped = false; > > > + mmap_unlocked = false; > > > + *lock_dropped = true; > > > result = hugepage_vma_revalidate(mm, addr, false, &vma, > > > cc); > > > if (result != SCAN_SUCCEED) { > > > @@ -2864,7 +2866,7 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start, > > > > (in hurry so I might be wrong) > > > > Do we have to handle when collapse_single_pmd() dropped the lock as well? > > There are some oddities about madvise that sadly I don't fully > understand. This is one of them. However, I dont believe khugepaged > had these lock_dropped semantics and just tracked the state of the > lock. (As said to David) with my fix-patch we pass a pointer to mmap_unlocked and will set the upstream lock_dropped appropriately as a result :) So we cover that case also. But yeah the handling in madvise is weird, and we maybe need to rethink a bit at some point. > > > > > -- > > Cheers, > > > > David > > > Cheers, Lorenzo