[PATCH] staging: zsmalloc: add user-definable alloc/free funcs

Discussion:

Seth Jennings

2012-03-16 21:04:48 UTC

This patch allows a zsmalloc user to define the page
allocation and free functions to be used when growing
or releasing parts of the memory pool.

The functions are passed in the struct zs_pool_ops parameter
of zs_create_pool() at pool creation time. If this parameter
is NULL, zsmalloc uses alloc_page and __free_page() by default.

While there is no current user of this functionality, zcache
development plans to make use of it in the near future.

Patch applies to Greg's staging-next branch.

Signed-off-by: Seth Jennings <***@linux.vnet.ibm.com>
---
drivers/staging/zcache/zcache-main.c | 2 +-
drivers/staging/zram/zram_drv.c | 3 +-
drivers/staging/zsmalloc/zsmalloc-main.c | 39 +++++++++++++++++++++++-------
drivers/staging/zsmalloc/zsmalloc.h | 8 +++++-
drivers/staging/zsmalloc/zsmalloc_int.h | 2 +
5 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index b698464..7ef5313 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -984,7 +984,7 @@ int zcache_new_client(uint16_t cli_id)
goto out;
cli->allocated = 1;
#ifdef CONFIG_FRONTSWAP
- cli->zspool = zs_create_pool("zcache", ZCACHE_GFP_MASK);
+ cli->zspool = zs_create_pool("zcache", ZCACHE_GFP_MASK, NULL);
if (cli->zspool == NULL)
goto out;
#endif
diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 7f13819..278eb4d 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -663,7 +663,8 @@ int zram_init_device(struct zram *zram)
/* zram devices sort of resembles non-rotational disks */
queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);

- zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM);
+ zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM,
+ NULL);
if (!zram->mem_pool) {
pr_err("Error creating memory pool\n");
ret = -ENOMEM;
diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c b/drivers/staging/zsmalloc/zsmalloc-main.c
index 09caa4f..c8bfb77 100644
--- a/drivers/staging/zsmalloc/zsmalloc-main.c
+++ b/drivers/staging/zsmalloc/zsmalloc-main.c
@@ -267,7 +267,7 @@ static unsigned long obj_idx_to_offset(struct page *page,
return off + obj_idx * class_size;
}

-static void free_zspage(struct page *first_page)
+static void free_zspage(struct zs_pool *pool, struct page *first_page)
{
struct page *nextp, *tmp;

@@ -282,7 +282,7 @@ static void free_zspage(struct page *first_page)
first_page->mapping = NULL;
first_page->freelist = NULL;
reset_page_mapcount(first_page);
- __free_page(first_page);
+ (*pool->ops->free_page)(first_page);

/* zspage with only 1 system page */
if (!nextp)
@@ -345,7 +345,7 @@ static void init_zspage(struct page *first_page, struct size_class *class)
/*
* Allocate a zspage for the given size class
*/
-static struct page *alloc_zspage(struct size_class *class, gfp_t flags)
+static struct page *alloc_zspage(struct zs_pool *pool, struct size_class *class)
{
int i, error;
struct page *first_page = NULL;
@@ -365,7 +365,7 @@ static struct page *alloc_zspage(struct size_class *class, gfp_t flags)
for (i = 0; i < class->zspage_order; i++) {
struct page *page, *prev_page;

- page = alloc_page(flags);
+ page = (*pool->ops->alloc_page)(pool->flags);
if (!page)
goto cleanup;

@@ -398,7 +398,7 @@ static struct page *alloc_zspage(struct size_class *class, gfp_t flags)

cleanup:
if (unlikely(error) && first_page) {
- free_zspage(first_page);
+ free_zspage(pool, first_page);
first_page = NULL;
}

@@ -482,7 +482,24 @@ fail:
return notifier_to_errno(ret);
}

-struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
+
+static inline struct page *zs_alloc_page(gfp_t flags)
+{
+ return alloc_page(flags);
+}
+
+static inline void zs_free_page(struct page *page)
+{
+ __free_page(page);
+}
+
+static struct zs_pool_ops default_ops = {
+ .alloc_page = zs_alloc_page,
+ .free_page = zs_free_page
+};
+
+struct zs_pool *zs_create_pool(const char *name, gfp_t flags,
+ struct zs_pool_ops *ops)
{
int i, error, ovhd_size;
struct zs_pool *pool;
@@ -492,7 +509,7 @@ struct zs_pool *zs_create_pool(const char *name, gfp_t flags)

ovhd_size = roundup(sizeof(*pool), PAGE_SIZE);
pool = kzalloc(ovhd_size, GFP_KERNEL);
- if (!pool)
+ if (!pool || (ops && (!ops->alloc_page || !ops->free_page)))
return NULL;

for (i = 0; i < ZS_SIZE_CLASSES; i++) {
@@ -524,6 +541,10 @@ struct zs_pool *zs_create_pool(const char *name, gfp_t flags)

pool->flags = flags;
pool->name = name;
+ if (ops)
+ pool->ops = ops;
+ else
+ pool->ops = &default_ops;

error = 0; /* Success */

@@ -592,7 +613,7 @@ void *zs_malloc(struct zs_pool *pool, size_t size)

if (!first_page) {
spin_unlock(&class->lock);
- first_page = alloc_zspage(class, pool->flags);
+ first_page = alloc_zspage(pool, class);
if (unlikely(!first_page))
return NULL;

@@ -658,7 +679,7 @@ void zs_free(struct zs_pool *pool, void *obj)
spin_unlock(&class->lock);

if (fullness == ZS_EMPTY)
- free_zspage(first_page);
+ free_zspage(pool, first_page);
}
EXPORT_SYMBOL_GPL(zs_free);

diff --git a/drivers/staging/zsmalloc/zsmalloc.h b/drivers/staging/zsmalloc/zsmalloc.h
index 949384e..51fb32e 100644
--- a/drivers/staging/zsmalloc/zsmalloc.h
+++ b/drivers/staging/zsmalloc/zsmalloc.h
@@ -17,7 +17,13 @@

struct zs_pool;

-struct zs_pool *zs_create_pool(const char *name, gfp_t flags);
+struct zs_pool_ops {
+ struct page * (*alloc_page)(gfp_t);
+ void (*free_page)(struct page *);
+};
+
+struct zs_pool *zs_create_pool(const char *name, gfp_t flags,
+ struct zs_pool_ops *ops);
void zs_destroy_pool(struct zs_pool *pool);

void *zs_malloc(struct zs_pool *pool, size_t size);
diff --git a/drivers/staging/zsmalloc/zsmalloc_int.h b/drivers/staging/zsmalloc/zsmalloc_int.h
index 92eefc6..ade09c1 100644
--- a/drivers/staging/zsmalloc/zsmalloc_int.h
+++ b/drivers/staging/zsmalloc/zsmalloc_int.h
@@ -16,6 +16,7 @@
#include <linux/kernel.h>
#include <linux/spinlock.h>
#include <linux/types.h>
+#include "zsmalloc.h"

/*
* This must be power of 2 and greater than of equal to sizeof(link_free).
@@ -146,6 +147,7 @@ struct link_free {
};

struct zs_pool {
+ struct zs_pool_ops *ops;
struct size_class size_class[ZS_SIZE_CLASSES];

gfp_t flags; /* allocation flags used when growing pool */

--
1.7.5.4

Greg Kroah-Hartman

2012-03-16 21:32:27 UTC

Permalink

Post by Seth Jennings
This patch allows a zsmalloc user to define the page
allocation and free functions to be used when growing
or releasing parts of the memory pool.
The functions are passed in the struct zs_pool_ops parameter
of zs_create_pool() at pool creation time. If this parameter
is NULL, zsmalloc uses alloc_page and __free_page() by default.
While there is no current user of this functionality, zcache
development plans to make use of it in the near future.

I'm starting to get tired of seeing new features be added to this chunk
of code, and the other related bits, without any noticable movement
toward getting it merged into the mainline tree.

So, I'm going to take a stance here and say, no more new features until
it gets merged into the "real" part of the kernel tree, as you all
should not be spinning your wheels on new stuff, when there's no
guarantee that the whole thing could just be rejected outright tomorrow.

I'm sorry, I know this isn't fair for your specific patch, but we have
to stop this sometime, and as this patch adds code isn't even used by
anyone, its a good of a time as any.

greg k-h

Seth Jennings

2012-03-19 18:54:56 UTC

Permalink

Post by Greg Kroah-Hartman

I'm starting to get tired of seeing new features be added to this chunk
of code, and the other related bits, without any noticable movement
toward getting it merged into the mainline tree.

Fair enough

Post by Greg Kroah-Hartman
So, I'm going to take a stance here and say, no more new features until
it gets merged into the "real" part of the kernel tree, as you all
should not be spinning your wheels on new stuff, when there's no
guarantee that the whole thing could just be rejected outright tomorrow.
I'm sorry, I know this isn't fair for your specific patch, but we have
to stop this sometime, and as this patch adds code isn't even used by
anyone, its a good of a time as any.

So, this the my first "promotion from staging" rodeo. I would love to
see this code mainlined ASAP. How would I/we go about doing that?

I guess another way to ask is, what needs to be done in the way of
code quality and acks in the community to promote zcache to
/drivers/misc for example?

Also, the tmem part of zcache will (probably, Dan?) be broken
out an placed in /lib.

--
Seth

Greg Kroah-Hartman

2012-03-19 23:34:09 UTC

Permalink

Post by Seth Jennings

Post by Greg Kroah-Hartman
I'm sorry, I know this isn't fair for your specific patch, but we have
to stop this sometime, and as this patch adds code isn't even used by
anyone, its a good of a time as any.

So, this the my first "promotion from staging" rodeo. I would love to
see this code mainlined ASAP. How would I/we go about doing that?

What subsystem should this code live in? The -mm code, I'm guessing,
right? If so, submit it to the linux-mm mailing list for inclusion, you
can point them at what is in drivers/staging right now, or probably it's
easier if you just make a new patch that adds the code that is in
drivers/staging/ to the correct location in the kernel. That way it's
easier to review and change. When it finally gets accepted, we can then
delete the drivers/staging code.

hope this helps,

greg k-h

Konrad Rzeszutek Wilk

2012-03-26 15:50:19 UTC

Permalink

Post by Greg Kroah-Hartman

Post by Seth Jennings

So, this the my first "promotion from staging" rodeo. I would love to
see this code mainlined ASAP. How would I/we go about doing that?

Hey Greg,

Little background - for zcache to kick-butts (both Dan and Seth posted some
pretty awesome benchmark numbers) it depends on the frontswap - which is in
the #linux-next. Dan made an attempt to post it for a GIT PULL and an interesting
conversation ensued where folks decided it needs more additions before they were
comfortable with it. zcache isn't using those additions, but I don't see why
it couldn't use them.

The things that bouncing around in my head are:
- get a punch-out list (ie todo) of what MM needs for the zcache to get out.
I think posting it as a new driver would be right way to do it (And then
based on feedback work out the issues in drivers/staging). But what
about authorship - there are mulitple authors ?

- zcache is a bit different that the normal drivers type - and it is unclear
yet what will be required to get it out - and both Seth and Nitin have this
hungry look in their eyes of wanting to make it super-duper. So doing
the work to do it - is not going to be a problem at all - just some form
of clear goals of what we need "now" vs "would love to have".

- folks are using it, which means continued -stable kernel back-porting.

So with that in mind I was wondering whether you would be up for:
- me sending to you before a merge window some updates to the zcache
as a git pull - that way you won't have to deal with a bunch of
small patches and when there is something you don't like we can fix
it up to your liking. The goal would be for us - Dan, Nitin, Seth and me
working on promoting the driver out of staging and you won't have to
be bugged every time we have a new change that might be perceived
as feature, but is in fact a step towards mainstreaming it. I figured
that is what you are most annoyed at - handling those uncoordinated
requests and not seeing a clear target.

- alongside of that, I work on making those frontswap changes folks
have asked for. Since those changes can affect zcache, that means
adding them in zcache alongside.

Hopefully, by the time those two items are done, both pieces can go in
the kernel at the same time-ish.

Greg Kroah-Hartman

2012-03-27 17:06:00 UTC

Permalink

Post by Konrad Rzeszutek Wilk

Post by Greg Kroah-Hartman

Post by Seth Jennings

So, this the my first "promotion from staging" rodeo. I would love to
see this code mainlined ASAP. How would I/we go about doing that?

What does authorship matter here? To move files out of staging, just
send a patch that does that, all authorship history is preserved.

And as a new driver, that's up to the mm developers, not me.

Post by Konrad Rzeszutek Wilk
- zcache is a bit different that the normal drivers type - and it is unclear
yet what will be required to get it out - and both Seth and Nitin have this
hungry look in their eyes of wanting to make it super-duper. So doing
the work to do it - is not going to be a problem at all - just some form
of clear goals of what we need "now" vs "would love to have".

Again, work with the -mm developers.

Post by Konrad Rzeszutek Wilk
- folks are using it, which means continued -stable kernel back-porting.

What do you mean by this?

Post by Konrad Rzeszutek Wilk
- me sending to you before a merge window some updates to the zcache
as a git pull - that way you won't have to deal with a bunch of
small patches and when there is something you don't like we can fix
it up to your liking. The goal would be for us - Dan, Nitin, Seth and me
working on promoting the driver out of staging and you won't have to
be bugged every time we have a new change that might be perceived
as feature, but is in fact a step towards mainstreaming it. I figured
that is what you are most annoyed at - handling those uncoordinated
requests and not seeing a clear target.

Lots of small patches are fine, as long as they are obviously working
toward getting the code out of staging. This specific patch was just
adding a new feature, one that no one could even use, so that was not
something that would help it get out of the staging tree.

So no, I don't need patches batched up, and a git pull, I just need to
see that every patch I am sent is working toward getting it out of here.

Post by Konrad Rzeszutek Wilk
- alongside of that, I work on making those frontswap changes folks
have asked for. Since those changes can affect zcache, that means
adding them in zcache alongside.

Ok.

Post by Konrad Rzeszutek Wilk
Hopefully, by the time those two items are done, both pieces can go in
the kernel at the same time-ish.

That would be good to see have happen.

thanks,

greg k-h