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

[Andrew Morton]

On Wed, 29 May 2013 10:45:00 -0500 Seth Jennings <sjenning@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/