On Mon, Apr 08, 2013 at 01:37:12PM -0700, Andrew Morton wrote:
> 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.
I'm preparing a new version with this and the other changes you
gave me. Thank you!
Should I add the memory notifier code to mm/nommu.c too?
I'm guessing that if a system doesn't have an mmu that it also
won't be hotplugging memory.
> 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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>