[RFC, 3/7] NUMA hotplug emulator

[RFC, 3/7] NUMA hotplug emulator

[Shaohui Zheng]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

Userland interface to hotplug-add fake offlined nodes.

Add a sysfs entry "probe" under /sys/devices/system/node/:

 - to show all fake offlined nodes:
    $ cat /sys/devices/system/node/probe

 - to hotadd a fake offlined node, e.g. nodeid is N:
    $ echo N > /sys/devices/system/node/probe

Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com>
Signed-off-by: Shaohui Zheng <shaohui.zheng@intel.com>
---
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9458685..2c078c8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1214,6 +1214,20 @@ config NUMA_EMU
 	  into virtual nodes when booted with "numa=fake=N", where N is the
 	  number of nodes. This is only useful for debugging.
 
+config NUMA_HOTPLUG_EMU
+	bool "NUMA hotplug emulator"
+	depends on X86_64 && NUMA && HOTPLUG
+	---help---
+
+config NODE_HOTPLUG_EMU
+	bool "Node hotplug emulation"
+	depends on NUMA_HOTPLUG_EMU && MEMORY_HOTPLUG
+	---help---
+	  Enable Node hotplug emulation. The machine will be setup with
+	  hidden virtual nodes when booted with "numa=hide=N*size", where
+	  N is the number of hidden nodes, size is the memory size per
+	  hidden node. This is only useful for debugging.
+
 config NODES_SHIFT
 	int "Maximum NUMA Nodes (as a power of 2)" if !MAXSMP
 	range 1 10
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 057979a..a0be257 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -535,6 +535,26 @@ void unregister_one_node(int nid)
 	unregister_node(&node_devices[nid]);
 }
 
+#ifdef CONFIG_NODE_HOTPLUG_EMU
+static ssize_t store_nodes_probe(struct sysdev_class *class,
+				  struct sysdev_class_attribute *attr,
+				  const char *buf, size_t count)
+{
+	long nid;
+	int ret;
+
+	strict_strtol(buf, 0, &nid);
+	if (nid < 0 || nid > nr_node_ids - 1) {
+		printk(KERN_ERR "Invalid NUMA node id: %d (0 <= nid < %d).\n",
+			nid, nr_node_ids);
+		return -EPERM;
+	}
+	hotadd_hidden_nodes(nid);
+
+	return count;
+}
+#endif
+
 /*
  * node states attributes
  */
@@ -563,26 +583,35 @@ static ssize_t show_node_state(struct sysdev_class *class,
 	return print_nodes_state(na->state, buf);
 }
 
-#define _NODE_ATTR(name, state) \
+#define _NODE_ATTR_RO(name, state) \
 	{ _SYSDEV_CLASS_ATTR(name, 0444, show_node_state, NULL), state }
 
+#define _NODE_ATTR_RW(name, store_func, state) \
+	{ _SYSDEV_CLASS_ATTR(name, 0644, show_node_state, store_func), state }
+
 static struct node_attr node_state_attr[] = {
-	_NODE_ATTR(possible, N_POSSIBLE),
-	_NODE_ATTR(online, N_ONLINE),
-	_NODE_ATTR(has_normal_memory, N_NORMAL_MEMORY),
-	_NODE_ATTR(has_cpu, N_CPU),
+	[N_POSSIBLE] = _NODE_ATTR_RO(possible, N_POSSIBLE),
+#ifdef CONFIG_NODE_HOTPLUG_EMU
+	[N_HIDDEN] = _NODE_ATTR_RW(probe, store_nodes_probe, N_HIDDEN),
+#endif
+	[N_ONLINE] = _NODE_ATTR_RO(online, N_ONLINE),
+	[N_NORMAL_MEMORY] = _NODE_ATTR_RO(has_normal_memory, N_NORMAL_MEMORY),
 #ifdef CONFIG_HIGHMEM
-	_NODE_ATTR(has_high_memory, N_HIGH_MEMORY),
+	[N_HIGH_MEMORY] = _NODE_ATTR_RO(has_high_memory, N_HIGH_MEMORY),
 #endif
+	[N_CPU] = _NODE_ATTR_RO(has_cpu, N_CPU),
 };
 
 static struct sysdev_class_attribute *node_state_attrs[] = {
-	&node_state_attr[0].attr,
-	&node_state_attr[1].attr,
-	&node_state_attr[2].attr,
-	&node_state_attr[3].attr,
+	&node_state_attr[N_POSSIBLE].attr,
+#ifdef CONFIG_NODE_HOTPLUG_EMU
+	&node_state_attr[N_HIDDEN].attr,
+#endif
+	&node_state_attr[N_ONLINE].attr,
+	&node_state_attr[N_NORMAL_MEMORY].attr,
+	&node_state_attr[N_CPU].attr,
 #ifdef CONFIG_HIGHMEM
-	&node_state_attr[4].attr,
+	&node_state_attr[N_HIGH_MEMORY].attr,
 #endif
 	NULL
 };
