Patchwork [RFC,2/12] memory-hogplug : check memory offline in offline_pages

login
register
mail settings
Submitter Yasuaki Ishimatsu
Date June 27, 2012, 5:44 a.m.
Message ID <4FEA9DB1.7010303@jp.fujitsu.com>
Download mbox | patch
Permalink /patch/167550/
State Not Applicable
Headers show

Comments

Yasuaki Ishimatsu - June 27, 2012, 5:44 a.m.
When offline_pages() is called to offlined memory, the function fails since
all memory has been offlined. In this case, the function should succeed.
The patch adds the check function into offline_pages().

CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

---
 drivers/base/memory.c  |   20 ++++++++++++++++++++
 include/linux/memory.h |    1 +
 mm/memory_hotplug.c    |    5 +++++
 3 files changed, 26 insertions(+)
Wen Congyang - June 27, 2012, 8:49 a.m.
At 06/27/2012 01:44 PM, Yasuaki Ishimatsu Wrote:
> When offline_pages() is called to offlined memory, the function fails since
> all memory has been offlined. In this case, the function should succeed.
> The patch adds the check function into offline_pages().

You miss such case: some pages are online, while some pages are offline.
offline_pages() will fail too in such case.

Thanks
Wen Congyang

> 
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Christoph Lameter <cl@linux.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> 
> ---
>  drivers/base/memory.c  |   20 ++++++++++++++++++++
>  include/linux/memory.h |    1 +
>  mm/memory_hotplug.c    |    5 +++++
>  3 files changed, 26 insertions(+)
> 
> Index: linux-3.5-rc4/drivers/base/memory.c
> ===================================================================
> --- linux-3.5-rc4.orig/drivers/base/memory.c	2012-06-26 13:28:16.726211752 +0900
> +++ linux-3.5-rc4/drivers/base/memory.c	2012-06-26 13:34:22.423639904 +0900
> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>  }
>  EXPORT_SYMBOL(unregister_memory_isolate_notifier);
> 
> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
> +{
> +	struct memory_block *mem;
> +	struct mem_section *section;
> +	unsigned long pfn, section_nr;
> +
> +	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> +		section_nr = pfn_to_section_nr(pfn);
> +		section = __nr_to_section(section_nr);
> +		mem = find_memory_block(section);
> +		if (!mem)
> +			continue;
> +		if (mem->state == MEM_OFFLINE)
> +			continue;
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * register_memory - Setup a sysfs device for a memory block
>   */
> Index: linux-3.5-rc4/include/linux/memory.h
> ===================================================================
> --- linux-3.5-rc4.orig/include/linux/memory.h	2012-06-25 04:53:04.000000000 +0900
> +++ linux-3.5-rc4/include/linux/memory.h	2012-06-26 13:34:22.424639891 +0900
> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>  extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>  							struct memory_block *);
>  extern struct memory_block *find_memory_block(struct mem_section *);
> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>  #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
>  enum mem_add_context { BOOT, HOTPLUG };
>  #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
> Index: linux-3.5-rc4/mm/memory_hotplug.c
> ===================================================================
> --- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
> +++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
> 
>  	lock_memory_hotplug();
> 
> +	if (memory_is_offline(start_pfn, end_pfn)) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	zone = page_zone(pfn_to_page(start_pfn));
>  	node = zone_to_nid(zone);
>  	nr_pages = end_pfn - start_pfn;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
Yasuaki Ishimatsu - June 28, 2012, 5:06 a.m.
Hi Wen,

2012/06/27 17:49, Wen Congyang wrote:
> At 06/27/2012 01:44 PM, Yasuaki Ishimatsu Wrote:
>> When offline_pages() is called to offlined memory, the function fails since
>> all memory has been offlined. In this case, the function should succeed.
>> The patch adds the check function into offline_pages().
> 
> You miss such case: some pages are online, while some pages are offline.
> offline_pages() will fail too in such case.

