Re: [patch] hugetlb strict commit accounting

["'David Gibson'" <david@gibson.dropbear.id.au>]

On Thu, Mar 09, 2006 at 03:43:12AM -0800, Chen, Kenneth W wrote:
> David Gibson wrote on Thursday, March 09, 2006 3:27 AM
> > Um... as far as I can tell, this patch doesn't actually reserve
> > anything.  There are no changes to the fault handler ot
> > alloc_huge_page(), so there's nothing to stop PRIVATE mappings dipping
> > into the supposedly reserved pool.
> 
> Well, the reservation is already done at mmap time for shared mapping. Why
> does kernel need to do anything at fault time?  Doing it at fault time is
> an indication of weakness (or brokenness) - you already promised at mmap
> time that there will be a page available for faulting.  Why check them
> again at fault time?

You can't know (or bound) at mmap() time how many pages a PRIVATE
mapping will take (because of fork()).  So unless you have a test at
fault time (essentialy deciding whether to draw from "reserved" and
"unreserved" hugepage pool) a supposedly reserved SHARED mapping will
OOM later if there have been enough COW faults to use up all the
hugepages before it's instantiated.

> I don't think your implementation handles PRIVATE mapping either, Isn't it?
> Private mapping doesn't enter into the page cache hanging of address_space
> pointer, so either way, it is busted.

Depends what you mean by "handle".  With my patch PRIVATE mappings are
never reserved or guaranteed (I couldn't think of any set of sane
semantics for it), *but* they will never eat into the pool reserved
for SHARED mappings.  With yours, they can, so:
	p = mmap(SHARED)
	/* Lots of COW faults elsewhere */
	*p = x;
Will result in an OOM Kill on the last line.

> > This looks a bit like a case of "let's make it an atomic_t to sprinkle
> > it with magic atomicity dust" without thinking about what operations
> > are and need to be atomic.  I think resv_huge_pages should be an
> > ordinary int, but protected by a lock (exactly which lock is not
> > immediately obvious).
> 
> Yeah, I agree.  It crossed my mind whether I should fix that or post a
> fairly straightforward back port.  I decided to do the latter and I got
> bitten :-(  That is in the pipeline if people agree that this variable
> reservation system is a better one.
> 
> 
> > What is the list of regions (mapping->private_list) protected by?
> > mmap_sem (the only thing I can think of off hand that's already taken)
> > doesn't cut it, because the mapping can be accessed by multiple mms.
> 
> I think it is the inode->i_mutex.

Ok, you should double check that's taken in all the right places, but
it sounds plausible.

-- 
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/