Message-Id: <200603091143.k29BhAg19160@unix-os.sc.intel.com>
From: "Chen, Kenneth W" <kenneth.w.chen@intel.com>
To: "'David Gibson'" <david@gibson.dropbear.id.au>
Cc: <wli@holomorphy.com>, "'Andrew Morton'" <akpm@osdl.org>,
       <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>
Subject: RE: [patch] hugetlb strict commit accounting
Date: Thu, 9 Mar 2006 03:43:12 -0800
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: 7bit
Thread-Index: AcZDbMghlXpYtzMfTSmocFe7P0uhNQAAEKjA
In-Reply-To: <20060309112635.GB9479@localhost.localdomain>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1850
Lines: 42

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?

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.


> 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.

- Ken

-
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/