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?
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
