memory hotplug - looking for good place for cpuset hook

memory hotplug - looking for good place for cpuset hook

[Paul Jackson]

Dave,

I'm looking for a good place to add yet another cpuset hook, this
one to keep a nodemask in my top (root) cpuset always equal to the
current value of node_online_map.

The motivation for this, if you're interested, comes from the
following threads, which added similar cpuset tracking of the
cpu_online_map:

  cpusets not cpu hotplug aware
  http://lkml.org/lkml/2006/8/21/128

  [PATCH] cpuset: top_cpuset tracks hotplug changes to cpu_online_map
  http://lkml.org/lkml/2006/8/24/107

(I cc'd the victims of these threads here, in case they're interested.)

>From what I see so far, the right place to call my cpuset routine to
update its copy of node_online_map would be right after the call:

	node_set_online(nid);

in the routine mm/memory_hotplug.c:add_memory().

Does that seem like a plausible sounding place to you?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: memory hotplug - looking for good place for cpuset hook

[KAMEZAWA Hiroyuki]

On Fri, 25 Aug 2006 01:53:59 -0700
Paul Jackson <pj@sgi.com> wrote:

> >From what I see so far, the right place to call my cpuset routine to
> update its copy of node_online_map would be right after the call:
> 
> 	node_set_online(nid);
> 
> in the routine mm/memory_hotplug.c:add_memory().
> 
> Does that seem like a plausible sounding place to you?
> 
maybe

if (new_pgdat) {
	register_one_node(nid); <-- add sysfs entry of node
	<here>
}

is good.

(When I implements node-hotplug invoked by cpu-hotplug, I'll care cpuset.)

-Kame

Re: memory hotplug - looking for good place for cpuset hook

[Dave Hansen]

On Fri, 2006-08-25 at 18:47 +0900, KAMEZAWA Hiroyuki wrote:
> 
> if (new_pgdat) {
>         register_one_node(nid); <-- add sysfs entry of node
>         <here>
> }
> 
> is good.
> 
> (When I implements node-hotplug invoked by cpu-hotplug, I'll care
> cpuset.) 

Looks like someone beat me to the punch.  Putting it in there would be
fine with me.  Gets it a wee bit farther from memory hotplug anyway. ;)

-- Dave

Re: memory hotplug - looking for good place for cpuset hook

[Paul Jackson]

Dave wrote:
> Gets it a wee bit farther from memory hotplug anyway. ;)

huh?  Where Kame suggested is just a couple lines of code and one test
below where I suggested.  How is this farther from memory hotplug, as
it still seems to be in one of the main memory hotplug routines -
add_memory()?

And why would I want to place a hook that tracks hotplug added memory
nodes 'farther from memory hotplug'?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: memory hotplug - looking for good place for cpuset hook

[Paul Jackson]

Kame wrote:
> maybe
> 
> if (new_pgdat) {
> 	register_one_node(nid); <-- add sysfs entry of node
> 	<here>
> }

I am not sure why you suggest this, but perhaps you ar saying that I
don't need to call my new cpuset hook (that tracks node_online_map)
everytime we set a bit here in node_online_map, but only when we set a
bit that wasn't previously set.

Hmmm ... this mm/memory_hotplug.c:add_memory() code seems to have
more code than it needs:

================================================================
int add_memory(int nid, u64 start, u64 size)
{
        pg_data_t *pgdat = NULL;
        int new_pgdat = 0;
        ...
        if (!node_online(nid)) {
                pgdat = hotadd_new_pgdat(nid, start);
                if (!pgdat)
                        return -ENOMEM;
		new_pgdat = 1;
                ...
        }
        ...
        /* we online node here. we can't roll back from here. */
        node_set_online(nid);

        if (new_pgdat) {
                ret = register_one_node(nid);                         
================================================================

It looks like the call to node_set_online() could also be called
only if it wasn't already set, and that the 'new_pgdat' is not
needed as we can just test for pgdat != NULL.

That leads to this code that's a couple lines shorter:

================================================================
int add_memory(int nid, u64 start, u64 size)
{
        pg_data_t *pgdat = NULL;
        ...
        if (!node_online(nid)) {
                pgdat = hotadd_new_pgdat(nid, start);
                if (!pgdat)
                        return -ENOMEM;
               ...
        }
        ...
        if (pgdat) {
                /* we online node here. we can't roll back from here. */
                node_set_online(nid);
                ret = register_one_node(nid);                         
================================================================

Is this second code chunk just as good?

I'd still be inclined to add my new cpuset hook to track
node_online_map right after the node_set_online() call, since
that's what changes node_online_map.  I don't think I care
whether or not the "sysfs entry of node" is setup or not.

> (When I implements node-hotplug invoked by cpu-hotplug, I'll care cpuset.)

Good - thanks.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: memory hotplug - looking for good place for cpuset hook

[KAMEZAWA Hiroyuki]

On Fri, 25 Aug 2006 09:57:18 -0700
Paul Jackson <pj@sgi.com> wrote:

> ================================================================
> int add_memory(int nid, u64 start, u64 size)
> {
>         pg_data_t *pgdat = NULL;
>         ...
>         if (!node_online(nid)) {
>                 pgdat = hotadd_new_pgdat(nid, start);
>                 if (!pgdat)
>                         return -ENOMEM;
>                ...
>         }
>         ...
>         if (pgdat) {
>                 /* we online node here. we can't roll back from here. */
>                 node_set_online(nid);
>                 ret = register_one_node(nid);                         
> ================================================================
> 
> Is this second code chunk just as good?
> 

Ah yes. I think yours is better logic.

> I'd still be inclined to add my new cpuset hook to track
> node_online_map right after the node_set_online() call, since
> that's what changes node_online_map.  I don't think I care
> whether or not the "sysfs entry of node" is setup or not.
> 
Ok.

Thanks,
-Kame

Re: memory hotplug - looking for good place for cpuset hook

[Paul Jackson]

Kame wrote:
> Ah yes. I think yours is better logic.

Ok - good.  I'll let you worry about whether it
is worth putting in a patch.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401