You are right. But current code fails, when the function is called to offline
memory. In this case, the function should succeed. So the patch confirms
whether the memory was offlined or not. And if memory has already been
offlined, offline_pages return 0.

Thanks,
Yasuaki Ishimatsu

> 
> Thanks
> Wen Congyang
> 
>>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> ---
>>   drivers/base/memory.c  |   20 ++++++++++++++++++++
>>   include/linux/memory.h |    1 +
>>   mm/memory_hotplug.c    |    5 +++++
>>   3 files changed, 26 insertions(+)
>>
>> Index: linux-3.5-rc4/drivers/base/memory.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/drivers/base/memory.c	2012-06-26 13:28:16.726211752 +0900
>> +++ linux-3.5-rc4/drivers/base/memory.c	2012-06-26 13:34:22.423639904 +0900
>> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>>   }
>>   EXPORT_SYMBOL(unregister_memory_isolate_notifier);
>>
>> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
>> +{
>> +	struct memory_block *mem;
>> +	struct mem_section *section;
>> +	unsigned long pfn, section_nr;
>> +
>> +	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +		section_nr = pfn_to_section_nr(pfn);
>> +		section = __nr_to_section(section_nr);
>> +		mem = find_memory_block(section);
>> +		if (!mem)
>> +			continue;
>> +		if (mem->state == MEM_OFFLINE)
>> +			continue;
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   /*
>>    * register_memory - Setup a sysfs device for a memory block
>>    */
>> Index: linux-3.5-rc4/include/linux/memory.h
>> ===================================================================
>> --- linux-3.5-rc4.orig/include/linux/memory.h	2012-06-25 04:53:04.000000000 +0900
>> +++ linux-3.5-rc4/include/linux/memory.h	2012-06-26 13:34:22.424639891 +0900
>> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>>   extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>>   							struct memory_block *);
>>   extern struct memory_block *find_memory_block(struct mem_section *);
>> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>>   #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
>>   enum mem_add_context { BOOT, HOTPLUG };
>>   #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
>> Index: linux-3.5-rc4/mm/memory_hotplug.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
>> +++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
>> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
>>
>>   	lock_memory_hotplug();
>>
>> +	if (memory_is_offline(start_pfn, end_pfn)) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>> +
>>   	zone = page_zone(pfn_to_page(start_pfn));
>>   	node = zone_to_nid(zone);
>>   	nr_pages = end_pfn - start_pfn;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
KOSAKI Motohiro - June 28, 2012, 5:26 a.m.
On Wed, Jun 27, 2012 at 1:44 AM, Yasuaki Ishimatsu
<isimatu.yasuaki@jp.fujitsu.com> wrote:
> When offline_pages() is called to offlined memory, the function fails since
> all memory has been offlined. In this case, the function should succeed.
> The patch adds the check function into offline_pages().

I don't understand your point. I think following misoperation should
fail. Otherwise
administrator have no way to know their fault.

$ echo offline > memoryN/state
$ echo offline > memoryN/state

In general, we don't like to ignore an error except the standard require it.

>
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Christoph Lameter <cl@linux.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>
> ---
>  drivers/base/memory.c  |   20 ++++++++++++++++++++
>  include/linux/memory.h |    1 +
>  mm/memory_hotplug.c    |    5 +++++
>  3 files changed, 26 insertions(+)
>
> Index: linux-3.5-rc4/drivers/base/memory.c
> ===================================================================
> --- linux-3.5-rc4.orig/drivers/base/memory.c    2012-06-26 13:28:16.726211752 +0900
> +++ linux-3.5-rc4/drivers/base/memory.c 2012-06-26 13:34:22.423639904 +0900
> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>  }
>  EXPORT_SYMBOL(unregister_memory_isolate_notifier);
>
> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)

I dislike this function name. 'memory' is too vague to me.


