On Thu, Feb 16, 2023 at 3:07 PM Peter Xu <peterx@xxxxxxxxxx> 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
>
