De-couple sysfs memory directories from memory sections

This patch splits the memory_block struct into a memory_block struct to cover each sysfs directory and a new memory_block_section struct for each memory section covered by the sysfs directory.

This also updates the routine handling memory_block creation and manipulation to use these updated structures.

Signed -off-by: Nathan Fontenot <[email protected]>

---
 drivers/base/memory.c  |  228 +++++++++++++++++++++++++++++++++++--------------
 include/linux/memory.h |   11 +-
 2 files changed, 172 insertions(+), 67 deletions(-)

Index: linux-2.6/drivers/base/memory.c
===================================================================

drivers/base/memory.c +5 -6

+static inline int base_memory_block_id(int section_nr)
+{
+	return (section_nr / sections_per_block) * sections_per_block;
+}
 
 static struct sysdev_class memory_sysdev_class = {
 	.name = MEMORY_CLASS_NAME,

@@ -94,10 +102,9 @@
 }
 
 static void
-unregister_memory(struct memory_block *memory, struct mem_section *section)
+unregister_memory(struct memory_block *memory)
 {
 	BUG_ON(memory->sysdev.cls != &memory_sysdev_class);
-	BUG_ON(memory->sysdev.id != __section_nr(section));
 
 	/* drop the ref. we got in remove_memory_block() */
 	kobject_put(&memory->sysdev.kobj);

@@ -123,13 +130,20 @@
 static ssize_t show_mem_removable(struct sys_device *dev,
 			struct sysdev_attribute *attr, char *buf)
 {
-	unsigned long start_pfn;
-	int ret;
-	struct memory_block *mem =
-		container_of(dev, struct memory_block, sysdev);

drivers/base/memory.c +1 -2

@@ -182,16 +196,16 @@
  * OK to have direct references to sparsemem variables in here.
  */
 static int
-memory_block_action(struct memory_block *mem, unsigned long action)
+memory_block_action(struct memory_block_section *mbs, unsigned long action)
 {
 	int i;
 	unsigned long psection;
 	unsigned long start_pfn, start_paddr;
 	struct page *first_page;
 	int ret;
-	int old_state = mem->state;

ot-option-to-disable-memory-hotplug.patch

drivers/base/memory.c +6 -4

-	psection = mem->phys_index;
+	psection = mbs->phys_index;
 	first_page = pfn_to_page(psection << PFN_SECTION_SHIFT);
 
 	/*

@@ -217,18 +231,18 @@
 			ret = online_pages(start_pfn, PAGES_PER_SECTION);
 			break;
 		case MEM_OFFLINE:
-			mem->state = MEM_GOING_OFFLINE;
+			mbs->state = MEM_GOING_OFFLINE;
 			start_paddr = page_to_pfn(first_page) << PAGE_SHIFT;
 			ret = remove_memory(start_paddr,
 					    PAGES_PER_SECTION << PAGE_SHIFT);
 			if (ret) {
-				mem->state = old_state;
+				mbs->state = old_state;
 				break;
 			}
 			break;
 		default:
 			WARN(1, KERN_WARNING "%s(%p, %ld) unknown action: %ld\n",
-					__func__, mem, action, action);
+					__func__, mbs, action, action);
 			ret = -EINVAL;
 	}
 

@@ -238,19 +252,40 @@
 static int memory_block_change_state(struct memory_block *mem,
 		unsigned long to_state, unsigned long from_state_req)
 {
+	struct memory_block_section *mbs;
+	struct list_head *pos;

drivers/base/memory.c +7 -2

+			if (mbs->state == from_state_req)
+				continue;
+
+			if (memory_block_action(mbs, to_state))
+				printk(KERN_ERR "Could not re-enable memory "
+				       "section %lx\n", mbs->phys_index);
+		}
 	}
 
-	ret = memory_block_action(mem, to_state);
 	if (!ret)
 		mem->state = to_state;
 
-out:
 	mutex_unlock(&mem->state_mutex);
 	return ret;
 }

drivers/base/memory.c +1 -29

 	struct memory_block *mem;
-	unsigned int phys_section_nr;
 	int ret = -EINVAL;
 
 	mem = container_of(dev, struct memory_block, sysdev);
-	phys_section_nr = mem->phys_index;
-
-	if (!present_section_nr(phys_section_nr))
-		goto out;
 
 	if (!strncmp(buf, "online", min((int)count, 6)))
 		ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 	else if(!strncmp(buf, "offline", min((int)count, 7)))
 		ret = memory_block_change_state(mem, MEM_OFFLINE, MEM_ONLINE);
-out:
+
 	if (ret)
 		return ret;
 	return count;

@@ -435,39 +465,6 @@
 	return 0;
 }
 
-static int add_memory_block(int nid, struct mem_section *section,
-			unsigned long state, enum mem_add_context context)
-{
-	struct memory_block *mem = kzalloc(sizeof(*mem), GFP_KERNEL);
-	unsigned long start_pfn;
-	int ret = 0;
-
-	if (!mem)
-		return -ENOMEM;
-
-	mem->phys_index = __section_nr(section);
-	mem->state = state;
-	mutex_init(&mem->state_mutex);
-	start_pfn = section_nr_to_pfn(mem->phys_index);
-	mem->phys_device = arch_get_memory_phys_device(start_pfn);
-
-	ret = register_memory(mem, section);
-	if (!ret)
-		ret = mem_create_simple_file(mem, phys_index);
-	if (!ret)
-		ret = mem_create_simple_file(mem, state);
-	if (!ret)
-		ret = mem_create_simple_file(mem, phys_device);

drivers/base/memory.c +0 -0

 /*
  * For now, we have a linear search to go find the appropriate
  * memory_block corresponding to a particular phys_index. If

@@ -482,12 +479,13 @@
 	struct sys_device *sysdev;

drivers/base/memory.c +13 -0

 	kobj = kset_find_obj(&memory_sysdev_class.kset, name);
 	if (!kobj)

@@ -498,19 +496,97 @@
 
 	return mem;
 }
+static int add_mem_block_section(struct memory_block *mem,
+				 int section_nr, unsigned long state)
+{
+	struct memory_block_section *mbs;
+
+	mbs = kzalloc(sizeof(*mbs), GFP_KERNEL);
+	if (!mbs)
+		return -ENOMEM;
+
+	mbs->phys_index = section_nr;
+	mbs->state = state;
+
+	list_add(&mbs->next, &mem->sections);

drivers/base/memory.c +23 -0

+
+		ret = register_memory(mem, section);
+		if (!ret)
+			ret = mem_create_simple_file(mem, phys_index);
+		if (!ret)
+			ret = mem_create_simple_file(mem, state);
+		if (!ret)
+			ret = mem_create_simple_file(mem, phys_device);
+		if (!ret)
+			ret = mem_create_simple_file(mem, removable);
+		if (!ret) {
+			if (context == HOTPLUG)
+				ret = register_mem_sect_under_node(mem, nid);
+		}
+	} else {
+		kobject_put(&mem->sysdev.kobj);
+	}
+
+	if (!ret)
+		ret = add_mem_block_section(mem, __section_nr(section), state);
+
+	return ret;
+}
 
 
 
 
 

drivers/base/memory.c +39 -0

+		if (mbs->phys_index == section_nr) {
+			list_del(&mbs->next);
+			kfree(mbs);
+		}
+	}
+
+	mutex_unlock(&mem->state_mutex);
+
+	if (list_empty(&mem->sections)) {
+		unregister_mem_sect_under_nodes(mem);
+		mem_remove_simple_file(mem, phys_index);
+		mem_remove_simple_file(mem, state);
+		mem_remove_simple_file(mem, phys_device);
+		mem_remove_simple_file(mem, removable);
+		unregister_memory(mem);
+		kfree(mem);
+	}
 
 	return 0;
 }

@@ -532,6 +608,24 @@
 	return remove_memory_block(0, section, 0);
 }
 
+u32 __weak memory_block_size(void)
+{
+	return MIN_MEMORY_BLOCK_SIZE;
+}
+
+static u32 get_memory_block_size(void)
+{
+	u32 blk_sz;
+
+	blk_sz = memory_block_size();
+
+	/* Validate blk_sz is a power of 2 and not less than section size */
+	if ((blk_sz & (blk_sz - 1)) || (blk_sz < MIN_MEMORY_BLOCK_SIZE))
+		blk_sz = MIN_MEMORY_BLOCK_SIZE;
+
+	return blk_sz;
+}
+
 /*
  * Initialize the sysfs support for memory devices...
  */

@@ -540,12 +634,16 @@
 	unsigned int i;
 	int ret;
 	int err;
+	int block_sz;
 
 	memory_sysdev_class.kset.uevent_ops = &memory_uevent_ops;
 	ret = sysdev_class_register(&memory_sysdev_class);
 	if (ret)
 		goto out;
 
+	block_sz = get_memory_block_size();
+	sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
+
 	/*
 	 * Create entries for memory sections that were found
 	 * during boot and have been initialized

Index: linux-2.6/include/linux/memory.h
===================================================================

include/linux/memory.h +9 -3

@@ -19,9 +19,15 @@
 #include <linux/node.h>
 #include <linux/compiler.h>
 #include <linux/mutex.h>
+#include <linux/list.h>
 
-struct memory_block {
+struct memory_block_section {
+	unsigned long state;
 	unsigned long phys_index;
+	struct list_head next;
+};
+
+struct memory_block {
 	unsigned long state;
 	/*
 	 * This serializes all state change requests.  It isn't

@@ -34,6 +40,7 @@
 	void *hw;			/* optional pointer to fw/hw data */
 	int (*phys_callback)(struct memory_block *);
 	struct sys_device sysdev;
+	struct list_head sections;
 };
 
 int arch_get_memory_phys_device(unsigned long start_pfn);

@@ -113,7 +120,7 @@
 extern int remove_memory_block(unsigned long, struct mem_section *, int);
 extern int memory_notify(unsigned long val, void *v);
 extern int memory_isolate_notify(unsigned long val, void *v);
-extern struct memory_block *find_memory_block(unsigned long);
+extern struct memory_block *find_memory_block(struct mem_section *);
 extern int memory_is_hidden(struct mem_section *);
 #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
 enum mem_add_context { BOOT, HOTPLUG };
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

plz cc linux-mm in the next time...
And please incudes updates for Documentation/memory-hotplug.txt.

Quoted 4 lines · On Mon, 12 Jul 2010 10:42:06 -0500 Nathan Fontenot <[email protected]> wrote:

This patch splits the memory_block struct into a memory_block struct to cover each sysfs directory and a new memory_block_section struct for each memory section covered by the sysfs directory.

Quoted 3 lines

This also updates the routine handling memory_block creation and manipulation to use these updated structures.

Could you clarify the number of memory_block_section per memory_block ?

Quoted 15 lines

Signed -off-by: Nathan Fontenot <[email protected]>

---
 drivers/base/memory.c  |  228 +++++++++++++++++++++++++++++++++++--------------
 include/linux/memory.h |   11 +-
 2 files changed, 172 insertions(+), 67 deletions(-)

Index: linux-2.6/drivers/base/memory.c
===================================================================

drivers/base/memory.c +0 -0

@@ -28,6 +28,14 @@
 #include <asm/uaccess.h>
 

drivers/base/memory.c line 34

Quoted 5 lines

drivers/base/memory.c +4 -0

 #define MEMORY_CLASS_NAME	"memory"
+#define MIN_MEMORY_BLOCK_SIZE	(1 << SECTION_SIZE_BITS)
+
+static int sections_per_block;
+

some default value, plz. Does this can be determined only by .config ?

Quoted 33 lines

drivers/base/memory.c +9 -6

+static inline int base_memory_block_id(int section_nr)
+{
+	return (section_nr / sections_per_block) * sections_per_block;
+}
 
 static struct sysdev_class memory_sysdev_class = {
 	.name = MEMORY_CLASS_NAME,

@@ -94,10 +102,9 @@
 }
 
 static void
-unregister_memory(struct memory_block *memory, struct mem_section *section)
+unregister_memory(struct memory_block *memory)
 {
 	BUG_ON(memory->sysdev.cls != &memory_sysdev_class);
-	BUG_ON(memory->sysdev.id != __section_nr(section));
 
 	/* drop the ref. we got in remove_memory_block() */
 	kobject_put(&memory->sysdev.kobj);

@@ -123,13 +130,20 @@
 static ssize_t show_mem_removable(struct sys_device *dev,
 			struct sysdev_attribute *attr, char *buf)
 {
-	unsigned long start_pfn;
-	int ret;
-	struct memory_block *mem =
-		container_of(dev, struct memory_block, sysdev);
+	struct list_head *pos, *tmp;
+	struct memory_block *mem;
+	int ret = 1;
+

drivers/base/memory.c line 142

Quoted 6 lines

drivers/base/memory.c +6 -0

+	mem = container_of(dev, struct memory_block, sysdev);
+	list_for_each_safe(pos, tmp, &mem->sections) {
+		struct memory_block_section *mbs;
+		unsigned long start_pfn;
+
+		mbs = list_entry(pos, struct memory_block_section, next);

list_for_each_entry ?

drivers/base/memory.c line 145

Quoted 6 lines

drivers/base/memory.c +3 -0

+		start_pfn = section_nr_to_pfn(mbs->phys_index);
+		ret &= is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
+	}

Hmm, them, only when the whole memory block is removable, it's shown as removable. Right ?
Does it meets ppc guy's requirements ?

drivers/base/memory.c line 148

Quoted 6 lines

drivers/base/memory.c +0 -2

-	start_pfn = section_nr_to_pfn(mem->phys_index);
-	ret = is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
 	return sprintf(buf, "%d\n", ret);
 }

Hmm...can't you print removable information as bitmap, here ? overkill ?

Quoted 12 lines

drivers/base/memory.c +1 -1

@@ -182,16 +196,16 @@
  * OK to have direct references to sparsemem variables in here.
  */
 static int
-memory_block_action(struct memory_block *mem, unsigned long action)
+memory_block_action(struct memory_block_section *mbs, unsigned long action)
 {
 	int i;
 	unsigned long psection;

drivers/base/memory.c

Quoted 6 lines

drivers/base/memory.c +0 -1

 	unsigned long start_pfn, start_paddr;
 	struct page *first_page;
 	int ret;
-	int old_state = mem->state;

ot-option-to-disable-memory-hotplug.patch

drivers/base/memory.c +1 -0

+	int old_state = mbs->state;

Where is this noise from ?

Quoted 39 lines

drivers/base/memory.c +7 -4

-	psection = mem->phys_index;
+	psection = mbs->phys_index;
 	first_page = pfn_to_page(psection << PFN_SECTION_SHIFT);
 
 	/*

@@ -217,18 +231,18 @@
 			ret = online_pages(start_pfn, PAGES_PER_SECTION);
 			break;
 		case MEM_OFFLINE:
-			mem->state = MEM_GOING_OFFLINE;
+			mbs->state = MEM_GOING_OFFLINE;
 			start_paddr = page_to_pfn(first_page) << PAGE_SHIFT;
 			ret = remove_memory(start_paddr,
 					    PAGES_PER_SECTION << PAGE_SHIFT);
 			if (ret) {
-				mem->state = old_state;
+				mbs->state = old_state;
 				break;
 			}
 			break;
 		default:
 			WARN(1, KERN_WARNING "%s(%p, %ld) unknown action: %ld\n",
-					__func__, mem, action, action);
+					__func__, mbs, action, action);
 			ret = -EINVAL;
 	}
 

@@ -238,19 +252,40 @@
 static int memory_block_change_state(struct memory_block *mem,
 		unsigned long to_state, unsigned long from_state_req)
 {
+	struct memory_block_section *mbs;
+	struct list_head *pos;
 	int ret = 0;
+
 	mutex_lock(&mem->state_mutex);
 

drivers/base/memory.c line 263

Quoted 6 lines

drivers/base/memory.c +3 -3

-	if (mem->state != from_state_req) {
-		ret = -EINVAL;
-		goto out;
+	list_for_each(pos, &mem->sections) {
+		mbs = list_entry(pos, struct memory_block_section, next);
+

list_for_each_entry() ?

drivers/base/memory.c line 270

Quoted 8 lines

drivers/base/memory.c +7 -0

+		if (mbs->state != from_state_req)
+			continue;
+
+		ret = memory_block_action(mbs, to_state);
+		if (ret)
+			break;
+	}

Then, all actions will be affect all memory sections under memory block ? (Hmm..maybe have to see following patches ?)

drivers/base/memory.c line 276

Quoted 8 lines

drivers/base/memory.c +6 -0

+
+	if (ret) {
+		list_for_each(pos, &mem->sections) {
+			mbs = list_entry(pos, struct memory_block_section,
+					 next);
+

list_for_each_entry() ?

Quoted 14 lines

drivers/base/memory.c +7 -1

+			if (mbs->state == from_state_req)
+				continue;
+
+			if (memory_block_action(mbs, to_state))
+				printk(KERN_ERR "Could not re-enable memory "
+				       "section %lx\n", mbs->phys_index);
+		}
 	}
 
-	ret = memory_block_action(mem, to_state);
 	if (!ret)
 		mem->state = to_state;
 

drivers/base/memory.c line 296

Quoted 7 lines

drivers/base/memory.c +0 -1

-out:
 	mutex_unlock(&mem->state_mutex);
 	return ret;
 }

@@ -260,20 +295,15 @@
 		struct sysdev_attribute *attr, const char *buf, size_t count)
 {

Hmm, store_mem_state() ? What diff option are you using ?

Quoted 53 lines

drivers/base/memory.c +1 -34

 	struct memory_block *mem;
-	unsigned int phys_section_nr;
 	int ret = -EINVAL;
 
 	mem = container_of(dev, struct memory_block, sysdev);
-	phys_section_nr = mem->phys_index;
-
-	if (!present_section_nr(phys_section_nr))
-		goto out;
 
 	if (!strncmp(buf, "online", min((int)count, 6)))
 		ret = memory_block_change_state(mem, MEM_ONLINE, MEM_OFFLINE);
 	else if(!strncmp(buf, "offline", min((int)count, 7)))
 		ret = memory_block_change_state(mem, MEM_OFFLINE, MEM_ONLINE);
-out:
+
 	if (ret)
 		return ret;
 	return count;

@@ -435,39 +465,6 @@
 	return 0;
 }
 
-static int add_memory_block(int nid, struct mem_section *section,
-			unsigned long state, enum mem_add_context context)
-{
-	struct memory_block *mem = kzalloc(sizeof(*mem), GFP_KERNEL);
-	unsigned long start_pfn;
-	int ret = 0;
-
-	if (!mem)
-		return -ENOMEM;
-
-	mem->phys_index = __section_nr(section);
-	mem->state = state;
-	mutex_init(&mem->state_mutex);
-	start_pfn = section_nr_to_pfn(mem->phys_index);
-	mem->phys_device = arch_get_memory_phys_device(start_pfn);
-
-	ret = register_memory(mem, section);
-	if (!ret)
-		ret = mem_create_simple_file(mem, phys_index);
-	if (!ret)
-		ret = mem_create_simple_file(mem, state);
-	if (!ret)
-		ret = mem_create_simple_file(mem, phys_device);
-	if (!ret)
-		ret = mem_create_simple_file(mem, removable);
-	if (!ret) {
-		if (context == HOTPLUG)
-			ret = register_mem_sect_under_node(mem, nid);

drivers/base/memory.c line 470

Quoted 5 lines

drivers/base/memory.c +0 -5

-	}
-
-	return ret;
-}
-

please divide clean-up and logic-change patches into their own..

Quoted 11 lines

drivers/base/memory.c +1 -0

 /*
  * For now, we have a linear search to go find the appropriate
  * memory_block corresponding to a particular phys_index. If

@@ -482,12 +479,13 @@
 	struct sys_device *sysdev;
 	struct memory_block *mem;
 	char name[sizeof(MEMORY_CLASS_NAME) + 9 + 1];
+	int block_id = base_memory_block_id(__section_nr(section));
 

drivers/base/memory.c line 488

Quoted 6 lines

drivers/base/memory.c +1 -1

 	/*
 	 * This only works because we know that section == sysdev->id
 	 * slightly redundant with sysdev_register()
 	 */
-	sprintf(&name[0], "%s%d", MEMORY_CLASS_NAME, __section_nr(section));
+	sprintf(&name[0], "%s%d", MEMORY_CLASS_NAME, block_id);

Hmm. Then, the user has to calculate block-id in addtion to section-id. Can't we use memory block name as memory%d-%d(start-end) ?

Quoted 27 lines

drivers/base/memory.c +17 -0

 	kobj = kset_find_obj(&memory_sysdev_class.kset, name);
 	if (!kobj)

@@ -498,19 +496,97 @@
 
 	return mem;
 }
+static int add_mem_block_section(struct memory_block *mem,
+				 int section_nr, unsigned long state)
+{
+	struct memory_block_section *mbs;
+
+	mbs = kzalloc(sizeof(*mbs), GFP_KERNEL);
+	if (!mbs)
+		return -ENOMEM;
+
+	mbs->phys_index = section_nr;
+	mbs->state = state;
+
+	list_add(&mbs->next, &mem->sections);
+	return 0;
+}
+
+static int add_memory_block(int nid, struct mem_section *section,

drivers/base/memory.c line 521

Quoted 6 lines

drivers/base/memory.c +6 -0

+			unsigned long state, enum mem_add_context context)
+{
+	struct memory_block *mem;
+	int ret = 0;
+
+	mem = find_memory_block(section);

I guess you need to add changes to find_memory_block. section-ID != block-ID.

Quoted 1 line

drivers/base/memory.c +1 -0

+	int block_id = base_memory_block_id(__section_nr(section));

drivers/base/memory.c line 534

Quoted 12 lines

drivers/base/memory.c +12 -0

+	if (!mem) {
+		unsigned long start_pfn;
+
+		mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+		if (!mem)
+			return -ENOMEM;
+
+		mem->state = state;
+		mutex_init(&mem->state_mutex);
+		start_pfn = section_nr_to_pfn(__section_nr(section));
+		mem->phys_device = arch_get_memory_phys_device(start_pfn);
+		INIT_LIST_HEAD(&mem->sections);

I'm not sure this phys_device is properly set in any arch...but this changes in granule will not affect ?

Quoted 29 lines

drivers/base/memory.c +23 -0

+
+		ret = register_memory(mem, section);
+		if (!ret)
+			ret = mem_create_simple_file(mem, phys_index);
+		if (!ret)
+			ret = mem_create_simple_file(mem, state);
+		if (!ret)
+			ret = mem_create_simple_file(mem, phys_device);
+		if (!ret)
+			ret = mem_create_simple_file(mem, removable);
+		if (!ret) {
+			if (context == HOTPLUG)
+				ret = register_mem_sect_under_node(mem, nid);
+		}
+	} else {
+		kobject_put(&mem->sysdev.kobj);
+	}
+
+	if (!ret)
+		ret = add_mem_block_section(mem, __section_nr(section), state);
+
+	return ret;
+}
 
 
 
 
 

drivers/base/memory.c line 572

Quoted 10 lines

drivers/base/memory.c +3 -0

 
 int remove_memory_block(unsigned long node_id, struct mem_section *section,
 		int phys_device)
 {
 	struct memory_block *mem;
+	struct memory_block_section *mbs;
+	struct list_head *pos, *tmp;
+	int section_nr = __section_nr(section);
 
 	mem = find_memory_block(section);

ditto.

drivers/base/memory.c

Quoted 12 lines

drivers/base/memory.c +6 -6

-	unregister_mem_sect_under_nodes(mem);
-	mem_remove_simple_file(mem, phys_index);
-	mem_remove_simple_file(mem, state);
-	mem_remove_simple_file(mem, phys_device);
-	mem_remove_simple_file(mem, removable);
-	unregister_memory(mem, section);
+	mutex_lock(&mem->state_mutex);
+
+	/* remove the specified section */
+	list_for_each_safe(pos, tmp, &mem->sections) {
+		mbs = list_entry(pos, struct memory_block_section, next);
+

list_for_each_entry_safe ?

Quoted 107 lines

drivers/base/memory.c +39 -0

+		if (mbs->phys_index == section_nr) {
+			list_del(&mbs->next);
+			kfree(mbs);
+		}
+	}
+
+	mutex_unlock(&mem->state_mutex);
+
+	if (list_empty(&mem->sections)) {
+		unregister_mem_sect_under_nodes(mem);
+		mem_remove_simple_file(mem, phys_index);
+		mem_remove_simple_file(mem, state);
+		mem_remove_simple_file(mem, phys_device);
+		mem_remove_simple_file(mem, removable);
+		unregister_memory(mem);
+		kfree(mem);
+	}
 
 	return 0;
 }

@@ -532,6 +608,24 @@
 	return remove_memory_block(0, section, 0);
 }
 
+u32 __weak memory_block_size(void)
+{
+	return MIN_MEMORY_BLOCK_SIZE;
+}
+
+static u32 get_memory_block_size(void)
+{
+	u32 blk_sz;
+
+	blk_sz = memory_block_size();
+
+	/* Validate blk_sz is a power of 2 and not less than section size */
+	if ((blk_sz & (blk_sz - 1)) || (blk_sz < MIN_MEMORY_BLOCK_SIZE))
+		blk_sz = MIN_MEMORY_BLOCK_SIZE;
+
+	return blk_sz;
+}
+
 /*
  * Initialize the sysfs support for memory devices...
  */

@@ -540,12 +634,16 @@
 	unsigned int i;
 	int ret;
 	int err;
+	int block_sz;
 
 	memory_sysdev_class.kset.uevent_ops = &memory_uevent_ops;
 	ret = sysdev_class_register(&memory_sysdev_class);
 	if (ret)
 		goto out;
 
+	block_sz = get_memory_block_size();
+	sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
+
 	/*
 	 * Create entries for memory sections that were found
 	 * during boot and have been initialized

Index: linux-2.6/include/linux/memory.h
===================================================================

include/linux/memory.h +9 -3

@@ -19,9 +19,15 @@
 #include <linux/node.h>
 #include <linux/compiler.h>
 #include <linux/mutex.h>
+#include <linux/list.h>
 
-struct memory_block {
+struct memory_block_section {
+	unsigned long state;
 	unsigned long phys_index;
+	struct list_head next;
+};
+
+struct memory_block {
 	unsigned long state;
 	/*
 	 * This serializes all state change requests.  It isn't

@@ -34,6 +40,7 @@
 	void *hw;			/* optional pointer to fw/hw data */
 	int (*phys_callback)(struct memory_block *);
 	struct sys_device sysdev;
+	struct list_head sections;
 };
 
 int arch_get_memory_phys_device(unsigned long start_pfn);