-- 
Thanks & Regards,
Shaohui


[-- Attachment #2: 003-hotplug-emulator-userland-interface-to-add-fake-node.patch --]
[-- Type: text/x-diff, Size: 3562 bytes --]

Userland interface to hotplug-add fake offlined nodes.

Add a sysfs entry "probe" under /sys/devices/system/node/:

 - to show all fake offlined nodes:
    $ cat /sys/devices/system/node/probe

 - to hotadd a fake offlined node, e.g. nodeid is N:
    $ echo N > /sys/devices/system/node/probe

Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com>
Signed-off-by: Shaohui Zheng <shaohui.zheng@intel.com>
---
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9458685..2c078c8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1214,6 +1214,20 @@ config NUMA_EMU
 	  into virtual nodes when booted with "numa=fake=N", where N is the
 	  number of nodes. This is only useful for debugging.
 
+config NUMA_HOTPLUG_EMU
+	bool "NUMA hotplug emulator"
+	depends on X86_64 && NUMA && HOTPLUG
+	---help---
+
+config NODE_HOTPLUG_EMU
+	bool "Node hotplug emulation"
+	depends on NUMA_HOTPLUG_EMU && MEMORY_HOTPLUG
+	---help---
+	  Enable Node hotplug emulation. The machine will be setup with
+	  hidden virtual nodes when booted with "numa=hide=N*size", where
+	  N is the number of hidden nodes, size is the memory size per
+	  hidden node. This is only useful for debugging.
+
 config NODES_SHIFT
 	int "Maximum NUMA Nodes (as a power of 2)" if !MAXSMP
 	range 1 10
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 057979a..a0be257 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -535,6 +535,26 @@ void unregister_one_node(int nid)
 	unregister_node(&node_devices[nid]);
 }
 
+#ifdef CONFIG_NODE_HOTPLUG_EMU
+static ssize_t store_nodes_probe(struct sysdev_class *class,
+				  struct sysdev_class_attribute *attr,
+				  const char *buf, size_t count)
+{
+	long nid;
+	int ret;
+
+	strict_strtol(buf, 0, &nid);
+	if (nid < 0 || nid > nr_node_ids - 1) {
+		printk(KERN_ERR "Invalid NUMA node id: %d (0 <= nid < %d).\n",
+			nid, nr_node_ids);
+		return -EPERM;
+	}
+	hotadd_hidden_nodes(nid);
+
+	return count;
+}
+#endif
+
 /*
  * node states attributes
  */
@@ -563,26 +583,35 @@ static ssize_t show_node_state(struct sysdev_class *class,
 	return print_nodes_state(na->state, buf);
 }
 
-#define _NODE_ATTR(name, state) \
+#define _NODE_ATTR_RO(name, state) \
 	{ _SYSDEV_CLASS_ATTR(name, 0444, show_node_state, NULL), state }
 
