Re: [PATCHv3 3/6] mm/zpool: implement common zpool api to zbud/zsmalloc

[Seth Jennings]

On Tue, May 27, 2014 at 08:06:28PM -0400, Dan Streetman wrote:
> On Tue, May 27, 2014 at 6:06 PM, Seth Jennings <sjennings@xxxxxxxxxxxxxx> wrote:
> > On Sat, May 24, 2014 at 03:06:06PM -0400, Dan Streetman wrote:
<snip>
> >> + * Returns: 0 on success, negative value on error/failure.
> >> + */
> >> +int zpool_shrink(struct zpool *pool, size_t size);
> >
> > This should take a number of pages to be reclaimed, not a size.  The
> > user can evict their own object to reclaim a certain number of bytes
> > from the pool.  What the user can't do is reclaim a page since it is not
> > aware of the arrangement of the stored objects in the memory pages.
> 
> Yes I suppose that's true, I'll update it for v4...
> 
> >
> > Also in patch 5/6 of six I see:
> >
> > -               if (zbud_reclaim_page(zswap_pool, 8)) {
> > +               if (zpool_shrink(zswap_pool, PAGE_SIZE)) {
> >
> > but then in 4/6 I see:
> >
> > +int zbud_zpool_shrink(void *pool, size_t size)
> > +{
> > +       return zbud_reclaim_page(pool, 8);
> > +}
> >
> > That is why it didn't completely explode on you since the zbud logic
> > is still reclaiming pages.
> 
> Ha, yes clearly I neglected to translate between the size and the
> number of pages there, oops!
> 
> On this topic - 8 retries seems very arbitrary.  Does it make sense to
> include retrying in zbud and/or zpool at all?  The caller can easily
> retry any number of times themselves, especially since zbud (and
> eventually zsmalloc) will return -EAGAIN if the caller should retry.

Yeah, the retries argument in the zbud API isn't good.  You can change
the zbud_reclaim_page() to just try once and return -EAGAIN if you want
and I'll be in favor of that.

That did make me think of something else though.  The zpool API is
zpool_shrink() with, what will be, a number of pages.  The zbud API is
zbud_reclaim_page() which, as the name implies, reclaims one page.  So
it seems that you would need a loop in zbud_zpool_shrink() to try to
reclaim a multiple number of pages.

> 
> >
> >> +
> >> +/**
> >> + * zpool_map_handle() - Map a previously allocated handle into memory
> >> + * @pool     The zpool that the handle was allocated from
> >> + * @handle   The handle to map
> >> + * @mm       How the memory should be mapped
> >> + *
<snip>
> >> +int zpool_evict(void *pool, unsigned long handle)
> >> +{
> >> +     struct zpool *zpool;
> >> +
> >> +     spin_lock(&pools_lock);
> >> +     list_for_each_entry(zpool, &pools_head, list) {
> >
> > You can do a container_of() here:
> >
> > zpool = container_of(pool, struct zpool, pool);
> 
> unfortunately, that's not true, since the driver pool isn't actually a
> member of the struct zpool.  The struct zpool only has a pointer to
> the driver pool.

Ah yes, got my user API vs driver API crossed here :-/

Meh, can't think of a better way for now and it doesn't cause contention
on the hot paths so... works for me.

Seth

> 
> I really wanted to use container_of(), but I think zbud/zsmalloc would
> need alternate pool creation functions that create struct zpools of
> the appropriate size with their pool embedded, and the
> driver->create() function would need to alloc and return the entire
> struct zpool, instead of just the driver pool.  Do you think that's a
> better approach?  Or is there another better way I'm missing?
> 
--
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/