Re: [PATCH 17/18] mm, hugetlb: retry if we fail to allocate ahugepage with use_reserve

[David Gibson]

On Wed, Aug 07, 2013 at 06:18:32PM +0900, Joonsoo Kim wrote:
> On Tue, Aug 06, 2013 at 06:38:49PM -0700, Davidlohr Bueso wrote:
> > On Wed, 2013-08-07 at 11:03 +1000, David Gibson wrote:
> > > On Tue, Aug 06, 2013 at 05:18:44PM -0700, Davidlohr Bueso wrote:
> > > > On Mon, 2013-08-05 at 16:36 +0900, Joonsoo Kim wrote:
> > > > > > Any mapping that doesn't use the reserved pool, not just
> > > > > > MAP_NORESERVE.  For example, if a process makes a MAP_PRIVATE mapping,
> > > > > > then fork()s then the mapping is instantiated in the child, that will
> > > > > > not draw from the reserved pool.
> > > > > > 
> > > > > > > Should we ensure them to allocate the last hugepage?
> > > > > > > They map a region with MAP_NORESERVE, so don't assume that their requests
> > > > > > > always succeed.
> > > > > > 
> > > > > > If the pages are available, people get cranky if it fails for no
> > > > > > apparent reason, MAP_NORESERVE or not.  They get especially cranky if
> > > > > > it sometimes fails and sometimes doesn't due to a race condition.
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > Hmm... Okay. I will try to implement another way to protect race condition.
> > > > > Maybe it is the best to use a table mutex :)
> > > > > Anyway, please give me a time, guys.
> > > > 
> > > > So another option is to take the mutex table patchset for now as it
> > > > *does* improve things a great deal, then, when ready, get rid of the
> > > > instantiation lock all together.
> > > 
> > > We still don't have a solid proposal for doing that. Joonsoo Kim's
> > > patchset misses cases (non reserved mappings).  I'm also not certain
> > > there aren't a few edge cases which can lead to even reserved mappings
> > > failing, and if that happens the patches will lead to livelock.
> > > 
> > 
> > Exactly, which is why I suggest minimizing the lock contention until we
> > do have such a proposal.
> 
> Okay. my proposal is not complete and maybe much time is needed.
> And I'm not sure that my *retry* approach can eventually cover all
> the race situations, currently.

Yes.  The difficulty with retrying is knowing when its safe to to
so.  If you don't retry enough, you get SIGBUS when you should be able
to allocate, if you retry too much, you freeze up trying to find a
page that isn't there.

I once attempted an approach involving an atomic counter of the number
of "in flight" hugepages, only retrying when it's non zero.  Working
out a safe ordering for all the updates to get all the cases right
made my brain melt though, and I never got it working.

> If you have to hurry, I don't have strong objection to your patches,
> but, IMHO, we should go slow, because it is not just trivial change.
> Hugetlb code is too subtle, so it is hard to confirm it's solidness.
> Following is the race problem what I found with those patches.
> 
> I assume that nr_free_hugepage is 2.
> 
> 1. parent process map an 1 hugepage sizeid region with MAP_PRIVATE
> 2. parent process write something to this region, so fault occur.
> 3. fault handling.
> 4. fork
> 5. parent process write something to this hugepage, so cow-fault occur.
> 6. while parent allocate a new page and do copy_user_huge_page()
> 	in fault handler, child process write something to this hugepage,
> 	so cow-fault occur. This access is not protected by table mutex,
> 	because mm is different.
> 7. child process die, because there is no free hugepage.
> 
> If we have no race, child process would not die,
> because all we needed is only 2 hugepages, one for parent,
> and the other for child.

Ouch, good catch.  Unlike the existing form of the race, I doubt this
one has been encountered in the wild, but it shows how subtle this is.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: pgp00000.pgp
Description: PGP signature