Re: [PATCH 1/2] mem-hotplug: Introduce {un}lock_memory_hotplug()

[Hugh Dickins]

On Wed, 1 Dec 2010, KOSAKI Motohiro wrote:

> Now, hwpoison are using lock_system_sleep() for prevent a race
> with memory hotplug. However lock_system_sleep() is no-op if
> CONFIG_HIBERNATION=n. Therefore we need new lock.
> 
> Cc: Andi Kleen <andi@xxxxxxxxxxxxxx>
> Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> Suggested-by: Hugh Dickins <hughd@xxxxxxxxxx>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>

Yes, that looks better than what I had had in mind (further abusing
pm_mutex).  I notice that if MEMORY_HOTPLUG is off but HIBERNATION on,
lock_memory_hotplug does nothing where lock_system_sleep did something;
but I don't think that's a problem at all.  Thanks.

Acked-by: Hugh Dickins <hughd@xxxxxxxxxx>

> ---
>  include/linux/memory_hotplug.h |    6 ++++++
>  mm/memory-failure.c            |    8 ++++----
>  mm/memory_hotplug.c            |   31 ++++++++++++++++++++++++-------
>  3 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 4307231..31c237a 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -161,6 +161,9 @@ extern void register_page_bootmem_info_node(struct pglist_data *pgdat);
>  extern void put_page_bootmem(struct page *page);
>  #endif
>  
> +void lock_memory_hotplug(void);
> +void unlock_memory_hotplug(void);
> +
>  #else /* ! CONFIG_MEMORY_HOTPLUG */
>  /*
>   * Stub functions for when hotplug is off
> @@ -192,6 +195,9 @@ static inline void register_page_bootmem_info_node(struct pglist_data *pgdat)
>  {
>  }
>  
> +static inline void lock_memory_hotplug(void) {}
> +static inline void unlock_memory_hotplug(void) {}
> +
>  #endif /* ! CONFIG_MEMORY_HOTPLUG */
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 1243241..46ab2c0 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -51,6 +51,7 @@
>  #include <linux/slab.h>
>  #include <linux/swapops.h>
>  #include <linux/hugetlb.h>
> +#include <linux/memory_hotplug.h>
>  #include "internal.h"
>  
>  int sysctl_memory_failure_early_kill __read_mostly = 0;
> @@ -1230,11 +1231,10 @@ static int get_any_page(struct page *p, unsigned long pfn, int flags)
>  		return 1;
>  
>  	/*
> -	 * The lock_system_sleep prevents a race with memory hotplug,
> -	 * because the isolation assumes there's only a single user.
> +	 * The lock_memory_hotplug prevents a race with memory hotplug.
>  	 * This is a big hammer, a better would be nicer.
>  	 */
> -	lock_system_sleep();
> +	lock_memory_hotplug();
>  
>  	/*
>  	 * Isolate the page, so that it doesn't get reallocated if it
> @@ -1264,7 +1264,7 @@ static int get_any_page(struct page *p, unsigned long pfn, int flags)
>  		ret = 1;
>  	}
>  	unset_migratetype_isolate(p);
> -	unlock_system_sleep();
> +	unlock_memory_hotplug();
>  	return ret;
>  }
>  
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 833e286..7549a01 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -34,6 +34,23 @@
>  
>  #include "internal.h"
>  
> +DEFINE_MUTEX(mem_hotplug_mutex);
> +
> +void lock_memory_hotplug(void)
> +{
> +	mutex_lock(&mem_hotplug_mutex);
> +
> +	/* for exclusive hibernation if CONFIG_HIBERNATION=y */
> +	lock_system_sleep();
> +}
> +
> +void unlock_memory_hotplug(void)
> +{
> +	unlock_system_sleep();
> +	mutex_unlock(&mem_hotplug_mutex);
> +}
> +
> +
>  /* add this memory to iomem resource */
>  static struct resource *register_memory_resource(u64 start, u64 size)
>  {
> @@ -491,7 +508,7 @@ int mem_online_node(int nid)
>  	pg_data_t	*pgdat;
>  	int	ret;
>  
> -	lock_system_sleep();
> +	lock_memory_hotplug();
>  	pgdat = hotadd_new_pgdat(nid, 0);
>  	if (pgdat) {
>  		ret = -ENOMEM;
> @@ -502,7 +519,7 @@ int mem_online_node(int nid)
>  	BUG_ON(ret);
>  
>  out:
> -	unlock_system_sleep();
> +	unlock_memory_hotplug();
>  	return ret;
>  }
>  
> @@ -514,7 +531,7 @@ int __ref add_memory(int nid, u64 start, u64 size)
>  	struct resource *res;
>  	int ret;
>  
> -	lock_system_sleep();
> +	lock_memory_hotplug();
>  
>  	res = register_memory_resource(start, size);
>  	ret = -EEXIST;
> @@ -561,7 +578,7 @@ error:
>  		release_memory_resource(res);
>  
>  out:
> -	unlock_system_sleep();
> +	unlock_memory_hotplug();
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(add_memory);
> @@ -789,7 +806,7 @@ static int offline_pages(unsigned long start_pfn,
>  	if (!test_pages_in_a_zone(start_pfn, end_pfn))
>  		return -EINVAL;
>  
> -	lock_system_sleep();
> +	lock_memory_hotplug();
>  
>  	zone = page_zone(pfn_to_page(start_pfn));
>  	node = zone_to_nid(zone);
> @@ -877,7 +894,7 @@ repeat:
>  	vm_total_pages = nr_free_pagecache_pages();
>  
>  	memory_notify(MEM_OFFLINE, &arg);
> -	unlock_system_sleep();
> +	unlock_memory_hotplug();
>  	return 0;
>  
>  failed_removal:
> @@ -888,7 +905,7 @@ failed_removal:
>  	undo_isolate_page_range(start_pfn, end_pfn);
>  
>  out:
> -	unlock_system_sleep();
> +	unlock_memory_hotplug();
>  	return ret;
>  }
>  
> -- 
> 1.6.5.2
--
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/