Re: [Lhms-devel] [RFC] patch [1/1] x86_64 numa aware sparsemem add_memory functinality

keith mannthey
2006-06-23T17:57:54Z

On Fri, 2006-06-23 at 10:13 -0700, Dave Hansen wrote:
> > int add_memory(u64 start, u64 size)
> > {
> > - struct pglist_data *pgdat = NODE_DATA(0);
> > + struct pglist_data *pgdat = NODE_DATA(new_memory_to_node(start,start+size));
> > struct zone *zone = pgdat->node_zones + MAX_NR_ZONES-2;
>
> How about just having new_memory_to_node() take the range and return the
> pgdat? Should make that line a bit shorter.

In the -mm tree things are a little different. The acpi layer (and
something for ppc) is passing the nid down the a generic add memory
call.

This int add_memory(u64 start, u64 size) is going away with something
more like int add_memory(int nid, u64 start, u64 size) this changes
things some.

I have patches against the -mm stack but I had a little trouble with
my testbox's file-system last night so I should have them out this
afternoon.

> > -#ifndef RESERVE_HOTADD
> > +#if !defined(RESERVE_HOTADD) && !defined(CONFIG_MEMORY_HOTPLUG)
> > #define hotadd_percent 0 /* Ignore all settings */
> > #endif
> > static u8 pxm2node[256] = { [0 ... 255] = 0xff };
> > @@ -219,9 +219,9 @@
> > allocated += mem;
> > return 1;
> > }
> > -
> > +#endif
> > /*
>
> Could this use another Kconfig option which gives a name to this
> condition?

This is sort of a redundant force off. I am not sure if there is a
code path to the SRAT code without RESERVE_HOTADD or
CONFIG_MEMORY_HOTPLUG defined.

hotadd_percent can only change from 0 with an explicit command-line
numa=hotadd=XXX boot so maybe taking this
#define hotadd_percent 0
out all together might be the better way to go if the code patch is
going to be shared.

>
> > +#ifdef RESERVE_HOTADD
> > if (!hotadd_enough_memory(&nodes_add[node])) {
> > printk(KERN_ERR "SRAT: Hotplug area too large\n");
> > return -1;
> > }
> > -
> > +#endif
>
> This #ifdef is probably better handled by an #ifdef in the header for
> hotadd_enough_memory().

hotadd_enough_memory is static there is no header entry for it.

Thanks for the feedback,
Keith