Infinite loop on boot in free_early_partial due to start==end on tip/master

Infinite loop on boot in free_early_partial due to start==end on tip/master

[Ian Campbell]

Adding some debug, including a WARN_ON(start >= end) I get:

(early) [    0.000000] pcpu_embed_first_chunk CPU0 copy and return the unused part ffff880001f1f000 0x0. unit_size 122880 size sum 122880
(early) [    0.000000] free_early_partial 0x1f1f000-0x1f1f000
(early) [    0.000000] ------------[ cut here ]------------
(early) [    0.000000] WARNING: at /local/scratch/ianc/devel/kernels/linux-2.6/kernel/early_res.c:359 free_early_partial+0x46/0x15c()
(early) [    0.000000] Modules linked in:(early) 
(early) [    0.000000] Pid: 0, comm: swapper Not tainted 2.6.33-x86_64-xenU-tip+ #119
(early) [    0.000000] Call Trace:
(early) [    0.000000]  [<ffffffff81b3938e>] ? free_early_partial+0x46/0x15c
(early) [    0.000000]  [<ffffffff81047317>] warn_slowpath_common+0x77/0xa4
(early) [    0.000000]  [<ffffffff81047353>] warn_slowpath_null+0xf/0x11
(early) [    0.000000]  [<ffffffff81b3938e>] free_early_partial+0x46/0x15c
(early) [    0.000000]  [<ffffffff81b29a05>] pcpu_fc_free+0x1f/0x23
(early) [    0.000000]  [<ffffffff81b3fcf1>] pcpu_embed_first_chunk+0x22f/0x363
(early) [    0.000000]  [<ffffffff81b299e6>] ? pcpu_fc_free+0x0/0x23
(early) [    0.000000]  [<ffffffff81b29a09>] ? pcpu_fc_alloc+0x0/0xa9
(early) [    0.000000]  [<ffffffff81b29825>] setup_per_cpu_areas+0x82/0x238
(early) [    0.000000]  [<ffffffff81b1eabc>] start_kernel+0x14b/0x3ab
(early) [    0.000000]  [<ffffffff81b1e2ba>] x86_64_start_reservations+0xa5/0xa9
(early) [    0.000000]  [<ffffffff81b21d61>] xen_start_kernel+0x52e/0x535
(early) [    0.000000] ---[ end trace 4eaa2a86a8e2da22 ]---

I'm seeing this in a Xen guest but I don't think it relates to Xen
specifically. The problem occurs when ai->unit_size == size_sum in
pcpu_embed_first_chunk(). It looks like I just got unlucky with the
various sizes of stuff coming out of pcpu_build_alloc_info().

It's also possible that the presence or absence of PSE is responsible,
since it impacts atom_size which feeds into alloc_size and has an impact
on all these calculations.

I was able to reproduce on native 32 bit by disabling PSE and
artificially increasing dyn_size.

(early) [    0.000000] pcpu_build_alloc_info initial max_upa/upa is 1
(early) [    0.000000] pcpu_build_alloc_info after expansion best_upa/upa is 1
(early) [    0.000000] reserve_early_without_check: 0x1ce3000-0x1ce4000 "BOOTMEM"
(early) [    0.000000] pcpu_build_alloc_info unit_size 122880 == alloc_size 122880 / upa 1
(early) [    0.000000] pcpu_embed_first_chunk size_sum = 122880
(early) [    0.000000] pcpu_embed_first_chunk  static_size   = 91304
(early) [    0.000000] pcpu_embed_first_chunk  reserved_size = 8192
(early) [    0.000000] pcpu_embed_first_chunk  dyn_size      = 23384
(early) [    0.000000] pcpu_embed_first_chunk unit_size 122880

I guess the arises from fb90ef93 "early_res: Add free_early_partial"
since I don't see any other changes in this area recently.

Following patch fixes this specific issue and adds a warning to catch
future potential errors of this type.

--- 
Subject: x86: do not free zero sized per cpu areas

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index ef6370b..89a3205 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -140,7 +140,8 @@ static void __init pcpu_fc_free(void *ptr, size_t size)
 #ifdef CONFIG_NO_BOOTMEM
 	u64 start = __pa(ptr);
 	u64 end = start + size;
-	free_early_partial(start, end);
+	if (start < end)
+		free_early_partial(start, end);
 #else
 	free_bootmem(__pa(ptr), size);
 #endif