> +{
> +       struct memory_block *mem;
> +       struct mem_section *section;
> +       unsigned long pfn, section_nr;
> +
> +       for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> +               section_nr = pfn_to_section_nr(pfn);
> +               section = __nr_to_section(section_nr);
> +               mem = find_memory_block(section);

This seems to have strong sparse dependency.
Hm, I wonder why memory-hotplug.c can enable when X86_64_ACPI_NUMA.

# eventually, we can have this option just 'select SPARSEMEM'
config MEMORY_HOTPLUG
	bool "Allow for memory hot-add"
	depends on SPARSEMEM || X86_64_ACPI_NUMA


> +               if (!mem)
> +                       continue;
> +               if (mem->state == MEM_OFFLINE)
> +                       continue;
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
>  /*
>  * register_memory - Setup a sysfs device for a memory block
>  */
> Index: linux-3.5-rc4/include/linux/memory.h
> ===================================================================
> --- linux-3.5-rc4.orig/include/linux/memory.h   2012-06-25 04:53:04.000000000 +0900
> +++ linux-3.5-rc4/include/linux/memory.h        2012-06-26 13:34:22.424639891 +0900
> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>  extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>                                                        struct memory_block *);
>  extern struct memory_block *find_memory_block(struct mem_section *);
> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>  #define CONFIG_MEM_BLOCK_SIZE  (PAGES_PER_SECTION<<PAGE_SHIFT)
>  enum mem_add_context { BOOT, HOTPLUG };
>  #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
> Index: linux-3.5-rc4/mm/memory_hotplug.c
> ===================================================================
> --- linux-3.5-rc4.orig/mm/memory_hotplug.c      2012-06-26 13:28:16.743211538 +0900
> +++ linux-3.5-rc4/mm/memory_hotplug.c   2012-06-26 13:48:38.264940468 +0900
> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
>
>        lock_memory_hotplug();
>
> +       if (memory_is_offline(start_pfn, end_pfn)) {
> +               ret = 0;
> +               goto out;
> +       }
> +
>        zone = page_zone(pfn_to_page(start_pfn));
>        node = zone_to_nid(zone);
>        nr_pages = end_pfn - start_pfn;
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
KOSAKI Motohiro - June 28, 2012, 5:27 a.m.
On Thu, Jun 28, 2012 at 1:06 AM, Yasuaki Ishimatsu
<isimatu.yasuaki@jp.fujitsu.com> wrote:
> Hi Wen,
>
> 2012/06/27 17:49, Wen Congyang wrote:
>> At 06/27/2012 01:44 PM, Yasuaki Ishimatsu Wrote:
>>> When offline_pages() is called to offlined memory, the function fails since
>>> all memory has been offlined. In this case, the function should succeed.
>>> The patch adds the check function into offline_pages().
>>
>> You miss such case: some pages are online, while some pages are offline.
>> offline_pages() will fail too in such case.
>
> You are right. But current code fails, when the function is called to offline
> memory. In this case, the function should succeed. So the patch confirms
> whether the memory was offlined or not. And if memory has already been
> offlined, offline_pages return 0.

Can you please explain why the caller can't check it? I hope to avoid
an ignorance
as far as we can.
Yasuaki Ishimatsu - June 28, 2012, 6:01 a.m.
Hi Kosaki-san,

2012/06/28 14:27, KOSAKI Motohiro wrote:
> On Thu, Jun 28, 2012 at 1:06 AM, Yasuaki Ishimatsu
> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>> Hi Wen,
>>
>> 2012/06/27 17:49, Wen Congyang wrote:
>>> At 06/27/2012 01:44 PM, Yasuaki Ishimatsu Wrote:
>>>> When offline_pages() is called to offlined memory, the function fails since
>>>> all memory has been offlined. In this case, the function should succeed.
>>>> The patch adds the check function into offline_pages().
>>>
>>> You miss such case: some pages are online, while some pages are offline.
>>> offline_pages() will fail too in such case.
>>
>> You are right. But current code fails, when the function is called to offline
>> memory. In this case, the function should succeed. So the patch confirms
>> whether the memory was offlined or not. And if memory has already been
>> offlined, offline_pages return 0.
>
> Can you please explain why the caller can't check it? I hope to avoid
> an ignorance
> as far as we can.

