Thread (47 messages) 47 messages, 6 authors, 2013-02-07

Re: [PATCH v5 03/14] memory-hotplug: remove redundant codes

From: Kamezawa Hiroyuki <hidden>
Date: 2012-12-26 03:21:47
Also in: linux-acpi, linux-mm, linux-s390, linux-sh, lkml, sparclinux

(2012/12/24 21:09), Tang Chen wrote:
quoted hunk ↗ jump to hunk
From: Wen Congyang <redacted>

offlining memory blocks and checking whether memory blocks are offlined
are very similar. This patch introduces a new function to remove
redundant codes.

Signed-off-by: Wen Congyang <redacted>
---
  mm/memory_hotplug.c |  101 ++++++++++++++++++++++++++++-----------------------
  1 files changed, 55 insertions(+), 46 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index d43d97b..dbb04d8 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1381,20 +1381,14 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
  	return __offline_pages(start_pfn, start_pfn + nr_pages, 120 * HZ);
  }
  
-int remove_memory(u64 start, u64 size)
please add explanation of this function here. If (*func) returns val other than 0,
this function will fail and returns callback's return value...right ?

+static int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
+		void *arg, int (*func)(struct memory_block *, void *))
  {
  	struct memory_block *mem = NULL;
  	struct mem_section *section;
-	unsigned long start_pfn, end_pfn;
  	unsigned long pfn, section_nr;
  	int ret;
-	int return_on_error = 0;
-	int retry = 0;
-
-	start_pfn = PFN_DOWN(start);
-	end_pfn = start_pfn + PFN_DOWN(size);
  
-repeat:
Shouldn't we check lock is held here ? (VM_BUG_ON(!mutex_is_locked(&mem_hotplug_mutex);

quoted hunk ↗ jump to hunk
  	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
  		section_nr = pfn_to_section_nr(pfn);
  		if (!present_section_nr(section_nr))
@@ -1411,22 +1405,61 @@ repeat:
  		if (!mem)
  			continue;
  
-		ret = offline_memory_block(mem);
+		ret = func(mem, arg);
  		if (ret) {
-			if (return_on_error) {
-				kobject_put(&mem->dev.kobj);
-				return ret;
-			} else {
-				retry = 1;
-			}
+			kobject_put(&mem->dev.kobj);
+			return ret;
  		}
  	}
  
  	if (mem)
  		kobject_put(&mem->dev.kobj);
  
-	if (retry) {
-		return_on_error = 1;
+	return 0;
+}
+
+static int offline_memory_block_cb(struct memory_block *mem, void *arg)
+{
+	int *ret = arg;
+	int error = offline_memory_block(mem);
+
+	if (error != 0 && *ret == 0)
+		*ret = error;
+
+	return 0;
Always returns 0 and run through all mem blocks for scan-and-retry, right ?
You need explanation here !

+}
+
+static int is_memblock_offlined_cb(struct memory_block *mem, void *arg)
+{
+	int ret = !is_memblock_offlined(mem);
+
+	if (unlikely(ret))
+		pr_warn("removing memory fails, because memory "
+			"[%#010llx-%#010llx] is onlined\n",
+			PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)),
+			PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1))-1);
+
+	return ret;
+}
+
+int remove_memory(u64 start, u64 size)
+{
+	unsigned long start_pfn, end_pfn;
+	int ret = 0;
+	int retry = 1;
+
+	start_pfn = PFN_DOWN(start);
+	end_pfn = start_pfn + PFN_DOWN(size);
+
+repeat:
please explan why you repeat here .
quoted hunk ↗ jump to hunk
+	walk_memory_range(start_pfn, end_pfn, &ret,
+			  offline_memory_block_cb);
+	if (ret) {
+		if (!retry)
+			return ret;
+
+		retry = 0;
+		ret = 0;
  		goto repeat;
  	}
  
@@ -1444,37 +1477,13 @@ repeat:
  	 * memory blocks are offlined.
  	 */
  
-	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
-		section_nr = pfn_to_section_nr(pfn);
-		if (!present_section_nr(section_nr))
-			continue;
-
-		section = __nr_to_section(section_nr);
-		/* same memblock? */
-		if (mem)
-			if ((section_nr >= mem->start_section_nr) &&
-			    (section_nr <= mem->end_section_nr))
-				continue;
-
-		mem = find_memory_block_hinted(section, mem);
-		if (!mem)
-			continue;
-
-		ret = is_memblock_offlined(mem);
-		if (!ret) {
-			pr_warn("removing memory fails, because memory "
-				"[%#010llx-%#010llx] is onlined\n",
-				PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)),
-				PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1)) - 1);
-
-			kobject_put(&mem->dev.kobj);
-			unlock_memory_hotplug();
-			return ret;
-		}
please explain what you do here. confirming all memory blocks are offlined
before returning 0 ....right ? 
+	ret = walk_memory_range(start_pfn, end_pfn, NULL,
+				is_memblock_offlined_cb);
+	if (ret) {
+		unlock_memory_hotplug();
+		return ret;
  	}
  
-	if (mem)
-		kobject_put(&mem->dev.kobj);
  	unlock_memory_hotplug();
  
  	return 0;
Thanks,
-Kame
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help