diff --git a/kernel/early_res.c b/kernel/early_res.c
index 3cb2c66..fbde443 100644
--- a/kernel/early_res.c
+++ b/kernel/early_res.c
@@ -333,6 +333,8 @@ void __init free_early_partial(u64 start, u64 end)
 	struct early_res *r;
 	int i;
 
+	WARN_ON(start>=end);
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)

Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Yinghai Lu]

> 
> It's also possible that the presence or absence of PSE is responsible,
> since it impacts atom_size which feeds into alloc_size and has an impact
> on all these calculations.
> 
> I was able to reproduce on native 32 bit by disabling PSE and
> artificially increasing dyn_size.
> 
> (early) [    0.000000] pcpu_build_alloc_info initial max_upa/upa is 1
> (early) [    0.000000] pcpu_build_alloc_info after expansion best_upa/upa is 1
> (early) [    0.000000] reserve_early_without_check: 0x1ce3000-0x1ce4000 "BOOTMEM"
> (early) [    0.000000] pcpu_build_alloc_info unit_size 122880 == alloc_size 122880 / upa 1
> (early) [    0.000000] pcpu_embed_first_chunk size_sum = 122880
> (early) [    0.000000] pcpu_embed_first_chunk  static_size   = 91304
> (early) [    0.000000] pcpu_embed_first_chunk  reserved_size = 8192
> (early) [    0.000000] pcpu_embed_first_chunk  dyn_size      = 23384
> (early) [    0.000000] pcpu_embed_first_chunk unit_size 122880
> 
> I guess the arises from fb90ef93 "early_res: Add free_early_partial"
> since I don't see any other changes in this area recently.
> 
> Following patch fixes this specific issue and adds a warning to catch
> future potential errors of this type.
> 
> --- 
> Subject: x86: do not free zero sized per cpu areas
> 
> This avoids an infinite loop in free_early_partial().
> 
> Add a warning to free_early_partial to catch future problems.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
> index ef6370b..89a3205 100644
> --- a/arch/x86/kernel/setup_percpu.c
> +++ b/arch/x86/kernel/setup_percpu.c
> @@ -140,7 +140,8 @@ static void __init pcpu_fc_free(void *ptr, size_t size)
>  #ifdef CONFIG_NO_BOOTMEM
>  	u64 start = __pa(ptr);
>  	u64 end = start + size;
> -	free_early_partial(start, end);
> +	if (start < end)
> +		free_early_partial(start, end);
>  #else
>  	free_bootmem(__pa(ptr), size);
>  #endif
> diff --git a/kernel/early_res.c b/kernel/early_res.c
> index 3cb2c66..fbde443 100644
> --- a/kernel/early_res.c
> +++ b/kernel/early_res.c
> @@ -333,6 +333,8 @@ void __init free_early_partial(u64 start, u64 end)
>  	struct early_res *r;
>  	int i;
>  
> +	WARN_ON(start>=end);
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)
> 

can you change to 

> @@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
>  	struct early_res *r;
>  	int i;
>  
> +	if (unlikely(start>=end)) {
> +		WARN_ONCE(1, "free_early_partial get wrong start/end\n:);
> +		return;
> +	}
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)
> 

Thanks

Yinghai

Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Ian Campbell]

On Fri, 2010-03-05 at 18:16 +0000, Yinghai Lu wrote:
> 
> > +     if (unlikely(start>=end)) {
> > +             WARN_ONCE(1, "free_early_partial get wrong start/end
> \n:);
> > +             return;
> > +     }
> > + 

Sure, although this can be written:

if (WARN_ONCE(start >= end, "free_early_partial.....\n"))
return;

Updated patch to follow.

Ian.

[PATCH] x86: do not free zero sized per cpu areas

[Ian Campbell]

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/setup_percpu.c |    3 ++-
 kernel/early_res.c             |    5 +++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index ef6370b..89a3205 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -140,7 +140,8 @@ static void __init pcpu_fc_free(void *ptr, size_t size)
 #ifdef CONFIG_NO_BOOTMEM
 	u64 start = __pa(ptr);
 	u64 end = start + size;
-	free_early_partial(start, end);
+	if (start < end)
+		free_early_partial(start, end);
 #else
 	free_bootmem(__pa(ptr), size);
 #endif
diff --git a/kernel/early_res.c b/kernel/early_res.c
index 3cb2c66..f3a861b 100644
--- a/kernel/early_res.c
+++ b/kernel/early_res.c
@@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
 	struct early_res *r;
 	int i;
 