+#define _NODE_ATTR_RW(name, store_func, state) \
+	{ _SYSDEV_CLASS_ATTR(name, 0644, show_node_state, store_func), state }
+
 static struct node_attr node_state_attr[] = {
-	_NODE_ATTR(possible, N_POSSIBLE),
-	_NODE_ATTR(online, N_ONLINE),
-	_NODE_ATTR(has_normal_memory, N_NORMAL_MEMORY),
-	_NODE_ATTR(has_cpu, N_CPU),
+	[N_POSSIBLE] = _NODE_ATTR_RO(possible, N_POSSIBLE),
+#ifdef CONFIG_NODE_HOTPLUG_EMU
+	[N_HIDDEN] = _NODE_ATTR_RW(probe, store_nodes_probe, N_HIDDEN),
+#endif
+	[N_ONLINE] = _NODE_ATTR_RO(online, N_ONLINE),
+	[N_NORMAL_MEMORY] = _NODE_ATTR_RO(has_normal_memory, N_NORMAL_MEMORY),
 #ifdef CONFIG_HIGHMEM
-	_NODE_ATTR(has_high_memory, N_HIGH_MEMORY),
+	[N_HIGH_MEMORY] = _NODE_ATTR_RO(has_high_memory, N_HIGH_MEMORY),
 #endif
+	[N_CPU] = _NODE_ATTR_RO(has_cpu, N_CPU),
 };
 
 static struct sysdev_class_attribute *node_state_attrs[] = {
-	&node_state_attr[0].attr,
-	&node_state_attr[1].attr,
-	&node_state_attr[2].attr,
-	&node_state_attr[3].attr,
+	&node_state_attr[N_POSSIBLE].attr,
+#ifdef CONFIG_NODE_HOTPLUG_EMU
+	&node_state_attr[N_HIDDEN].attr,
+#endif
+	&node_state_attr[N_ONLINE].attr,
+	&node_state_attr[N_NORMAL_MEMORY].attr,
+	&node_state_attr[N_CPU].attr,
 #ifdef CONFIG_HIGHMEM
-	&node_state_attr[4].attr,
+	&node_state_attr[N_HIGH_MEMORY].attr,
 #endif
 	NULL
 };

Re: [RFC, 3/7] NUMA hotplug emulator

[Greg KH]

On Thu, May 13, 2010 at 07:48:35PM +0800, Shaohui Zheng wrote:
> Userland interface to hotplug-add fake offlined nodes.

Why include 2 copies of the patch in one email?

> Add a sysfs entry "probe" under /sys/devices/system/node/:
> 
>  - to show all fake offlined nodes:
>     $ cat /sys/devices/system/node/probe
> 
>  - to hotadd a fake offlined node, e.g. nodeid is N:
>     $ echo N > /sys/devices/system/node/probe

As you are trying to add a new sysfs file, please create the matching
Documentation/ABI/ file as well.

Also note that sysfs files are "one value per file", which I don't think
this file follows, right?

thanks,

greg k-h

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Dave Hansen]

On Thu, 2010-05-13 at 09:55 -0700, Greg KH wrote:
> > Add a sysfs entry "probe" under /sys/devices/system/node/:
> > 
> >  - to show all fake offlined nodes:
> >     $ cat /sys/devices/system/node/probe
> > 
> >  - to hotadd a fake offlined node, e.g. nodeid is N:
> >     $ echo N > /sys/devices/system/node/probe
> 
> As you are trying to add a new sysfs file, please create the matching
> Documentation/ABI/ file as well.
> 
> Also note that sysfs files are "one value per file", which I don't think
> this file follows, right?

I think in this case, it was meant to be a list of acceptable parameters
rather than a set of values, kinda like /sys/power/state.  Instead, I
guess we could have:

	/sys/devices/system/node/probeable/3
	/sys/devices/system/node/probeable/43
	/sys/devices/system/node/probeable/65
	/sys/devices/system/node/probeable/5145

and the knowledge that you need to pick one of those to echo
into /sys/devices/system/node/probe.  But, it's a lot more self
explanatory if you 'cat /sys/devices/system/node/probe', and then pick
one of those to echo back into the file.

Seems like a decent place to violate the "rule". :)

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Greg KH]

