From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34400) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qs3pH-0001MD-ES for qemu-devel@nongnu.org; Fri, 12 Aug 2011 22:19:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qs3pE-0003BG-HU for qemu-devel@nongnu.org; Fri, 12 Aug 2011 22:19:51 -0400 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:53886) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qs3pD-0003B3-VM for qemu-devel@nongnu.org; Fri, 12 Aug 2011 22:19:48 -0400 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [202.81.31.247]) by e23smtp02.au.ibm.com (8.14.4/8.13.1) with ESMTP id p7D2D1ue031149 for ; Sat, 13 Aug 2011 12:13:01 +1000 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p7D2IFQU884836 for ; Sat, 13 Aug 2011 12:18:15 +1000 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p7D2JHZb021216 for ; Sat, 13 Aug 2011 12:19:17 +1000 Date: Sat, 13 Aug 2011 11:08:39 +1000 From: David Gibson Message-ID: <20110813010839.GC30552@yookeroo.fritz.box> References: <20110811064059.GU6342@yookeroo.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] Fix refcounting in hugetlbfs quota handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hugh Dickins Cc: kvm , linux-kernel@vger.kernel.org, Marcelo Tosatti , agraf@suse.de, qemu-devel@nongnu.org, Adam Litke , Minchan Kim , Jan Kiszka , Avi Kivity , KOSAKI Motohiro , Andrew Hastings , Paul Mackerras , Andrew Morton , Linus Torvalds , KAMEZAWA Hiroyuki , Mel Gorman On Fri, Aug 12, 2011 at 12:15:21PM -0700, Hugh Dickins wrote: > On Fri, 12 Aug 2011, Minchan Kim wrote: > > On Fri, Aug 12, 2011 at 9:48 AM, Linus Torvalds > > wrote: > > > On Wed, Aug 10, 2011 at 11:40 PM, David Gibson > > > wrote: > > >> > > >> This patch, therefore, stores a pointer to the inode instead of th= e > > >> address_space in the page private data for hugepages. =A0More > > >> importantly it correctly adjusts the reference count on the inodes > > >> when they're added to the page private data. =A0This ensures that = the > > >> inode (and therefore the super block) will not be freed before we = use > > >> it from free_huge_page. > > > > > > Looks sane, but I *really* want some acks from people who use/know > > > hugetlbfs. Who would that be? I'm adding random people who have > > > acked/signed-off patches to hugetlbfs recently.. > >=20 > > At least, code itself looks good to me but your random choice was fai= led. > > Maybe people you want are as follows. > > http://marc.info/?t=3D126928975800003&r=3D1&w=3D2 > >=20 > > Ccing right persons. >=20 > I don't know much about hugetlbfs these days, but I think the patch > is very wrong. >=20 > The real change is where alloc_huge_page() does igrab(inode) and > free_huge_pages() does iput(inode)? >=20 > That makes me very nervous, partly because a final iput() is a complex > operation, which we wouldn't expect to be doing when "freeing" a page. >=20 > My first worry was that free_huge_page() could actually get called at > interrupt time (when it's in a pagevec of pages to be freed as a batch, > then another put_page is done at interrupt time which frees that batch)= : > I worried that we use spin_lock not spin_lock_irqsave on inode->i_lock. > To be honest though, I've not followed up whether that's actually a > possibility, the compound page path is too twisty for a quick answer; > and even if it's a possibility, it's one that's already ignored in the > case of hugetlb_lock. >=20 > Setting that aside, I think this thing of grabbing a reference to inode > for each page just does not work as you wish: when we unlink an inode, > all its pages should be freed; but because they are themselves holding > references to the inode, it and its pages stick around forever. Ugh, yes. You're absolutely right. That circular reference will mess everything up. Thinking it through and testing fail. > A quick experiment with your patch versus without confirmed that: > meminfo HugePages_Free stayed down with your patch, but went back to > HugePages_Total without it. Please check, perhaps I'm just mistaken. >=20 > Sorry, I've not looked into what a constructive alternative might be; > and it's not the first time we've had this difficulty - it came up last > year when the ->freepage function was added, that the inode may be gone > by the time ->freepage(page) is called. Ok, so. In fact the quota functions we call at free time only need the super block, not the inode per se. If we put a superblock pointer instead of an inode pointer in page private, and refcounted that, I think that should remove the circular ref. The only reason I didn't do it before is that the superblock refcounting functions didn't seem to be globally visible in an obvious way. Does that sound like a reasonable approach? > On a side note, very good description - thank you, but I wish you'd > split the patch into two, the fix and then the inode-instead-of-mapping > cleanup. Though personally I'd prefer not to make that "cleanup": it's > normal for a struct address space * to be used in struct page (if I del= ved > I guess I'd find good reason why this one is in page->private instead o= f > page->mapping: perhaps because it's needed after page->mapping is reset > to NULL, perhaps because it's needed on COWed copies of hugetlbfs pages= ). That is an interesting question. But it doesn't address the basic point. mappings aren't refcounted themselves, and as far as I can tell their lifetime is bound to that of their inode. --=20 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