+	if (WARN_ONCE(start >= end,
+		      "free_early_partial got wrong start/end %#llx/%#llx\n",
+		      start, end))
+		return;
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)
-- 
1.5.6.5

Re: [PATCH] x86: do not free zero sized per cpu areas

[Yinghai Lu]

On 03/05/2010 11:49 AM, Ian Campbell wrote:
> This avoids an infinite loop in free_early_partial().
> 
> Add a warning to free_early_partial to catch future problems.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/setup_percpu.c |    3 ++-
>  kernel/early_res.c             |    5 +++++
>  2 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
> index ef6370b..89a3205 100644
> --- a/arch/x86/kernel/setup_percpu.c
> +++ b/arch/x86/kernel/setup_percpu.c
> @@ -140,7 +140,8 @@ static void __init pcpu_fc_free(void *ptr, size_t size)
>  #ifdef CONFIG_NO_BOOTMEM
>  	u64 start = __pa(ptr);
>  	u64 end = start + size;
> -	free_early_partial(start, end);
> +	if (start < end)
> +		free_early_partial(start, end);
>  #else
>  	free_bootmem(__pa(ptr), size);
>  #endif
> diff --git a/kernel/early_res.c b/kernel/early_res.c
> index 3cb2c66..f3a861b 100644
> --- a/kernel/early_res.c
> +++ b/kernel/early_res.c
> @@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
>  	struct early_res *r;
>  	int i;
>  
> +	if (WARN_ONCE(start >= end,
> +		      "free_early_partial got wrong start/end %#llx/%#llx\n",
> +		      start, end))
> +		return;
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)

Acked-by: Yinghai Lu <yinghai@kernel.org>

[PATCH] x86: do not free zero sized per cpu areas

[Yinghai Lu]

From: Ian Campbell <ian.campbell@citrix.com>

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/setup_percpu.c |    3 ++-
 kernel/early_res.c             |    5 +++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index ef6370b..89a3205 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -140,7 +140,8 @@ static void __init pcpu_fc_free(void *ptr, size_t size)
 #ifdef CONFIG_NO_BOOTMEM
 	u64 start = __pa(ptr);
 	u64 end = start + size;
-	free_early_partial(start, end);
+	if (start < end)
+		free_early_partial(start, end);
 #else
 	free_bootmem(__pa(ptr), size);
 #endif
diff --git a/kernel/early_res.c b/kernel/early_res.c
index 3cb2c66..f3a861b 100644
--- a/kernel/early_res.c
+++ b/kernel/early_res.c
@@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
 	struct early_res *r;
 	int i;
 
+	if (WARN_ONCE(start >= end,
+		      "free_early_partial got wrong start/end %#llx/%#llx\n",
+		      start, end))
+		return;
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)
-- 
1.5.6.5

Re: [PATCH] x86: do not free zero sized per cpu areas

[H. Peter Anvin]

On 03/19/2010 12:24 PM, Yinghai Lu wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> This avoids an infinite loop in free_early_partial().
> 
> Add a warning to free_early_partial to catch future problems.

Wouldn't it make more sense to simply make free_early_partial() a noop
for the zero range, instead of pushing all those tests into the callers?
 It still might make sense to have a WARN_ONCE() for a *negative* range,
however...

	-hpa

[PATCH -v3] x86: do not free zero sized per cpu areas

[Yinghai Lu]

From: Ian Campbell <ian.campbell@citrix.com>

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

-v3: according to hpa, don't bother caller.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
---
 kernel/early_res.c             |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/early_res.c b/kernel/early_res.c
index 3cb2c66..f3a861b 100644
--- a/kernel/early_res.c
+++ b/kernel/early_res.c
@@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
 	struct early_res *r;
 	int i;
 
+	if (WARN_ONCE(start >= end,
+		      "free_early_partial got wrong start/end %#llx/%#llx\n",
+		      start, end))
+		return;
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)
-- 
1.5.6.5

Re: [PATCH -v3] x86: do not free zero sized per cpu areas

[H. Peter Anvin]

On 03/19/2010 03:18 PM, Yinghai Lu wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> This avoids an infinite loop in free_early_partial().
> 
> Add a warning to free_early_partial to catch future problems.
> 
> -v3: according to hpa, don't bother caller.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  kernel/early_res.c             |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/early_res.c b/kernel/early_res.c
> index 3cb2c66..f3a861b 100644
> --- a/kernel/early_res.c
> +++ b/kernel/early_res.c
> @@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
>  	struct early_res *r;
>  	int i;
>  
> +	if (WARN_ONCE(start >= end,
> +		      "free_early_partial got wrong start/end %#llx/%#llx\n",
> +		      start, end))
> +		return;
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)

