mm: slub: invalid memory access in setup_object

DISCUSSION Sasha Levin 5 participants 16 messages Jun 25, 2014 – Jul 8, 2014

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Sasha Levin

author

2014-06-25 16:51 UTC #0 lore raw

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next kernel I've stumbled on the following spew:

[  791.659908] BUG: unable to handle kernel paging request at ffff880302e12000
[  791.661580] IP: memset (arch/x86/lib/memset_64.S:83)
[  791.661580] PGD 17b7d067 PUD 704947067 PMD 70492f067 PTE 8000000302e12060
[  791.661580] Oops: 0002 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  791.661580] Dumping ftrace buffer:
[  791.661580]    (ftrace buffer empty)
[  791.667964] Modules linked in:
[  791.667964] CPU: 13 PID: 10630 Comm: trinity-c20 Not tainted 3.16.0-rc2-next-20140624-sasha-00024-g332b58d #726
[  791.669480] task: ffff8803d5123000 ti: ffff8803ba460000 task.ti: ffff8803ba460000
[  791.669480] RIP: memset (arch/x86/lib/memset_64.S:83)
[  791.669480] RSP: 0018:ffff8803ba463b18  EFLAGS: 00010003
[  791.669480] RAX: 6b6b6b6b6b6b6b6b RBX: ffff880036851540 RCX: 0000000000000068
[  791.669480] RDX: 0000000000002a77 RSI: 000000000000006b RDI: ffff880302e12000
[  791.669480] RBP: ffff8803ba463b40 R08: 0000000000000001 R09: 0000000000000000
[  791.669480] R10: ffff880302e11000 R11: ffffffffffffffd8 R12: ffff880302e11000
[  791.669480] R13: 00000000000000bb R14: ffff880302e11000 R15: ffffffffffffffff
[  791.669480] FS:  00007f37693b3700(0000) GS:ffff880334e00000(0000) knlGS:0000000000000000
[  791.669480] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  791.669480] CR2: ffff880302e12000 CR3: 00000003b744f000 CR4: 00000000000006a0
[  791.669480] Stack:
[  791.669480]  ffffffff902f4273 ffff8803ba463b30 ffff880036851540 ffff880302e11000
[  791.669480]  ffffea000c0b8440 ffff8803ba463b60 ffffffff902f48b0 ffff880036851540
[  791.669480]  ffff880302e11000 ffff8803ba463bc0 ffffffff902f6886 00000000000000d0
[  791.669480] Call Trace:
[  791.669480] ? init_object (mm/slub.c:665)
[  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
[  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)
[  791.669480] __slab_alloc (mm/slub.c:2186 mm/slub.c:2344)
[  791.690803] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:90 arch/x86/kernel/kvmclock.c:86)
[  791.690803] ? copy_process (kernel/fork.c:306 kernel/fork.c:1193)
[  791.690803] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:305)
[  791.690803] ? get_parent_ip (kernel/sched/core.c:2550)
[  791.690803] kmem_cache_alloc_node (mm/slub.c:2417 mm/slub.c:2486)
[  791.690803] ? sched_clock_cpu (kernel/sched/clock.c:311)
[  791.690803] ? copy_process (kernel/fork.c:306 kernel/fork.c:1193)
[  791.690803] copy_process (kernel/fork.c:306 kernel/fork.c:1193)
[  791.690803] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:90 arch/x86/kernel/kvmclock.c:86)
[  791.690803] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:305)
[  791.690803] ? sched_clock_local (kernel/sched/clock.c:214)
[  791.690803] ? put_lock_stats.isra.12 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
[  791.690803] do_fork (kernel/fork.c:1609)
[  791.690803] ? get_parent_ip (kernel/sched/core.c:2550)
[  791.690803] ? context_tracking_user_exit (./arch/x86/include/asm/paravirt.h:809 (discriminator 2) kernel/context_tracking.c:184 (discriminator 2))
[  791.690803] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[  791.690803] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2557 kernel/locking/lockdep.c:2599)
[  791.690803] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
[  791.690803] SyS_clone (kernel/fork.c:1695)
[  791.690803] stub_clone (arch/x86/kernel/entry_64.S:637)
[  791.690803] ? tracesys (arch/x86/kernel/entry_64.S:542)
[ 791.690803] Code: b8 01 01 01 01 01 01 01 01 48 0f af c1 41 89 f9 41 83 e1 07 75 70 48 89 d1 48 c1 e9 06 74 39 66 0f 1f 84 00 00 00 00 00 48 ff c9 <48> 89 07 48 89 47 08 48 89 47 10 48 89 47 18 48 89 47 20 48 89
All code
========
   0:	b8 01 01 01 01       	mov    $0x1010101,%eax
   5:	01 01                	add    %eax,(%rcx)
   7:	01 01                	add    %eax,(%rcx)
   9:	48 0f af c1          	imul   %rcx,%rax
   d:	41 89 f9             	mov    %edi,%r9d
  10:	41 83 e1 07          	and    $0x7,%r9d
  14:	75 70                	jne    0x86
  16:	48 89 d1             	mov    %rdx,%rcx
  19:	48 c1 e9 06          	shr    $0x6,%rcx
  1d:	74 39                	je     0x58
  1f:	66 0f 1f 84 00 00 00 	nopw   0x0(%rax,%rax,1)
  26:	00 00
  28:	48 ff c9             	dec    %rcx
  2b:*	48 89 07             	mov    %rax,(%rdi)		<-- trapping instruction
  2e:	48 89 47 08          	mov    %rax,0x8(%rdi)
  32:	48 89 47 10          	mov    %rax,0x10(%rdi)
  36:	48 89 47 18          	mov    %rax,0x18(%rdi)
  3a:	48 89 47 20          	mov    %rax,0x20(%rdi)
  3e:	48 89 00             	mov    %rax,(%rax)