Of course, caller side can check it. But there is a possibility that
offline_pages() is called by many functions. So I do not think that it
is good that all functions which call offline_pages() check it.

Thanks,
Yasuaki Ishimatsu
Yasuaki Ishimatsu - June 28, 2012, 6:51 a.m.
Hi Kosaki-san,

2012/06/28 14:26, KOSAKI Motohiro wrote:
> On Wed, Jun 27, 2012 at 1:44 AM, Yasuaki Ishimatsu
> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>> When offline_pages() is called to offlined memory, the function fails since
>> all memory has been offlined. In this case, the function should succeed.
>> The patch adds the check function into offline_pages().
>
> I don't understand your point. I think following misoperation should
> fail. Otherwise
> administrator have no way to know their fault.
>
> $ echo offline > memoryN/state
> $ echo offline > memoryN/state
>
> In general, we don't like to ignore an error except the standard require it.

I understood the intention of previous mail (why the caller can't check it? ).
I'll move memory_is_offline() to caller side.

>>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> ---
>>   drivers/base/memory.c  |   20 ++++++++++++++++++++
>>   include/linux/memory.h |    1 +
>>   mm/memory_hotplug.c    |    5 +++++
>>   3 files changed, 26 insertions(+)
>>
>> Index: linux-3.5-rc4/drivers/base/memory.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/drivers/base/memory.c    2012-06-26 13:28:16.726211752 +0900
>> +++ linux-3.5-rc4/drivers/base/memory.c 2012-06-26 13:34:22.423639904 +0900
>> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>>   }
>>   EXPORT_SYMBOL(unregister_memory_isolate_notifier);
>>
>> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
>
> I dislike this function name. 'memory' is too vague to me.

O.K.
I retry to change the name of the function.

>
>
>> +{
>> +       struct memory_block *mem;
>> +       struct mem_section *section;
>> +       unsigned long pfn, section_nr;
>> +
>> +       for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +               section_nr = pfn_to_section_nr(pfn);
>> +               section = __nr_to_section(section_nr);
>> +               mem = find_memory_block(section);
>
> This seems to have strong sparse dependency.

Thanks.
I will consider other CONFIG_.

Thanks.
Yasuaki Ishimatsu

> Hm, I wonder why memory-hotplug.c can enable when X86_64_ACPI_NUMA.
>
> # eventually, we can have this option just 'select SPARSEMEM'
> config MEMORY_HOTPLUG
> 	bool "Allow for memory hot-add"
> 	depends on SPARSEMEM || X86_64_ACPI_NUMA
>
>
>> +               if (!mem)
>> +                       continue;
>> +               if (mem->state == MEM_OFFLINE)
>> +                       continue;
>> +               return false;
>> +       }
>> +
>> +       return true;
>> +}
>> +
>>   /*
>>   * register_memory - Setup a sysfs device for a memory block
>>   */
>> Index: linux-3.5-rc4/include/linux/memory.h
>> ===================================================================
>> --- linux-3.5-rc4.orig/include/linux/memory.h   2012-06-25 04:53:04.000000000 +0900
>> +++ linux-3.5-rc4/include/linux/memory.h        2012-06-26 13:34:22.424639891 +0900
>> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>>   extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>>                                                         struct memory_block *);
>>   extern struct memory_block *find_memory_block(struct mem_section *);
>> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>>   #define CONFIG_MEM_BLOCK_SIZE  (PAGES_PER_SECTION<<PAGE_SHIFT)
>>   enum mem_add_context { BOOT, HOTPLUG };
>>   #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
>> Index: linux-3.5-rc4/mm/memory_hotplug.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/mm/memory_hotplug.c      2012-06-26 13:28:16.743211538 +0900
>> +++ linux-3.5-rc4/mm/memory_hotplug.c   2012-06-26 13:48:38.264940468 +0900
>> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
>>
>>         lock_memory_hotplug();
>>
>> +       if (memory_is_offline(start_pfn, end_pfn)) {
>> +               ret = 0;
>> +               goto out;
>> +       }
>> +
>>         zone = page_zone(pfn_to_page(start_pfn));
>>         node = zone_to_nid(zone);
>>         nr_pages = end_pfn - start_pfn;
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
Yasuaki Ishimatsu - June 28, 2012, 7:01 a.m.
Hi David,

2012/06/27 15:16, David Rientjes wrote:
> On Wed, 27 Jun 2012, Yasuaki Ishimatsu wrote:
>
>> Index: linux-3.5-rc4/mm/memory_hotplug.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
>> +++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
>> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
>>
>>   	lock_memory_hotplug();
>>
>> +	if (memory_is_offline(start_pfn, end_pfn)) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>> +
>>   	zone = page_zone(pfn_to_page(start_pfn));
>>   	node = zone_to_nid(zone);
>>   	nr_pages = end_pfn - start_pfn;
>
> Are there additional prerequisites for this patch?  Otherwise it changes
> the return value of offline_memory() which will now call
> acpi_memory_powerdown_device() in the acpi memhotplug case when disabling.
> Is that a problem?

I have understood there is a person who expects "offline_pages()" to fail
in this case by kosaki's comment. So I'll move memory_is_offline to caller
side.

Thanks,
Yasuaki Ishimatsu

>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
Jiang Liu - June 30, 2012, 3:46 p.m.
On 06/27/2012 01:44 PM, Yasuaki Ishimatsu wrote:
> When offline_pages() is called to offlined memory, the function fails since
> all memory has been offlined. In this case, the function should succeed.
> The patch adds the check function into offline_pages().
> 
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Christoph Lameter <cl@linux.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> 
> ---
>  drivers/base/memory.c  |   20 ++++++++++++++++++++
>  include/linux/memory.h |    1 +
>  mm/memory_hotplug.c    |    5 +++++
>  3 files changed, 26 insertions(+)
> 
> Index: linux-3.5-rc4/drivers/base/memory.c
> ===================================================================
> --- linux-3.5-rc4.orig/drivers/base/memory.c	2012-06-26 13:28:16.726211752 +0900
> +++ linux-3.5-rc4/drivers/base/memory.c	2012-06-26 13:34:22.423639904 +0900
> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>  }
>  EXPORT_SYMBOL(unregister_memory_isolate_notifier);
> 
> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
> +{
> +	struct memory_block *mem;
> +	struct mem_section *section;
> +	unsigned long pfn, section_nr;
> +
> +	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> +		section_nr = pfn_to_section_nr(pfn);
> +		section = __nr_to_section(section_nr);
Is it possible for __nr_to_section return NULL here?

> +		mem = find_memory_block(section);
> +		if (!mem)
> +			continue;
> +		if (mem->state == MEM_OFFLINE)
> +			continue;
> +		return false;
> +	}
> +
> +	return true;
> +}
Need a put_dev(&mem->dev) for the last memory block device handled before return.

> +
>  /*
>   * register_memory - Setup a sysfs device for a memory block
>   */
> Index: linux-3.5-rc4/include/linux/memory.h
> ===================================================================
> --- linux-3.5-rc4.orig/include/linux/memory.h	2012-06-25 04:53:04.000000000 +0900
> +++ linux-3.5-rc4/include/linux/memory.h	2012-06-26 13:34:22.424639891 +0900
> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>  extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>  							struct memory_block *);
>  extern struct memory_block *find_memory_block(struct mem_section *);
> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>  #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
>  enum mem_add_context { BOOT, HOTPLUG };
>  #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
> Index: linux-3.5-rc4/mm/memory_hotplug.c
> ===================================================================
> --- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
> +++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
> 
>  	lock_memory_hotplug();
> 
> +	if (memory_is_offline(start_pfn, end_pfn)) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	zone = page_zone(pfn_to_page(start_pfn));
>  	node = zone_to_nid(zone);
>  	nr_pages = end_pfn - start_pfn;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Jiang Liu - June 30, 2012, 3:51 p.m.
On 06/27/2012 01:44 PM, Yasuaki Ishimatsu wrote:
> When offline_pages() is called to offlined memory, the function fails since
> all memory has been offlined. In this case, the function should succeed.
> The patch adds the check function into offline_pages().
> 
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Christoph Lameter <cl@linux.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> 
> ---
>  drivers/base/memory.c  |   20 ++++++++++++++++++++
>  include/linux/memory.h |    1 +
>  mm/memory_hotplug.c    |    5 +++++
>  3 files changed, 26 insertions(+)
> 
> Index: linux-3.5-rc4/drivers/base/memory.c
> ===================================================================
> --- linux-3.5-rc4.orig/drivers/base/memory.c	2012-06-26 13:28:16.726211752 +0900
> +++ linux-3.5-rc4/drivers/base/memory.c	2012-06-26 13:34:22.423639904 +0900
> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>  }
>  EXPORT_SYMBOL(unregister_memory_isolate_notifier);
> 
> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
> +{
> +	struct memory_block *mem;
> +	struct mem_section *section;
> +	unsigned long pfn, section_nr;
> +
> +	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> +		section_nr = pfn_to_section_nr(pfn);
> +		section = __nr_to_section(section_nr);
> +		mem = find_memory_block(section);
Seems find_memory_block_hinted() is more efficient than find_memory_block() here.

> +		if (!mem)
> +			continue;
> +		if (mem->state == MEM_OFFLINE)
> +			continue;
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * register_memory - Setup a sysfs device for a memory block
>   */
> Index: linux-3.5-rc4/include/linux/memory.h
> ===================================================================
> --- linux-3.5-rc4.orig/include/linux/memory.h	2012-06-25 04:53:04.000000000 +0900
> +++ linux-3.5-rc4/include/linux/memory.h	2012-06-26 13:34:22.424639891 +0900
> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>  extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>  							struct memory_block *);
>  extern struct memory_block *find_memory_block(struct mem_section *);
> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>  #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
>  enum mem_add_context { BOOT, HOTPLUG };
>  #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
> Index: linux-3.5-rc4/mm/memory_hotplug.c
> ===================================================================
> --- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
> +++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
> 
>  	lock_memory_hotplug();
> 
> +	if (memory_is_offline(start_pfn, end_pfn)) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	zone = page_zone(pfn_to_page(start_pfn));
>  	node = zone_to_nid(zone);
>  	nr_pages = end_pfn - start_pfn;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Yasuaki Ishimatsu - July 2, 2012, 2:53 a.m.
Hi Jiang,

Thank you for your feedback.

