On Wed, May 29, 2013 at 11:34:34AM -0700, Andrew Morton wrote:
> 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.
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
