Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 1 Jun 2021 12:10:48 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Matthew Wilcox <willy@infradead.org>
cc:     Hugh Dickins <hughd@google.com>, Xu Yu <xuyu@linux.alibaba.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, gavin.dg@linux.alibaba.com,
        Greg Thelen <gthelen@google.com>, Wei Xu <weixugc@google.com>,
        Nicholas Piggin <npiggin@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>
Subject: Re: [PATCH] mm, thp: relax migration wait when failed to get tail
 page
In-Reply-To: <YLZqKJ4anEGpAZfp@casper.infradead.org>
Message-ID: <alpine.LSU.2.11.2106011114580.1045@eggly.anvils>
References: <bc8567d7a2c08ab6fdbb8e94008157265d5d28a3.1622564942.git.xuyu@linux.alibaba.com> <alpine.LSU.2.11.2106010947370.1090@eggly.anvils> <YLZqKJ4anEGpAZfp@casper.infradead.org>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Precedence: bulk

On Tue, 1 Jun 2021, Matthew Wilcox wrote:
> On Tue, Jun 01, 2021 at 09:55:56AM -0700, Hugh Dickins wrote:
> > 
> > Well caught: you're absolutely right that there's a bug there.
> > But isn't cond_resched() just papering over the real bug, and
> > what it should do is a "page = compound_head(page);" before the
> > get_page_unless_zero()? How does that work out in your testing?
> 
> You do realise you're strengthening my case for folios by suggesting
> that, don't you?  ;-)

Hah! Well, I do realize that I'm offering you a marketing opportunity.
And you won't believe how many patches I dread to post for fear of that ;-)

But I'm not so sure that it strengthens your case: apparently folios
had not detected this?  Or do you have a hoard of folio-detected fixes
waiting for the day, and a folio-kit for each of the stable releases?

> 
> I was going to suggest that it won't make any difference because the
> page reference count is frozen, but the freezing happens after the call
> to unmap_page(), so it may make a difference.

I think that's a good point: I may have just jumped on the missing
compound_head(), without thinking it through as far as you have.

I'm having trouble remembering the dynamics now; but I think there
are cond_resched()s in the unmap_page() part, so the splitter may
get preempted even on a non-preempt kernel; whereas the frozen
part is all done expeditiously, with interrupts disabled.

Greg discovered the same issue recently, but we all got sidetracked,
and I don't know where his investigation ended up.  He was in favour
of cond_resched(), I was in favour of compound_head(); and I think I
was coming to the conclusion that if cond_resched() is needed, it
should not be there in __migration_entry_wait(), but somewhere up
in mm/gup.c, so that other faults that retry, expecting to reschedule
on return to userspace, do not get trapped in kernelspace this way.

Waiting on migration entries from THP splitting is an egregious
example, but others may be suffering too.

Hugh