2012/07/01 0:46, Jiang Liu wrote:
> On 06/27/2012 01:44 PM, Yasuaki Ishimatsu wrote:
>> When offline_pages() is called to offlined memory, the function fails since
>> all memory has been offlined. In this case, the function should succeed.
>> The patch adds the check function into offline_pages().
>>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> ---
>>   drivers/base/memory.c  |   20 ++++++++++++++++++++
>>   include/linux/memory.h |    1 +
>>   mm/memory_hotplug.c    |    5 +++++
>>   3 files changed, 26 insertions(+)
>>
>> Index: linux-3.5-rc4/drivers/base/memory.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/drivers/base/memory.c	2012-06-26 13:28:16.726211752 +0900
>> +++ linux-3.5-rc4/drivers/base/memory.c	2012-06-26 13:34:22.423639904 +0900
>> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>>   }
>>   EXPORT_SYMBOL(unregister_memory_isolate_notifier);
>>
>> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
>> +{
>> +	struct memory_block *mem;
>> +	struct mem_section *section;
>> +	unsigned long pfn, section_nr;
>> +
>> +	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +		section_nr = pfn_to_section_nr(pfn);
>> +		section = __nr_to_section(section_nr);
> Is it possible for __nr_to_section return NULL here?

Yes. I'll add NULL check.

> 
>> +		mem = find_memory_block(section);
>> +		if (!mem)
>> +			continue;
>> +		if (mem->state == MEM_OFFLINE)
>> +			continue;
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
> Need a put_dev(&mem->dev) for the last memory block device handled before return.

Thanks.
I think kobject_put(&mem->dev.kobj) should be handled for all memory block
devices found by find_memory_block(). I'll update it.

Thanks,
Yasuaki Ishimatsu

> 
>> +
>>   /*
>>    * register_memory - Setup a sysfs device for a memory block
>>    */
>> Index: linux-3.5-rc4/include/linux/memory.h
>> ===================================================================
>> --- linux-3.5-rc4.orig/include/linux/memory.h	2012-06-25 04:53:04.000000000 +0900
>> +++ linux-3.5-rc4/include/linux/memory.h	2012-06-26 13:34:22.424639891 +0900
>> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>>   extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>>   							struct memory_block *);
>>   extern struct memory_block *find_memory_block(struct mem_section *);
>> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>>   #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
>>   enum mem_add_context { BOOT, HOTPLUG };
>>   #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
>> Index: linux-3.5-rc4/mm/memory_hotplug.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
>> +++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
>> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
>>
>>   	lock_memory_hotplug();
>>
>> +	if (memory_is_offline(start_pfn, end_pfn)) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>> +
>>   	zone = page_zone(pfn_to_page(start_pfn));
>>   	node = zone_to_nid(zone);
>>   	nr_pages = end_pfn - start_pfn;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
Yasuaki Ishimatsu - July 2, 2012, 2:56 a.m.
Hi Jiang,

