Re: [PATCH 00 of 34] Transparent Hugepage support #14

[Andrea Arcangeli <aarcange@redhat.com>]

On Mon, Mar 22, 2010 at 10:38:23AM -0500, Christoph Lameter wrote:
> On Fri, 19 Mar 2010, Andrea Arcangeli wrote:
> 
> > > Look at the patches. They add synchronization to pte operations.
> >
> > The whole point is that it's fundamentally unavoidable and not
> > specific to any of split_huge_page, compound_lock or
> > get_page/put_page!
> 
> Really? Why did no one else run into this before?

Of course we run into this before too, and the locking in fact already
exists and you probably added it yourself when introducing the more
scalare PT lock for the pte.

For the pmd we always used the page_table_lock and I kept using
it. Except that before it was only necessary in __pte_alloc (see
mainline), now it's needed in more places because the pmd can be
modified as it points to actual pages that can go away, and not only
to ptes.

> > Problem with O_DIRECT is that I couldn't use mmu notifier to prevent
> > it to take the pin on the page, because there is no way to interrupt
> > DMA synchronously before mmu_notifier_invalidate_* returns... So I had
> > to add compound_lock and keep gup API backwards compatible and have
> > the proper serialization happen _only_ for PageTail inside put_page.
> 
> You can take a refcount *before* breaking up a 2M page then you dont have
> to fear the put_page.

If you take it _before_ it will go into the head page regardless of
which subpage was returned by gup. We need to know which subpages are
under DMA. The pin has to go to the tail pages or head page depending
on the physical address that was requested by gup. To fix this we need
at the very least to change gup api to ask for hugepages which it
can't right now because it'd break all drivers.

> Keep a reference count in the head page and a pointer to the subpage? Page
> can only be broken up if all page references of the 2M page can be
> accounted for. This implies no "atomic" breakup but this way it does not
> require changes to synchronization.

"can only broken up". See my last sentence of my previous reply to
figure out exactly why we're not ok if "page cannot be broken up".

besides even if we add a error retval, we can't have mprotect/mremap
fail at most swapout could be deferred because "page cannot be broken
up" but even that is risky and I've been extra careful not to require
any memory allocation or sleeping lock in split_huge_page to make it
ideal to use in swap path without risking any functional regression
whatsoever.

> That is the basic crux here. Do not require that it cannot fail. Its a bad
> move and results in a mess.

Allowing it to fail would result in a mess. Obviously I wasn't clear
enough in the last sentence of my previous mail so I'll have to
repeate: any effort in handling the failure (which in some case it
can't be handled as syscalls can't fail just because a page is 2M)
should instead be spent to _remove_ the split_huge_page call.

> No its not. I am proposing to *keep* the existing syncronization methods.
> Use what is there. Do not invent new synchronization.

You don't see the huge value of the invention. When you go around
implementing your own split_huge_page that will fail, you might see
it (or we can compare the size of the two patches then, knowing that
I add 1 line to mremap and you add hundreds or maybe you can't use
your split_huge_page there at all forcing you to handle 2M pages
immediately from the start). And mremap is not the big deal, it's that
if you can't use it in mremap you practically won't be able to use it
anywhere except in swap, where you will introduce a nasty unknown and
potential livelock or deadlock.

> We already have 2M pmd handling in the kernel and can consider huge pmd
> entries while walking the page tables! Go incrementally use what
> is there.

There's no such thing unless you talk about the hugetlbfs paths. In
the core paths that the only thing I care about in transparent
hugepage, pmd is only checked to be present. If it's present it can't
go away. This is not true anymore as it can point to user pages and
not ptes. I am definitely reusing the same synchronization for ptes
that already is used by __pte_alloc which is the only bit that
requires synchronization right now, to atomically check if the pmd is
still not present, before overwriting it.

Best of all, I had to add zero atomic ops and just 1 branch in already
hot l1 cache (and no writes to the l1 cache either, just 1 more read)
in order to add the pagefault slow path for huge pmd. So unless you
actively take advantage of hugepages, the page_table_lock locking will
be zero cost and in the future nothing prevents us to add a more
scalar PMD lock like it exists for the pte (but keep in mind it's much
512 times less important for PMD than it is for the PTE).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>