lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 May 2013 11:34:34 -0700 From: Andrew Morton <akpm@...ux-foundation.org> To: Seth Jennings <sjenning@...ux.vnet.ibm.com> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Nitin Gupta <ngupta@...are.org>, Minchan Kim <minchan@...nel.org>, Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>, Dan Magenheimer <dan.magenheimer@...cle.com>, Robert Jennings <rcj@...ux.vnet.ibm.com>, Jenifer Hopper <jhopper@...ibm.com>, Mel Gorman <mgorman@...e.de>, Johannes Weiner <jweiner@...hat.com>, Rik van Riel <riel@...hat.com>, Larry Woodman <lwoodman@...hat.com>, Benjamin Herrenschmidt <benh@...nel.crashing.org>, Dave Hansen <dave@...1.net>, Joe Perches <joe@...ches.com>, Joonsoo Kim <iamjoonsoo.kim@....com>, Cody P Schafer <cody@...ux.vnet.ibm.com>, Hugh Dickens <hughd@...gle.com>, Paul Mackerras <paulus@...ba.org>, Heesub Shin <heesub.shin@...sung.com>, linux-mm@...ck.org, linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org Subject: Re: [PATCHv12 2/4] zbud: add to mm/ On Wed, 29 May 2013 10:45:00 -0500 Seth Jennings <sjenning@...ux.vnet.ibm.com> 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@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists