Prev: [RFC patch 0/3] RCU head debug objects
Next: debug registers, write breakpoints, and x86
From: Yinghai Lu on 19 Mar 2010 17:30
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(a)citrix.com>
>>>>
>>>> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk(a)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(a)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)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ian Campbell on 19 Mar 2010 17:40
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)
> >


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 20 Mar 2010 03:30
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [RFC patch 0/3] RCU head debug objects
Next: debug registers, write breakpoints, and x86