Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965720Ab3E2Sem (ORCPT ); Wed, 29 May 2013 14:34:42 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:49535 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965290Ab3E2Seh (ORCPT ); Wed, 29 May 2013 14:34:37 -0400 Date: Wed, 29 May 2013 11:34:34 -0700 From: Andrew Morton To: Seth Jennings Cc: Greg Kroah-Hartman , Nitin Gupta , Minchan Kim , Konrad Rzeszutek Wilk , Dan Magenheimer , Robert Jennings , Jenifer Hopper , Mel Gorman , Johannes Weiner , Rik van Riel , Larry Woodman , Benjamin Herrenschmidt , Dave Hansen , Joe Perches , Joonsoo Kim , Cody P Schafer , Hugh Dickens , Paul Mackerras , Heesub Shin , linux-mm@kvack.org, linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org Subject: Re: [PATCHv12 2/4] zbud: add to mm/ Message-Id: <20130529113434.b2ced4cc1e66c7a0a520d908@linux-foundation.org> In-Reply-To: <20130529154500.GB428@cerebellum> References: <1369067168-12291-1-git-send-email-sjenning@linux.vnet.ibm.com> <1369067168-12291-3-git-send-email-sjenning@linux.vnet.ibm.com> <20130528145911.bd484cbb0bb7a27c1623c520@linux-foundation.org> <20130529154500.GB428@cerebellum> X-Mailer: Sylpheed 3.2.0beta5 (GTK+ 2.24.10; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4162 Lines: 116 On Wed, 29 May 2013 10:45:00 -0500 Seth Jennings wrote: > > > +struct zbud_page { > > > + union { > > > + struct page page; > > > + struct { > > > + unsigned long donotuse; > > > + u16 first_chunks; > > > + u16 last_chunks; > > > + struct list_head buddy; > > > + struct list_head lru; > > > + }; > > > + }; > > > +}; > > > > Whoa. So zbud scribbles on existing pageframes? > > Yes. > > > > > Please tell us about this, in some detail. How is it done and why is > > this necessary? > > > > Presumably the pageframe must be restored at some stage, so this code > > has to be kept in sync with external unrelated changes to core MM? > > Yes, this is done in free_zbud_page(). > > > > > Why was it implemented in this fashion rather than going into the main > > `struct page' definition and adding the appropriate unionised fields? > > Yes, modifying the struct page is the cleaner way. I thought that adding more > convolution to struct page would create more friction on the path to getting > this merged. Plus overlaying the struct page was the approach used by zsmalloc > and so I was thinking more along these lines. I'd be interested in seeing what the modifications to struct page look like. It really is the better way. > If you'd rather add the zbud fields directly into unions in struct page, > I'm ok with that if you are. > > Of course, this doesn't avoid having to reset the fields for the page allocator > before we free them. Even slub/slob reset the mapcount before calling > __free_page(), for example. > > > > > I worry about any code which independently looks at the pageframe > > tables and expects to find page struts there. One example is probably > > memory_failure() but there are probably others. ^^ this, please. It could be kinda fatal. > > > > > > ... > > > > > > +int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp, > > > + unsigned long *handle) > > > +{ > > > + int chunks, i, freechunks; > > > + struct zbud_page *zbpage = NULL; > > > + enum buddy bud; > > > + struct page *page; > > > + > > > + if (size <= 0 || gfp & __GFP_HIGHMEM) > > > + return -EINVAL; > > > + if (size > PAGE_SIZE) > > > + return -E2BIG; > > > > Means "Argument list too long" and isn't appropriate here. > > Ok, I need a return value other than -EINVAL to convey to the user that the > allocation is larger than what the allocator can hold. I don't see an existing > errno that would be more suited for that. Do you have a suggestion? ENOMEM perhaps. That's also somewhat misleading, but I guess there's precedent for ENOMEM meaning "allocation too large" as well as "out of memory". > > > +int zbud_reclaim_page(struct zbud_pool *pool, unsigned int retries) > > > +{ > > > + int i, ret, freechunks; > > > + struct zbud_page *zbpage; > > > + unsigned long first_handle = 0, last_handle = 0; > > > + > > > + spin_lock(&pool->lock); > > > + if (!pool->ops || !pool->ops->evict || list_empty(&pool->lru) || > > > + retries == 0) { > > > + spin_unlock(&pool->lock); > > > + return -EINVAL; > > > + } > > > + for (i = 0; i < retries; i++) { > > > + zbpage = list_tail_entry(&pool->lru, struct zbud_page, lru); > > > + list_del(&zbpage->lru); > > > + list_del(&zbpage->buddy); > > > + /* Protect zbpage against free */ > > > > Against free by who? What other code paths can access this page at > > this time? > > zbud has no way of serializing with the user (zswap) to prevent it calling > zbud_free() during zbud reclaim. To prevent the zbud page from being freed > while reclaim is operating on it, we set the reclaim flag in the struct page. > zbud_free() checks this flag and, if set, only sets the chunk length of the > allocation to 0, but does not actually free the zbud page. That is left to > this reclaim path. Sounds strange. Page refcounting is a well-established protocol and works well in other places? -- 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/