No, that's wrong.

The workaround is still needed for the case of equality to avoid the
infinite loop.

So you need an:

	if (start == end)
		return;

	-hpa

Re: [PATCH -v3] x86: do not free zero sized per cpu areas

[Yinghai Lu]

On 03/19/2010 03:21 PM, H. Peter Anvin wrote:
> On 03/19/2010 03:18 PM, Yinghai Lu wrote:
>> From: Ian Campbell <ian.campbell@citrix.com>
>>
>> This avoids an infinite loop in free_early_partial().
>>
>> Add a warning to free_early_partial to catch future problems.
>>
>> -v3: according to hpa, don't bother caller.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> ---
>>  kernel/early_res.c             |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/early_res.c b/kernel/early_res.c
>> index 3cb2c66..f3a861b 100644
>> --- a/kernel/early_res.c
>> +++ b/kernel/early_res.c
>> @@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
>>  	struct early_res *r;
>>  	int i;
>>  
>> +	if (WARN_ONCE(start >= end,
>> +		      "free_early_partial got wrong start/end %#llx/%#llx\n",
>> +		      start, end))
>> +		return;
>> +
>>  try_next:
>>  	i = find_overlapped_early(start, end);
>>  	if (i >= max_early_res)
> 
> No, that's wrong.
> 
> The workaround is still needed for the case of equality to avoid the
> infinite loop.
> 
> So you need an:
> 
> 	if (start == end)
> 		return;
> 

confused, do you mean like this
if (start < end), find_overlapped_early will stop the loop.

From: Ian Campbell <ian.campbell@citrix.com>

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

-v3: according to hpa, don't bother caller.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
---
 kernel/early_res.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-2.6/kernel/early_res.c
===================================================================
--- linux-2.6.orig/kernel/early_res.c
+++ linux-2.6/kernel/early_res.c
@@ -333,6 +333,14 @@ void __init free_early_partial(u64 start
 	struct early_res *r;
 	int i;
 
+	if (start >= end) {
+		WARN_ONCE(1,
+		      "free_early_partial got wrong start/end %#llx/%#llx\n",
+		      start, end);
+
+		return;
+	}
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)

Re: [PATCH -v3] x86: do not free zero sized per cpu areas

[H. Peter Anvin]

On 03/19/2010 04:24 PM, Yinghai Lu wrote:
> 
> confused, do you mean like this
> if (start < end), find_overlapped_early will stop the loop.
> 

No.

What I mean is that free_early_partial(), when start == end, is a
legitimate operation that doesn't free anything -- the range is zero
bytes long.  Just return.

When start > end, there is an explicit error we should warn about.

	-hpa

[PATCH -v4] x86: do not free zero sized per cpu areas

[Yinghai Lu]

From: Ian Campbell <ian.campbell@citrix.com>

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

-v3: according to hpa, don't bother caller.
-v4: start == end is ok?

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
---
 kernel/early_res.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6/kernel/early_res.c
===================================================================
--- linux-2.6.orig/kernel/early_res.c
+++ linux-2.6/kernel/early_res.c
@@ -333,6 +333,17 @@ void __init free_early_partial(u64 start
 	struct early_res *r;
 	int i;
 
+	if (start == end)
+		return;
+
+	if (start > end) {
+		WARN_ONCE(1,
+		      "free_early_partial got wrong start/end %#llx/%#llx\n",
+		      start, end);
+
+		return;
+	}
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)

Re: [PATCH -v4] x86: do not free zero sized per cpu areas

[Linus Torvalds]