2012/07/01 0:51, Jiang Liu wrote:
> On 06/27/2012 01:44 PM, Yasuaki Ishimatsu wrote:
>> When offline_pages() is called to offlined memory, the function fails since
>> all memory has been offlined. In this case, the function should succeed.
>> The patch adds the check function into offline_pages().
>>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> ---
>>   drivers/base/memory.c  |   20 ++++++++++++++++++++
>>   include/linux/memory.h |    1 +
>>   mm/memory_hotplug.c    |    5 +++++
>>   3 files changed, 26 insertions(+)
>>
>> Index: linux-3.5-rc4/drivers/base/memory.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/drivers/base/memory.c	2012-06-26 13:28:16.726211752 +0900
>> +++ linux-3.5-rc4/drivers/base/memory.c	2012-06-26 13:34:22.423639904 +0900
>> @@ -70,6 +70,26 @@ void unregister_memory_isolate_notifier(
>>   }
>>   EXPORT_SYMBOL(unregister_memory_isolate_notifier);
>>
>> +bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
>> +{
>> +	struct memory_block *mem;
>> +	struct mem_section *section;
>> +	unsigned long pfn, section_nr;
>> +
>> +	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
>> +		section_nr = pfn_to_section_nr(pfn);
>> +		section = __nr_to_section(section_nr);
>> +		mem = find_memory_block(section);
> Seems find_memory_block_hinted() is more efficient than find_memory_block() here.

Thanks. I'll update it.

Thanks,
Yasuaki Ishimatsu

> 
>> +		if (!mem)
>> +			continue;
>> +		if (mem->state == MEM_OFFLINE)
>> +			continue;
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   /*
>>    * register_memory - Setup a sysfs device for a memory block
>>    */
>> Index: linux-3.5-rc4/include/linux/memory.h
>> ===================================================================
>> --- linux-3.5-rc4.orig/include/linux/memory.h	2012-06-25 04:53:04.000000000 +0900
>> +++ linux-3.5-rc4/include/linux/memory.h	2012-06-26 13:34:22.424639891 +0900
>> @@ -120,6 +120,7 @@ extern int memory_isolate_notify(unsigne
>>   extern struct memory_block *find_memory_block_hinted(struct mem_section *,
>>   							struct memory_block *);
>>   extern struct memory_block *find_memory_block(struct mem_section *);
>> +extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
>>   #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
>>   enum mem_add_context { BOOT, HOTPLUG };
>>   #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
>> Index: linux-3.5-rc4/mm/memory_hotplug.c
>> ===================================================================
>> --- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
>> +++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
>> @@ -887,6 +887,11 @@ static int __ref offline_pages(unsigned
>>
>>   	lock_memory_hotplug();
>>
>> +	if (memory_is_offline(start_pfn, end_pfn)) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>> +
>>   	zone = page_zone(pfn_to_page(start_pfn));
>>   	node = zone_to_nid(zone);
>>   	nr_pages = end_pfn - start_pfn;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

Patch

Index: linux-3.5-rc4/drivers/base/memory.c
===================================================================
--- linux-3.5-rc4.orig/drivers/base/memory.c	2012-06-26 13:28:16.726211752 +0900
+++ linux-3.5-rc4/drivers/base/memory.c	2012-06-26 13:34:22.423639904 +0900
@@ -70,6 +70,26 @@  void unregister_memory_isolate_notifier(
 }
 EXPORT_SYMBOL(unregister_memory_isolate_notifier);

+bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct memory_block *mem;
+	struct mem_section *section;
+	unsigned long pfn, section_nr;
+
+	for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		section_nr = pfn_to_section_nr(pfn);
+		section = __nr_to_section(section_nr);
+		mem = find_memory_block(section);
+		if (!mem)
+			continue;
+		if (mem->state == MEM_OFFLINE)
+			continue;
+		return false;
+	}
+
+	return true;
+}
+
 /*
  * register_memory - Setup a sysfs device for a memory block
  */
Index: linux-3.5-rc4/include/linux/memory.h
===================================================================
--- linux-3.5-rc4.orig/include/linux/memory.h	2012-06-25 04:53:04.000000000 +0900
+++ linux-3.5-rc4/include/linux/memory.h	2012-06-26 13:34:22.424639891 +0900
@@ -120,6 +120,7 @@  extern int memory_isolate_notify(unsigne
 extern struct memory_block *find_memory_block_hinted(struct mem_section *,
 							struct memory_block *);
 extern struct memory_block *find_memory_block(struct mem_section *);
+extern bool memory_is_offline(unsigned long start_pfn, unsigned long end_pfn);
 #define CONFIG_MEM_BLOCK_SIZE	(PAGES_PER_SECTION<<PAGE_SHIFT)
 enum mem_add_context { BOOT, HOTPLUG };
 #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
Index: linux-3.5-rc4/mm/memory_hotplug.c
===================================================================
--- linux-3.5-rc4.orig/mm/memory_hotplug.c	2012-06-26 13:28:16.743211538 +0900
+++ linux-3.5-rc4/mm/memory_hotplug.c	2012-06-26 13:48:38.264940468 +0900
@@ -887,6 +887,11 @@  static int __ref offline_pages(unsigned

 	lock_memory_hotplug();

+	if (memory_is_offline(start_pfn, end_pfn)) {
+		ret = 0;
+		goto out;
+	}
+
 	zone = page_zone(pfn_to_page(start_pfn));
 	node = zone_to_nid(zone);
 	nr_pages = end_pfn - start_pfn;