@@ -113,7 +120,7 @@
 extern int remove_memory_block(unsigned long, struct mem_section *, int);
 extern int memory_notify(unsigned long val, void *v);
 extern int memory_isolate_notify(unsigned long val, void *v);
-extern struct memory_block *find_memory_block(unsigned long);
+extern struct memory_block *find_memory_block(struct mem_section *);
 extern int memory_is_hidden(struct mem_section *);
 #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
 enum mem_add_context { BOOT, HOTPLUG };
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

drivers/base/memory.c +3 -4

@@ -123,13 +130,20 @@
 static ssize_t show_mem_removable(struct sys_device *dev,
 			struct sysdev_attribute *attr, char *buf)
 {
-	unsigned long start_pfn;
-	int ret;
-	struct memory_block *mem =
-		container_of(dev, struct memory_block, sysdev);
+	struct list_head *pos, *tmp;
+	struct memory_block *mem;
+	int ret = 1;

drivers/base/memory.c +3 -5

-	start_pfn = section_nr_to_pfn(mem->phys_index);
-	ret = is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
 	return sprintf(buf, "%d\n", ret);
 }
 
 

@@ -238,19 +252,40 @@
 static int memory_block_change_state(struct memory_block *mem,
 		unsigned long to_state, unsigned long from_state_req)
 {
+	struct memory_block_section *mbs;
+	struct list_head *pos;
 	int ret = 0;
+
 	mutex_lock(&mem->state_mutex);
 
-	if (mem->state != from_state_req) {
-		ret = -EINVAL;
-		goto out;

(quoted diff) +15 -2

+	if (ret) {
+		list_for_each(pos, &mem->sections) {
+			mbs = list_entry(pos, struct memory_block_section,
+					 next);
+
+			if (mbs->state == from_state_req)
+				continue;
+
+			if (memory_block_action(mbs, to_state))
+				printk(KERN_ERR "Could not re-enable memory "
+				       "section %lx\n", mbs->phys_index);
+		}
 	}
 
-	ret = memory_block_action(mem, to_state);
 	if (!ret)
 		mem->state = to_state;
 
-out:
 	mutex_unlock(&mem->state_mutex);
 	return ret;
 }

@@ -498,19 +496,97 @@
 
 	return mem;
 }
