Re: [patch] mm: node-setup agnostic free_bootmem()

[Johannes Weiner <hannes@saeurebad.de>]

Hi Yinghai,

"Yinghai Lu" <yhlu.kernel@gmail.com> writes:

> On Sun, Apr 27, 2008 at 5:40 PM, Ingo Molnar <mingo@elte.hu> wrote:
>>
>>  * Johannes Weiner <hannes@saeurebad.de> wrote:
>>
>>  > > so i very much agree that your changes are cleaner, i just wanted to
>>  > > have one that has all the fixes included.
>>  >
>>  > I had planned this to be another patch because there are more then one
>>  > boundary check I wanted to tighten.  I can merge them though if you
>>  > like.
>>
>>  no, better to have them in separate patches.
>>
>>  > > Would you like to post a patch against current -git or should i
>>  > > extract the cleaner reserve_bootmem() from your previous patch?
>>  >
>>  > I just moved and have only sporadic internet access and free time
>>  > slots available.  Would be nice if you could do it!
>>
>>  sure, find the merged patch below, against latest -git, boot-tested on
>>  x86. Is this what you had in mind?
>>
>>         Ingo
>>
>>  ---------------->
>>  Subject: mm: node-setup agnostic free_bootmem()
>>  From: Johannes Weiner <hannes@saeurebad.de>
>>  Date: Wed, 16 Apr 2008 13:36:31 +0200
>>
>>  Make free_bootmem() look up the node holding the specified address
>>  range which lets it work transparently on single-node and multi-node
>>  configurations.
>>
>>  If the address range exceeds the node range, it well be marked free
>>  across node boundaries, too.
>>
>>  Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
>>  CC: Andi Kleen <andi@firstfloor.org>
>>  CC: Yinghai Lu <yhlu.kernel@gmail.com>
>>  CC: Yasunori Goto <y-goto@jp.fujitsu.com>
>>  CC: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>  CC: Christoph Lameter <clameter@sgi.com>
>>  CC: Andrew Morton <akpm@linux-foundation.org>
>>  Signed-off-by: Ingo Molnar <mingo@elte.hu>
>>  ---
>>   mm/bootmem.c |   27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>>  Index: linux-x86.q/mm/bootmem.c
>>  ===================================================================
>>  --- linux-x86.q.orig/mm/bootmem.c
>>  +++ linux-x86.q/mm/bootmem.c
>>  @@ -493,8 +493,31 @@ int __init reserve_bootmem(unsigned long
>>   void __init free_bootmem(unsigned long addr, unsigned long size)
>>   {
>>         bootmem_data_t *bdata;
>>  -       list_for_each_entry(bdata, &bdata_list, list)
>>  -               free_bootmem_core(bdata, addr, size);
>>  +       unsigned long pos = addr;
>>  +       unsigned long partsize = size;
>>  +
>>  +       list_for_each_entry(bdata, &bdata_list, list) {
>>  +               unsigned long remainder = 0;
>>  +
>>  +               if (pos < bdata->node_boot_start)
>>  +                       continue;
>>  +
>>  +               if (PFN_DOWN(pos + partsize) > bdata->node_low_pfn) {
>>  +                       remainder = PFN_DOWN(pos + partsize) - bdata->node_low_pfn;
>>  +                       partsize -= remainder;
>>  +               }
>>  +
>>  +               free_bootmem_core(bdata, pos, partsize);
>>  +
>>  +               if (!remainder)
>>  +                       return;
>>  +
>>  +               pos = PFN_PHYS(bdata->node_low_pfn + 1);
>>  +       }
>>  +       printk(KERN_ERR "free_bootmem: request: addr=%lx, size=%lx, "
>>  +                       "state: pos=%lx, partsize=%lx\n", addr, size,
>>  +                       pos, partsize);
>>  +       BUG();
>>   }
>>
>>   unsigned long __init free_all_bootmem(void)
>>
>
> it will not work with cross nodes.
>
> for example: node 0: 0-2g, 4-6g, node1: 2-4g, 6-8g.
> and if ramdisk sit cross 2G boundary. you will only free the range
> before 2g.

Yes, you stated that several times but this is not a technical argument:
These setups are afaik not yet supported by the kernel at all.  And you
could not explain the node layout with the patch that implements support
for these configurations.

So as long as you don't explain in technical detail why my patch won't
work, I will have to ignore your objections.

My opinion is that my patch should go in as is and the patch that adds
support for these node-setups should change bootmem accordingly, if
needed at all.

	Hannes