Re: [PATCH 2/7] mm/thp: try_to_unmap() use TTU_SYNC for safe DEBUG_VM splitting

From: Peter Xu <peterx@redhat.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Yang Shi <shy828301@gmail.com>,
	Wang Yugui <wangyugui@e16-tech.com>,
	Matthew Wilcox <willy@infradead.org>,
	Naoya Horiguchi <naoya.horiguchi@nec.com>,
	Alistair Popple <apopple@nvidia.com>,
	Ralph Campbell <rcampbell@nvidia.com>, Zi Yan <ziy@nvidia.com>,
	Miaohe Lin <linmiaohe@huawei.com>,
	Minchan Kim <minchan@kernel.org>, Jue Wang <juew@google.com>,
	Jan Kara <jack@suse.cz>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/7] mm/thp: try_to_unmap() use TTU_SYNC for safe DEBUG_VM splitting
Date: Fri, 4 Jun 2021 10:48:37 -0400	[thread overview]
Message-ID: <YLo9RZOrCrp/1f4D@t490s> (raw)
In-Reply-To: <alpine.LSU.2.11.2106031945280.12760@eggly.anvils>

On Thu, Jun 03, 2021 at 07:54:11PM -0700, Hugh Dickins wrote:
> On Thu, 3 Jun 2021, Peter Xu wrote:
> > On Tue, Jun 01, 2021 at 02:07:53PM -0700, Hugh Dickins wrote:
> > > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> > > index 2cf01d933f13..b45d22738b45 100644
> > > --- a/mm/page_vma_mapped.c
> > > +++ b/mm/page_vma_mapped.c
> > > @@ -212,6 +212,14 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> > >  			pvmw->ptl = NULL;
> > >  		}
> > >  	} else if (!pmd_present(pmde)) {
> > > +		/*
> > > +		 * If PVMW_SYNC, take and drop THP pmd lock so that we
> > > +		 * cannot return prematurely, while zap_huge_pmd() has
> > > +		 * cleared *pmd but not decremented compound_mapcount().
> > > +		 */
> > > +		if ((pvmw->flags & PVMW_SYNC) &&
> > > +		    PageTransCompound(pvmw->page))
> > > +			spin_unlock(pmd_lock(mm, pvmw->pmd));
> > >  		return false;
> > >  	}
> > >  	if (!map_pte(pvmw))
> > 
> > Sorry if I missed something important, but I'm totally confused on how this
> > unlock is pairing with another lock()..
> 
> I imagine you're reading that as spin_unlock(pmd_lockptr(blah));
> no, the lock is right there, inside spin_unlock(pmd_lock(blah)).

Heh, yeah... Sorry about that.

> 
> > 
> > And.. isn't PVMW_SYNC only meaningful for pte-level only (as I didn't see a
> > reference of it outside map_pte)?
> 
> But you are pointing directly to its reference outside map_pte()!

Right, I was trying to look for the lock() so I needed to look at all the rest
besides this one. :)

I didn't follow Yang's patch, but if Yang's patch can make kernel not crashing
and fault handling done all well, then I'm kind of agree with him: having
workaround code (like taking lock and quickly releasing..) only for debug code
seems an overkill to me, not to mention that the debug code will be even more
strict after this patch, as it means it's even less likely that one can
reproduce one production host race with DEBUG_VM..  Normally debugging code
would affect code execution already, and for this one we're enlarging that gap
"explicitly" - not sure whether it's good.

This also makes me curious what if we make !DEBUG_VM strict too - how much perf
we'll lose?  Haven't even tried to think about it with details, but just raise
it up. Say, is there any chance we make the debug/non-debug paths run the same
logic (e.g. of SYNC version)?

-- 
Peter Xu