On Thu, May 13, 2010 at 10:54:52AM -0700, Dave Hansen wrote:
> On Thu, 2010-05-13 at 09:55 -0700, Greg KH wrote:
> > > Add a sysfs entry "probe" under /sys/devices/system/node/:
> > > 
> > >  - to show all fake offlined nodes:
> > >     $ cat /sys/devices/system/node/probe
> > > 
> > >  - to hotadd a fake offlined node, e.g. nodeid is N:
> > >     $ echo N > /sys/devices/system/node/probe
> > 
> > As you are trying to add a new sysfs file, please create the matching
> > Documentation/ABI/ file as well.
> > 
> > Also note that sysfs files are "one value per file", which I don't think
> > this file follows, right?
> 
> I think in this case, it was meant to be a list of acceptable parameters
> rather than a set of values, kinda like /sys/power/state.  Instead, I
> guess we could have:
> 
> 	/sys/devices/system/node/probeable/3
> 	/sys/devices/system/node/probeable/43
> 	/sys/devices/system/node/probeable/65
> 	/sys/devices/system/node/probeable/5145
> 
> and the knowledge that you need to pick one of those to echo
> into /sys/devices/system/node/probe.  But, it's a lot more self
> explanatory if you 'cat /sys/devices/system/node/probe', and then pick
> one of those to echo back into the file.
> 
> Seems like a decent place to violate the "rule". :)

How big would this "list" be?  What will it look like exactly?

thanks,

greg k-h

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Haicheng Li]

Dave Hansen wrote:
> On Thu, 2010-05-13 at 09:55 -0700, Greg KH wrote:
>>> Add a sysfs entry "probe" under /sys/devices/system/node/:
>>>
>>>  - to show all fake offlined nodes:
>>>     $ cat /sys/devices/system/node/probe
>>>
>>>  - to hotadd a fake offlined node, e.g. nodeid is N:
>>>     $ echo N > /sys/devices/system/node/probe
>> As you are trying to add a new sysfs file, please create the matching
>> Documentation/ABI/ file as well.
>>
>> Also note that sysfs files are "one value per file", which I don't think
>> this file follows, right?
> 
> I think in this case, it was meant to be a list of acceptable parameters
> rather than a set of values, kinda like /sys/power/state.

Right.

> Instead, I guess we could have:
> 
> 	/sys/devices/system/node/probeable/3
> 	/sys/devices/system/node/probeable/43
> 	/sys/devices/system/node/probeable/65
> 	/sys/devices/system/node/probeable/5145
> 
> and the knowledge that you need to pick one of those to echo
> into /sys/devices/system/node/probe.

I think this way would make things complex if we just want to show user which node could be hotadded.

>  But, it's a lot more self
> explanatory if you 'cat /sys/devices/system/node/probe', and then pick
> one of those to echo back into the file.

agreed.

> Seems like a decent place to violate the "rule". :)



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Shaohui Zheng]

On Thu, May 13, 2010 at 09:55:11AM -0700, Greg KH wrote:
> On Thu, May 13, 2010 at 07:48:35PM +0800, Shaohui Zheng wrote:
> > Userland interface to hotplug-add fake offlined nodes.
> 
> Why include 2 copies of the patch in one email?
I always try to attach the patch as attachment, it is the same with the mail
content, I guess it should take convenience when you need to save the patch 
to local, it might be a bad habbit, I will be careful when I send patch next time.
thanks for the reminding.

> 
> > Add a sysfs entry "probe" under /sys/devices/system/node/:
> > 
> >  - to show all fake offlined nodes:
> >     $ cat /sys/devices/system/node/probe
> > 
> >  - to hotadd a fake offlined node, e.g. nodeid is N:
> >     $ echo N > /sys/devices/system/node/probe
> 
> As you are trying to add a new sysfs file, please create the matching
> Documentation/ABI/ file as well.

Agree, We will document it in.

> 
> Also note that sysfs files are "one value per file", which I don't think
> this file follows, right?

