MIME-Version: 1.0
References: <20230214075710.2401855-1-stevensd@google.com> <20230214075710.2401855-2-stevensd@google.com>
 <Y+1hMsm4eQIUtag5@x1n> <CAD=HUj69L2e-Z4TB19qFt8h1cn0r1oGbWovJGMOjjyvfDcQ7NA@mail.gmail.com>
 <Y+5Akpz4CvGywt6R@x1n> <CAHbLzkqUF8Y759ZBVGXWdKwDS4f+ZJakUhoqf8Dqvx0Jam4c1g@mail.gmail.com>
 <Y+63LQo49CCyXRQn@x1n>
In-Reply-To: <Y+63LQo49CCyXRQn@x1n>
From:   Yang Shi <shy828301@gmail.com>
Date:   Thu, 16 Feb 2023 15:52:37 -0800
Message-ID: <CAHbLzkokih0j=niLuWJLBDfTS8biiDTjC6gudw+qcDCBpMneAA@mail.gmail.com>
Subject: Re: [PATCH 2/2] mm/khugepaged: skip shmem with userfaultfd
To:     Peter Xu <peterx@redhat.com>
Cc:     David Stevens <stevensd@chromium.org>, linux-mm@kvack.org,
        Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 16, 2023 at 3:07 PM Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Yang,
>
> On Thu, Feb 16, 2023 at 01:58:55PM -0800, Yang Shi wrote:
> > > IIUC we released it before copying the pages:
> >
> > The huge page is locked until the copy is done. It should be fine
> > unless the users inspect the page content without acquiring page lock.
>
> The current patch from David has replaced "insert hpage into holes" with
> "insert RETRY entries into holes", so IMHO the hpage is not visible at all
> when releasing page cache lock here.

IIRC his patch (just this patch, don't include patch #1) conceptually does:

acquire xa lock

fill the holes with retry entry
if (nr_none == nr_none_check && uffd missing pass) /* no hole is
filled since holding xa_lock and no uffd missing */
    install huge page in page cache <-- huge page is visible here
else {
    set error code
    replace retry entry back to NULL
}

release xa_lock

if (succeed) {
    copy content to huge page
    unlock huge page
} else
    restore the small pages


Am I missing something?

>
> All the accessors (including RCU protected ones to access page cache; those
> may not need to take the page lock) should be spinning on the RETRY entry,
> which it seems fine to me.  But my question was whether it's legal to keep
> them spinning even after releasing the page cache lock.

After releasing the page cache lock, they should see NULL entry or
huge page IIUC.

>
> Thanks,
>
> >
> > >
> > > xa_locked:
> > >         xas_unlock_irq(&xas);  <-------------------------------- here
> > > xa_unlocked:
> > >
> > >         /*
> > >          * If collapse is successful, flush must be done now before copying.
> > >          * If collapse is unsuccessful, does flush actually need to be done?
> > >          * Do it anyway, to clear the state.
> > >          */
> > >         try_to_unmap_flush();
> > >
> > > Before insertion of the multi-index:
> > >
> > >         /* Join all the small entries into a single multi-index entry. */
> > >         xas_set_order(&xas, start, HPAGE_PMD_ORDER);
> > >         xas_store(&xas, hpage);
> > >
> > > Thanks,
> > >
> > > --
> > > Peter Xu
> > >
> >
>
> --
> Peter Xu
>