Date:   Mon, 6 Feb 2023 15:52:19 -0500
From:   Peter Xu <peterx@redhat.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     David Stevens <stevensd@chromium.org>, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Yang Shi <shy828301@gmail.com>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mm/khugepaged: skip shmem with userfaultfd
Message-ID: <Y+Fog2dO1kpRBMvr@x1n>
References: <20230206112856.1802547-1-stevensd@google.com>
 <Y+FOk+ty7OKmkwLL@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Y+FOk+ty7OKmkwLL@casper.infradead.org>
Precedence: bulk

On Mon, Feb 06, 2023 at 07:01:39PM +0000, Matthew Wilcox wrote:
> On Mon, Feb 06, 2023 at 08:28:56PM +0900, David Stevens wrote:
> > This change first makes sure that the intermediate page cache state
> > during collapse is not visible by moving when gaps are filled to after
> > the page cache lock is acquired for the final time. This is necessary
> > because the synchronization provided by locking hpage is insufficient
> > for functions which operate on the page cache without actually locking
> > individual pages to examine their content (e.g. shmem_mfill_atomic_pte).
> 
> I've been a little scared of touching khugepaged because, well, look at
> that function.  But if we are going to touch it, how about this patch
> first?  It does _part_ of what you need by not filling in the holes,
> but obviously not the part that looks at uffd.  
> 
> It leaves the old pages in-place and frozen.  I think this should be
> safe, but I haven't booted it (not entirely sure what test I'd run
> to prove that it's not broken)

That logic existed since Kirill's original commit to add shmem thp support
on khugepaged, so Kirill should be the best to tell.. but so far it seems
reasonalbe to me to have that extra operation.

The problem is khugepaged will release pgtable lock during collapsing, so
AFAICT there can be a race where some other thread tries to insert pages
into page cache in parallel with khugepaged right after khugepaged released
the page cache lock.

For example, it seems to me new page cache can be inserted when khugepaged
is copying small page content to the new hpage.

-- 
Peter Xu