Agree, the cpu/probe interface should write only, and we should create another
file to indicate the hidden nodes, such as cpu/hidden. We will follow this rule
when we send the formal patch.

Thanks greg's comments so much, have a nice day.

> 
> thanks,
> 
> greg k-h

-- 
Thanks & Regards,
Shaohui

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Wu Fengguang]

On Fri, May 14, 2010 at 09:45:35AM +0800, Zheng, Shaohui wrote:
> On Thu, May 13, 2010 at 09:55:11AM -0700, Greg KH wrote:
> > On Thu, May 13, 2010 at 07:48:35PM +0800, Shaohui Zheng wrote:
> > > Userland interface to hotplug-add fake offlined nodes.
> > 
> > Why include 2 copies of the patch in one email?
> I always try to attach the patch as attachment, it is the same with the mail
> content, I guess it should take convenience when you need to save the patch 
> to local, it might be a bad habbit, I will be careful when I send patch next time.
> thanks for the reminding.

Shaohui, git/quilt are great tools for submitting patch series.

> > 
> > > Add a sysfs entry "probe" under /sys/devices/system/node/:
> > > 
> > >  - to show all fake offlined nodes:
> > >     $ cat /sys/devices/system/node/probe
> > > 
> > >  - to hotadd a fake offlined node, e.g. nodeid is N:
> > >     $ echo N > /sys/devices/system/node/probe
> > 
> > As you are trying to add a new sysfs file, please create the matching
> > Documentation/ABI/ file as well.
> 
> Agree, We will document it in.
> 
> > 
> > Also note that sysfs files are "one value per file", which I don't think
> > this file follows, right?
> 
> Agree, the cpu/probe interface should write only, and we should create another
> file to indicate the hidden nodes, such as cpu/hidden. We will follow this rule
> when we send the formal patch.

I'd prefer to avoid new interfaces if not absolutely necessary.

Thanks,
Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Shaohui Zheng]

On Fri, May 14, 2010 at 10:01:35AM +0800, Wu Fengguang wrote:
> On Fri, May 14, 2010 at 09:45:35AM +0800, Zheng, Shaohui wrote:
> > On Thu, May 13, 2010 at 09:55:11AM -0700, Greg KH wrote:
> > > On Thu, May 13, 2010 at 07:48:35PM +0800, Shaohui Zheng wrote:
> > > > Userland interface to hotplug-add fake offlined nodes.
> > > 
> > > Why include 2 copies of the patch in one email?
> > I always try to attach the patch as attachment, it is the same with the mail
> > content, I guess it should take convenience when you need to save the patch 
> > to local, it might be a bad habbit, I will be careful when I send patch next time.
> > thanks for the reminding.
> 
> Shaohui, git/quilt are great tools for submitting patch series.
Thanks fengguang, I am the new fish in LKML, so I send the patches manually one by
one, I will try this tool next time.

> 
> > > 
> > > > Add a sysfs entry "probe" under /sys/devices/system/node/:
> > > > 
> > > >  - to show all fake offlined nodes:
> > > >     $ cat /sys/devices/system/node/probe
> > > > 
> > > >  - to hotadd a fake offlined node, e.g. nodeid is N:
> > > >     $ echo N > /sys/devices/system/node/probe
> > > 
> > > As you are trying to add a new sysfs file, please create the matching
> > > Documentation/ABI/ file as well.
> > 
> > Agree, We will document it in.
> > 
> > > 
> > > Also note that sysfs files are "one value per file", which I don't think
> > > this file follows, right?
> > 
> > Agree, the cpu/probe interface should write only, and we should create another
> > file to indicate the hidden nodes, such as cpu/hidden. We will follow this rule
> > when we send the formal patch.
> 
> I'd prefer to avoid new interfaces if not absolutely necessary.
> 
> Thanks,
> Fengguang

-- 
Thanks & Regards,
Shaohui

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Haicheng Li]

