Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966979Ab3E2Umy (ORCPT ); Wed, 29 May 2013 16:42:54 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:42174 "EHLO e7.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966874Ab3E2Umr (ORCPT ); Wed, 29 May 2013 16:42:47 -0400 Date: Wed, 29 May 2013 15:42:36 -0500 From: Seth Jennings To: Andrew Morton 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: <20130529204236.GD428@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> <20130529113434.b2ced4cc1e66c7a0a520d908@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130529113434.b2ced4cc1e66c7a0a520d908@linux-foundation.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13052920-5806-0000-0000-0000216778AF Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5015 Lines: 138 On Wed, May 29, 2013 at 11:34:34AM -0700, Andrew Morton wrote: > 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. I'll do it then. > > > 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. I'll look into this. The expected behavior is that memory_failure() should handle zbud pages in the same way that it handles in-use slub/slab/slob pages and return -EBUSY. > > > > > > > > > ... > > > > > > > > +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". Works for me. > > > > > +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? Yes, refcounting seemed like overkill for this situation since the refcount will only ever be 1 or 2 (2 if under reclaim) which basically reduces it to a boolean. I'm also not sure if there is room left in the struct page for a refcount with all the existing zbud metadata. However, if you really don't like this, I can look at doing it via refcounts. Seth -- 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/