From: Shaohui Zheng <[email protected]>
Add add_memory interface to support to memory hotplug emulation for each online
node under debugfs. The reserved memory can be added into desired node with
this interface.
The layout on debugfs:
mem_hotplug/node0/add_memory
mem_hotplug/node1/add_memory
mem_hotplug/node2/add_memory
...
Add a memory section(128M) to node 3(boots with mem=1024m)
echo 0x40000000 > mem_hotplug/node3/add_memory
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
Signed-off-by: Haicheng Li <[email protected]>
Signed-off-by: Shaohui Zheng <[email protected]>
---
Index: linux-hpe4/mm/memory_hotplug.c
===================================================================
--- linux-hpe4.orig/mm/memory_hotplug.c 2010-12-10 13:22:44.753331000 +0800
+++ linux-hpe4/mm/memory_hotplug.c 2010-12-10 13:41:48.803331000 +0800
@@ -933,6 +933,81 @@
static struct dentry *memhp_debug_root;
+#ifdef CONFIG_ARCH_MEMORY_PROBE
+
+static ssize_t add_memory_store(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ u64 phys_addr = 0;
+ int nid = file->private_data - NULL;
+ int ret;
+
+ printk(KERN_INFO "Add a memory section to node: %d.\n", nid);
+ phys_addr = simple_strtoull(buf, NULL, 0);
+
+ ret = add_memory(nid, phys_addr, PAGES_PER_SECTION << PAGE_SHIFT);
+ if (ret)
+ count = ret;
+
+ return count;
+}
+
+static int add_memory_open(struct inode *inode, struct file *file)
+{
+ file->private_data = inode->i_private;
+ return 0;
+}
+
+static const struct file_operations add_memory_file_ops = {
+ .open = add_memory_open,
+ .write = add_memory_store,
+ .llseek = generic_file_llseek,
+};
+
+/*
+ * Create add_memory debugfs entry under specified node
+ */
+static int debugfs_create_add_memory_entry(int nid)
+{
+ char buf[32];
+ static struct dentry *node_debug_root;
+
+ snprintf(buf, sizeof(buf), "node%d", nid);
+ node_debug_root = debugfs_create_dir(buf, memhp_debug_root);
+ if (!node_debug_root)
+ return -ENOMEM;
+
+ /* the nid information was represented by the offset of pointer(NULL+nid) */
+ if (!debugfs_create_file("add_memory", S_IWUSR, node_debug_root,
+ NULL + nid, &add_memory_file_ops))
+ return -ENOMEM;
+
+ return 0;
+}
+
+static int __init memory_debug_init(void)
+{
+ int nid;
+
+ if (!memhp_debug_root)
+ memhp_debug_root = debugfs_create_dir("mem_hotplug", NULL);
+ if (!memhp_debug_root)
+ return -ENOMEM;
+
+ for_each_online_node(nid)
+ debugfs_create_add_memory_entry(nid);
+
+ return 0;
+}
+
+module_init(memory_debug_init);
+#else
+static debugfs_create_add_memory_entry(int nid)
+{
+ return 0;
+}
+#endif /* CONFIG_ARCH_MEMORY_PROBE */
+
static ssize_t add_node_store(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
@@ -963,6 +1038,8 @@
return -ENOMEM;
ret = add_memory(nid, start, size);
+
+ debugfs_create_add_memory_entry(nid);
return ret ? ret : count;
}
Index: linux-hpe4/Documentation/memory-hotplug.txt
===================================================================
--- linux-hpe4.orig/Documentation/memory-hotplug.txt 2010-12-10 13:22:44.733331000 +0800
+++ linux-hpe4/Documentation/memory-hotplug.txt 2010-12-10 13:42:12.783331002 +0800
@@ -19,6 +19,7 @@
4.1 Hardware(Firmware) Support
4.2 Notify memory hot-add event by hand
4.3 Node hotplug emulation
+ 4.4 Memory hotplug emulation
5. Logical Memory hot-add phase
5.1. State of memory
5.2. How to online memory
@@ -239,6 +240,25 @@
Once the new node has been added, it is possible to online the memory by
toggling the "state" of its memory section(s) as described in section 5.1.
+4.4 Memory hotplug emulation
+------------
+With debugfs, it is possible to test memory hotplug with software method, we
+can add memory section to desired node with add_memory interface. It is a much
+more powerful interface than "probe" described in section 4.2.
+
+There is an add_memory interface for each online node at the debugfs mount
+point.
+ mem_hotplug/node0/add_memory
+ mem_hotplug/node1/add_memory
+ mem_hotplug/node2/add_memory
+ ...
+
+Add a memory section(128M) to node 3(boots with mem=1024m)
+
+ echo 0x40000000 > mem_hotplug/node3/add_memory
+
+Once the new memory section has been added, it is possible to online the memory
+by toggling the "state" described in section 5.1.
------------------------------
5. Logical Memory hot-add phase
--
Thanks & Regards,
Shaohui
On Fri, 10 Dec 2010 15:31:26 +0800
[email protected] wrote:
> From: Shaohui Zheng <[email protected]>
>
> Add add_memory interface to support to memory hotplug emulation for each online
> node under debugfs. The reserved memory can be added into desired node with
> this interface.
>
> The layout on debugfs:
> mem_hotplug/node0/add_memory
> mem_hotplug/node1/add_memory
> mem_hotplug/node2/add_memory
> ...
>
> Add a memory section(128M) to node 3(boots with mem=1024m)
>
> echo 0x40000000 > mem_hotplug/node3/add_memory
>
>
> ...
>
> +#ifdef CONFIG_ARCH_MEMORY_PROBE
> +
> +static ssize_t add_memory_store(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u64 phys_addr = 0;
Even more unneeded initalisation.
Please check the whole patchset for this. It's bad because it can
sometimes generate more code and because it can sometimes hide bugs by
suppressing used-uninitialsied warnings.
> + int nid = file->private_data - NULL;
Well that was sneaky.
It would be more conventional to just use the typecast:
int nid = (long)file->private_data;
> + int ret;
> +
> + printk(KERN_INFO "Add a memory section to node: %d.\n", nid);
> + phys_addr = simple_strtoull(buf, NULL, 0);
checkpatch
> + ret = add_memory(nid, phys_addr, PAGES_PER_SECTION << PAGE_SHIFT);
> + if (ret)
> + count = ret;
> +
> + return count;
> +}
> +
> +static int add_memory_open(struct inode *inode, struct file *file)
> +{
> + file->private_data = inode->i_private;
Was this usage of i_private and private_data documented in comments
somewhere?
> + return 0;
> +}
> +
> +static const struct file_operations add_memory_file_ops = {
> + .open = add_memory_open,
> + .write = add_memory_store,
> + .llseek = generic_file_llseek,
> +};
> +
> +/*
> + * Create add_memory debugfs entry under specified node
> + */
> +static int debugfs_create_add_memory_entry(int nid)
> +{
> + char buf[32];
> + static struct dentry *node_debug_root;
> +
> + snprintf(buf, sizeof(buf), "node%d", nid);
> + node_debug_root = debugfs_create_dir(buf, memhp_debug_root);
> + if (!node_debug_root)
> + return -ENOMEM;
hm, debugfs_create_dir() was poorly designed - it should return an
ERR_PTR() so callers don't need to assume ENOMEM, which may be incorrect.
> + /* the nid information was represented by the offset of pointer(NULL+nid) */
> + if (!debugfs_create_file("add_memory", S_IWUSR, node_debug_root,
> + NULL + nid, &add_memory_file_ops))
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static int __init memory_debug_init(void)
> +{
> + int nid;
> +
> + if (!memhp_debug_root)
> + memhp_debug_root = debugfs_create_dir("mem_hotplug", NULL);
> + if (!memhp_debug_root)
> + return -ENOMEM;
> +
> + for_each_online_node(nid)
> + debugfs_create_add_memory_entry(nid);
> +
> + return 0;
> +}
> +
> +module_init(memory_debug_init);
> +#else
> +static debugfs_create_add_memory_entry(int nid)
"static int".
> +{
> + return 0;
> +}
> +#endif /* CONFIG_ARCH_MEMORY_PROBE */
> +
> static ssize_t add_node_store(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> @@ -963,6 +1038,8 @@
> return -ENOMEM;
>
> ret = add_memory(nid, start, size);
> +
> + debugfs_create_add_memory_entry(nid);
> return ret ? ret : count;
> }
>
>
> ...
>
On Wed, Dec 22, 2010 at 04:27:36PM -0800, Andrew Morton wrote:
> On Fri, 10 Dec 2010 15:31:26 +0800
> [email protected] wrote:
>
> > From: Shaohui Zheng <[email protected]>
> >
> > Add add_memory interface to support to memory hotplug emulation for each online
> > node under debugfs. The reserved memory can be added into desired node with
> > this interface.
> >
> > The layout on debugfs:
> > mem_hotplug/node0/add_memory
> > mem_hotplug/node1/add_memory
> > mem_hotplug/node2/add_memory
> > ...
> >
> > Add a memory section(128M) to node 3(boots with mem=1024m)
> >
> > echo 0x40000000 > mem_hotplug/node3/add_memory
> >
> >
> > ...
> >
> > +#ifdef CONFIG_ARCH_MEMORY_PROBE
> > +
> > +static ssize_t add_memory_store(struct file *file, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + u64 phys_addr = 0;
>
> Even more unneeded initalisation.
>
> Please check the whole patchset for this. It's bad because it can
> sometimes generate more code and because it can sometimes hide bugs by
> suppressing used-uninitialsied warnings.
>
Yes, It is a my habit to initialize variable when define it. I will check them
one by one.
> > + int nid = file->private_data - NULL;
>
> Well that was sneaky.
>
> It would be more conventional to just use the typecast:
>
> int nid = (long)file->private_data;
>
>
An explicit typecast looks much better.
> > + int ret;
> > +
> > + printk(KERN_INFO "Add a memory section to node: %d.\n", nid);
> > + phys_addr = simple_strtoull(buf, NULL, 0);
>
> checkpatch
>
We ignored the warning for function simple_strtoull in the whole patchset.
We will solve it one by one.
> > + ret = add_memory(nid, phys_addr, PAGES_PER_SECTION << PAGE_SHIFT);
> > + if (ret)
> > + count = ret;
> > +
> > + return count;
> > +}
> > +
> > +static int add_memory_open(struct inode *inode, struct file *file)
> > +{
> > + file->private_data = inode->i_private;
>
> Was this usage of i_private and private_data documented in comments
> somewhere?
>
Yes, I added the usage information when create the add_memory entry, it seems
that I should also add comment here.
/* the nid information was represented by the offset of pointer(NULL+nid) */
if (!debugfs_create_file("add_memory", S_IWUSR, node_debug_root,
NULL + nid, &add_memory_file_ops))
> > + return 0;
> > +}
> > +
> > +static const struct file_operations add_memory_file_ops = {
> > + .open = add_memory_open,
> > + .write = add_memory_store,
> > + .llseek = generic_file_llseek,
> > +};
> > +
> > +/*
> > + * Create add_memory debugfs entry under specified node
> > + */
> > +static int debugfs_create_add_memory_entry(int nid)
> > +{
> > + char buf[32];
> > + static struct dentry *node_debug_root;
> > +
> > + snprintf(buf, sizeof(buf), "node%d", nid);
> > + node_debug_root = debugfs_create_dir(buf, memhp_debug_root);
> > + if (!node_debug_root)
> > + return -ENOMEM;
>
> hm, debugfs_create_dir() was poorly designed - it should return an
> ERR_PTR() so callers don't need to assume ENOMEM, which may be incorrect.
>
Totally agree. I see that the simliar call on debugfs_create_dir. For the failure,
most of them assume ENOMEM, some of them assume as EINVAL.
> > + /* the nid information was represented by the offset of pointer(NULL+nid) */
> > + if (!debugfs_create_file("add_memory", S_IWUSR, node_debug_root,
> > + NULL + nid, &add_memory_file_ops))
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +static int __init memory_debug_init(void)
> > +{
> > + int nid;
> > +
> > + if (!memhp_debug_root)
> > + memhp_debug_root = debugfs_create_dir("mem_hotplug", NULL);
> > + if (!memhp_debug_root)
> > + return -ENOMEM;
> > +
> > + for_each_online_node(nid)
> > + debugfs_create_add_memory_entry(nid);
> > +
> > + return 0;
> > +}
> > +
> > +module_init(memory_debug_init);
> > +#else
> > +static debugfs_create_add_memory_entry(int nid)
>
> "static int".
>
Good catching.
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_ARCH_MEMORY_PROBE */
> > +
> > static ssize_t add_node_store(struct file *file, const char __user *buf,
> > size_t count, loff_t *ppos)
> > {
> > @@ -963,6 +1038,8 @@
> > return -ENOMEM;
> >
> > ret = add_memory(nid, start, size);
> > +
> > + debugfs_create_add_memory_entry(nid);
> > return ret ? ret : count;
> > }
> >
> >
> > ...
> >
--
Thanks & Regards,
Shaohui