linux-kernel - Re: [PATCHv12 2/4] zbud: add to mm/

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20130529113434.b2ced4cc1e66c7a0a520d908@linux-foundation.org>
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/