On Fri, 19 Mar 2010, Yinghai Lu wrote:
> +
> +	if (start > end) {
> +		WARN_ONCE(1,
> +		      "free_early_partial got wrong start/end %#llx/%#llx\n",
> +		      start, end);
> +
> +		return;

This should be written as

	if (WARN_ONCE(start > end, "free_early_partial(%#llx/%#llx)\n", start, end))
		return;

rather than having a separate conditional.

			Linus

[PATCH -v5] x86: do not free zero sized per cpu areas

[Yinghai Lu]

From: Ian Campbell <ian.campbell@citrix.com>

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

-v5: put back start > end back into WARN_ONCE()

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
---
 kernel/early_res.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-2.6/kernel/early_res.c
===================================================================
--- linux-2.6.orig/kernel/early_res.c
+++ linux-2.6/kernel/early_res.c
@@ -333,6 +333,14 @@ void __init free_early_partial(u64 start
 	struct early_res *r;
 	int i;
 
+	if (start == end)
+		return;
+
+	if (WARN_ONCE(start > end,
+		      "free_early_partial got wrong start/end %#llx/%#llx\n",
+		      start, end))
+		return;
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)

Re: [PATCH -v5] x86: do not free zero sized per cpu areas

[Linus Torvalds]

On Fri, 19 Mar 2010, Yinghai Lu wrote:
> +	if (WARN_ONCE(start > end,
> +		      "free_early_partial got wrong start/end %#llx/%#llx\n",
> +		      start, end))

I really don't want multi-line single statements like this.

Shorten the string a bit, and it should all fit fine on one line, and be 
much more greppable.

			Linus

[PATCH -v6] x86: do not free zero sized per cpu areas

[Yinghai Lu]

From: Ian Campbell <ian.campbell@citrix.com>

This avoids an infinite loop in free_early_partial().

Add a warning to free_early_partial to catch future problems.

-v5: put back start > end back into WARN_ONCE()
-v6: use one line for if according to linus

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
---
 kernel/early_res.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-2.6/kernel/early_res.c
===================================================================
--- linux-2.6.orig/kernel/early_res.c
+++ linux-2.6/kernel/early_res.c
@@ -333,6 +333,12 @@ void __init free_early_partial(u64 start
 	struct early_res *r;
 	int i;
 
+	if (start == end)
+		return;
+
+	if (WARN_ONCE(start > end, "free_early_partial: wrong range [%#llx, %#llx]\n", start, end))
+		return;
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)

Re: [PATCH -v6] x86: do not free zero sized per cpu areas

[Ian Campbell]

Looks good to me, thanks Yinghai.

On Sat, 2010-03-20 at 06:38 +0000, Yinghai Lu wrote: 
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> This avoids an infinite loop in free_early_partial().
> 
> Add a warning to free_early_partial to catch future problems.
> 
> -v5: put back start > end back into WARN_ONCE()
> -v6: use one line for if according to linus
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  kernel/early_res.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: linux-2.6/kernel/early_res.c
> ===================================================================
> --- linux-2.6.orig/kernel/early_res.c
> +++ linux-2.6/kernel/early_res.c
> @@ -333,6 +333,12 @@ void __init free_early_partial(u64 start
>  	struct early_res *r;
>  	int i;
>  
> +	if (start == end)
> +		return;
> +
> +	if (WARN_ONCE(start > end, "free_early_partial: wrong range [%#llx, %#llx]\n", start, end))
> +		return;
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)

Re: [LKML] [PATCH -v6] x86: do not free zero sized per cpu areas

[Konrad Rzeszutek Wilk]

On Fri, Mar 19, 2010 at 11:38:30PM -0700, Yinghai Lu wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> This avoids an infinite loop in free_early_partial().
> 
> Add a warning to free_early_partial to catch future problems.
> 
> -v5: put back start > end back into WARN_ONCE()
> -v6: use one line for if according to linus
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> ---
>  kernel/early_res.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: linux-2.6/kernel/early_res.c
> ===================================================================
> --- linux-2.6.orig/kernel/early_res.c
> +++ linux-2.6/kernel/early_res.c
> @@ -333,6 +333,12 @@ void __init free_early_partial(u64 start
>  	struct early_res *r;
>  	int i;
>  
> +	if (start == end)
> +		return;
> +
> +	if (WARN_ONCE(start > end, "free_early_partial: wrong range [%#llx, %#llx]\n", start, end))
> +		return;
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -v3] x86: do not free zero sized per cpu areas

[Ian Campbell]

On Fri, 2010-03-19 at 23:24 +0000, Yinghai Lu wrote: 
> On 03/19/2010 03:21 PM, H. Peter Anvin wrote:
> > On 03/19/2010 03:18 PM, Yinghai Lu wrote:
> >> From: Ian Campbell <ian.campbell@citrix.com>
> >>
> >> This avoids an infinite loop in free_early_partial().
> >>
> >> Add a warning to free_early_partial to catch future problems.
> >>
> >> -v3: according to hpa, don't bother caller.
> >>
> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Ingo Molnar <mingo@elte.hu>
> >> ---
> >>  kernel/early_res.c             |    5 +++++
> >>  1 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/kernel/early_res.c b/kernel/early_res.c
> >> index 3cb2c66..f3a861b 100644
> >> --- a/kernel/early_res.c
> >> +++ b/kernel/early_res.c
> >> @@ -333,6 +333,11 @@ void __init free_early_partial(u64 start, u64 end)
> >>  	struct early_res *r;
> >>  	int i;
> >>  
> >> +	if (WARN_ONCE(start >= end,
> >> +		      "free_early_partial got wrong start/end %#llx/%#llx\n",
> >> +		      start, end))
> >> +		return;
> >> +
> >>  try_next:
> >>  	i = find_overlapped_early(start, end);
> >>  	if (i >= max_early_res)
> > 
> > No, that's wrong.
> > 
> > The workaround is still needed for the case of equality to avoid the
> > infinite loop.
> > 
> > So you need an:
> > 
> > 	if (start == end)
> > 		return;
> > 
> 
> confused, do you mean like this
> if (start < end), find_overlapped_early will stop the loop.

I think Peter means:

if (start == end)
return;

if (WARN_ONCE(start < end, "blah"))
return;

i.e. silently ignore a zero sized region and verbosely ignore a
negatively sized one.

Ian.


> From: Ian Campbell <ian.campbell@citrix.com>
> 
> This avoids an infinite loop in free_early_partial().
> 
> Add a warning to free_early_partial to catch future problems.
> 
> -v3: according to hpa, don't bother caller.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  kernel/early_res.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> Index: linux-2.6/kernel/early_res.c
> ===================================================================
> --- linux-2.6.orig/kernel/early_res.c
> +++ linux-2.6/kernel/early_res.c
> @@ -333,6 +333,14 @@ void __init free_early_partial(u64 start
>  	struct early_res *r;
>  	int i;
>  
> +	if (start >= end) {
> +		WARN_ONCE(1,
> +		      "free_early_partial got wrong start/end %#llx/%#llx\n",
> +		      start, end);
> +
> +		return;
> +	}
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)

Re: [PATCH -v3] x86: do not free zero sized per cpu areas

[Ian Campbell]

On Fri, 2010-03-19 at 23:35 +0000, Ian Campbell wrote: 
> if (WARN_ONCE(start < end, "blah"))

or rather "start > end"...

Ian.

-- 
Ian Campbell

I am a friend of the working man, and I would rather be his friend
than be one.
		-- Clarence Darrow

Re: [PATCH -v3] x86: do not free zero sized per cpu areas

[H. Peter Anvin]

On 03/19/2010 04:35 PM, Ian Campbell wrote:
> 
> I think Peter means:
> 
> if (start == end)
> return;
> 
> if (WARN_ONCE(start < end, "blah"))
> return;

start > end!

> i.e. silently ignore a zero sized region and verbosely ignore a
> negatively sized one.

Correct.

	-hpa

Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Konrad Rzeszutek Wilk]

