zcache: fix preemptable memory allocation in atomic context

In zcache_get_pool_by_id, the refcount of zcache_host is not increased, but it is always decreased in zcache_put_pool

Signed-off-by Xiao Guangrong <[email protected]>

From: Seth Jennings [mailto:[email protected]]
Sent: Tuesday, June 19, 2012 8:29 AM
To: Xiao Guangrong
Cc: Andrew Morton; Dan Magenheimer; LKML; [email protected]
Subject: Re: [PATCH 02/10] zcache: fix refcount leak

Quoted 4 lines · Xiao Guangrong

In zcache_get_pool_by_id, the refcount of zcache_host is not increased, but it is always decreased in zcache_put_pool

Signed-off-by Xiao Guangrong <[email protected]>

Acked-by Seth Jennings <[email protected]>

My preference would be to fix it the opposite way, by checking and ignoring zcache_host in zcache_put_pool. The ref-counting is to ensure that a client isn't accidentally destroyed while in use (for multiple-client users such as ramster and kvm) and since zcache_host is a static struct, it should never be deleted so need not be ref-counted.

Quoted 6 lines · Dan Magenheimer

My preference would be to fix it the opposite way, by checking and ignoring zcache_host in zcache_put_pool. The ref-counting is to ensure that a client isn't accidentally destroyed while in use (for multiple-client users such as ramster and kvm) and since zcache_host is a static struct, it should never be deleted so need not be ref-counted.

If we do that, we'll need to comment it. If we don't, it won't be obvious why we are refcounting every zcache client except one. It'll look like a bug.

Quoted 10 lines · Seth Jennings

Quoted 6 lines · Dan Magenheimer

My preference would be to fix it the opposite way, by checking and ignoring zcache_host in zcache_put_pool. The ref-counting is to ensure that a client isn't accidentally destroyed while in use (for multiple-client users such as ramster and kvm) and since zcache_host is a static struct, it should never be deleted so need not be ref-counted.

Quoted 3 lines

If we do that, we'll need to comment it. If we don't, it won't be obvious why we are refcounting every zcache client except one. It'll look like a bug.

Okay, i will fix it like Dan's way and comment it.

From: Xiao Guangrong [mailto:[email protected]]
Subject: Re: [PATCH 02/10] zcache: fix refcount leak

Quoted 12 lines · Xiao Guangrong

Quoted 10 lines · Seth Jennings

Quoted 6 lines · Dan Magenheimer

My preference would be to fix it the opposite way, by checking and ignoring zcache_host in zcache_put_pool. The ref-counting is to ensure that a client isn't accidentally destroyed while in use (for multiple-client users such as ramster and kvm) and since zcache_host is a static struct, it should never be deleted so need not be ref-counted.

Quoted 6 lines

Quoted 3 lines

If we do that, we'll need to comment it. If we don't, it won't be obvious why we are refcounting every zcache client except one. It'll look like a bug.

Okay, i will fix it like Dan's way and comment it.

Hmm...But i notice that zcache_host is the same as other clients, all of them are static struct:

| static struct zcache_client zcache_host;
| static struct zcache_client zcache_clients[MAX_CLIENTS];

And all of them are not destroyed.

Quoted 27 lines

From: Xiao Guangrong [mailto:[email protected]]
Subject: Re: [PATCH 02/10] zcache: fix refcount leak

Quoted 12 lines · Xiao Guangrong

Quoted 10 lines · Seth Jennings

Quoted 6 lines · Dan Magenheimer

My preference would be to fix it the opposite way, by checking and ignoring zcache_host in zcache_put_pool. The ref-counting is to ensure that a client isn't accidentally destroyed while in use (for multiple-client users such as ramster and kvm) and since zcache_host is a static struct, it should never be deleted so need not be ref-counted.

Quoted 6 lines

Quoted 3 lines

If we do that, we'll need to comment it. If we don't, it won't be obvious why we are refcounting every zcache client except one. It'll look like a bug.

Okay, i will fix it like Dan's way and comment it.

Hmm...But i notice that zcache_host is the same as other clients, all of them are static struct:

Quoted 4 lines

| static struct zcache_client zcache_host;
| static struct zcache_client zcache_clients[MAX_CLIENTS];

