Re: [PATCH 2/2] mm/khugepaged: skip shmem with userfaultfd

[Yang Shi <shy828301@gmail.com>]

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
>