> Subject: x86: do not free zero sized per cpu areas
> 
> This avoids an infinite loop in free_early_partial().

The patch fixed PV guests booting on Xen. I've added my
Tested-by .. Please merge it in the release.

Thanks!
> 
> Add a warning to free_early_partial to catch future problems.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
> index ef6370b..89a3205 100644
> --- a/arch/x86/kernel/setup_percpu.c
> +++ b/arch/x86/kernel/setup_percpu.c
> @@ -140,7 +140,8 @@ static void __init pcpu_fc_free(void *ptr, size_t size)
>  #ifdef CONFIG_NO_BOOTMEM
>  	u64 start = __pa(ptr);
>  	u64 end = start + size;
> -	free_early_partial(start, end);
> +	if (start < end)
> +		free_early_partial(start, end);
>  #else
>  	free_bootmem(__pa(ptr), size);
>  #endif
> diff --git a/kernel/early_res.c b/kernel/early_res.c
> index 3cb2c66..fbde443 100644
> --- a/kernel/early_res.c
> +++ b/kernel/early_res.c
> @@ -333,6 +333,8 @@ void __init free_early_partial(u64 start, u64 end)
>  	struct early_res *r;
>  	int i;
>  
> +	WARN_ON(start>=end);
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)

Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Yinghai Lu]