And all of them are not destroyed.

Yes, the code currently in zcache was a first step towards supporting multiple clients. Ramster goes one step further and kvm will require even a tiny bit more work.

FYI, I'm working on a unification version of zcache that can support all of these cleanly as well as better support for eviction that will make standalone zcache more suitable for promotion from staging and enterprise-ready. Due to various summer commitments, it will probably be a few weeks before it is ready for posting.

fix:

drivers/staging/zcache/zcache-main.c: In function a??zcache_comp_opa??:
drivers/staging/zcache/zcache-main.c:112:2: warning: a??reta?? may be used uninitial

Signed-off-by Xiao Guangrong <[email protected]>

---
 drivers/staging/zcache/zcache-main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/zcache/zcache-main.c +0 -0

index 32fe0ba..74a3ac8 100644

Quoted 15 lines · Xiao Guangrong

fix:

drivers/staging/zcache/zcache-main.c: In function ‘zcache_comp_op’:
drivers/staging/zcache/zcache-main.c:112:2: warning: ‘ret’ may be used uninitial

Signed-off-by Xiao Guangrong <[email protected]>

---
 drivers/staging/zcache/zcache-main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/zcache/zcache-main.c +0 -0

index 32fe0ba..74a3ac8 100644

drivers/staging/zcache/zcache-main.c static inline int zcache_comp_op(enum comp_op op, · line 99

Quoted 8 lines

drivers/staging/zcache/zcache-main.c +1 -1

@@ -93,7 +93,7 @@ static inline int zcache_comp_op(enum comp_op op,
 				u8 *dst, unsigned int *dlen)
 {
 	struct crypto_comp *tfm;
-	int ret;
+	int ret = -1;
 
 	BUG_ON(!zcache_comp_pcpu_tfms);
 	tfm = *per_cpu_ptr(zcache_comp_pcpu_tfms, get_cpu());

What about adding a default case in the switch like this?

default:

	ret = -EINVAL;

That way we don't assign ret twice.

zcache is enabled only if one of CONFIG_CLEANCACHE and CONFIG_FRONTSWAP is enabled, see the Kconfig:

	depends on (CLEANCACHE || FRONTSWAP) && CRYPTO=y && X86

So, we can remove the check in the source code

Signed-off-by Xiao Guangrong <[email protected]>

These functions are called only when system is initializing, so mark __init for them to free memory

Signed-off-by Xiao Guangrong <[email protected]>

tmem_obj_find and insertion tmem-obj have the some logic, we can integrate the code

Signed-off-by Xiao Guangrong <[email protected]>

---
 drivers/staging/zcache/tmem.c |   58 +++++++++++++++++++++-------------------
 1 files changed, 30 insertions(+), 28 deletions(-)

drivers/staging/zcache/tmem.c +26 -23

index 1ca66ea..cdf2d3c 100644
@@ -72,33 +72,48 @@ void tmem_register_pamops(struct tmem_pamops *m)
  * the hashbucket lock must be held.
  */
 
-/* searches for object==oid in pool, returns locked object if found */
-static struct tmem_obj *tmem_obj_find(struct tmem_hashbucket *hb,
-					struct tmem_oid *oidp)
+static struct tmem_obj
+*__tmem_obj_find(struct tmem_hashbucket*hb, struct tmem_oid *oidp,
+		 struct rb_node *parent, struct rb_node **link)
 {
-	struct rb_node *rbnode;
+	struct rb_node **rbnode;
 	struct tmem_obj *obj;
 
-	rbnode = hb->obj_rb_root.rb_node;
-	while (rbnode) {
-		BUG_ON(RB_EMPTY_NODE(rbnode));
-		obj = rb_entry(rbnode, struct tmem_obj, rb_tree_node);
+	rbnode = &hb->obj_rb_root.rb_node;
+	while (*rbnode) {
+		BUG_ON(RB_EMPTY_NODE(*rbnode));
+		obj = rb_entry(*rbnode, struct tmem_obj,
+			       rb_tree_node);
 		switch (tmem_oid_compare(oidp, &obj->oid)) {
 		case 0: /* equal */
 			goto out;
 		case -1:
-			rbnode = rbnode->rb_left;
+			rbnode = &(*rbnode)->rb_left;
 			break;
 		case 1:
-			rbnode = rbnode->rb_right;
+			rbnode = &(*rbnode)->rb_right;
 			break;
 		}
 	}
+
+	if (parent)
+		parent = &obj->rb_tree_node;
+	if (link)
+		link = rbnode;
+
 	obj = NULL;
 out:
 	return obj;
 }
 
+
+/* searches for object==oid in pool, returns locked object if found */
+static struct tmem_obj *tmem_obj_find(struct tmem_hashbucket *hb,
+					struct tmem_oid *oidp)
+{
+	return __tmem_obj_find(hb, oidp, NULL, NULL);
+}
+
 static void tmem_pampd_destroy_all_in_obj(struct tmem_obj *);
 
 /* free an object that has no more pampds in it */

@@ -131,8 +146,7 @@ static void tmem_obj_init(struct tmem_obj *obj, struct tmem_hashbucket *hb,
 					struct tmem_oid *oidp)
 {
 	struct rb_root *root = &hb->obj_rb_root;
-	struct rb_node **new = &(root->rb_node), *parent = NULL;
-	struct tmem_obj *this;
+	struct rb_node **new = NULL, *parent = NULL;
 
 	BUG_ON(pool == NULL);
 	atomic_inc(&pool->obj_count);

@@ -144,22 +158,10 @@ static void tmem_obj_init(struct tmem_obj *obj, struct tmem_hashbucket *hb,
 	obj->pampd_count = 0;
 	(*tmem_pamops.new_obj)(obj);
 	SET_SENTINEL(obj, OBJ);
-	while (*new) {
-		BUG_ON(RB_EMPTY_NODE(*new));
-		this = rb_entry(*new, struct tmem_obj, rb_tree_node);
-		parent = *new;
-		switch (tmem_oid_compare(oidp, &this->oid)) {
-		case 0:
-			BUG(); /* already present; should never happen! */
-			break;
-		case -1:
-			new = &(*new)->rb_left;
-			break;

This patch causes a crash, details below.

zcache_do_preload uses ZCACHE_GFP_MASK to allocate memory that will be sleep, but zcache_do_preload is called in zcache_put_page where IRQ is disabled

Fix it by use GFP_ATOMIC flag

Did you get a might_sleep warning on this? I haven't seen this being an issue.

GFP_ATOMIC only modifies the existing mask to allow allocation use the emergency pool. It is __GFP_WAIT not being set that prevents sleep. We don't want to use the emergency pool since we make large, long lived allocations with this mask.

I just noticed you sent this patchset to Andrew, but the staging tree is maintained by Greg. You're going to want to send these patches to him.

Greg Kroah-Hartman <[email protected]>

Quoted 5 lines · Seth Jennings

I just noticed you sent this patchset to Andrew, but the staging tree is maintained by Greg. You're going to want to send these patches to him.

Greg Kroah-Hartman <[email protected]>

After this series is redone, right? As it is, this submission didn't look ok, so I'm hoping a second round is forthcoming...

Quoted 5 lines · Greg Kroah-Hartman

Quoted 4 lines · Seth Jennings

I just noticed you sent this patchset to Andrew, but the staging tree is maintained by Greg. You're going to want to send these patches to him.

Quoted 4 lines

Quoted 1 line

Greg Kroah-Hartman <[email protected]>

After this series is redone, right? As it is, this submission didn't look ok, so I'm hoping a second round is forthcoming...

Yes. That is the cleanest way since there are dependencies among the patches. You could pull 04-08 and be ok, but you might just prefer a repost.

Quoted 8 lines · Seth Jennings

Quoted 7 lines · Greg Kroah-Hartman

Quoted 5 lines · Seth Jennings

I just noticed you sent this patchset to Andrew, but the staging tree is maintained by Greg. You're going to want to send these patches to him.

Greg Kroah-Hartman <[email protected]>

Quoted 6 lines

Quoted 2 lines

After this series is redone, right? As it is, this submission didn't look ok, so I'm hoping a second round is forthcoming...

Yes. That is the cleanest way since there are dependencies among the patches. You could pull 04-08 and be ok, but you might just prefer a repost.

I do prefer a repost, thanks.