+static int add_mem_block_section(struct memory_block *mem,
+				 int section_nr, unsigned long state)
+{

(quoted diff) +2 -0

+	return 0;
+}

Quoted 17 lines · Nathan Fontenot

drivers/base/memory.c +9 -4

@@ -123,13 +130,20 @@
 static ssize_t show_mem_removable(struct sys_device *dev,
 			struct sysdev_attribute *attr, char *buf)
 {
-	unsigned long start_pfn;
-	int ret;
-	struct memory_block *mem =
-		container_of(dev, struct memory_block, sysdev);
+	struct list_head *pos, *tmp;
+	struct memory_block *mem;
+	int ret = 1;
+
+	mem = container_of(dev, struct memory_block, sysdev);
+	list_for_each_safe(pos, tmp, &mem->sections) {
+		struct memory_block_section *mbs;
+		unsigned long start_pfn;
+

drivers/base/memory.c line 145

Quoted 4 lines

drivers/base/memory.c +4 -0

+		mbs = list_entry(pos, struct memory_block_section, next);
+		start_pfn = section_nr_to_pfn(mbs->phys_index);
+		ret &= is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
+	}

I don't see you deleting anyting from the list in this loop. Why do you need to use list_for_each_safe? That won't protect you if someone else is messing with the list.

Quoted 29 lines

drivers/base/memory.c +10 -5

-	start_pfn = section_nr_to_pfn(mem->phys_index);
-	ret = is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
 	return sprintf(buf, "%d\n", ret);
 }
 
 

