Re: [PATCH -mm 11/14] bootmem: respect goal more likely

Johannes Weiner
2008-06-03T16:17:50Z

Hi,

Yasunori Goto <y-goto@jp.fujitsu.com> writes:

> Hmm, my ia64 (NUMA) box can't boot up with this patch.
> I'll chase its cause deeply tomorrow.

I think I found it.

>> --- a/mm/bootmem.c
>> +++ b/mm/bootmem.c
>> @@ -408,6 +408,7 @@ static void * __init alloc_bootmem_core(
>> unsigned long size, unsigned long align,
>> unsigned long goal, unsigned long limit)
>> {
>> + unsigned long fallback = 0;
>> unsigned long min, max, start, step;
>>
>> BUG_ON(!size);
>> @@ -441,10 +442,11 @@ static void * __init alloc_bootmem_core(
>>
>> max -= PFN_DOWN(bdata->node_boot_start);
>> start -= PFN_DOWN(bdata->node_boot_start);
>> + fallback -= PFN_DOWN(bdata->node_boot_start);
>>
>> if (bdata->last_success > start) {
>> - /* Set goal here to trigger a retry on failure */
>> - start = goal = ALIGN(bdata->last_success, step);
>> + fallback = start;
>> + start = ALIGN(bdata->last_success, step);
>> }
>>
>> while (1) {
>> @@ -491,10 +493,39 @@ find_block:
>> return region;
>> }
>>
>> + if (fallback) {
>> + start = ALIGN(fallback, step);
>> + fallback = 0;
>> + goto find_block;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static void * __init ___alloc_bootmem_nopanic(unsigned long size,
>> + unsigned long align,
>> + unsigned long goal,
>> + unsigned long limit)
>> +{
>> + bootmem_data_t *bdata;
>> +
>> +restart:
>> + list_for_each_entry(bdata, &bdata_list, list) {
>> + void *region;
>> +
>> + if (goal && goal < bdata->node_boot_start)
>> + continue;

This check is backwards and probably made your boot fail.

>> + if (limit && limit < bdata->node_boot_start)
>> + continue;

Changed this to break, because we don't need to search any further if
the current node already starts at/above the limit (remember, we walk a
list sorted by ->node_boot_start here).

I also made the checks more intuitively understandable.

Could you try the following fix on top of this patch?

Hannes

--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -513,10 +513,10 @@ restart:
list_for_each_entry(bdata, &bdata_list, list) {
void *region;

- if (goal && goal < bdata->node_boot_start)
- continue;
- if (limit && limit < bdata->node_boot_start)
+ if (goal && bdata->node_low_pfn <= PFN_DOWN(goal))
continue;
+ if (limit && bdata->node_boot_start >= limit)
+ break;

region = alloc_bootmem_core(bdata, size, align, goal, limit);
if (region)