On 03/19/2010 11:28 AM, Konrad Rzeszutek Wilk wrote:
>> Subject: x86: do not free zero sized per cpu areas
>>
>> This avoids an infinite loop in free_early_partial().
> 
> The patch fixed PV guests booting on Xen. I've added my
> Tested-by .. Please merge it in the release.
> 
> Thanks!
>>
>> Add a warning to free_early_partial to catch future problems.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

please check update version...

please check if this patch help...

http://patchwork.kernel.org/patch/83832/


YH

Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Ian Campbell]

On Fri, 2010-03-19 at 19:04 +0000, Yinghai Lu wrote: 
> On 03/19/2010 11:28 AM, Konrad Rzeszutek Wilk wrote:
> >> Subject: x86: do not free zero sized per cpu areas
> >>
> >> This avoids an infinite loop in free_early_partial().
> > 
> > The patch fixed PV guests booting on Xen. I've added my
> > Tested-by .. Please merge it in the release.
> > 
> > Thanks!
> >>
> >> Add a warning to free_early_partial to catch future problems.
> >>
> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> please check update version...
> 
> please check if this patch help...
> 
> http://patchwork.kernel.org/patch/83832/

Sorry Konrad, looks like I directed you to an older version of the patch
by mistake.

Ian.

Re: [LKML] Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Konrad Rzeszutek Wilk]

On Fri, Mar 19, 2010 at 07:34:49PM +0000, Ian Campbell wrote:
> On Fri, 2010-03-19 at 19:04 +0000, Yinghai Lu wrote: 
> > On 03/19/2010 11:28 AM, Konrad Rzeszutek Wilk wrote:
> > >> Subject: x86: do not free zero sized per cpu areas
> > >>
> > >> This avoids an infinite loop in free_early_partial().
> > > 
> > > The patch fixed PV guests booting on Xen. I've added my
> > > Tested-by .. Please merge it in the release.
> > > 
> > > Thanks!
> > >>
> > >> Add a warning to free_early_partial to catch future problems.
> > >>
> > >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > please check update version...
> > 
> > please check if this patch help...
> > 
> > http://patchwork.kernel.org/patch/83832/
> 
> Sorry Konrad, looks like I directed you to an older version of the patch
> by mistake.

Np. I tested this one and it worked succesfully.

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

index ef6370b..89a3205 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -140,7 +140,8 @@  static void __init pcpu_fc_free(void *ptr, size_t
size)
 #ifdef CONFIG_NO_BOOTMEM
 	u64 start = __pa(ptr);
 	u64 end = start + size;
-	free_early_partial(start, end);
+	if (start < end)
+		free_early_partial(start, end);
 #else
 	free_bootmem(__pa(ptr), size);
 #endif
diff --git a/kernel/early_res.c b/kernel/early_res.c
index 3cb2c66..f3a861b 100644
--- a/kernel/early_res.c
+++ b/kernel/early_res.c
@@ -333,6 +333,11 @@  void __init free_early_partial(u64 start, u64 end)
 	struct early_res *r;
 	int i;
 
+	if (WARN_ONCE(start >= end,
+		      "free_early_partial got wrong start/end
%#llx/%#llx\n",
+		      start, end))
+		return;
+
 try_next:
 	i = find_overlapped_early(start, end);
 	if (i >= max_early_res)

> 
> Ian.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [LKML] Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Yinghai Lu]

On 03/19/2010 01:04 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 19, 2010 at 07:34:49PM +0000, Ian Campbell wrote:
>> On Fri, 2010-03-19 at 19:04 +0000, Yinghai Lu wrote: 
>>> On 03/19/2010 11:28 AM, Konrad Rzeszutek Wilk wrote:
>>>>> Subject: x86: do not free zero sized per cpu areas
>>>>>
>>>>> This avoids an infinite loop in free_early_partial().
>>>>
>>>> The patch fixed PV guests booting on Xen. I've added my
>>>> Tested-by .. Please merge it in the release.
>>>>
>>>> Thanks!
>>>>>
>>>>> Add a warning to free_early_partial to catch future problems.
>>>>>
>>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>>
>>>> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>
>>> please check update version...
>>>
>>> please check if this patch help...
>>>
>>> http://patchwork.kernel.org/patch/83832/
>>
>> Sorry Konrad, looks like I directed you to an older version of the patch
>> by mistake.
> 
> Np. I tested this one and it worked succesfully.
> 
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> index ef6370b..89a3205 100644
> --- a/arch/x86/kernel/setup_percpu.c
> +++ b/arch/x86/kernel/setup_percpu.c
> @@ -140,7 +140,8 @@  static void __init pcpu_fc_free(void *ptr, size_t
> size)
>  #ifdef CONFIG_NO_BOOTMEM
>  	u64 start = __pa(ptr);
>  	u64 end = start + size;
> -	free_early_partial(start, end);
> +	if (start < end)
> +		free_early_partial(start, end);