@@ -238,19 +252,40 @@
 static int memory_block_change_state(struct memory_block *mem,
 		unsigned long to_state, unsigned long from_state_req)
 {
+	struct memory_block_section *mbs;
+	struct list_head *pos;
 	int ret = 0;
+
 	mutex_lock(&mem->state_mutex);
 
-	if (mem->state != from_state_req) {
-		ret = -EINVAL;
-		goto out;
+	list_for_each(pos, &mem->sections) {
+		mbs = list_entry(pos, struct memory_block_section, next);
+
+		if (mbs->state != from_state_req)
+			continue;
+
+		ret = memory_block_action(mbs, to_state);

Review line 270

Quoted 3 lines

(quoted diff) +3 -0

+		if (ret)
+			break;
+	}

Would it be better here to loop through all the sections and ensure they are in the proper state first before starting to change the state of any of them? Then you could easily return -EINVAL if one or more is in the incorrect state and wouldn't need to the code below.

Quoted 38 lines

(quoted diff) +21 -2

+	if (ret) {
+		list_for_each(pos, &mem->sections) {
+			mbs = list_entry(pos, struct memory_block_section,
+					 next);
+
+			if (mbs->state == from_state_req)
+				continue;
+
+			if (memory_block_action(mbs, to_state))
+				printk(KERN_ERR "Could not re-enable memory "
+				       "section %lx\n", mbs->phys_index);
+		}
 	}
 
-	ret = memory_block_action(mem, to_state);
 	if (!ret)
 		mem->state = to_state;
 
-out:
 	mutex_unlock(&mem->state_mutex);
 	return ret;
 }

@@ -498,19 +496,97 @@
 
 	return mem;
 }
