by Tejun Heo

[permalink] [raw]

Subject: Re: Early boot panic on machine with lots of memory

On Tue, Jun 19, 2012 at 02:20:59PM -0700, Tejun Heo wrote:
> Something like the following should fix it.
>
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 32a0a5e..2770970 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -148,11 +148,15 @@ phys_addr_t __init_memblock memblock_find_in_range(phys_addr_t start,
> */
> int __init_memblock memblock_free_reserved_regions(void)
> {
> +#ifndef CONFIG_DEBUG_PAGEALLOC
> if (memblock.reserved.regions == memblock_reserved_init_regions)
> return 0;
>
> return memblock_free(__pa(memblock.reserved.regions),
> sizeof(struct memblock_region) * memblock.reserved.max);
> +#else
> + return 0;
> +#endif

BTW, this is just ugly and I don't think we're saving any noticeable
amount by doing this "free - give it to page allocator - reserve
again" dancing. We should just allocate regions aligned to page
boundaries and free them later when memblock is no longer in use.

Thanks.

--
tejun

2012-06-20 02:57:48

On Thu, Jun 21, 2012 at 1:17 PM, Tejun Heo <[email protected]> wrote:
> Hello, Yinghai.
>
> On Tue, Jun 19, 2012 at 07:57:45PM -0700, Yinghai Lu wrote:
>> if it is that case, that change could fix other problem problem too.
>> --- during the one free reserved.regions could double the array.
>
> Yeah, that sounds much more attractive to me too. ?Some comments on
> the patch tho.
>
>> ?/**
>> ? * memblock_double_array - double the size of the memblock regions array
>> ? * @type: memblock type of the regions array being doubled
>> @@ -216,7 +204,7 @@ static int __init_memblock memblock_doub
>>
>> ? ? ? /* Calculate new doubled size */
>> ? ? ? old_size = type->max * sizeof(struct memblock_region);
>> - ? ? new_size = old_size << 1;
>> + ? ? new_size = PAGE_ALIGN(old_size << 1);
>
> We definintely can use some comments explaining why we want page
> alignment. ?It's kinda subtle.

yes.

>
> This is a bit confusing here because old_size is the proper size
> without padding while new_size is page aligned size with possible
> padding. ?Maybe discerning {old|new}_alloc_size is clearer? ?Also, I
> think adding @new_cnt variable which is calculated together would make
> the code easier to follow. ?So, sth like,
>
> ? ? ? ?/* explain why page aligning is necessary */
> ? ? ? ?old_size = type->max * sizeof(struct memblock_region);
> ? ? ? ?old_alloc_size = PAGE_ALIGN(old_size);
>
> ? ? ? ?new_max = type->max << 1;
> ? ? ? ?new_size = new_max * sizeof(struct memblock_region);
> ? ? ? ?new_alloc_size = PAGE_ALIGN(new_size);
>
> and use alloc_sizes for alloc/frees and sizes for everything else.

ok, will add new_alloc_size, old_alloc_size.

>
>> ?unsigned long __init free_low_memory_core_early(int nodeid)
>> ?{
>> ? ? ? unsigned long count = 0;
>> - ? ? phys_addr_t start, end;
>> + ? ? phys_addr_t start, end, size;
>> ? ? ? u64 i;
>>
>> - ? ? /* free reserved array temporarily so that it's treated as free area */
>> - ? ? memblock_free_reserved_regions();
>> + ? ? for_each_free_mem_range(i, MAX_NUMNODES, &start, &end, NULL)
>> + ? ? ? ? ? ? count += __free_memory_core(start, end);
>>
>> - ? ? for_each_free_mem_range(i, MAX_NUMNODES, &start, &end, NULL) {
>> - ? ? ? ? ? ? unsigned long start_pfn = PFN_UP(start);
>> - ? ? ? ? ? ? unsigned long end_pfn = min_t(unsigned long,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? PFN_DOWN(end), max_low_pfn);
>> - ? ? ? ? ? ? if (start_pfn < end_pfn) {
>> - ? ? ? ? ? ? ? ? ? ? __free_pages_memory(start_pfn, end_pfn);
>> - ? ? ? ? ? ? ? ? ? ? count += end_pfn - start_pfn;
>> - ? ? ? ? ? ? }
>> - ? ? }
>> + ? ? /* free range that is used for reserved array if we allocate it */
>> + ? ? size = get_allocated_memblock_reserved_regions_info(&start);
>> + ? ? if (size)
>> + ? ? ? ? ? ? count += __free_memory_core(start, start + size);
>
> I'm afraid this is too early. ?We don't want the region to be unmapped
> yet. ?This should only happen after all memblock usages are finished
> which I don't think is the case yet.

No, it is not early. at that time memblock usage is done.

Also I tested one system with huge memory, duplicated the problem on
KVM that Sasha met.
my patch fixes the problem.

please check attached patch.

Also I add another patch to double check if there is any reference
with reserved.region.
so far there is no reference found.

Thanks

Yinghai

Attachments:

fix_free_memblock_reserve_v4_5.patch (6.61 kB)
memblock_reserved_clear_check.patch (3.83 kB)
Download all attachments

2012-06-22 01:58:43

On Fri, Jun 22, 2012 at 12:29 PM, Tejun Heo <[email protected]> wrote:
> I wish we had a single call - say, memblock_die(), or whatever - so
> that there's a clear indication that memblock usage is done, but yeah
> maybe another day. ?Will review the patch itself. ?BTW, can't you post
> patches inline anymore? ?Attaching is better than corrupt but is still
> a bit annoying for review.

please check the three patches:

Thanks

Yinghai

Attachments:

fix_free_memblock_reserve_v4_5.patch (6.61 kB)
fix_free_memblock_memory.patch (3.54 kB)
memblock_clear_check.patch (5.13 kB)
Download all attachments

2012-06-27 18:13:37

by Tejun Heo

[permalink] [raw]

Subject: Re: Early boot panic on machine with lots of memory

Hello, Yinghai.

Sorry about the delay. I'm in bug storm somehow. :(

On Fri, Jun 22, 2012 at 07:14:43PM -0700, Yinghai Lu wrote:
> On Fri, Jun 22, 2012 at 12:29 PM, Tejun Heo <[email protected]> wrote:
> > I wish we had a single call - say, memblock_die(), or whatever - so
> > that there's a clear indication that memblock usage is done, but yeah
> > maybe another day. ?Will review the patch itself. ?BTW, can't you post
> > patches inline anymore? ?Attaching is better than corrupt but is still
> > a bit annoying for review.
>
> please check the three patches:

Heh, reviewing is cumbersome this way but here are my comments.

* "[PATCH] memblock: free allocated memblock_reserved_regions later"
looks okay to me.

* "[PATCH] memblock: Free allocated memblock.memory.regions" makes me
wonder whether it would be better to have something like the
following instead.

typedef void memblock_free_region_fn_t(unsigned long start, unsigned size);

void memblock_free_regions(memblock_free_region_fn_t free_fn)
{
/* call free_fn() on reserved and memory regions arrays */
/* clear both structures so that any further usage triggers warning */
}

* "memblock: Add checking about illegal using memblock".
Hmm... wouldn't it be better to be less explicit? I think it's
adding too much opencoded identical checks. Maybe implement a
common check & warning function?

Thanks.

--
tejun

2012-06-27 19:22:19

by Yinghai Lu

[permalink] [raw]

Subject: Re: Early boot panic on machine with lots of memory

On Wed, Jun 27, 2012 at 11:13 AM, Tejun Heo <[email protected]> wrote:
> Hello, Yinghai.
>
> Sorry about the delay. ?I'm in bug storm somehow. :(
>
> On Fri, Jun 22, 2012 at 07:14:43PM -0700, Yinghai Lu wrote:
>> On Fri, Jun 22, 2012 at 12:29 PM, Tejun Heo <[email protected]> wrote:
>> > I wish we had a single call - say, memblock_die(), or whatever - so
>> > that there's a clear indication that memblock usage is done, but yeah
>> > maybe another day. ?Will review the patch itself. ?BTW, can't you post
>> > patches inline anymore? ?Attaching is better than corrupt but is still
>> > a bit annoying for review.
>>
>> please check the three patches:
>
> Heh, reviewing is cumbersome this way but here are my comments.
>
> * "[PATCH] memblock: free allocated memblock_reserved_regions later"
> ?looks okay to me.

Good, this one should go to 3.5, right?

>
> * "[PATCH] memblock: Free allocated memblock.memory.regions" makes me
> ?wonder whether it would be better to have something like the
> ?following instead.
>
> ?typedef void memblock_free_region_fn_t(unsigned long start, unsigned size);
>
> ?void memblock_free_regions(memblock_free_region_fn_t free_fn)
> ?{
> ? ? ? ?/* call free_fn() on reserved and memory regions arrays */
> ? ? ? ?/* clear both structures so that any further usage triggers warning */
> ?}

ok, will check it.

>
> * "memblock: Add checking about illegal using memblock".
> ?Hmm... wouldn't it be better to be less explicit? ?I think it's
> ?adding too much opencoded identical checks. ?Maybe implement a
> ?common check & warning function?

yes.

Thanks

Yinghai

2012-06-27 19:26:15

by Tejun Heo

[permalink] [raw]

Subject: Re: Early boot panic on machine with lots of memory

On Wed, Jun 27, 2012 at 12:22:14PM -0700, Yinghai Lu wrote:
> On Wed, Jun 27, 2012 at 11:13 AM, Tejun Heo <[email protected]> wrote:
> > Hello, Yinghai.
> >
> > Sorry about the delay. ?I'm in bug storm somehow. :(
> >
> > On Fri, Jun 22, 2012 at 07:14:43PM -0700, Yinghai Lu wrote:
> >> On Fri, Jun 22, 2012 at 12:29 PM, Tejun Heo <[email protected]> wrote:
> >> > I wish we had a single call - say, memblock_die(), or whatever - so
> >> > that there's a clear indication that memblock usage is done, but yeah
> >> > maybe another day. ?Will review the patch itself. ?BTW, can't you post
> >> > patches inline anymore? ?Attaching is better than corrupt but is still
> >> > a bit annoying for review.
> >>
> >> please check the three patches:
> >
> > Heh, reviewing is cumbersome this way but here are my comments.
> >
> > * "[PATCH] memblock: free allocated memblock_reserved_regions later"
> > ?looks okay to me.
>
> Good, this one should go to 3.5, right?

Yes, I think so.

Thank you!

--
tejun

2012-06-27 21:15:18

Hello, Yinghai.

Just one nitpick.

On Fri, Jun 29, 2012 at 11:27:57AM -0700, Yinghai Lu wrote:
> /**
> * memblock_double_array - double the size of the memblock regions array
> * @type: memblock type of the regions array being doubled
> @@ -204,6 +192,7 @@ static int __init_memblock memblock_doub
> phys_addr_t new_area_size)
> {
> struct memblock_region *new_array, *old_array;
> + phys_addr_t old_alloc_size, new_alloc_size;
> phys_addr_t old_size, new_size, addr;
> int use_slab = slab_is_available();
> int *in_slab;
> @@ -217,6 +206,12 @@ static int __init_memblock memblock_doub
> /* Calculate new doubled size */
> old_size = type->max * sizeof(struct memblock_region);
> new_size = old_size << 1;
> + /*
> + * We need to allocated new one align to PAGE_SIZE,
> + * so late could free them completely.

An extra space and probably "so we can free them completely later"
fits better.

Thank you!

--
tejun