All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hugh Dickins <hughd@google.com>
To: Peter Xu <peterx@redhat.com>
Cc: Hugh Dickins <hughd@google.com>,
	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 15:26:19 -0700 (PDT)	[thread overview]
Message-ID: <alpine.LSU.2.11.2106041454430.14037@eggly.anvils> (raw)
In-Reply-To: <YLo9RZOrCrp/1f4D@t490s>

On Fri, 4 Jun 2021, Peter Xu wrote:
> 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.

I'll expand that line, as Kirill asks too.

> 
> > 
> > > 
> > > 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)?

And Yang Shi suggests the same.

Yes, I'm not fond of doing that differently for DEBUG_VM or not;
but could never quite decide which way to jump.

For so long as we worry about whether split_huge_page() is working
correctly (and Wang Yugui still has a case that we have not solved),
we do want the warning; and for so long as we have the warning, we
do need the TTU_SYNC to prevent showing the warning unnecessarily.

How much overhead added by doing TTU_SYNC now on !DEBUG_VM?  On any
sensible anon THP case, I don't think it could add overhead at all.
But in some shmem cases (multiply mapped but sparsely populated,
populated differently by different tasks) it could add overhead:
dirtying lock cachelines in tasks which don't have the page mapped.

But we're only talking about huge page splitting, that should not
be #1 on anyone's performance list; and has all sorts of other
overheads of its own.  I think I'll go with your preference, and
make this TTU_SYNC for all.  We can easily revert to DEBUG_VM only
if some regression is noticed.

Hugh

  reply	other threads:[~2021-06-04 22:27 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-01 21:03 [PATCH 0/7] mm/thp: fix THP splitting unmap BUGs and related Hugh Dickins
2021-06-01 21:03 ` Hugh Dickins
2021-06-01 21:05 ` [PATCH 1/7] mm/thp: fix __split_huge_pmd_locked() on shmem migration entry Hugh Dickins
2021-06-01 21:05   ` Hugh Dickins
2021-06-03 21:26   ` Yang Shi
2021-06-03 21:26     ` Yang Shi
2021-06-04  2:22     ` Hugh Dickins
2021-06-04  2:22       ` Hugh Dickins
2021-06-04 18:03       ` Yang Shi
2021-06-04 18:03         ` Yang Shi
2021-06-04 21:52         ` Hugh Dickins
2021-06-04 21:52           ` Hugh Dickins
2021-06-04 15:34   ` Kirill A. Shutemov
2021-06-04 21:29     ` Hugh Dickins
2021-06-04 21:29       ` Hugh Dickins
2021-06-01 21:07 ` [PATCH 2/7] mm/thp: try_to_unmap() use TTU_SYNC for safe DEBUG_VM splitting Hugh Dickins
2021-06-01 21:07   ` Hugh Dickins
2021-06-02  1:59   ` Alistair Popple
2021-06-03 21:45   ` Yang Shi
2021-06-03 21:45     ` Yang Shi
2021-06-04  2:45     ` Hugh Dickins
2021-06-04  2:45       ` Hugh Dickins
2021-06-04 18:24       ` Yang Shi
2021-06-04 18:24         ` Yang Shi
2021-06-03 21:48   ` Peter Xu
2021-06-04  2:54     ` Hugh Dickins
2021-06-04  2:54       ` Hugh Dickins
2021-06-04 14:48       ` Peter Xu
2021-06-04 22:26         ` Hugh Dickins [this message]
2021-06-04 22:26           ` Hugh Dickins
2021-06-04 15:47       ` Kirill A. Shutemov
2021-06-01 21:09 ` [PATCH 3/7] mm/thp: fix vma_address() if virtual address below file offset Hugh Dickins
2021-06-01 21:09   ` Hugh Dickins
2021-06-01 21:30   ` Matthew Wilcox
2021-06-03 21:36     ` Hugh Dickins
2021-06-03 21:36       ` Hugh Dickins
2021-06-03 21:40       ` [PATCH v2 " Hugh Dickins
2021-06-03 21:40         ` Hugh Dickins
2021-06-04 15:53         ` Kirill A. Shutemov
2021-06-04 17:36         ` Matthew Wilcox
2021-06-04 22:35           ` Hugh Dickins
2021-06-04 22:35             ` Hugh Dickins
2021-06-01 21:11 ` [PATCH 4/7] mm/thp: fix page_address_in_vma() on file THP tails Hugh Dickins
2021-06-01 21:11   ` Hugh Dickins
2021-06-01 21:32   ` Matthew Wilcox
2021-06-03 22:06   ` Yang Shi
2021-06-03 22:06     ` Yang Shi
2021-06-04 15:54   ` Kirill A. Shutemov
2021-06-01 21:13 ` [PATCH 5/7] mm/thp: fix page_vma_mapped_walk() if huge page mapped by ptes Hugh Dickins
2021-06-01 21:13   ` Hugh Dickins
2021-06-04 16:24   ` Kirill A. Shutemov
2021-06-04 17:42     ` Matthew Wilcox
2021-06-04 22:56     ` Hugh Dickins
2021-06-04 22:56       ` Hugh Dickins
2021-06-01 21:15 ` [PATCH 6/7] mm/thp: unmap_mapping_page() to fix THP truncate_cleanup_page() Hugh Dickins
2021-06-01 21:15   ` Hugh Dickins
2021-06-04 16:39   ` Kirill A. Shutemov
2021-06-04 23:07     ` Hugh Dickins
2021-06-04 23:07       ` Hugh Dickins
2021-06-01 21:17 ` [PATCH 7/7] mm/thp: remap_page() is only needed on anonymous THP Hugh Dickins
2021-06-01 21:17   ` Hugh Dickins
2021-06-03 22:09   ` Yang Shi
2021-06-03 22:09     ` Yang Shi
2021-06-04 16:41   ` Kirill A. Shutemov
2021-06-02  2:07 ` [PATCH 0/7] mm/thp: fix THP splitting unmap BUGs and related Alistair Popple
2021-06-03 22:21 ` Hugh Dickins
2021-06-03 22:21   ` Hugh Dickins
2021-06-03 23:03   ` Andrew Morton
2021-06-03 22:26 ` [PATCH 6.1/7] mm: thp: replace DEBUG_VM BUG with VM_WARN when unmap fails for split Hugh Dickins
2021-06-03 22:26   ` Hugh Dickins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LSU.2.11.2106041454430.14037@eggly.anvils \
    --to=hughd@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=apopple@nvidia.com \
    --cc=jack@suse.cz \
    --cc=juew@google.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linmiaohe@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=minchan@kernel.org \
    --cc=naoya.horiguchi@nec.com \
    --cc=peterx@redhat.com \
    --cc=rcampbell@nvidia.com \
    --cc=shy828301@gmail.com \
    --cc=wangyugui@e16-tech.com \
    --cc=willy@infradead.org \
    --cc=ziy@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.