Greg KH wrote:
>> Add a sysfs entry "probe" under /sys/devices/system/node/:
>>
>>  - to show all fake offlined nodes:
>>     $ cat /sys/devices/system/node/probe
>>
>>  - to hotadd a fake offlined node, e.g. nodeid is N:
>>     $ echo N > /sys/devices/system/node/probe
> 
> As you are trying to add a new sysfs file, please create the matching
> Documentation/ABI/ file as well.

Yes, we missed it. thank you.

> Also note that sysfs files are "one value per file", which I don't think
> this file follows, right?

Sorry, we didn't make the descriptions clear enough.
It should follow "one value per file". It intends to show acceptable parameters.

For example, if we have 4 fake offlined nodes, like node 2-5, then:
   $ cat /sys/devices/system/node/probe
     2-5

Then user hotadds node3 to system:
   $ echo 3 > /sys/devices/system/node/probe
   $ cat /sys/devices/system/node/probe
     2,4-5

-haicheng

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Haicheng Li]

Shaohui Zheng wrote:
> Userland interface to hotplug-add fake offlined nodes.
> 
> Add a sysfs entry "probe" under /sys/devices/system/node/:
> 
>  - to show all fake offlined nodes:
>     $ cat /sys/devices/system/node/probe
> 
>  - to hotadd a fake offlined node, e.g. nodeid is N:
>     $ echo N > /sys/devices/system/node/probe
> 
> Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com>
> Signed-off-by: Shaohui Zheng <shaohui.zheng@intel.com>
> ---
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 057979a..a0be257 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -535,6 +535,26 @@ void unregister_one_node(int nid)
>  	unregister_node(&node_devices[nid]);
>  }
>  
> +#ifdef CONFIG_NODE_HOTPLUG_EMU
> +static ssize_t store_nodes_probe(struct sysdev_class *class,
> +				  struct sysdev_class_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	long nid;
> +	int ret;
> +
> +	strict_strtol(buf, 0, &nid);
> +	if (nid < 0 || nid > nr_node_ids - 1) {

Shaohui,

In fact, no need to do such check here, hotadd_hidden_nodes() can handle such invalid parameter by 
itself.

> +		printk(KERN_ERR "Invalid NUMA node id: %d (0 <= nid < %d).\n",
> +			nid, nr_node_ids);
> +		return -EPERM;

Per Andi's earlier review comments, -EPERM is odd, we'd fix it.

> +	}
> +	hotadd_hidden_nodes(nid);
> +
> +	return count;
> +}
> +#endif

Pls. replace it with following code:

+#ifdef CONFIG_NODE_HOTPLUG_EMU
+static ssize_t store_nodes_probe(struct sysdev_class *class,
+                                 struct sysdev_class_attribute *attr,
+                                 const char *buf, size_t count)
+{
+       long nid;
+       int ret;
+
+       ret = strict_strtol(buf, 0, &nid);
+       if (ret == -EINVAL)
+               return ret;
+
+       ret = hotadd_hidden_nodes(nid);
+       if (!ret)
+               return count;
+       else
+               return -EIO;
+}
+#endif


-haicheng

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Wu Fengguang]

> Pls. replace it with following code:
> 
> +#ifdef CONFIG_NODE_HOTPLUG_EMU
> +static ssize_t store_nodes_probe(struct sysdev_class *class,
> +                                 struct sysdev_class_attribute *attr,
> +                                 const char *buf, size_t count)
> +{
> +       long nid;
> +       int ret;
> +
> +       ret = strict_strtol(buf, 0, &nid);
> +       if (ret == -EINVAL)
> +               return ret;
> +
> +       ret = hotadd_hidden_nodes(nid);
> +       if (!ret)
> +               return count;
> +       else
> +               return -EIO;
> +}
> +#endif

How about this?

       err = strict_strtol(buf, 0, &nid);
       if (err < 0)
               return err;

       err = hotadd_hidden_nodes(nid);
       if (err < 0)
               return err;

       return count;

Thanks,
Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Shaohui Zheng]

