Re: [PATCH v8 3/3] mm: reinititalise user and admin reserves ifmemory is added or removed

[Andrew Morton]

On Mon, 8 Apr 2013 15:07:38 -0400 Andrew Shewmaker <agshew@xxxxxxxxx> wrote:

> This patch alters the admin and user reserves of the previous patches 
> in this series when memory is added or removed.
> 
> If memory is added and the reserves have been eliminated or increased above
> the default max, then we'll trust the admin.
> 
> If memory is removed and there isn't enough free memory, then we
> need to reset the reserves.
> 
> Otherwise keep the reserve set by the admin.
> 
> The reserve reset code is the same as the reserve initialization code.
> 
> Does this sound reasonable to other people? I figured that hot removal
> with too large of memory in the reserves was the most important case 
> to get right.

Seems reasonable to me.

I don't understand the magic numbers 1<<13 and 1<<17.  How could I? 
Please add comments explaining how and why these were chosen.

Your patch adds 400 bytes of unusable code to the
CONFIG_MEMORY_HOTPLUG=n kernel.  We have a fix for that in the CPU
hotplug case: register_hotcpu_notifier().  Memory hotplug has its own
hotplug_memory_notifier() but a) it's broken and b) it just doesn't
work!  With my gcc-4.4.4, the unused functions are still included in
the .o file.

So I did this:

From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Subject: include/linux/memory.h: implement register_hotmemory_notifier()

When CONFIG_MEMORY_HOTPLUG=n, we don't want the memory-hotplug notifier
handlers to be included in the .o files, for space reasons.

The existing hotplug_memory_notifier() tries to handle this but testing
with gcc-4.4.4 shows that it doesn't work - the hotplug functions are
still present in the .o files.

So implement a new register_hotmemory_notifier() which is a copy of
register_hotcpu_notifier(), and which actually works as desired. 
hotplug_memory_notifier() and register_memory_notifier() callsites should
be converted to use this new register_hotmemory_notifier().

While we're there, let's repair the existing hotplug_memory_notifier(): it
simply stomps on the register_memory_notifier() return value, so
well-behaved code cannot check for errors.  Apparently non of the existing
callers were well-behaved :(

Cc: Andrew Shewmaker <agshew@xxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---

 include/linux/memory.h   |   15 ++++++++++++---
 include/linux/notifier.h |    5 ++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff -puN include/linux/memory.h~include-linux-memoryh-implement-register_hotmemory_notifier include/linux/memory.h
--- a/include/linux/memory.h~include-linux-memoryh-implement-register_hotmemory_notifier
+++ a/include/linux/memory.h
@@ -18,6 +18,7 @@
 #include <linux/node.h>
 #include <linux/compiler.h>
 #include <linux/mutex.h>
+#include <linux/notifier.h>
 
 #define MIN_MEMORY_BLOCK_SIZE     (1UL << SECTION_SIZE_BITS)
 
@@ -127,13 +128,21 @@ enum mem_add_context { BOOT, HOTPLUG };
 #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
 
 #ifdef CONFIG_MEMORY_HOTPLUG
-#define hotplug_memory_notifier(fn, pri) {			\
+#define hotplug_memory_notifier(fn, pri) ({			\
 	static __meminitdata struct notifier_block fn##_mem_nb =\
 		{ .notifier_call = fn, .priority = pri };	\
 	register_memory_notifier(&fn##_mem_nb);			\
-}
+})
+#define register_hotmemory_notifier(nb)		register_memory_notifier(nb)
+#define unregister_hotmemory_notifier(nb) 	unregister_memory_notifier(nb)
 #else
-#define hotplug_memory_notifier(fn, pri) do { } while (0)
+static inline int hotplug_memory_notifier(notifier_fn_t fn, int priority)
+{
+	return 0;
+}
+/* These aren't inline functions due to a GCC bug. */
+#define register_hotmemory_notifier(nb)    ({ (void)(nb); 0; })
+#define unregister_hotmemory_notifier(nb)  ({ (void)(nb); })
 #endif
 
 /*
diff -puN include/linux/notifier.h~include-linux-memoryh-implement-register_hotmemory_notifier include/linux/notifier.h
--- a/include/linux/notifier.h~include-linux-memoryh-implement-register_hotmemory_notifier
+++ a/include/linux/notifier.h
@@ -47,8 +47,11 @@
  * runtime initialization.
  */
 
+typedef	int (*notifier_fn_t)(struct notifier_block *nb,
+			unsigned long action, void *data);
+
 struct notifier_block {
-	int (*notifier_call)(struct notifier_block *, unsigned long, void *);
+	notifier_fn_t notifier_call;
 	struct notifier_block __rcu *next;
 	int priority;
 };
_


And then I changed your patch thusly:

--- a/mm/mmap.c~mm-reinititalise-user-and-admin-reserves-if-memory-is-added-or-removed-fix
+++ a/mm/mmap.c
@@ -3198,7 +3198,7 @@ static struct notifier_block reserve_mem
 
 int __meminit init_reserve_notifier(void)
 {
-	if (register_memory_notifier(&reserve_mem_nb))
+	if (register_hotmemory_notifier(&reserve_mem_nb))
 		printk("Failed registering memory add/remove notifier for admin reserve");
 
 	return 0;
_

and voila, no more bloat.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/