Code starting with the faulting instruction
===========================================

   0:	48 89 07             	mov    %rax,(%rdi)
   3:	48 89 47 08          	mov    %rax,0x8(%rdi)
   7:	48 89 47 10          	mov    %rax,0x10(%rdi)
   b:	48 89 47 18          	mov    %rax,0x18(%rdi)
   f:	48 89 47 20          	mov    %rax,0x20(%rdi)
  13:	48 89 00             	mov    %rax,(%rax)
[  791.690803] RIP memset (arch/x86/lib/memset_64.S:83)
[  791.690803]  RSP <ffff8803ba463b18>
[  791.690803] CR2: ffff880302e12000

Thanks,
Sasha

--
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>

Christoph Lameter

reviewer

2014-06-25 17:30 UTC #1 lore raw

Quoted 3 lines · Sasha Levin

[  791.669480] ? init_object (mm/slub.c:665)
[  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
[  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)

So we just got a new page from the page allocator but somehow cannot write to it. This is the first write access to the page.

David Rientjes

reviewer

2014-06-30 22:03 UTC #2 lore raw

Quoted 9 lines · Christoph Lameter

Quoted 3 lines · Sasha Levin
[  791.669480] ? init_object (mm/slub.c:665)
[  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
[  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)
So we just got a new page from the page allocator but somehow cannot write to it. This is the first write access to the page.

I'd be inclined to think that this was a result of "slub: reduce duplicate creation on the first object" from -mm[*] that was added the day before Sasha reported the problem.

It's not at all clear to me that that patch is correct. Wei?

Sasha, with a revert of that patch, does this reproduce?

[*] http://ozlabs.org/~akpm/mmotm/broken-out/slub-reduce-duplicate-creation-on-the-first-object.patch

Wei Yang

2014-07-01 01:40 UTC #3 lore raw

Quoted 16 lines · David Rientjes

Quoted 9 lines · Christoph Lameter
Quoted 3 lines · Sasha Levin
[  791.669480] ? init_object (mm/slub.c:665)
[  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
[  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)
So we just got a new page from the page allocator but somehow cannot write to it. This is the first write access to the page.
I'd be inclined to think that this was a result of "slub: reduce duplicate creation on the first object" from -mm[*] that was added the day before Sasha reported the problem.

Quoted 5 lines

It's not at all clear to me that that patch is correct. Wei?
Sasha, with a revert of that patch, does this reproduce?
[*] http://ozlabs.org/~akpm/mmotm/broken-out/slub-reduce-duplicate-creation-on-the-first-object.patch

David,

So sad to see the error after applying my patch. In which case this is triggered? The kernel with this patch runs fine on my laptop. Maybe there is some corner case I missed? If you could tell me the way you reproduce it, I would have a try on my side.

I did a simple test for this patch, my test code and result is attached.

1. kmem_cache.c
The test module.
2. kmem_log.txt
In this log, you can see 26 objects are initialized once exactly, while without this patch, the first object will be initialized twice.

     Fetch a cache from kmem_cache
     new_slab: page->objects is 26
     new_slab: setup on ffff880097038000, ffff8800970384e0
       init_once: [00]ffff880097038000 is created
     new_slab: setup on ffff8800970384e0, ffff8800970389c0
       init_once: [01]ffff8800970384e0 is created
     new_slab: setup on ffff8800970389c0, ffff880097038ea0
       init_once: [02]ffff8800970389c0 is created
     new_slab: setup on ffff880097038ea0, ffff880097039380
       init_once: [03]ffff880097038ea0 is created
     new_slab: setup on ffff880097039380, ffff880097039860
       init_once: [04]ffff880097039380 is created
     new_slab: setup on ffff880097039860, ffff880097039d40
       init_once: [05]ffff880097039860 is created
     new_slab: setup on ffff880097039d40, ffff88009703a220
       init_once: [06]ffff880097039d40 is created
     new_slab: setup on ffff88009703a220, ffff88009703a700
       init_once: [07]ffff88009703a220 is created
     new_slab: setup on ffff88009703a700, ffff88009703abe0
       init_once: [08]ffff88009703a700 is created
     new_slab: setup on ffff88009703abe0, ffff88009703b0c0
       init_once: [09]ffff88009703abe0 is created
     new_slab: setup on ffff88009703b0c0, ffff88009703b5a0
       init_once: [10]ffff88009703b0c0 is created
     new_slab: setup on ffff88009703b5a0, ffff88009703ba80
       init_once: [11]ffff88009703b5a0 is created
     new_slab: setup on ffff88009703ba80, ffff88009703bf60
       init_once: [12]ffff88009703ba80 is created
     new_slab: setup on ffff88009703bf60, ffff88009703c440
       init_once: [13]ffff88009703bf60 is created
     new_slab: setup on ffff88009703c440, ffff88009703c920
       init_once: [14]ffff88009703c440 is created
     new_slab: setup on ffff88009703c920, ffff88009703ce00
       init_once: [15]ffff88009703c920 is created
     new_slab: setup on ffff88009703ce00, ffff88009703d2e0
       init_once: [16]ffff88009703ce00 is created
     new_slab: setup on ffff88009703d2e0, ffff88009703d7c0
       init_once: [17]ffff88009703d2e0 is created
     new_slab: setup on ffff88009703d7c0, ffff88009703dca0
       init_once: [18]ffff88009703d7c0 is created
     new_slab: setup on ffff88009703dca0, ffff88009703e180
       init_once: [19]ffff88009703dca0 is created
     new_slab: setup on ffff88009703e180, ffff88009703e660
       init_once: [20]ffff88009703e180 is created
     new_slab: setup on ffff88009703e660, ffff88009703eb40
       init_once: [21]ffff88009703e660 is created
     new_slab: setup on ffff88009703eb40, ffff88009703f020
       init_once: [22]ffff88009703eb40 is created
     new_slab: setup on ffff88009703f020, ffff88009703f500
       init_once: [23]ffff88009703f020 is created
     new_slab: setup on ffff88009703f500, ffff88009703f9e0
       init_once: [24]ffff88009703f500 is created
     new_slab: do it again? ffff88009703f9e0
       init_once: [25]ffff88009703f9e0 is created

Signature · 2 lines

Richard Yang
Help you, Help me

/*
* ===================================================================================== *
* Filename: kmem_cache.c
*
* Description: /proc/slabinfo
*

 *        Version:  1.0
 *        Created:  04/26/2014 09:12:04 PM
 *       Revision:  none
 *       Compiler:  gcc

*
* Author: Wei Yang (weiyang), [email protected]
* Company:
*
* ===================================================================================== */

#include <linux/init.h>
#include <linux/module.h>
#include <linux/slab.h>
MODULE_LICENSE("Dual BSD/GPL");

static struct kmem_cache *test_cache;
void *tmp;

static void init_once(void *foo)

{
	static int num;
	printk(KERN_ERR "%s: [%02d]%p is created\n", __func__, num++, foo);
}

static int kmem_cache_test_init(void)

{
	test_cache = kmem_cache_create("test_cache", 1234, 4,
			0, init_once);
	if (test_cache == NULL)
		return -ENOMEM;

	printk(KERN_ERR "Fetch a cache from kmem_cache\n", __func__);
	tmp = kmem_cache_zalloc(test_cache, GFP_KERNEL);

	return 0;
}
static void kmem_cache_test_exit(void)
{
	kmem_cache_free(test_cache, tmp);
	kmem_cache_destroy(test_cache);
}

module_init(kmem_cache_test_init);
module_exit(kmem_cache_test_exit);

Christoph Lameter

reviewer

2014-07-01 14:58 UTC #4 lore raw

In reply to David Rientjes

Quoted 1 line · David Rientjes

It's not at all clear to me that that patch is correct. Wei?

Looks ok to me. But I do not like the convoluted code in new_slab() which Wei's patch does not make easier to read. Makes it difficult for the reader to see whats going on.

Lets drop the use of the variable named "last".

Subject: slub: Only call setup_object once for each object

Modify the logic for object initialization to be less convoluted and initialize an object only once.

Signed-off-by Christoph Lameter <[email protected]>

mm/slub.c +3 -8

Index: linux/mm/slub.c
===================================================================
@@ -1409,7 +1409,6 @@ static struct page *new_slab(struct kmem
 {
 	struct page *page;
 	void *start;
-	void *last;
 	void *p;
 	int order;

@@ -1432,15 +1431,11 @@ static struct page *new_slab(struct kmem
 	if (unlikely(s->flags & SLAB_POISON))
 		memset(start, POISON_INUSE, PAGE_SIZE << order);
 
-	last = start;
 	for_each_object(p, s, start, page->objects) {
-		setup_object(s, page, last);
-		set_freepointer(s, last, p);
-		last = p;
+		setup_object(s, page, p);
+		set_freepointer(s, p, p + s->size);
 	}
-	setup_object(s, page, last);
-	set_freepointer(s, last, NULL);
-
+	set_freepointer(s, start + (page->objects - 1) * s->size, NULL);
 	page->freelist = start;
 	page->inuse = page->objects;
 	page->frozen = 1;

Andrew Morton

maintainer

2014-07-01 21:49 UTC #5 lore raw

Quoted 8 lines · On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <[email protected]> wrote:

Quoted 1 line · David Rientjes
It's not at all clear to me that that patch is correct. Wei?
Looks ok to me. But I do not like the convoluted code in new_slab() which Wei's patch does not make easier to read. Makes it difficult for the reader to see whats going on.

Quoted 8 lines

Lets drop the use of the variable named "last".
Subject: slub: Only call setup_object once for each object
Modify the logic for object initialization to be less convoluted and initialize an object only once.

Well, um. Wei's changelog was much better:

: When a kmem_cache is created with ctor, each object in the kmem_cache will : be initialized before use. In the slub implementation, the first object : will be initialized twice.
:
: This patch avoids the duplication of initialization of the first object. :
: Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a : slab").

I can copy that text over and add the reported-by etc (ho hum) but I have a tiny feeling that this patch hasn't been rigorously tested? Perhaps someone (Wei?) can do that?

And we still don't know why Sasha's kernel went oops.

Sasha Levin

author

2014-07-01 21:52 UTC #6 lore raw

Quoted 30 lines · Andrew Morton

Quoted 8 lines · On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <[email protected]> wrote:
Quoted 1 line · David Rientjes
It's not at all clear to me that that patch is correct. Wei?
Looks ok to me. But I do not like the convoluted code in new_slab() which Wei's patch does not make easier to read. Makes it difficult for the reader to see whats going on.
Quoted 8 lines
Lets drop the use of the variable named "last".
Subject: slub: Only call setup_object once for each object
Modify the logic for object initialization to be less convoluted and initialize an object only once.
Well, um. Wei's changelog was much better:
: When a kmem_cache is created with ctor, each object in the kmem_cache will : be initialized before use. In the slub implementation, the first object : will be initialized twice.
:
: This patch avoids the duplication of initialization of the first object. :
: Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a : slab").

Quoted 5 lines

I can copy that text over and add the reported-by etc (ho hum) but I have a tiny feeling that this patch hasn't been rigorously tested? Perhaps someone (Wei?) can do that?
And we still don't know why Sasha's kernel went oops.

I only saw this oops once, and after David's message yesterday I tried reverting the patch he pointed out, but not much changed.

Is there a better way to stress test slub?

Thanks,
Sasha

Christoph Lameter

reviewer

2014-07-02 14:45 UTC #7 lore raw

Quoted 1 line · Sasha Levin

Is there a better way to stress test slub?

The typical way to test is by stressing the network subsystem with small packets that require small allocations. Or do a filesystem test that requires lots of metadata (file creations, removal, renames etc).

But I also posted some in kernel benchmarks a while back

https://lkml.org/lkml/2009/10/13/459

Pekka had a project going to get these merged.

https://lkml.org/lkml/2009/11/29/17

Wei Yang

2014-07-02 02:06 UTC #8 lore raw

In reply to Andrew Morton

Quoted 30 lines · Andrew Morton

Quoted 8 lines · On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <[email protected]> wrote:
Quoted 1 line · David Rientjes
It's not at all clear to me that that patch is correct. Wei?
Looks ok to me. But I do not like the convoluted code in new_slab() which Wei's patch does not make easier to read. Makes it difficult for the reader to see whats going on.
Quoted 8 lines
Lets drop the use of the variable named "last".
Subject: slub: Only call setup_object once for each object
Modify the logic for object initialization to be less convoluted and initialize an object only once.
Well, um. Wei's changelog was much better:
: When a kmem_cache is created with ctor, each object in the kmem_cache will : be initialized before use. In the slub implementation, the first object : will be initialized twice.
:
: This patch avoids the duplication of initialization of the first object. :
: Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a : slab").

Quoted 3 lines

I can copy that text over and add the reported-by etc (ho hum) but I have a tiny feeling that this patch hasn't been rigorously tested? Perhaps someone (Wei?) can do that?

Ok, I will apply this one and give a shot.

Quoted 2 lines

And we still don't know why Sasha's kernel went oops.

Yep, if there is some procedure to reproduce it, I'd like to do it at my side.

Signature · 2 lines

Richard Yang
Help you, Help me

Christoph Lameter

reviewer

2014-07-02 15:07 UTC #9 lore raw

In reply to Andrew Morton

Quoted 2 lines · Andrew Morton

I can copy that text over and add the reported-by etc (ho hum) but I have a tiny feeling that this patch hasn't been rigorously tested?

The testing so far was to verify that a kernel successfully builds with the patch and then booted upo in a kvm instance.

Wei Yang

2014-07-03 02:23 UTC #10 lore raw

In reply to Andrew Morton

Quoted 30 lines · Andrew Morton

Quoted 8 lines · On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <[email protected]> wrote:
Quoted 1 line · David Rientjes
It's not at all clear to me that that patch is correct. Wei?
Looks ok to me. But I do not like the convoluted code in new_slab() which Wei's patch does not make easier to read. Makes it difficult for the reader to see whats going on.
Quoted 8 lines
Lets drop the use of the variable named "last".
Subject: slub: Only call setup_object once for each object
Modify the logic for object initialization to be less convoluted and initialize an object only once.
Well, um. Wei's changelog was much better:
: When a kmem_cache is created with ctor, each object in the kmem_cache will : be initialized before use. In the slub implementation, the first object : will be initialized twice.
:
: This patch avoids the duplication of initialization of the first object. :
: Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a : slab").

Quoted 3 lines

I can copy that text over and add the reported-by etc (ho hum) but I have a tiny feeling that this patch hasn't been rigorously tested? Perhaps someone (Wei?) can do that?

The result of the simple test is the same. And my laptop works a whole day with this patch.

Thanks

Quoted 2 lines

And we still don't know why Sasha's kernel went oops.

Signature · 2 lines

Richard Yang
Help you, Help me

Wei Yang

2014-07-02 02:05 UTC #11 lore raw

In reply to Christoph Lameter

Quoted 7 lines · Christoph Lameter

Quoted 1 line · David Rientjes
It's not at all clear to me that that patch is correct. Wei?
Looks ok to me. But I do not like the convoluted code in new_slab() which Wei's patch does not make easier to read. Makes it difficult for the reader to see whats going on.

My patch is somewhat convoluted since I wanted to preserve the original logic and make minimal change. And yes, it looks not that nice to audience.

I feel a little hurt by this patch. What I found and worked is gone with this patch.

Quoted 42 lines

Lets drop the use of the variable named "last".

Subject: slub: Only call setup_object once for each object

Modify the logic for object initialization to be less convoluted and initialize an object only once.

Signed-off-by Christoph Lameter <[email protected]>

Index: linux/mm/slub.c
===================================================================

mm/slub.c +3 -8

@@ -1409,7 +1409,6 @@ static struct page *new_slab(struct kmem
 {
 	struct page *page;
 	void *start;
-	void *last;
 	void *p;
 	int order;

@@ -1432,15 +1431,11 @@ static struct page *new_slab(struct kmem
 	if (unlikely(s->flags & SLAB_POISON))
 		memset(start, POISON_INUSE, PAGE_SIZE << order);
 
-	last = start;
 	for_each_object(p, s, start, page->objects) {
-		setup_object(s, page, last);
-		set_freepointer(s, last, p);
-		last = p;
+		setup_object(s, page, p);
+		set_freepointer(s, p, p + s->size);
 	}
-	setup_object(s, page, last);
-	set_freepointer(s, last, NULL);
-
+	set_freepointer(s, start + (page->objects - 1) * s->size, NULL);
 	page->freelist = start;
 	page->inuse = page->objects;
 	page->frozen = 1;

Signature · 2 lines

Richard Yang
Help you, Help me

Christoph Lameter

reviewer

2014-07-02 14:20 UTC #12 lore raw

Quoted 2 lines · Wei Yang

My patch is somewhat convoluted since I wanted to preserve the original logic and make minimal change. And yes, it looks not that nice to audience.

Well I was the author of the initial "convoluted" logic.

Quoted 2 lines

I feel a little hurt by this patch. What I found and worked is gone with this patch.

Ok how about giving this one additional revision. Maybe you can make the function even easier to read? F.e. the setting of the NULL pointer at the end of the loop is ugly.

Wei Yang

2014-07-03 12:40 UTC #13 lore raw

Quoted 5 lines · Christoph Lameter

Quoted 2 lines · Wei Yang
My patch is somewhat convoluted since I wanted to preserve the original logic and make minimal change. And yes, it looks not that nice to audience.

Quoted 8 lines

Well I was the author of the initial "convoluted" logic.
Quoted 2 lines
I feel a little hurt by this patch. What I found and worked is gone with this patch.
Ok how about giving this one additional revision. Maybe you can make the function even easier to read? F.e. the setting of the NULL pointer at the end of the loop is ugly.

Hi, Christoph

Here is my refined version, hope this is more friendly to the audience.

Christoph Lameter

reviewer Acked-by

2014-07-07 13:51 UTC #14 lore raw

Quoted 1 line · Wei Yang

Here is my refined version, hope this is more friendly to the audience.

Acked-by Christoph Lameter <[email protected]>

Wei Yang

2014-07-08 01:34 UTC #15 lore raw

Quoted 5 lines · Christoph Lameter

Quoted 1 line · Wei Yang
Here is my refined version, hope this is more friendly to the audience.
Acked-by Christoph Lameter <[email protected]>

Thanks. I am glad to work with you.

Signature · 2 lines

Richard Yang
Help you, Help me