On Fri, May 14, 2010 at 12:11:36PM +0800, Wu Fengguang wrote:
> > Pls. replace it with following code:
> > 
> > +#ifdef CONFIG_NODE_HOTPLUG_EMU
> > +static ssize_t store_nodes_probe(struct sysdev_class *class,
> > +                                 struct sysdev_class_attribute *attr,
> > +                                 const char *buf, size_t count)
> > +{
> > +       long nid;
> > +       int ret;
> > +
> > +       ret = strict_strtol(buf, 0, &nid);
> > +       if (ret == -EINVAL)
> > +               return ret;
> > +
> > +       ret = hotadd_hidden_nodes(nid);
> > +       if (!ret)
> > +               return count;
> > +       else
> > +               return -EIO;
> > +}
> > +#endif
> 
> How about this?
> 
>        err = strict_strtol(buf, 0, &nid);
>        if (err < 0)
>                return err;
> 
>        err = hotadd_hidden_nodes(nid);
>        if (err < 0)
>                return err;
> 
>        return count;
> 
> Thanks,
> Fengguang

Both seems good.

Haicheng's code is prefered, since it was already reviewed.

-- 
Thanks & Regards,
Shaohui

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Haicheng Li]

Wu Fengguang wrote:
>> Pls. replace it with following code:
>>
>> +#ifdef CONFIG_NODE_HOTPLUG_EMU
>> +static ssize_t store_nodes_probe(struct sysdev_class *class,
>> +                                 struct sysdev_class_attribute *attr,
>> +                                 const char *buf, size_t count)
>> +{
>> +       long nid;
>> +       int ret;
>> +
>> +       ret = strict_strtol(buf, 0, &nid);
>> +       if (ret == -EINVAL)
>> +               return ret;
>> +
>> +       ret = hotadd_hidden_nodes(nid);
>> +       if (!ret)
>> +               return count;
>> +       else
>> +               return -EIO;
>> +}
>> +#endif
> 
> How about this?
> 
>        err = strict_strtol(buf, 0, &nid);
>        if (err < 0)
>                return err;

other negative value would be odd here.

>        err = hotadd_hidden_nodes(nid);
>        if (err < 0)
>                return err;

hotadd_hidden_nodes could return -EEXIST, which is also odd here, right?

>        return count;
> 
> Thanks,
> Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Wu Fengguang]

On Fri, May 14, 2010 at 01:32:02PM +0800, Haicheng Li wrote:
> Wu Fengguang wrote:
> >> Pls. replace it with following code:
> >>
> >> +#ifdef CONFIG_NODE_HOTPLUG_EMU
> >> +static ssize_t store_nodes_probe(struct sysdev_class *class,
> >> +                                 struct sysdev_class_attribute *attr,
> >> +                                 const char *buf, size_t count)
> >> +{
> >> +       long nid;
> >> +       int ret;
> >> +
> >> +       ret = strict_strtol(buf, 0, &nid);
> >> +       if (ret == -EINVAL)
> >> +               return ret;
> >> +
> >> +       ret = hotadd_hidden_nodes(nid);
> >> +       if (!ret)
> >> +               return count;
> >> +       else
> >> +               return -EIO;
> >> +}
> >> +#endif
> > 
> > How about this?
> > 
> >        err = strict_strtol(buf, 0, &nid);
> >        if (err < 0)
> >                return err;
> 
> other negative value would be odd here.

Yes, strict_strtoul() will be better.

> >        err = hotadd_hidden_nodes(nid);
> >        if (err < 0)
> >                return err;
> 
> hotadd_hidden_nodes could return -EEXIST, which is also odd here, right?

Why not? write(2) says "Other errors may occur, depending on the
object connected to fd."

Thanks,
Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Ankita Garg]

Hi,

