Skip to contentSkip to conversation

slab: initialize unused alien cache entry as NULL at alloc_alien_cache().

PATCH In review Haicheng Li 4 participants 8 messages Jan 6, 2010 – Jan 7, 2010 Memory management

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 slab: initialize unused alien cache entry as NULL at alloc_alien_cache().

Comparing with existing code, it's a simpler way to use kzalloc_node() to ensure that each unused alien cache entry is NULL.

---
 mm/slab.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
mm/slab.c +2 -4
index 7dfa481..5d1a782 100644
@@ -971,13 +971,11 @@ static struct array_cache **alloc_alien_cache(int node, int limit, gfp_t gfp)
 
 	if (limit > 1)
 		limit = 12;
-	ac_ptr = kmalloc_node(memsize, gfp, node);
+	ac_ptr = kzalloc_node(memsize, gfp, node);
 	if (ac_ptr) {
 		for_each_node(i) {
-			if (i == node || !node_online(i)) {
-				ac_ptr[i] = NULL;
+			if (i == node || !node_online(i))
 				continue;
-			}
 			ac_ptr[i] = alloc_arraycache(node, limit, 0xbaadf00d, gfp);
 			if (!ac_ptr[i]) {
 				for (i--; i >= 0; i--)
Signature · 8 lines
1.5.3.8


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#1 lore raw
Quoted 9 lines · Haicheng Li

Comparing with existing code, it's a simpler way to use kzalloc_node() to ensure that each unused alien cache entry is NULL.

I can find a trace of Andi acking the previous version of this patch but I don't see an ACK from Christoph nor a revieved-by from Matt. Was I not CC'd on those emails or what's going on here?

Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#2 lore raw
Quoted 4 lines · Pekka Enberg

I can find a trace of Andi acking the previous version of this patch but I don't see an ACK from Christoph nor a revieved-by from Matt. Was I not CC'd on those emails or what's going on here?

Pekka,

Christoph said he will ack this patch if remove the change of MAX_NUMNODES (see below), so I add him directly as Acked-by in this revised patch. And also, I got review comments from Matt for v1 and changed the patch accordingly.

Is it a violation of the rule? if so, I'm sorry, actually not quite clear with the rule.

Quoted 13 lines · Christoph Lameter

Review static void *alternate_node_alloc(struct kmem_cache *, · line 971

Quoted 8 lines · Haicheng Li
(quoted diff) +1 -1
@@ -966,18 +966,16 @@ static void *alternate_node_alloc(struct kmem_cache *,
 gfp_t);
 static struct array_cache **alloc_alien_cache(int node, int limit, gfp_t gfp)
 {
 	struct array_cache **ac_ptr;
-	int memsize = sizeof(void *) * nr_node_ids;
+	int memsize = sizeof(void *) * MAX_NUMNODES;
 	int i;

Remove this change and I will ack the patch.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#3 lore raw

Hi,

Quoted 11 lines · Haicheng Li
Quoted 4 lines · Pekka Enberg

I can find a trace of Andi acking the previous version of this patch but I don't see an ACK from Christoph nor a revieved-by from Matt. Was I not CC'd on those emails or what's going on here?

Christoph said he will ack this patch if remove the change of MAX_NUMNODES (see below), so I add him directly as Acked-by in this revised patch. And also, I got review comments from Matt for v1 and changed the patch accordingly.

Is it a violation of the rule? if so, I'm sorry, actually not quite clear with the rule.

See Section 14 of Documentation/SubmittingPatches. You should never add tags unless they came from the said person. The ACKs from Andi is fine, the one from Christoph is borderline but OK and the one from Matt is _not_ OK.

I can fix those up but I'll wait from an explicit ACK from Christoph first.

Pekka

#4 lore raw

understood. thanks!

Quoted 13 lines · Pekka Enberg

Hi,

Quoted 10 lines · Haicheng Li
Quoted 4 lines · Pekka Enberg

I can find a trace of Andi acking the previous version of this patch but I don't see an ACK from Christoph nor a revieved-by from Matt. Was I not CC'd on those emails or what's going on here?

Christoph said he will ack this patch if remove the change of MAX_NUMNODES (see below), so I add him directly as Acked-by in this revised patch. And also, I got review comments from Matt for v1 and changed the patch accordingly.

Quoted 7 lines
Quoted 1 line

Is it a violation of the rule? if so, I'm sorry, actually not quite clear with the rule.

See Section 14 of Documentation/SubmittingPatches. You should never add tags unless they came from the said person. The ACKs from Andi is fine, the one from Christoph is borderline but OK and the one from Matt is _not_ OK.

I can fix those up but I'll wait from an explicit ACK from Christoph first.

Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#6 lore raw
In reply to Haicheng Li

Haicheng Li kirjoitti:

Quoted 29 lines

Comparing with existing code, it's a simpler way to use kzalloc_node() to ensure that each unused alien cache entry is NULL.

CC Pekka Enberg <[email protected]>
CC Eric Dumazet <[email protected]>
---
 mm/slab.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
mm/slab.c +2 -4
index 7dfa481..5d1a782 100644
@@ -971,13 +971,11 @@ static struct array_cache **alloc_alien_cache(int 
 node, int limit, gfp_t gfp)
 
     if (limit > 1)
         limit = 12;
-    ac_ptr = kmalloc_node(memsize, gfp, node);
+    ac_ptr = kzalloc_node(memsize, gfp, node);
     if (ac_ptr) {
         for_each_node(i) {
-            if (i == node || !node_online(i)) {
-                ac_ptr[i] = NULL;
+            if (i == node || !node_online(i))
                 continue;
-            }
             ac_ptr[i] = alloc_arraycache(node, limit, 0xbaadf00d, gfp);
             if (!ac_ptr[i]) {
Quoted 1 line
                 for (i--; i >= 0; i--)

Christoph? Matt?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Acked-by
#7 lore raw
In reply to Pekka Enberg
Quoted 17 lines · Pekka Enberg

Haicheng Li kirjoitti:

Quoted 16 lines

Comparing with existing code, it's a simpler way to use kzalloc_node() to ensure that each unused alien cache entry is NULL.

CC Pekka Enberg <[email protected]>
CC Eric Dumazet <[email protected]>
---
 mm/slab.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
mm/slab.c +0 -0
index 7dfa481..5d1a782 100644
@@ -971,13 +971,11 @@ static struct array_cache **alloc_alien_cache(int 
 node, int limit, gfp_t gfp)
 
Quoted 16 lines
Quoted 13 lines
mm/slab.c +2 -4
     if (limit > 1)
         limit = 12;
-    ac_ptr = kmalloc_node(memsize, gfp, node);
+    ac_ptr = kzalloc_node(memsize, gfp, node);
     if (ac_ptr) {
         for_each_node(i) {
-            if (i == node || !node_online(i)) {
-                ac_ptr[i] = NULL;
+            if (i == node || !node_online(i))
                 continue;
-            }
             ac_ptr[i] = alloc_arraycache(node, limit, 0xbaadf00d, gfp);
             if (!ac_ptr[i]) {
Quoted 1 line
                 for (i--; i >= 0; i--)

Christoph? Matt?

Looks like a fine cleanup.

Signature · 8 lines
http://selenic.com : development and support for Mercurial and Linux


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>