+static int add_mem_block_section(struct memory_block *mem,
+				 int section_nr, unsigned long state)
+{
+	struct memory_block_section *mbs;
+
+	mbs = kzalloc(sizeof(*mbs), GFP_KERNEL);
+	if (!mbs)
+		return -ENOMEM;
+

Review line 511

Quoted 4 lines

(quoted diff) +4 -0

+	mbs->phys_index = section_nr;
+	mbs->state = state;
+
+	list_add(&mbs->next, &mem->sections);

I don't think there is sufficient protection for this list. Don't we need to be holding a lock of some sort when adding/deleting/iterating through this list?

Quoted 3 lines

(quoted diff) +2 -0

+	return 0;
+}

This patch moves the register/unregister_memory routines to avoid a forward declaration. It also moves the sysfs file creation and deletion for each directory into the register/ unregister routines to avoid duplicating it with these updates.

Signed-off-by Nathan Fontenot <[email protected]>

---
 drivers/base/memory.c |   93 +++++++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 45 deletions(-)

Index: linux-2.6/drivers/base/memory.c
===================================================================

drivers/base/memory.c +1 -25

@@ -87,31 +87,6 @@
 EXPORT_SYMBOL(unregister_memory_isolate_notifier);
 
 /*
- * register_memory - Setup a sysfs device for a memory block
- */
-static
-int register_memory(struct memory_block *memory, struct mem_section *section)
-{
-	int error;
-
-	memory->sysdev.cls = &memory_sysdev_class;
-	memory->sysdev.id = __section_nr(section);
-
-	error = sysdev_register(&memory->sysdev);
-	return error;
-}
-
-static void
-unregister_memory(struct memory_block *memory)
-{
-	BUG_ON(memory->sysdev.cls != &memory_sysdev_class);
-
-	/* drop the ref. we got in remove_memory_block() */
-	kobject_put(&memory->sysdev.kobj);
-	sysdev_unregister(&memory->sysdev);
-}
-
-/*
  * use this as the physical section index that this memsection
  * uses.
  */