On Thu, May 13, 2010 at 07:48:35PM +0800, Shaohui Zheng wrote:
> Userland interface to hotplug-add fake offlined nodes.
> 
> Add a sysfs entry "probe" under /sys/devices/system/node/:
> 
>  - to show all fake offlined nodes:
>     $ cat /sys/devices/system/node/probe
> 
>  - to hotadd a fake offlined node, e.g. nodeid is N:
>     $ echo N > /sys/devices/system/node/probe
> 
> Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com>
> Signed-off-by: Shaohui Zheng <shaohui.zheng@intel.com>
> ---
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 9458685..2c078c8 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1214,6 +1214,20 @@ config NUMA_EMU
>  	  into virtual nodes when booted with "numa=fake=N", where N is the
>  	  number of nodes. This is only useful for debugging.
> 
> +config NUMA_HOTPLUG_EMU
> +	bool "NUMA hotplug emulator"
> +	depends on X86_64 && NUMA && HOTPLUG
> +	---help---
> +
> +config NODE_HOTPLUG_EMU
> +	bool "Node hotplug emulation"
> +	depends on NUMA_HOTPLUG_EMU && MEMORY_HOTPLUG
> +	---help---
> +	  Enable Node hotplug emulation. The machine will be setup with
> +	  hidden virtual nodes when booted with "numa=hide=N*size", where
> +	  N is the number of hidden nodes, size is the memory size per
> +	  hidden node. This is only useful for debugging.
> +

The above dependencies do not work as expected. I could configure
NUMA_HOTPLUG_EMU & NODE_HOTPLUG_EMU without having MEMORY_HOTPLUG
turned on. By pushing the above definition below SPARSEMEM and memory
hot add and remove, the dependencies could be sorted out.

-- 
Regards,                                                                        
Ankita Garg (ankita@in.ibm.com)                                                 
Linux Technology Center                                                         
IBM India Systems & Technology Labs,                                            
Bangalore, India

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC, 3/7] NUMA hotplug emulator

[Shaohui Zheng]

On Fri, May 21, 2010 at 03:38:16PM +0530, Ankita Garg wrote:
> Hi,
> 
> On Thu, May 13, 2010 at 07:48:35PM +0800, Shaohui Zheng wrote:
> > Userland interface to hotplug-add fake offlined nodes.
> > 
> > Add a sysfs entry "probe" under /sys/devices/system/node/:
> > 
> >  - to show all fake offlined nodes:
> >     $ cat /sys/devices/system/node/probe
> > 
> >  - to hotadd a fake offlined node, e.g. nodeid is N:
> >     $ echo N > /sys/devices/system/node/probe
> > 
> > Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com>
> > Signed-off-by: Shaohui Zheng <shaohui.zheng@intel.com>
> > ---
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 9458685..2c078c8 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1214,6 +1214,20 @@ config NUMA_EMU
> >  	  into virtual nodes when booted with "numa=fake=N", where N is the
> >  	  number of nodes. This is only useful for debugging.
> > 
> > +config NUMA_HOTPLUG_EMU
> > +	bool "NUMA hotplug emulator"
> > +	depends on X86_64 && NUMA && HOTPLUG
> > +	---help---
> > +
> > +config NODE_HOTPLUG_EMU
> > +	bool "Node hotplug emulation"
> > +	depends on NUMA_HOTPLUG_EMU && MEMORY_HOTPLUG
> > +	---help---
> > +	  Enable Node hotplug emulation. The machine will be setup with
> > +	  hidden virtual nodes when booted with "numa=hide=N*size", where
> > +	  N is the number of hidden nodes, size is the memory size per
> > +	  hidden node. This is only useful for debugging.
> > +
> 
> The above dependencies do not work as expected. I could configure
> NUMA_HOTPLUG_EMU & NODE_HOTPLUG_EMU without having MEMORY_HOTPLUG
> turned on. By pushing the above definition below SPARSEMEM and memory
> hot add and remove, the dependencies could be sorted out.
Ankita, 
	The emulation code was tested by many times, but we did not try each 
combination for the new options, good catching.
    We will includes your suggestion in the formal patch. thanks so much.
> 
> -- 
> Regards,                                                                        
> Ankita Garg (ankita@in.ibm.com)                                                 
> Linux Technology Center                                                         
> IBM India Systems & Technology Labs,                                            
> Bangalore, India

-- 
Thanks & Regards,
Shaohui

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>