it seems we could remove this line

Tejun, how this could happen? free zero range ?

YH

>  #else
>  	free_bootmem(__pa(ptr), size);
>  #endif
> diff --git a/kernel/early_res.c b/kernel/early_res.c
> index 3cb2c66..f3a861b 100644
> --- a/kernel/early_res.c
> +++ b/kernel/early_res.c
> @@ -333,6 +333,11 @@  void __init free_early_partial(u64 start, u64 end)
>  	struct early_res *r;
>  	int i;
>  
> +	if (WARN_ONCE(start >= end,
> +		      "free_early_partial got wrong start/end
> %#llx/%#llx\n",
> +		      start, end))
> +		return;
> +
>  try_next:
>  	i = find_overlapped_early(start, end);
>  	if (i >= max_early_res)
> 

Re: [LKML] Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Ian Campbell]

On Fri, 2010-03-19 at 21:17 +0000, Yinghai Lu wrote: 
> > index ef6370b..89a3205 100644
> > --- a/arch/x86/kernel/setup_percpu.c
> > +++ b/arch/x86/kernel/setup_percpu.c
> > @@ -140,7 +140,8 @@  static void __init pcpu_fc_free(void *ptr, size_t
> > size)
> >  #ifdef CONFIG_NO_BOOTMEM
> >  	u64 start = __pa(ptr);
> >  	u64 end = start + size;
> > -	free_early_partial(start, end);
> > +	if (start < end)
> > +		free_early_partial(start, end);
> 
> it seems we could remove this line

> Tejun, how this could happen? free zero range ?

I was seeing start==end although I never fully understood why. It
happened somewhere within pcpu_embed_first_chunk and/or
pcpu_build_alloc_info. My original posting (in
https://patchwork.kernel.org/patch/83756/) has some speculation.

Ian.

> 
> YH
> 
> >  #else
> >  	free_bootmem(__pa(ptr), size);
> >  #endif
> > diff --git a/kernel/early_res.c b/kernel/early_res.c
> > index 3cb2c66..f3a861b 100644
> > --- a/kernel/early_res.c
> > +++ b/kernel/early_res.c
> > @@ -333,6 +333,11 @@  void __init free_early_partial(u64 start, u64 end)
> >  	struct early_res *r;
> >  	int i;
> >  
> > +	if (WARN_ONCE(start >= end,
> > +		      "free_early_partial got wrong start/end
> > %#llx/%#llx\n",
> > +		      start, end))
> > +		return;
> > +
> >  try_next:
> >  	i = find_overlapped_early(start, end);
> >  	if (i >= max_early_res)
> > 

Re: [LKML] Re: Infinite loop on boot in free_early_partial due to start==end on tip/master

[Tejun Heo]

Hello,

On 03/20/2010 06:17 AM, Yinghai Lu wrote:
>>   #ifdef CONFIG_NO_BOOTMEM
>>   	u64 start = __pa(ptr);
>>   	u64 end = start + size;
>> -	free_early_partial(start, end);
>> +	if (start<  end)
>> +		free_early_partial(start, end);
>
> it seems we could remove this line
>
> Tejun, how this could happen? free zero range ?

Well, the generic code assumes that the arch free callback can handle
zero length free, so on rare cases where the amount of used percpu
area in the first chunk equals the unit size, it happily call
free_fn() with zero length expecting the free function to ignore it.
Hmmm... well, given that it's a arch dependent callback and occurrence
of zero length free would be fairly rare, I think it would be better
to make the generic code avoid calling free with zero length.

Does the following patch fix the problem?

diff --git a/mm/percpu.c b/mm/percpu.c
index 768419d..d8d3f70 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1929,7 +1929,9 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, ssize_t dyn_size,
  			}
  			/* copy and return the unused part */
  			memcpy(ptr, __per_cpu_load, ai->static_size);
-			free_fn(ptr + size_sum, ai->unit_size - size_sum);
+			if (ai->unit_size > size_sum)
+				free_fn(ptr + size_sum,
+					ai->unit_size - size_sum);
  		}
  	}

-- 
tejun