@@ -346,6 +321,53 @@
 	sysdev_remove_file(&mem->sysdev, &attr_##attr_name)
 
 /*
+ * register_memory - Setup a sysfs device for a memory block

Quoted 49 lines · On Mon, 12 Jul 2010 10:44:10 -0500 Nathan Fontenot <[email protected]> wrote:

This patch moves the register/unregister_memory routines to avoid a forward declaration. It also moves the sysfs file creation and deletion for each directory into the register/ unregister routines to avoid duplicating it with these updates.

Signed-off-by Nathan Fontenot <[email protected]>

---
 drivers/base/memory.c |   93 +++++++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 45 deletions(-)

Index: linux-2.6/drivers/base/memory.c
===================================================================

drivers/base/memory.c +0 -25

@@ -87,31 +87,6 @@
 EXPORT_SYMBOL(unregister_memory_isolate_notifier);
 
 /*
- * register_memory - Setup a sysfs device for a memory block
- */
-static
-int register_memory(struct memory_block *memory, struct mem_section *section)
-{
-	int error;
-
-	memory->sysdev.cls = &memory_sysdev_class;
-	memory->sysdev.id = __section_nr(section);
-
-	error = sysdev_register(&memory->sysdev);
-	return error;
-}
-
-static void
-unregister_memory(struct memory_block *memory)
-{
-	BUG_ON(memory->sysdev.cls != &memory_sysdev_class);
-
-	/* drop the ref. we got in remove_memory_block() */
-	kobject_put(&memory->sysdev.kobj);
-	sysdev_unregister(&memory->sysdev);
-}
-
-/*
  * use this as the physical section index that this memsection
  * uses.
  */

@@ -346,6 +321,53 @@
 	sysdev_remove_file(&mem->sysdev, &attr_##attr_name)
 

drivers/base/memory.c line 334

Quoted 12 lines

drivers/base/memory.c +11 -0

 /*
+ * register_memory - Setup a sysfs device for a memory block
+ */
+static
+int register_memory(struct memory_block *memory, struct mem_section *section,
+		    int nid, enum mem_add_context context)
+{
+	int ret;
+
+	memory->sysdev.cls = &memory_sysdev_class;
+	memory->sysdev.id = __section_nr(section);
+

Why not block-ID but section-ID ?

-Kame

This patch introduces the new 'split' file in each memory sysfs directory and the associated routines needed to handle splitting a directory.

Signed-off-by; Nathan Fontenot <[email protected]>
---

 drivers/base/memory.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/base/memory.c
===================================================================

drivers/base/memory.c +32 -0

@@ -32,6 +32,9 @@
 
 static int sections_per_block;
 
+static int register_memory(struct memory_block *, struct mem_section *,
+			   int, enum mem_add_context);
+
 static inline int base_memory_block_id(int section_nr)
 {
 	return (section_nr / sections_per_block) * sections_per_block;

@@ -309,11 +312,100 @@
 	return sprintf(buf, "%d\n", mem->phys_device);
 }
 
+static void update_memory_block_phys_indexes(struct memory_block *mem)
+{
+	struct list_head *pos;
+	struct memory_block_section *mbs;
+	unsigned long min_index = 0xffffffff;
+	unsigned long max_index = 0;
+
+	list_for_each(pos, &mem->sections) {
+		mbs = list_entry(pos, struct memory_block_section, next);
+
+		if (mbs->phys_index < min_index)
+			min_index = mbs->phys_index;
+
+		if (mbs->phys_index > max_index)
+			max_index = mbs->phys_index;
+	}
+
+	mem->start_phys_index = min_index;
+	mem->end_phys_index = max_index;
+}
+
+static ssize_t
+store_mem_split_block(struct sys_device *dev, struct sysdev_attribute *attr,
+		      const char *buf, size_t count)
+{
+	struct memory_block *mem, *new_mem_blk;
+	struct memory_block_section *mbs;
+	struct list_head *pos, *tmp;
+	struct mem_section *section;

drivers/base/memory.c +21 -0

+
+	mutex_lock(&mem->state_mutex);
+
+	list_for_each(pos, &mem->sections) {
+		mbs = list_entry(pos, struct memory_block_section, next);
+
+		total_scns++;
+
+		if (min_scn_nr > mbs->phys_index)
+			min_scn_nr = mbs->phys_index;
+
+		if (max_scn_nr < mbs->phys_index)
+			max_scn_nr = mbs->phys_index;
+	}
+
+	new_mem_blk = kzalloc(sizeof(*new_mem_blk), GFP_KERNEL);
+	if (!new_mem_blk)
+		return -ENOMEM;
+
+	mutex_init(&new_mem_blk->state_mutex);
+	INIT_LIST_HEAD(&new_mem_blk->sections);

Quoted 6 lines · On Mon, 12 Jul 2010 10:45:25 -0500 Nathan Fontenot <[email protected]> wrote:

This patch introduces the new 'split' file in each memory sysfs directory and the associated routines needed to handle splitting a directory.

Signed-off-by; Nathan Fontenot <[email protected]>
---

pleae check diff option...

Quoted 54 lines

 drivers/base/memory.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/base/memory.c
===================================================================

drivers/base/memory.c +36 -0

@@ -32,6 +32,9 @@
 
 static int sections_per_block;
 
+static int register_memory(struct memory_block *, struct mem_section *,
+			   int, enum mem_add_context);
+
 static inline int base_memory_block_id(int section_nr)
 {
 	return (section_nr / sections_per_block) * sections_per_block;

@@ -309,11 +312,100 @@
 	return sprintf(buf, "%d\n", mem->phys_device);
 }
 
+static void update_memory_block_phys_indexes(struct memory_block *mem)
+{
+	struct list_head *pos;
+	struct memory_block_section *mbs;
+	unsigned long min_index = 0xffffffff;
+	unsigned long max_index = 0;
+
+	list_for_each(pos, &mem->sections) {
+		mbs = list_entry(pos, struct memory_block_section, next);
+
+		if (mbs->phys_index < min_index)
+			min_index = mbs->phys_index;
+
+		if (mbs->phys_index > max_index)
+			max_index = mbs->phys_index;
+	}
+
+	mem->start_phys_index = min_index;
+	mem->end_phys_index = max_index;
+}
+
+static ssize_t
+store_mem_split_block(struct sys_device *dev, struct sysdev_attribute *attr,
+		      const char *buf, size_t count)
+{
+	struct memory_block *mem, *new_mem_blk;
+	struct memory_block_section *mbs;
+	struct list_head *pos, *tmp;
+	struct mem_section *section;
+	int min_scn_nr = 0;
+	int max_scn_nr = 0;
+	int total_scns = 0;
+	int new_blk_min, new_blk_total;

drivers/base/memory.c line 353

Quoted 6 lines

drivers/base/memory.c +6 -0

+	int ret = -EINVAL;
+
+	mem = container_of(dev, struct memory_block, sysdev);
+
+	if (list_is_singular(&mem->sections))
+		return -EINVAL;

What this means ?

Quoted 26 lines

drivers/base/memory.c +24 -0

+
+	mutex_lock(&mem->state_mutex);
+
+	list_for_each(pos, &mem->sections) {
+		mbs = list_entry(pos, struct memory_block_section, next);
+
+		total_scns++;
+
+		if (min_scn_nr > mbs->phys_index)
+			min_scn_nr = mbs->phys_index;
+
+		if (max_scn_nr < mbs->phys_index)
+			max_scn_nr = mbs->phys_index;
+	}
+
+	new_mem_blk = kzalloc(sizeof(*new_mem_blk), GFP_KERNEL);
+	if (!new_mem_blk)
+		return -ENOMEM;
+
+	mutex_init(&new_mem_blk->state_mutex);
+	INIT_LIST_HEAD(&new_mem_blk->sections);
+	new_mem_blk->state = mem->state;
+
+	mutex_lock(&new_mem_blk->state_mutex);

drivers/base/memory.c line 384

Quoted 7 lines

drivers/base/memory.c +7 -0

+
+	new_blk_total = total_scns / 2;
+	new_blk_min = max_scn_nr - new_blk_total + 1;
+
+	section = __nr_to_section(new_blk_min);
+	ret = register_memory(new_mem_blk, section, 0, HOTPLUG);
+

'nid' is always 0 ?

And for what purpose this interface is ? Does this split memory block into 2 pieces of the same size ?? sounds __very__ strange interface to me.

If this is necessary, I hope move the whole things to configfs rather than something tricky.

Bye.
-Kame

Quoted 3 lines

And for what purpose this interface is ? Does this split memory block into 2 pieces of the same size ?? sounds __very__ strange interface to me.

Yes, this splits the memory_block into two blocks of the same size. This was suggested as something we may want to do. From ppc perspective I am not sure we would use this.

The split functionality is not required. The main goal of the patch set is to reduce the number of memory sysfs directories created. From a ppc perspective the split functionality is not really needed.

Quoted 4 lines · On Tue, 13 Jul 2010 10:51:58 -0500 Nathan Fontenot <[email protected]> wrote:

Quoted 3 lines

And for what purpose this interface is ? Does this split memory block into 2 pieces of the same size ?? sounds __very__ strange interface to me.

Quoted 8 lines

Yes, this splits the memory_block into two blocks of the same size. This was suggested as something we may want to do. From ppc perspective I am not sure we would use this.

The split functionality is not required. The main goal of the patch set is to reduce the number of memory sysfs directories created. From a ppc perspective the split functionality is not really needed.

Okay, this is an offer from me.

1. I think you can add an boot option as "don't create memory sysfs". please do.

2. I'd like to write a configfs module for handling memory hotplug even when sysfs directroy is not created.
Because configfs support rmdir/mkdir, the user (ppc's daemon?) has to do

When offlining section X.
# insmod configfs_memory.ko
# mount -t configfs none /configfs
# mkdir /configfs/memoryX
# echo offline > /configfs/memoryX/state
# rmdir /configfs/memoryX

And making this operation as the default bahavior for all arch's memory hotplug may be better...

Dave, how do you think ? Because ppc guys uses "probe" interface already, this can be handled... no ?

One problem is that I don't have enough knowledge about configfs..it seems complex.

Quoted 16 lines · KAMEZAWA Hiroyuki

Quoted 4 lines · On Tue, 13 Jul 2010 10:51:58 -0500 Nathan Fontenot <[email protected]> wrote:

Quoted 3 lines

And for what purpose this interface is ? Does this split memory block into 2 pieces of the same size ?? sounds __very__ strange interface to me.

Quoted 8 lines

Yes, this splits the memory_block into two blocks of the same size. This was suggested as something we may want to do. From ppc perspective I am not sure we would use this.

The split functionality is not required. The main goal of the patch set is to reduce the number of memory sysfs directories created. From a ppc perspective the split functionality is not really needed.

Quoted 4 lines

Okay, this is an offer from me.

1. I think you can add an boot option as "don't create memory sysfs". please do.

I posted a patch to do that a week or so ago, it didn't go over very well.

Quoted 15 lines

2. I'd like to write a configfs module for handling memory hotplug even when sysfs directroy is not created.
Because configfs support rmdir/mkdir, the user (ppc's daemon?) has to do

When offlining section X.
# insmod configfs_memory.ko
# mount -t configfs none /configfs
# mkdir /configfs/memoryX
# echo offline > /configfs/memoryX/state
# rmdir /configfs/memoryX

And making this operation as the default bahavior for all arch's memory hotplug may be better...

Quoted 2 lines

Dave, how do you think ? Because ppc guys uses "probe" interface already, this can be handled... no ?

ppc would still require the existance of the 'probe' interface.

Are you objecting to the 'split' functionality?

If so I do not see any reason from ppc
perspective that it is needed. This was something Dave suggested, unless I am missing something.

Since ppc needs the 'probe' interface in sysfs, and for ppc having mutliple memory_block_sections reside under a single memory_block makes memory hotplug simpler. On ppc we do emory hotplug operations on an LMB size basis. With my patches this now lets us set each memory_block to span an LMB's worth of memory. Now we could do emory hotplug in a single operation instead of multiple operations to offline/online all of the memory sections in an LMB.

Quoted 39 lines · On Tue, 13 Jul 2010 22:18:03 -0500 Nathan Fontenot <[email protected]> wrote:

Quoted 16 lines · KAMEZAWA Hiroyuki

Quoted 4 lines · On Tue, 13 Jul 2010 10:51:58 -0500 Nathan Fontenot <[email protected]> wrote:

Quoted 3 lines

And for what purpose this interface is ? Does this split memory block into 2 pieces of the same size ?? sounds __very__ strange interface to me.

Quoted 8 lines

Yes, this splits the memory_block into two blocks of the same size. This was suggested as something we may want to do. From ppc perspective I am not sure we would use this.

The split functionality is not required. The main goal of the patch set is to reduce the number of memory sysfs directories created. From a ppc perspective the split functionality is not really needed.

Quoted 4 lines

Okay, this is an offer from me.

1. I think you can add an boot option as "don't create memory sysfs". please do.

I posted a patch to do that a week or so ago, it didn't go over very well.

Quoted 15 lines

2. I'd like to write a configfs module for handling memory hotplug even when sysfs directroy is not created.
Because configfs support rmdir/mkdir, the user (ppc's daemon?) has to do

When offlining section X.
# insmod configfs_memory.ko
# mount -t configfs none /configfs
# mkdir /configfs/memoryX
# echo offline > /configfs/memoryX/state
# rmdir /configfs/memoryX

And making this operation as the default bahavior for all arch's memory hotplug may be better...

Quoted 6 lines

Quoted 2 lines

Dave, how do you think ? Because ppc guys uses "probe" interface already, this can be handled... no ?

ppc would still require the existance of the 'probe' interface.

Are you objecting to the 'split' functionality?

yes.

Quoted 11 lines

If so I do not see any reason from ppc
perspective that it is needed. This was something Dave suggested, unless I am missing something.

Since ppc needs the 'probe' interface in sysfs, and for ppc having mutliple memory_block_sections reside under a single memory_block makes memory hotplug simpler. On ppc we do emory hotplug operations on an LMB size basis. With my patches this now lets us set each memory_block to span an LMB's worth of memory. Now we could do emory hotplug in a single operation instead of multiple operations to offline/online all of the memory sections in an LMB.

Why per-section memory offlining is provided is for allowing good success-rate of memory offlining. Because memory-hotplug has to "migrate or free" all used page under a section, possibility of memory unplug depends on usage of memory. If a section contains unmovable page(kernel page), we can't offline sectin.

For example, comparing
1. offlining 128MB of memory at once
2. offlining 8 chunks of 16MB memory
"2" can get very good possibility and system-busy time can be much reduced.

IIUC, ppc's 1st requirement is "resizing" not "hot-removing some memory device", "2" is much welcomed. So, some fine-grained interface to section_size is appreciated. So, "multiple operations" is much better than single operation.

As I posted show/hide patch, I'm writing it in configfs. I think it meets IBM's requirements.
_But_, it's IBM's issue not Fujitsu's. So, final decistion will depend on you guys.

Anyway, I don't like a too fancy interface as "split".

2. I'd like to write a configfs module for handling memory hotplug even when sysfs directroy is not created.
Because configfs support rmdir/mkdir, the user (ppc's daemon?) has to do

When offlining section X.
# insmod configfs_memory.ko
# mount -t configfs none /configfs
# mkdir /configfs/memoryX
# echo offline > /configfs/memoryX/state
# rmdir /configfs/memoryX

And making this operation as the default bahavior for all arch's memory hotplug may be better...

Dave, how do you think ? Because ppc guys uses "probe" interface already, this can be handled... no ?

Quoted 11 lines · KAMEZAWA Hiroyuki

2. I'd like to write a configfs module for handling memory hotplug even when sysfs directroy is not created.
Because configfs support rmdir/mkdir, the user (ppc's daemon?) has to do

When offlining section X.
# insmod configfs_memory.ko
# mount -t configfs none /configfs
# mkdir /configfs/memoryX
# echo offline > /configfs/memoryX/state
# rmdir /configfs/memoryX

Quoted 5 lines

And making this operation as the default bahavior for all arch's memory hotplug may be better...

Dave, how do you think ? Because ppc guys uses "probe" interface already, this can be handled... no ?

I think creating a interface to duplicate the existing sysfs one is a bad idea. I also think removing the existing sysfs one isn't feasible since there are users, and it's truly part of the ABI. So, I'm not really a fan on the configfs interface. :(

I really do think the sysfs interface is fixable. We should at least give it a good shot before largely duplicating its functionality.

This set of patches de-couples the idea that there is a single directory in sysfs for each memory section.

Quoted 2 lines · Nathan Fontenot

This set of patches de-couples the idea that there is a single directory in sysfs for each memory section.

Any reason you didn't cc: the sysfs maintainer on these patches? If not, I'll gladly ignore them...

(hint, scripts/get_maintainer.pl is your friend...)

greg k-h