[PATCH 0/4] bugfix for memory hotplug

[PATCH 0/4] bugfix for memory hotplug

[wency]

From: Wen Congyang <wency@cn.fujitsu.com>

Wen Congyang (2):
  memory-hotplug: clear hwpoisoned flag when onlining pages
  memory-hotplug: auto offline page_cgroup when onlining memory block
    failed

Yasuaki Ishimatsu (2):
  memory-hotplug: add memory_block_release
  memory-hotplug: add node_device_release

 drivers/base/memory.c |    9 ++++++++-
 drivers/base/node.c   |   11 +++++++++++
 mm/memory_hotplug.c   |    8 ++++++++
 mm/page_cgroup.c      |    3 +++
 4 files changed, 30 insertions(+), 1 deletions(-)

[PATCH 1/4] memory-hotplug: add memory_block_release

[wency]

From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

When calling remove_memory_block(), the function shows following message at
device_release().

Device 'memory528' does not have a release() function, it is broken and must
be fixed.

remove_memory_block() calls kfree(mem). I think it shouled be called from
device_release(). So the patch implements memory_block_release()

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
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 |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 7dda4f7..da457e5 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -70,6 +70,13 @@ void unregister_memory_isolate_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_memory_isolate_notifier);
 
+static void release_memory_block(struct device *dev)
+{
+	struct memory_block *mem = container_of(dev, struct memory_block, dev);
+
+	kfree(mem);
+}
+
 /*
  * register_memory - Setup a sysfs device for a memory block
  */
@@ -80,6 +87,7 @@ int register_memory(struct memory_block *memory)
 
 	memory->dev.bus = &memory_subsys;
 	memory->dev.id = memory->start_section_nr / sections_per_block;
+	memory->dev.release = release_memory_block;
 
 	error = device_register(&memory->dev);
 	return error;
@@ -630,7 +638,6 @@ int remove_memory_block(unsigned long node_id, struct mem_section *section,
 		mem_remove_simple_file(mem, phys_device);
 		mem_remove_simple_file(mem, removable);
 		unregister_memory(mem);
-		kfree(mem);
 	} else
 		kobject_put(&mem->dev.kobj);
 
-- 
1.7.1

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[KOSAKI Motohiro]

On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>
> When calling remove_memory_block(), the function shows following message at
> device_release().
>
> Device 'memory528' does not have a release() function, it is broken and must
> be fixed.
>
> remove_memory_block() calls kfree(mem). I think it shouled be called from
> device_release(). So the patch implements memory_block_release()

Why do you think so? This is terribly bad change log. it has almost
zero information.
We can't review it.




>
> CC: David Rientjes <rientjes@google.com>
> CC: Jiang Liu <liuj97@gmail.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> 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 |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 7dda4f7..da457e5 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -70,6 +70,13 @@ void unregister_memory_isolate_notifier(struct notifier_block *nb)
>  }
>  EXPORT_SYMBOL(unregister_memory_isolate_notifier);
>
> +static void release_memory_block(struct device *dev)
> +{
> +       struct memory_block *mem = container_of(dev, struct memory_block, dev);
> +
> +       kfree(mem);
> +}
> +
>  /*
>   * register_memory - Setup a sysfs device for a memory block
>   */
> @@ -80,6 +87,7 @@ int register_memory(struct memory_block *memory)
>
>         memory->dev.bus = &memory_subsys;
>         memory->dev.id = memory->start_section_nr / sections_per_block;
> +       memory->dev.release = release_memory_block;
>
>         error = device_register(&memory->dev);
>         return error;
> @@ -630,7 +638,6 @@ int remove_memory_block(unsigned long node_id, struct mem_section *section,
>                 mem_remove_simple_file(mem, phys_device);
>                 mem_remove_simple_file(mem, removable);
>                 unregister_memory(mem);
> -               kfree(mem);
>         } else
>                 kobject_put(&mem->dev.kobj);
>
> --
> 1.7.1
>
> --
> 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>

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[Yasuaki Ishimatsu]

Hi Chen,

2012/09/27 19:20, Ni zhan Chen wrote:
> Hi Congyang,
>
> 2012/9/27 <wency@cn.fujitsu.com>
>
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> When calling remove_memory_block(), the function shows following message at
>> device_release().
>>
>> Device 'memory528' does not have a release() function, it is broken and
>> must
>> be fixed.
>>
>
> What's the difference between the patch and original implemetation?

The implementation is for removing a memory_block. So the purpose is
same as original one. But original code is bad manner. kobject_cleanup()
is called by remove_memory_block() at last. But release function for
releasing memory_block is not registered. As a result, the kernel message
is shown. IMHO, memory_block should be release by the releae function.

Thanks,
Yasuaki Ishimatsu

>
>
>> remove_memory_block() calls kfree(mem). I think it shouled be called from
>> device_release(). So the patch implements memory_block_release()
>>
>> CC: David Rientjes <rientjes@google.com>
>> CC: Jiang Liu <liuj97@gmail.com>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Paul Mackerras <paulus@samba.org>
>> 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 |    9 ++++++++-
>>   1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 7dda4f7..da457e5 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -70,6 +70,13 @@ void unregister_memory_isolate_notifier(struct
>> notifier_block *nb)
>>   }
>>   EXPORT_SYMBOL(unregister_memory_isolate_notifier);
>>
>> +static void release_memory_block(struct device *dev)
>> +{
>> +       struct memory_block *mem = container_of(dev, struct memory_block,
>> dev);
>> +
>> +       kfree(mem);
>> +}
>> +
>>   /*
>>    * register_memory - Setup a sysfs device for a memory block
>>    */
>> @@ -80,6 +87,7 @@ int register_memory(struct memory_block *memory)
>>
>>          memory->dev.bus = &memory_subsys;
>>          memory->dev.id = memory->start_section_nr / sections_per_block;
>> +       memory->dev.release = release_memory_block;
>>
>>          error = device_register(&memory->dev);
>>          return error;
>> @@ -630,7 +638,6 @@ int remove_memory_block(unsigned long node_id, struct
>> mem_section *section,
>>                  mem_remove_simple_file(mem, phys_device);
>>                  mem_remove_simple_file(mem, removable);
>>                  unregister_memory(mem);
>> -               kfree(mem);
>>          } else
>>                  kobject_put(&mem->dev.kobj);
>>
>> --
>> 1.7.1
>>
>> --
>> 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>
>>
>

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[KOSAKI Motohiro]

On Thu, Sep 27, 2012 at 8:24 PM, Yasuaki Ishimatsu
<isimatu.yasuaki@jp.fujitsu.com> wrote:
> Hi Chen,
>
>
> 2012/09/27 19:20, Ni zhan Chen wrote:
>>
>> Hi Congyang,
>>
>> 2012/9/27 <wency@cn.fujitsu.com>
>>
>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>
>>> When calling remove_memory_block(), the function shows following message
>>> at
>>> device_release().
>>>
>>> Device 'memory528' does not have a release() function, it is broken and
>>> must
>>> be fixed.
>>>
>>
>> What's the difference between the patch and original implemetation?
>
>
> The implementation is for removing a memory_block. So the purpose is
> same as original one. But original code is bad manner. kobject_cleanup()
> is called by remove_memory_block() at last. But release function for
> releasing memory_block is not registered. As a result, the kernel message
> is shown. IMHO, memory_block should be release by the releae function.

but your patch introduced use after free bug, if i understand correctly.
See unregister_memory() function. After your patch, kobject_put() call
release_memory_block() and kfree(). and then device_unregister() will
touch freed memory.

static void
unregister_memory(struct memory_block *memory)
{
	BUG_ON(memory->dev.bus != &memory_subsys);

	/* drop the ref. we got in remove_memory_block() */
	kobject_put(&memory->dev.kobj);
	device_unregister(&memory->dev);
}

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[Yasuaki Ishimatsu]

Hi Kosaki-san,

2012/09/28 10:35, KOSAKI Motohiro wrote:
> On Thu, Sep 27, 2012 at 8:24 PM, Yasuaki Ishimatsu
> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>> Hi Chen,
>>
>>
>> 2012/09/27 19:20, Ni zhan Chen wrote:
>>>
>>> Hi Congyang,
>>>
>>> 2012/9/27 <wency@cn.fujitsu.com>
>>>
>>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>>
>>>> When calling remove_memory_block(), the function shows following message
>>>> at
>>>> device_release().
>>>>
>>>> Device 'memory528' does not have a release() function, it is broken and
>>>> must
>>>> be fixed.
>>>>
>>>
>>> What's the difference between the patch and original implemetation?
>>
>>
>> The implementation is for removing a memory_block. So the purpose is
>> same as original one. But original code is bad manner. kobject_cleanup()
>> is called by remove_memory_block() at last. But release function for
>> releasing memory_block is not registered. As a result, the kernel message
>> is shown. IMHO, memory_block should be release by the releae function.
>
> but your patch introduced use after free bug, if i understand correctly.
> See unregister_memory() function. After your patch, kobject_put() call
> release_memory_block() and kfree(). and then device_unregister() will
> touch freed memory.

It is not correct. The kobject_put() is prepared against find_memory_block()
in remove_memory_block() since kobject->kref is incremented in it.
So release_memory_block() is called by device_unregister() correctly as follows:

[ 1014.589008] Pid: 126, comm: kworker/0:2 Not tainted 3.6.0-rc3-enable-memory-hotremove-and-root-bridge #3
[ 1014.702437] Call Trace:
[ 1014.731684]  [<ffffffff8144d096>] release_memory_block+0x16/0x30
[ 1014.803581]  [<ffffffff81438587>] device_release+0x27/0xa0
[ 1014.869312]  [<ffffffff8133e962>] kobject_cleanup+0x82/0x1b0
[ 1014.937062]  [<ffffffff8133ea9d>] kobject_release+0xd/0x10
[ 1015.002718]  [<ffffffff8133e7ec>] kobject_put+0x2c/0x60
[ 1015.065271]  [<ffffffff81438107>] put_device+0x17/0x20
[ 1015.126794]  [<ffffffff8143918a>] device_unregister+0x2a/0x60
[ 1015.195578]  [<ffffffff8144d55b>] remove_memory_block+0xbb/0xf0
[ 1015.266434]  [<ffffffff8144d5af>] unregister_memory_section+0x1f/0x30
[ 1015.343532]  [<ffffffff811c0a58>] __remove_section+0x68/0x110
[ 1015.412318]  [<ffffffff811c0be7>] __remove_pages+0xe7/0x120
[ 1015.479021]  [<ffffffff81653d8c>] arch_remove_memory+0x2c/0x80
[ 1015.548845]  [<ffffffff8165497b>] remove_memory+0x6b/0xd0
[ 1015.613474]  [<ffffffff813d946c>] acpi_memory_device_remove_memory+0x48/0x73
[ 1015.697834]  [<ffffffff813d94c2>] acpi_memory_device_remove+0x2b/0x44
[ 1015.774922]  [<ffffffff813a61e4>] acpi_device_remove+0x90/0xb2
[ 1015.844796]  [<ffffffff8143c2fc>] __device_release_driver+0x7c/0xf0
[ 1015.919814]  [<ffffffff8143c47f>] device_release_driver+0x2f/0x50
[ 1015.992753]  [<ffffffff813a70dc>] acpi_bus_remove+0x32/0x6d
[ 1016.059462]  [<ffffffff813a71a8>] acpi_bus_trim+0x91/0x102
[ 1016.125128]  [<ffffffff813a72a1>] acpi_bus_hot_remove_device+0x88/0x16b
[ 1016.204295]  [<ffffffff813a2e57>] acpi_os_execute_deferred+0x27/0x34
[ 1016.280350]  [<ffffffff81090599>] process_one_work+0x219/0x680
[ 1016.350173]  [<ffffffff81090538>] ? process_one_work+0x1b8/0x680
[ 1016.422072]  [<ffffffff813a2e30>] ? acpi_os_wait_events_complete+0x23/0x23
[ 1016.504357]  [<ffffffff810923ce>] worker_thread+0x12e/0x320
[ 1016.571064]  [<ffffffff810922a0>] ? manage_workers+0x110/0x110
[ 1016.640886]  [<ffffffff810983a6>] kthread+0xc6/0xd0
[ 1016.699290]  [<ffffffff8167b144>] kernel_thread_helper+0x4/0x10
[ 1016.770149]  [<ffffffff81670bb0>] ? retint_restore_args+0x13/0x13
[ 1016.843165]  [<ffffffff810982e0>] ? __init_kthread_worker+0x70/0x70
[ 1016.918200]  [<ffffffff8167b140>] ? gs_change+0x13/0x13

Thanks,
Yasuaki Ishimatsu

>
> static void
> unregister_memory(struct memory_block *memory)
> {
> 	BUG_ON(memory->dev.bus != &memory_subsys);
>
> 	/* drop the ref. we got in remove_memory_block() */
> 	kobject_put(&memory->dev.kobj);
> 	device_unregister(&memory->dev);
> }
>

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[Ni zhan Chen]

On 09/28/2012 11:45 AM, Yasuaki Ishimatsu wrote:
> Hi Kosaki-san,
>
> 2012/09/28 10:35, KOSAKI Motohiro wrote:
>> On Thu, Sep 27, 2012 at 8:24 PM, Yasuaki Ishimatsu
>> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>>> Hi Chen,
>>>
>>>
>>> 2012/09/27 19:20, Ni zhan Chen wrote:
>>>>
>>>> Hi Congyang,
>>>>
>>>> 2012/9/27 <wency@cn.fujitsu.com>
>>>>
>>>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>>>
>>>>> When calling remove_memory_block(), the function shows following 
>>>>> message
>>>>> at
>>>>> device_release().
>>>>>
>>>>> Device 'memory528' does not have a release() function, it is 
>>>>> broken and
>>>>> must
>>>>> be fixed.
>>>>>
>>>>
>>>> What's the difference between the patch and original implemetation?
>>>
>>>
>>> The implementation is for removing a memory_block. So the purpose is
>>> same as original one. But original code is bad manner. 
>>> kobject_cleanup()
>>> is called by remove_memory_block() at last. But release function for
>>> releasing memory_block is not registered. As a result, the kernel 
>>> message
>>> is shown. IMHO, memory_block should be release by the releae function.
>>
>> but your patch introduced use after free bug, if i understand correctly.
>> See unregister_memory() function. After your patch, kobject_put() call
>> release_memory_block() and kfree(). and then device_unregister() will
>> touch freed memory.
>

this patch is similiar to [RFC v9 PATCH 10/21] memory-hotplug: add 
memory_block_release, they handle the same issue, can these two patches 
be fold to one?

> It is not correct. The kobject_put() is prepared against 
> find_memory_block()
> in remove_memory_block() since kobject->kref is incremented in it.
> So release_memory_block() is called by device_unregister() correctly 
> as follows:
>
> [ 1014.589008] Pid: 126, comm: kworker/0:2 Not tainted 
> 3.6.0-rc3-enable-memory-hotremove-and-root-bridge #3
> [ 1014.702437] Call Trace:
> [ 1014.731684]  [<ffffffff8144d096>] release_memory_block+0x16/0x30
> [ 1014.803581]  [<ffffffff81438587>] device_release+0x27/0xa0
> [ 1014.869312]  [<ffffffff8133e962>] kobject_cleanup+0x82/0x1b0
> [ 1014.937062]  [<ffffffff8133ea9d>] kobject_release+0xd/0x10
> [ 1015.002718]  [<ffffffff8133e7ec>] kobject_put+0x2c/0x60
> [ 1015.065271]  [<ffffffff81438107>] put_device+0x17/0x20
> [ 1015.126794]  [<ffffffff8143918a>] device_unregister+0x2a/0x60
> [ 1015.195578]  [<ffffffff8144d55b>] remove_memory_block+0xbb/0xf0
> [ 1015.266434]  [<ffffffff8144d5af>] unregister_memory_section+0x1f/0x30
> [ 1015.343532]  [<ffffffff811c0a58>] __remove_section+0x68/0x110
> [ 1015.412318]  [<ffffffff811c0be7>] __remove_pages+0xe7/0x120
> [ 1015.479021]  [<ffffffff81653d8c>] arch_remove_memory+0x2c/0x80
> [ 1015.548845]  [<ffffffff8165497b>] remove_memory+0x6b/0xd0
> [ 1015.613474]  [<ffffffff813d946c>] 
> acpi_memory_device_remove_memory+0x48/0x73
> [ 1015.697834]  [<ffffffff813d94c2>] acpi_memory_device_remove+0x2b/0x44
> [ 1015.774922]  [<ffffffff813a61e4>] acpi_device_remove+0x90/0xb2
> [ 1015.844796]  [<ffffffff8143c2fc>] __device_release_driver+0x7c/0xf0
> [ 1015.919814]  [<ffffffff8143c47f>] device_release_driver+0x2f/0x50
> [ 1015.992753]  [<ffffffff813a70dc>] acpi_bus_remove+0x32/0x6d
> [ 1016.059462]  [<ffffffff813a71a8>] acpi_bus_trim+0x91/0x102
> [ 1016.125128]  [<ffffffff813a72a1>] 
> acpi_bus_hot_remove_device+0x88/0x16b
> [ 1016.204295]  [<ffffffff813a2e57>] acpi_os_execute_deferred+0x27/0x34
> [ 1016.280350]  [<ffffffff81090599>] process_one_work+0x219/0x680
> [ 1016.350173]  [<ffffffff81090538>] ? process_one_work+0x1b8/0x680
> [ 1016.422072]  [<ffffffff813a2e30>] ? 
> acpi_os_wait_events_complete+0x23/0x23
> [ 1016.504357]  [<ffffffff810923ce>] worker_thread+0x12e/0x320
> [ 1016.571064]  [<ffffffff810922a0>] ? manage_workers+0x110/0x110
> [ 1016.640886]  [<ffffffff810983a6>] kthread+0xc6/0xd0
> [ 1016.699290]  [<ffffffff8167b144>] kernel_thread_helper+0x4/0x10
> [ 1016.770149]  [<ffffffff81670bb0>] ? retint_restore_args+0x13/0x13
> [ 1016.843165]  [<ffffffff810982e0>] ? __init_kthread_worker+0x70/0x70
> [ 1016.918200]  [<ffffffff8167b140>] ? gs_change+0x13/0x13
>
> Thanks,
> Yasuaki Ishimatsu
>
>>
>> static void
>> unregister_memory(struct memory_block *memory)
>> {
>>     BUG_ON(memory->dev.bus != &memory_subsys);
>>
>>     /* drop the ref. we got in remove_memory_block() */
>>     kobject_put(&memory->dev.kobj);
>>     device_unregister(&memory->dev);
>> }
>>
>
>
>

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[Yasuaki Ishimatsu]

Hi Chen,

2012/09/28 15:04, Ni zhan Chen wrote:
> On 09/28/2012 11:45 AM, Yasuaki Ishimatsu wrote:
>> Hi Kosaki-san,
>>
>> 2012/09/28 10:35, KOSAKI Motohiro wrote:
>>> On Thu, Sep 27, 2012 at 8:24 PM, Yasuaki Ishimatsu
>>> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>>>> Hi Chen,
>>>>
>>>>
>>>> 2012/09/27 19:20, Ni zhan Chen wrote:
>>>>>
>>>>> Hi Congyang,
>>>>>
>>>>> 2012/9/27 <wency@cn.fujitsu.com>
>>>>>
>>>>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>>>>
>>>>>> When calling remove_memory_block(), the function shows following message
>>>>>> at
>>>>>> device_release().
>>>>>>
>>>>>> Device 'memory528' does not have a release() function, it is broken and
>>>>>> must
>>>>>> be fixed.
>>>>>>
>>>>>
>>>>> What's the difference between the patch and original implemetation?
>>>>
>>>>
>>>> The implementation is for removing a memory_block. So the purpose is
>>>> same as original one. But original code is bad manner. kobject_cleanup()
>>>> is called by remove_memory_block() at last. But release function for
>>>> releasing memory_block is not registered. As a result, the kernel message
>>>> is shown. IMHO, memory_block should be release by the releae function.
>>>
>>> but your patch introduced use after free bug, if i understand correctly.
>>> See unregister_memory() function. After your patch, kobject_put() call
>>> release_memory_block() and kfree(). and then device_unregister() will
>>> touch freed memory.
>>
>
> this patch is similiar to [RFC v9 PATCH 10/21] memory-hotplug: add memory_block_release, they handle the same issue, can these two patches be fold to one?

You're right. The patch is same as [RFC v9 PATCH 10/21].
The patch is a bug fix. So we separated it from memory-hotplug patch-set.

Thanks,
Yasuaki Ishimatsu

>> It is not correct. The kobject_put() is prepared against find_memory_block()
>> in remove_memory_block() since kobject->kref is incremented in it.
>> So release_memory_block() is called by device_unregister() correctly as follows:
>>
>> [ 1014.589008] Pid: 126, comm: kworker/0:2 Not tainted 3.6.0-rc3-enable-memory-hotremove-and-root-bridge #3
>> [ 1014.702437] Call Trace:
>> [ 1014.731684]  [<ffffffff8144d096>] release_memory_block+0x16/0x30
>> [ 1014.803581]  [<ffffffff81438587>] device_release+0x27/0xa0
>> [ 1014.869312]  [<ffffffff8133e962>] kobject_cleanup+0x82/0x1b0
>> [ 1014.937062]  [<ffffffff8133ea9d>] kobject_release+0xd/0x10
>> [ 1015.002718]  [<ffffffff8133e7ec>] kobject_put+0x2c/0x60
>> [ 1015.065271]  [<ffffffff81438107>] put_device+0x17/0x20
>> [ 1015.126794]  [<ffffffff8143918a>] device_unregister+0x2a/0x60
>> [ 1015.195578]  [<ffffffff8144d55b>] remove_memory_block+0xbb/0xf0
>> [ 1015.266434]  [<ffffffff8144d5af>] unregister_memory_section+0x1f/0x30
>> [ 1015.343532]  [<ffffffff811c0a58>] __remove_section+0x68/0x110
>> [ 1015.412318]  [<ffffffff811c0be7>] __remove_pages+0xe7/0x120
>> [ 1015.479021]  [<ffffffff81653d8c>] arch_remove_memory+0x2c/0x80
>> [ 1015.548845]  [<ffffffff8165497b>] remove_memory+0x6b/0xd0
>> [ 1015.613474]  [<ffffffff813d946c>] acpi_memory_device_remove_memory+0x48/0x73
>> [ 1015.697834]  [<ffffffff813d94c2>] acpi_memory_device_remove+0x2b/0x44
>> [ 1015.774922]  [<ffffffff813a61e4>] acpi_device_remove+0x90/0xb2
>> [ 1015.844796]  [<ffffffff8143c2fc>] __device_release_driver+0x7c/0xf0
>> [ 1015.919814]  [<ffffffff8143c47f>] device_release_driver+0x2f/0x50
>> [ 1015.992753]  [<ffffffff813a70dc>] acpi_bus_remove+0x32/0x6d
>> [ 1016.059462]  [<ffffffff813a71a8>] acpi_bus_trim+0x91/0x102
>> [ 1016.125128]  [<ffffffff813a72a1>] acpi_bus_hot_remove_device+0x88/0x16b
>> [ 1016.204295]  [<ffffffff813a2e57>] acpi_os_execute_deferred+0x27/0x34
>> [ 1016.280350]  [<ffffffff81090599>] process_one_work+0x219/0x680
>> [ 1016.350173]  [<ffffffff81090538>] ? process_one_work+0x1b8/0x680
>> [ 1016.422072]  [<ffffffff813a2e30>] ? acpi_os_wait_events_complete+0x23/0x23
>> [ 1016.504357]  [<ffffffff810923ce>] worker_thread+0x12e/0x320
>> [ 1016.571064]  [<ffffffff810922a0>] ? manage_workers+0x110/0x110
>> [ 1016.640886]  [<ffffffff810983a6>] kthread+0xc6/0xd0
>> [ 1016.699290]  [<ffffffff8167b144>] kernel_thread_helper+0x4/0x10
>> [ 1016.770149]  [<ffffffff81670bb0>] ? retint_restore_args+0x13/0x13
>> [ 1016.843165]  [<ffffffff810982e0>] ? __init_kthread_worker+0x70/0x70
>> [ 1016.918200]  [<ffffffff8167b140>] ? gs_change+0x13/0x13
>>
>> Thanks,
>> Yasuaki Ishimatsu
>>
>>>
>>> static void
>>> unregister_memory(struct memory_block *memory)
>>> {
>>>     BUG_ON(memory->dev.bus != &memory_subsys);
>>>
>>>     /* drop the ref. we got in remove_memory_block() */
>>>     kobject_put(&memory->dev.kobj);
>>>     device_unregister(&memory->dev);
>>> }
>>>
>>
>>
>>
>

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[Ni zhan Chen]

On 09/28/2012 11:45 AM, Yasuaki Ishimatsu wrote:
> Hi Kosaki-san,
>
> 2012/09/28 10:35, KOSAKI Motohiro wrote:
>> On Thu, Sep 27, 2012 at 8:24 PM, Yasuaki Ishimatsu
>> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>>> Hi Chen,
>>>
>>>
>>> 2012/09/27 19:20, Ni zhan Chen wrote:
>>>>
>>>> Hi Congyang,
>>>>
>>>> 2012/9/27 <wency@cn.fujitsu.com>
>>>>
>>>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>>>
>>>>> When calling remove_memory_block(), the function shows following 
>>>>> message
>>>>> at
>>>>> device_release().
>>>>>
>>>>> Device 'memory528' does not have a release() function, it is 
>>>>> broken and
>>>>> must
>>>>> be fixed.
>>>>>
>>>>
>>>> What's the difference between the patch and original implemetation?
>>>
>>>
>>> The implementation is for removing a memory_block. So the purpose is
>>> same as original one. But original code is bad manner. 
>>> kobject_cleanup()
>>> is called by remove_memory_block() at last. But release function for
>>> releasing memory_block is not registered. As a result, the kernel 
>>> message
>>> is shown. IMHO, memory_block should be release by the releae function.
>>
>> but your patch introduced use after free bug, if i understand correctly.
>> See unregister_memory() function. After your patch, kobject_put() call
>> release_memory_block() and kfree(). and then device_unregister() will
>> touch freed memory.
>
> It is not correct. The kobject_put() is prepared against 
> find_memory_block()
> in remove_memory_block() since kobject->kref is incremented in it.
> So release_memory_block() is called by device_unregister() correctly 
> as follows:

Another issue is memory hotplug which is not associated to this patch 
report to you:
IIUC, function register_mem_sect_under_node should be renamed to 
register_mem_block_under_node,
since this function is register memory block instead of memory section.

>
> [ 1014.589008] Pid: 126, comm: kworker/0:2 Not tainted 
> 3.6.0-rc3-enable-memory-hotremove-and-root-bridge #3
> [ 1014.702437] Call Trace:
> [ 1014.731684]  [<ffffffff8144d096>] release_memory_block+0x16/0x30
> [ 1014.803581]  [<ffffffff81438587>] device_release+0x27/0xa0
> [ 1014.869312]  [<ffffffff8133e962>] kobject_cleanup+0x82/0x1b0
> [ 1014.937062]  [<ffffffff8133ea9d>] kobject_release+0xd/0x10
> [ 1015.002718]  [<ffffffff8133e7ec>] kobject_put+0x2c/0x60
> [ 1015.065271]  [<ffffffff81438107>] put_device+0x17/0x20
> [ 1015.126794]  [<ffffffff8143918a>] device_unregister+0x2a/0x60
> [ 1015.195578]  [<ffffffff8144d55b>] remove_memory_block+0xbb/0xf0
> [ 1015.266434]  [<ffffffff8144d5af>] unregister_memory_section+0x1f/0x30
> [ 1015.343532]  [<ffffffff811c0a58>] __remove_section+0x68/0x110
> [ 1015.412318]  [<ffffffff811c0be7>] __remove_pages+0xe7/0x120
> [ 1015.479021]  [<ffffffff81653d8c>] arch_remove_memory+0x2c/0x80
> [ 1015.548845]  [<ffffffff8165497b>] remove_memory+0x6b/0xd0
> [ 1015.613474]  [<ffffffff813d946c>] 
> acpi_memory_device_remove_memory+0x48/0x73
> [ 1015.697834]  [<ffffffff813d94c2>] acpi_memory_device_remove+0x2b/0x44
> [ 1015.774922]  [<ffffffff813a61e4>] acpi_device_remove+0x90/0xb2
> [ 1015.844796]  [<ffffffff8143c2fc>] __device_release_driver+0x7c/0xf0
> [ 1015.919814]  [<ffffffff8143c47f>] device_release_driver+0x2f/0x50
> [ 1015.992753]  [<ffffffff813a70dc>] acpi_bus_remove+0x32/0x6d
> [ 1016.059462]  [<ffffffff813a71a8>] acpi_bus_trim+0x91/0x102
> [ 1016.125128]  [<ffffffff813a72a1>] 
> acpi_bus_hot_remove_device+0x88/0x16b
> [ 1016.204295]  [<ffffffff813a2e57>] acpi_os_execute_deferred+0x27/0x34
> [ 1016.280350]  [<ffffffff81090599>] process_one_work+0x219/0x680
> [ 1016.350173]  [<ffffffff81090538>] ? process_one_work+0x1b8/0x680
> [ 1016.422072]  [<ffffffff813a2e30>] ? 
> acpi_os_wait_events_complete+0x23/0x23
> [ 1016.504357]  [<ffffffff810923ce>] worker_thread+0x12e/0x320
> [ 1016.571064]  [<ffffffff810922a0>] ? manage_workers+0x110/0x110
> [ 1016.640886]  [<ffffffff810983a6>] kthread+0xc6/0xd0
> [ 1016.699290]  [<ffffffff8167b144>] kernel_thread_helper+0x4/0x10
> [ 1016.770149]  [<ffffffff81670bb0>] ? retint_restore_args+0x13/0x13
> [ 1016.843165]  [<ffffffff810982e0>] ? __init_kthread_worker+0x70/0x70
> [ 1016.918200]  [<ffffffff8167b140>] ? gs_change+0x13/0x13
>
> Thanks,
> Yasuaki Ishimatsu
>
>>
>> static void
>> unregister_memory(struct memory_block *memory)
>> {
>>     BUG_ON(memory->dev.bus != &memory_subsys);
>>
>>     /* drop the ref. we got in remove_memory_block() */
>>     kobject_put(&memory->dev.kobj);
>>     device_unregister(&memory->dev);
>> }
>>
>
>
>

Re: [PATCH 1/4] memory-hotplug: add memory_block_release

[KOSAKI Motohiro]

> It is not correct. The kobject_put() is prepared against find_memory_block()
> in remove_memory_block() since kobject->kref is incremented in it.
> So release_memory_block() is called by device_unregister() correctly as
> follows:

Ok. Looks good then.
Please rewrite the description more kindly at next spin. Current one
is really really bad.

[PATCH 2/4] memory-hotplug: add node_device_release

[wency]

From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

When calling unregister_node(), the function shows following message at
device_release().

Device 'node2' does not have a release() function, it is broken and must be
fixed.

So the patch implements node_device_release()

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 drivers/base/node.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index af1a177..07523fb 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -252,6 +252,16 @@ static inline void hugetlb_register_node(struct node *node) {}
 static inline void hugetlb_unregister_node(struct node *node) {}
 #endif
 
+static void node_device_release(struct device *dev)
+{
+	struct node *node_dev = to_node(dev);
+
+#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
+	flush_work(&node_dev->node_work);
+#endif
+
+	memset(node_dev, 0, sizeof(struct node));
+}
 
 /*
  * register_node - Setup a sysfs device for a node.
@@ -265,6 +275,7 @@ int register_node(struct node *node, int num, struct node *parent)
 
 	node->dev.id = num;
 	node->dev.bus = &node_subsys;
+	node->dev.release = node_device_release;
 	error = device_register(&node->dev);
 
 	if (!error){
-- 
1.7.1

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[Ni zhan Chen]

On 09/27/2012 01:45 PM, wency@cn.fujitsu.com wrote:
> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>
> When calling unregister_node(), the function shows following message at
> device_release().
>
> Device 'node2' does not have a release() function, it is broken and must be
> fixed.
>
> So the patch implements node_device_release()
looks reasonable to me.
>
> CC: David Rientjes <rientjes@google.com>
> CC: Jiang Liu <liuj97@gmail.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>   drivers/base/node.c |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index af1a177..07523fb 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -252,6 +252,16 @@ static inline void hugetlb_register_node(struct node *node) {}
>   static inline void hugetlb_unregister_node(struct node *node) {}
>   #endif
>   
> +static void node_device_release(struct device *dev)
> +{
> +	struct node *node_dev = to_node(dev);
> +
> +#if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
> +	flush_work(&node_dev->node_work);
> +#endif
> +
> +	memset(node_dev, 0, sizeof(struct node));
> +}
>   
>   /*
>    * register_node - Setup a sysfs device for a node.
> @@ -265,6 +275,7 @@ int register_node(struct node *node, int num, struct node *parent)
>   
>   	node->dev.id = num;
>   	node->dev.bus = &node_subsys;
> +	node->dev.release = node_device_release;
>   	error = device_register(&node->dev);
>   
>   	if (!error){

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[KOSAKI Motohiro]

On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>
> When calling unregister_node(), the function shows following message at
> device_release().

This description doesn't have the "following message".


> Device 'node2' does not have a release() function, it is broken and must be
> fixed.
>
> So the patch implements node_device_release()
>

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[Yasuaki Ishimatsu]

Hi Kosaki-san,

2012/09/28 5:13, KOSAKI Motohiro wrote:
> On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> When calling unregister_node(), the function shows following message at
>> device_release().
>
> This description doesn't have the "following message".
>
>

>> Device 'node2' does not have a release() function, it is broken and must be
>> fixed.

This is the messages. The message is shown by kobject_cleanup(), when calling
unregister_node().

Thanks,
Yasuaki Ishimatsu

>>
>> So the patch implements node_device_release()
>>

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[KOSAKI Motohiro]

On Thu, Sep 27, 2012 at 8:07 PM, Yasuaki Ishimatsu
<isimatu.yasuaki@jp.fujitsu.com> wrote:
> Hi Kosaki-san,
>
>
> 2012/09/28 5:13, KOSAKI Motohiro wrote:
>>
>> On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
>>>
>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>
>>> When calling unregister_node(), the function shows following message at
>>> device_release().
>>
>>
>> This description doesn't have the "following message".
>>
>>
>
>>> Device 'node2' does not have a release() function, it is broken and must
>>> be
>>> fixed.
>
>
> This is the messages. The message is shown by kobject_cleanup(), when
> calling
> unregister_node().

If so, you should quote the message. and don't mix it with your
subject. Moreover
your patch title is too silly. "add node_device_release() function" is
a way. you should
describe the effect of the patch. e.g. suppress "Device 'nodeXX' does
not have a release() function" warning.

Moreover, your explanation is still insufficient. Even if
node_device_release() is empty function, we can get rid of the
warning. Why do we need this node_device_release()
implementation?

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[Yasuaki Ishimatsu]

Hi Kosaki-san,

2012/09/28 10:13, KOSAKI Motohiro wrote:
> On Thu, Sep 27, 2012 at 8:07 PM, Yasuaki Ishimatsu
> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>> Hi Kosaki-san,
>>
>>
>> 2012/09/28 5:13, KOSAKI Motohiro wrote:
>>>
>>> On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
>>>>
>>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>>
>>>> When calling unregister_node(), the function shows following message at
>>>> device_release().
>>>
>>>
>>> This description doesn't have the "following message".
>>>
>>>
>>
>>>> Device 'node2' does not have a release() function, it is broken and must
>>>> be
>>>> fixed.
>>
>>
>> This is the messages. The message is shown by kobject_cleanup(), when
>> calling
>> unregister_node().
>
> If so, you should quote the message. and don't mix it with your
> subject. Moreover
> your patch title is too silly. "add node_device_release() function" is
> a way. you should
> describe the effect of the patch. e.g. suppress "Device 'nodeXX' does
> not have a release() function" warning.

What you say is correct. We should update subject and changelog.

>
> Moreover, your explanation is still insufficient. Even if
> node_device_release() is empty function, we can get rid of the
> warning.

I don't understand it. How can we get rid of the warning?

> Why do we need this node_device_release() implementation?

I think that this is a manner of releasing object related kobject.

Thanks,
Yasuaki Ishimatsu

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[KOSAKI Motohiro]

>> Moreover, your explanation is still insufficient. Even if
>> node_device_release() is empty function, we can get rid of the
>> warning.
>
>
> I don't understand it. How can we get rid of the warning?

See cpu_device_release() for example.



>> Why do we need this node_device_release() implementation?
>
> I think that this is a manner of releasing object related kobject.

No.  Usually we never call memset() from release callback.

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[Yasuaki Ishimatsu]

Hi Kosaki-san,

2012/09/28 10:37, KOSAKI Motohiro wrote:
>>> Moreover, your explanation is still insufficient. Even if
>>> node_device_release() is empty function, we can get rid of the
>>> warning.
>>
>>
>> I don't understand it. How can we get rid of the warning?
>
> See cpu_device_release() for example.

If we implement a function like cpu_device_release(), the warning
disappears. But the comment says in the function "Never copy this way...".
So I think it is illegal way.

>
>
>
>>> Why do we need this node_device_release() implementation?
>>
>> I think that this is a manner of releasing object related kobject.
>
> No.  Usually we never call memset() from release callback.
>

What we want to release is a part of array, not a pointer.
Therefore, there is only this way instead of kfree().

Thanks,
Yasuaki Ishimatsu

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[KOSAKI Motohiro]

>>> I don't understand it. How can we get rid of the warning?
>>
>> See cpu_device_release() for example.
>
> If we implement a function like cpu_device_release(), the warning
> disappears. But the comment says in the function "Never copy this way...".
> So I think it is illegal way.

What does "illegal" mean?
You still haven't explain any benefit of your code. If there is zero
benefit, just kill it.
I believe everybody think so.

Again, Which benefit do you have?


>>>> Why do we need this node_device_release() implementation?
>>>
>>> I think that this is a manner of releasing object related kobject.
>>
>> No.  Usually we never call memset() from release callback.
>
> What we want to release is a part of array, not a pointer.
> Therefore, there is only this way instead of kfree().

Why? Before your patch, we don't have memset() and did work it.
I can't understand what mean "only way".

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[Yasuaki Ishimatsu]

Hi Kosaki-san,

2012/09/29 7:19, KOSAKI Motohiro wrote:
>>>> I don't understand it. How can we get rid of the warning?
>>>
>>> See cpu_device_release() for example.
>>
>> If we implement a function like cpu_device_release(), the warning
>> disappears. But the comment says in the function "Never copy this way...".
>> So I think it is illegal way.
>
> What does "illegal" mean?

The "illegal" means the code should not be mimicked.

> You still haven't explain any benefit of your code. If there is zero
> benefit, just kill it.
> I believe everybody think so.
>
> Again, Which benefit do you have?

The patch has a benefit to delets a warning message.

>
>>>>> Why do we need this node_device_release() implementation?
>>>>
>>>> I think that this is a manner of releasing object related kobject.
>>>
>>> No.  Usually we never call memset() from release callback.
>>
>> What we want to release is a part of array, not a pointer.
>> Therefore, there is only this way instead of kfree().
>
> Why? Before your patch, we don't have memset() and did work it.

If we does not apply the patch, a warning message is shown.
So I think it did not work well.

> I can't understand what mean "only way".

For deleting a warning message, I created a node_device_release().
In the manner of releasing kobject, the function frees a object related
to the kobject. So most functions calls kfree() for releasing it.
In node_device_release(), we need to free a node struct. If the node
struct is pointer, I can free it by kfree. But the node struct is a part
of node_devices[] array. I cannot free it. So I filled the node struct
with 0.

But you think it is not good. Do you have a good solution?

Thanks,
Yasuaki Ishimatsu

> --
> 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/
>

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[KOSAKI Motohiro]

On Mon, Oct 1, 2012 at 2:54 AM, Yasuaki Ishimatsu
<isimatu.yasuaki@jp.fujitsu.com> wrote:
> Hi Kosaki-san,
>
>
> 2012/09/29 7:19, KOSAKI Motohiro wrote:
>>>>>
>>>>> I don't understand it. How can we get rid of the warning?
>>>>
>>>>
>>>> See cpu_device_release() for example.
>>>
>>>
>>> If we implement a function like cpu_device_release(), the warning
>>> disappears. But the comment says in the function "Never copy this
>>> way...".
>>> So I think it is illegal way.
>>
>>
>> What does "illegal" mean?
>
>
> The "illegal" means the code should not be mimicked.
>
>
>> You still haven't explain any benefit of your code. If there is zero
>> benefit, just kill it.
>> I believe everybody think so.
>>
>> Again, Which benefit do you have?
>
>
> The patch has a benefit to delets a warning message.
>
>
>>
>>>>>> Why do we need this node_device_release() implementation?
>>>>>
>>>>>
>>>>> I think that this is a manner of releasing object related kobject.
>>>>
>>>>
>>>> No.  Usually we never call memset() from release callback.
>>>
>>>
>>> What we want to release is a part of array, not a pointer.
>>> Therefore, there is only this way instead of kfree().
>>
>>
>> Why? Before your patch, we don't have memset() and did work it.
>
>
> If we does not apply the patch, a warning message is shown.
> So I think it did not work well.
>
>
>> I can't understand what mean "only way".
>
>
> For deleting a warning message, I created a node_device_release().
> In the manner of releasing kobject, the function frees a object related
> to the kobject. So most functions calls kfree() for releasing it.
> In node_device_release(), we need to free a node struct. If the node
> struct is pointer, I can free it by kfree. But the node struct is a part
> of node_devices[] array. I cannot free it. So I filled the node struct
> with 0.
>
> But you think it is not good. Do you have a good solution?

Do nothing. just add empty release function and kill a warning.
Obviously do nothing can't make any performance drop nor any
side effect.

meaningless memset() is just silly from point of cache pollution view.

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[Yasuaki Ishimatsu]

Hi Kosaki-san,

2012/10/02 3:12, KOSAKI Motohiro wrote:
> On Mon, Oct 1, 2012 at 2:54 AM, Yasuaki Ishimatsu
> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>> Hi Kosaki-san,
>>
>>
>> 2012/09/29 7:19, KOSAKI Motohiro wrote:
>>>>>>
>>>>>> I don't understand it. How can we get rid of the warning?
>>>>>
>>>>>
>>>>> See cpu_device_release() for example.
>>>>
>>>>
>>>> If we implement a function like cpu_device_release(), the warning
>>>> disappears. But the comment says in the function "Never copy this
>>>> way...".
>>>> So I think it is illegal way.
>>>
>>>
>>> What does "illegal" mean?
>>
>>
>> The "illegal" means the code should not be mimicked.
>>
>>
>>> You still haven't explain any benefit of your code. If there is zero
>>> benefit, just kill it.
>>> I believe everybody think so.
>>>
>>> Again, Which benefit do you have?
>>
>>
>> The patch has a benefit to delets a warning message.
>>
>>
>>>
>>>>>>> Why do we need this node_device_release() implementation?
>>>>>>
>>>>>>
>>>>>> I think that this is a manner of releasing object related kobject.
>>>>>
>>>>>
>>>>> No.  Usually we never call memset() from release callback.
>>>>
>>>>
>>>> What we want to release is a part of array, not a pointer.
>>>> Therefore, there is only this way instead of kfree().
>>>
>>>
>>> Why? Before your patch, we don't have memset() and did work it.
>>
>>
>> If we does not apply the patch, a warning message is shown.
>> So I think it did not work well.
>>
>>
>>> I can't understand what mean "only way".
>>
>>
>> For deleting a warning message, I created a node_device_release().
>> In the manner of releasing kobject, the function frees a object related
>> to the kobject. So most functions calls kfree() for releasing it.
>> In node_device_release(), we need to free a node struct. If the node
>> struct is pointer, I can free it by kfree. But the node struct is a part
>> of node_devices[] array. I cannot free it. So I filled the node struct
>> with 0.
>>
>> But you think it is not good. Do you have a good solution?
>
> Do nothing. just add empty release function and kill a warning.
> Obviously do nothing can't make any performance drop nor any
> side effect.
>
> meaningless memset() is just silly from point of cache pollution view.

I have the reason to have to fill the node struct with 0 by memset.
The node is a part of node struct array (node_devices[]).
If we add empty release function for suppressing warning,
some data remains in the node struct after hot removing memory.
So if we re-hot adds the memory, the node struct is reused by
register_onde_node(). But the node struct has some data, because
it was not initialized with 0. As a result, more waning is shown
by the remained data at hot addinig memory as follows:

[  374.037710] kobject (ffffffff82c15718): tried to init an initialized object, something is seriously wrong.
[  374.153169] Pid: 4, comm: kworker/0:0 Tainted: G        W    3.6.0 #5
[  374.230279] Call Trace:
[  374.259647]  [<ffffffff8133cf39>] kobject_init+0x89/0xa0
[  374.323286]  [<ffffffff8143632c>] device_initialize+0x2c/0xc0
[  374.392086]  [<ffffffff814376a6>] device_register+0x16/0x30
[  374.458856]  [<ffffffff81449b15>] register_node+0x25/0xe0
[  374.523434]  [<ffffffff8144a057>] register_one_node+0x67/0x140
[  374.593306]  [<ffffffff81652e40>] add_memory+0x100/0x1f0
[  374.656961]  [<ffffffffa00a31c6>] acpi_memory_enable_device+0x92/0xdf [acpi_memhotplug]
[  374.752811]  [<ffffffffa00a3753>] acpi_memory_device_add+0x10d/0x116 [acpi_memhotplug]
[  374.847622]  [<ffffffff813a4376>] acpi_device_probe+0x50/0x18a
[  374.917504]  [<ffffffff8124b053>] ? sysfs_create_link+0x13/0x20
[  374.988426]  [<ffffffff81439d7c>] really_probe+0x6c/0x320
[  375.053061]  [<ffffffff8143a077>] driver_probe_device+0x47/0xa0
[  375.123922]  [<ffffffff8143a180>] ? __driver_attach+0xb0/0xb0
[  375.192709]  [<ffffffff8143a180>] ? __driver_attach+0xb0/0xb0
[  375.261494]  [<ffffffff8143a1d3>] __device_attach+0x53/0x60
[  375.328206]  [<ffffffff81437dac>] bus_for_each_drv+0x6c/0xa0
[  375.395950]  [<ffffffff81439cf8>] device_attach+0xa8/0xc0
[  375.460578]  [<ffffffff814389d0>] bus_probe_device+0xb0/0xe0
[  375.528318]  [<ffffffff81437421>] device_add+0x301/0x570
[  375.591883]  [<ffffffff814376ae>] device_register+0x1e/0x30
[  375.658568]  [<ffffffff813a56bf>] acpi_device_register+0x1af/0x2bf
[  375.732590]  [<ffffffff813a59ae>] acpi_add_single_object+0x1df/0x2b9
[  375.808640]  [<ffffffff813ce320>] ? acpi_ut_release_mutex+0xac/0xb5
[  375.883646]  [<ffffffff813a5b93>] acpi_bus_check_add+0x10b/0x166
[  375.955529]  [<ffffffff810db4ad>] ? trace_hardirqs_on+0xd/0x10
[  376.025327]  [<ffffffff8109fa4f>] ? up+0x2f/0x50
[  376.080639]  [<ffffffff813a0cc1>] ? acpi_os_signal_semaphore+0x6b/0x74
[  376.158792]  [<ffffffff813c3f1d>] acpi_ns_walk_namespace+0xbe/0x17d
[  376.233854]  [<ffffffff813a5a88>] ? acpi_add_single_object+0x2b9/0x2b9
[  376.312012]  [<ffffffff813a5a88>] ? acpi_add_single_object+0x2b9/0x2b9
[  376.390162]  [<ffffffff813c43b3>] acpi_walk_namespace+0x8a/0xc4
[  376.461051]  [<ffffffff813a5c49>] acpi_bus_scan+0x5b/0x7c
[  376.525707]  [<ffffffff813a5cd6>] acpi_bus_add+0x2a/0x2c
[  376.589344]  [<ffffffff813d5dc6>] container_notify_cb+0x103/0x18d
[  376.662309]  [<ffffffff813b3946>] acpi_ev_notify_dispatch+0x41/0x5f
[  376.737386]  [<ffffffff813a0f47>] acpi_os_execute_deferred+0x27/0x34
[  376.813507]  [<ffffffff81090279>] process_one_work+0x219/0x680
[  376.883357]  [<ffffffff81090218>] ? process_one_work+0x1b8/0x680
[  376.955312]  [<ffffffff813a0f20>] ? acpi_os_wait_events_complete+0x23/0x23
[  377.037615]  [<ffffffff8109212e>] worker_thread+0x12e/0x320
[  377.104365]  [<ffffffff81092000>] ? manage_workers+0x190/0x190
[  377.174274]  [<ffffffff81098106>] kthread+0xc6/0xd0
[  377.232697]  [<ffffffff81678b04>] kernel_thread_helper+0x4/0x10
[  377.303588]  [<ffffffff8166e570>] ? retint_restore_args+0x13/0x13
[  377.376550]  [<ffffffff81098040>] ? __init_kthread_worker+0x70/0x70
[  377.451591]  [<ffffffff81678b00>] ? gs_change+0x13/0x13
[  377.514247] ------------[ cut here ]------------
[  377.569481] WARNING: at lib/kobject.c:166 kobject_add_internal+0x1b3/0x260()
[  377.653796] Hardware name: PRIMEQUEST 1800E
[  377.703865] kobject: (ffffffff82c15718): attempted to be registered with empty name!
[  377.796584] Modules linked in: bridge stp llc sunrpc ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4 iptable_filter ip_tables ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables binfmt_misc vfat fat dm_mirror dm_region_hash dm_log dm_mod uinput iTCO_wdt iTCO_vendor_support coretemp kvm_intel kvm crc32c_intel microcode pcspkr lpc_ich mfd_core i2c_i801 i2c_core ioatdma i7core_edac edac_core sg acpi_memhotplug e1000e igb dca sd_mod crc_t10dif lpfc scsi_transport_fc scsi_tgt mptsas mptscsih mptbase scsi_transport_sas scsi_mod
[  378.400511] Pid: 4, comm: kworker/0:0 Tainted: G        W    3.6.0 #5
[  378.477614] Call Trace:
[  378.506916]  [<ffffffff8106c10f>] warn_slowpath_common+0x7f/0xc0
[  378.578843]  [<ffffffff8106c206>] warn_slowpath_fmt+0x46/0x50
[  378.647658]  [<ffffffff810db09d>] ? mark_held_locks+0x8d/0x140
[  378.717504]  [<ffffffff8133cda3>] kobject_add_internal+0x1b3/0x260
[  378.791507]  [<ffffffff8133cff8>] kobject_add_varg+0x38/0x60
[  378.859187]  [<ffffffff8133d0d4>] kobject_add+0x44/0x70
[  378.921769]  [<ffffffff81098d76>] ? __init_waitqueue_head+0x46/0x60
[  378.996747]  [<ffffffff814371f4>] device_add+0xd4/0x570
[  379.059342]  [<ffffffff814376ae>] device_register+0x1e/0x30
[  379.126042]  [<ffffffff81449b15>] register_node+0x25/0xe0
[  379.190652]  [<ffffffff8144a057>] register_one_node+0x67/0x140
[  379.260532]  [<ffffffff81652e40>] add_memory+0x100/0x1f0
[  379.324175]  [<ffffffffa00a31c6>] acpi_memory_enable_device+0x92/0xdf [acpi_memhotplug]
[  379.419903]  [<ffffffffa00a3753>] acpi_memory_device_add+0x10d/0x116 [acpi_memhotplug]
[  379.514628]  [<ffffffff813a4376>] acpi_device_probe+0x50/0x18a
[  379.584489]  [<ffffffff8124b053>] ? sysfs_create_link+0x13/0x20
[  379.655351]  [<ffffffff81439d7c>] really_probe+0x6c/0x320
[  379.720047]  [<ffffffff8143a077>] driver_probe_device+0x47/0xa0
[  379.790863]  [<ffffffff8143a180>] ? __driver_attach+0xb0/0xb0
[  379.859667]  [<ffffffff8143a180>] ? __driver_attach+0xb0/0xb0
[  379.928526]  [<ffffffff8143a1d3>] __device_attach+0x53/0x60
[  379.995218]  [<ffffffff81437dac>] bus_for_each_drv+0x6c/0xa0
[  380.063010]  [<ffffffff81439cf8>] device_attach+0xa8/0xc0
[  380.127664]  [<ffffffff814389d0>] bus_probe_device+0xb0/0xe0
[  380.195449]  [<ffffffff81437421>] device_add+0x301/0x570
[  380.259081]  [<ffffffff814376ae>] device_register+0x1e/0x30
[  380.325840]  [<ffffffff813a56bf>] acpi_device_register+0x1af/0x2bf
[  380.399852]  [<ffffffff813a59ae>] acpi_add_single_object+0x1df/0x2b9
[  380.475874]  [<ffffffff813ce320>] ? acpi_ut_release_mutex+0xac/0xb5
[  380.550884]  [<ffffffff813a5b93>] acpi_bus_check_add+0x10b/0x166
[  380.622761]  [<ffffffff810db4ad>] ? trace_hardirqs_on+0xd/0x10
[  380.692529]  [<ffffffff8109fa4f>] ? up+0x2f/0x50
[  380.747792]  [<ffffffff813a0cc1>] ? acpi_os_signal_semaphore+0x6b/0x74
[  380.825974]  [<ffffffff813c3f1d>] acpi_ns_walk_namespace+0xbe/0x17d
[  380.901027]  [<ffffffff813a5a88>] ? acpi_add_single_object+0x2b9/0x2b9
[  380.979208]  [<ffffffff813a5a88>] ? acpi_add_single_object+0x2b9/0x2b9
[  381.057328]  [<ffffffff813c43b3>] acpi_walk_namespace+0x8a/0xc4
[  381.128253]  [<ffffffff813a5c49>] acpi_bus_scan+0x5b/0x7c
[  381.192922]  [<ffffffff813a5cd6>] acpi_bus_add+0x2a/0x2c
[  381.256577]  [<ffffffff813d5dc6>] container_notify_cb+0x103/0x18d
[  381.329551]  [<ffffffff813b3946>] acpi_ev_notify_dispatch+0x41/0x5f
[  381.404612]  [<ffffffff813a0f47>] acpi_os_execute_deferred+0x27/0x34
[  381.480709]  [<ffffffff81090279>] process_one_work+0x219/0x680
[  381.550600]  [<ffffffff81090218>] ? process_one_work+0x1b8/0x680
[  381.622535]  [<ffffffff813a0f20>] ? acpi_os_wait_events_complete+0x23/0x23
[  381.704882]  [<ffffffff8109212e>] worker_thread+0x12e/0x320
[  381.771655]  [<ffffffff81092000>] ? manage_workers+0x190/0x190
[  381.841554]  [<ffffffff81098106>] kthread+0xc6/0xd0
[  381.899995]  [<ffffffff81678b04>] kernel_thread_helper+0x4/0x10
[  381.970815]  [<ffffffff8166e570>] ? retint_restore_args+0x13/0x13
[  382.043793]  [<ffffffff81098040>] ? __init_kthread_worker+0x70/0x70
[  382.118880]  [<ffffffff81678b00>] ? gs_change+0x13/0x13
[  382.181437] ---[ end trace a3ee526778d7b765 ]---

Thanks,
Yasuaki Ishimatsu

> --
> 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/
>

Re: [PATCH 2/4] memory-hotplug: add node_device_release

[KOSAKI Motohiro]

> I have the reason to have to fill the node struct with 0 by memset.
> The node is a part of node struct array (node_devices[]).
> If we add empty release function for suppressing warning,
> some data remains in the node struct after hot removing memory.
> So if we re-hot adds the memory, the node struct is reused by
> register_onde_node(). But the node struct has some data, because
> it was not initialized with 0. As a result, more waning is shown
> by the remained data at hot addinig memory as follows:

Even though you call memset(0) at offline. It doesn't guarantee the memory
keep 0 until online. E.g. physical memory exchange during offline, bit
corruption
by cosmic ray, etc. So, you should fill zero at online phase explicitly if need.

The basic hotplug design is: you should forget everything at offline
and you shouldn't
assume any initialized data at online.

Thanks.

[PATCH 3/4] memory-hotplug: clear hwpoisoned flag when onlining pages

[wency]

From: Wen Congyang <wency@cn.fujitsu.com>

hwpoisoned may set when we offline a page by the sysfs interface
/sys/devices/system/memory/soft_offline_page or
/sys/devices/system/memory/hard_offline_page. If we don't clear
this flag when onlining pages, this page can't be freed, and will
not in free list. So we can't offline these pages again. So we
should clear this flag when onlining pages.

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 mm/memory_hotplug.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6a5b90d..9a5b10f 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -431,6 +431,14 @@ EXPORT_SYMBOL_GPL(__online_page_increment_counters);
 
 void __online_page_free(struct page *page)
 {
+#ifdef CONFIG_MEMORY_FAILURE
+	/* The page may be marked HWPoisoned by soft/hard offline page */
+	if (PageHWPoison(page)) {
+		atomic_long_sub(1, &mce_bad_pages);
+		ClearPageHWPoison(page);
+	}
+#endif
+
 	ClearPageReserved(page);
 	init_page_count(page);
 	__free_page(page);
-- 
1.7.1

Re: [PATCH 3/4] memory-hotplug: clear hwpoisoned flag when onlining pages

[Ni zhan Chen]

On 09/27/2012 01:45 PM, wency@cn.fujitsu.com wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
>
> hwpoisoned may set when we offline a page by the sysfs interface
> /sys/devices/system/memory/soft_offline_page or
> /sys/devices/system/memory/hard_offline_page. If we don't clear
> this flag when onlining pages, this page can't be freed, and will
> not in free list. So we can't offline these pages again. So we
> should clear this flag when onlining pages.

page hwpoisoned maybe cause by a multi-bit ECC memory or cache failure, 
so this page should not be used, why you online and free it again? can 
any users use it?

>
> CC: David Rientjes <rientjes@google.com>
> CC: Jiang Liu <liuj97@gmail.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>   mm/memory_hotplug.c |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6a5b90d..9a5b10f 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -431,6 +431,14 @@ EXPORT_SYMBOL_GPL(__online_page_increment_counters);
>   
>   void __online_page_free(struct page *page)
>   {
> +#ifdef CONFIG_MEMORY_FAILURE
> +	/* The page may be marked HWPoisoned by soft/hard offline page */
> +	if (PageHWPoison(page)) {
> +		atomic_long_sub(1, &mce_bad_pages);
> +		ClearPageHWPoison(page);
> +	}
> +#endif
> +
>   	ClearPageReserved(page);
>   	init_page_count(page);
>   	__free_page(page);

Re: [PATCH 3/4] memory-hotplug: clear hwpoisoned flag when onlining pages

[KOSAKI Motohiro]

On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
>
> hwpoisoned may set when we offline a page by the sysfs interface
> /sys/devices/system/memory/soft_offline_page or
> /sys/devices/system/memory/hard_offline_page. If we don't clear
> this flag when onlining pages, this page can't be freed, and will
> not in free list. So we can't offline these pages again. So we
> should clear this flag when onlining pages.

This seems wrong fix to me.  After offline, memory may or may not
change with new one. Thus we can't assume any memory status. Thus,
we should just forget hwpoison status at _offline_ event.

Re: [PATCH 3/4] memory-hotplug: clear hwpoisoned flag when onlining pages

[Wen Congyang]

At 09/28/2012 04:17 AM, KOSAKI Motohiro Wrote:
> On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> hwpoisoned may set when we offline a page by the sysfs interface
>> /sys/devices/system/memory/soft_offline_page or
>> /sys/devices/system/memory/hard_offline_page. If we don't clear
>> this flag when onlining pages, this page can't be freed, and will
>> not in free list. So we can't offline these pages again. So we
>> should clear this flag when onlining pages.
> 
> This seems wrong fix to me.  After offline, memory may or may not
> change with new one. Thus we can't assume any memory status. Thus,
> we should just forget hwpoison status at _offline_ event.
> 

Yes, agree with you. I will update this patch.

Thanks for reviewing.

Wen Congyang

[PATCH 4/4] memory-hotplug: auto offline page_cgroup when onlining memory block failed

[wency]

From: Wen Congyang <wency@cn.fujitsu.com>

When a memory block is onlined, we will try allocate memory on that node
to store page_cgroup. If onlining the memory block failed, we don't
offline the page cgroup, and we have no chance to offline this page cgroup
unless the memory block is onlined successfully again. It will cause
that we can't hot-remove the memory device on that node, because some
memory is used to store page cgroup. If onlining the memory block
is failed, there is no need to stort page cgroup for this memory. So
auto offline page_cgroup when onlining memory block failed.

CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 mm/page_cgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
index 5ddad0c..44db00e 100644
--- a/mm/page_cgroup.c
+++ b/mm/page_cgroup.c
@@ -251,6 +251,9 @@ static int __meminit page_cgroup_callback(struct notifier_block *self,
 				mn->nr_pages, mn->status_change_nid);
 		break;
 	case MEM_CANCEL_ONLINE:
+		offline_page_cgroup(mn->start_pfn,
+				mn->nr_pages, mn->status_change_nid);
+		break;
 	case MEM_GOING_OFFLINE:
 		break;
 	case MEM_ONLINE:
-- 
1.7.1

Re: [PATCH 4/4] memory-hotplug: auto offline page_cgroup when onlining memory block failed

[Ni zhan Chen]

On 09/27/2012 01:45 PM, wency@cn.fujitsu.com wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
>
> When a memory block is onlined, we will try allocate memory on that node
> to store page_cgroup. If onlining the memory block failed, we don't
> offline the page cgroup, and we have no chance to offline this page cgroup
> unless the memory block is onlined successfully again. It will cause
> that we can't hot-remove the memory device on that node, because some
> memory is used to store page cgroup. If onlining the memory block
> is failed, there is no need to stort page cgroup for this memory. So
> auto offline page_cgroup when onlining memory block failed.

looks reasonable to me. thanks.

>
> CC: David Rientjes <rientjes@google.com>
> CC: Jiang Liu <liuj97@gmail.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>   mm/page_cgroup.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
> index 5ddad0c..44db00e 100644
> --- a/mm/page_cgroup.c
> +++ b/mm/page_cgroup.c
> @@ -251,6 +251,9 @@ static int __meminit page_cgroup_callback(struct notifier_block *self,
>   				mn->nr_pages, mn->status_change_nid);
>   		break;
>   	case MEM_CANCEL_ONLINE:
> +		offline_page_cgroup(mn->start_pfn,
> +				mn->nr_pages, mn->status_change_nid);
> +		break;
>   	case MEM_GOING_OFFLINE:
>   		break;
>   	case MEM_ONLINE:

Re: [PATCH 4/4] memory-hotplug: auto offline page_cgroup when onlining memory block failed

[KOSAKI Motohiro]

On Thu, Sep 27, 2012 at 1:45 AM,  <wency@cn.fujitsu.com> wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
>
> When a memory block is onlined, we will try allocate memory on that node
> to store page_cgroup. If onlining the memory block failed, we don't
> offline the page cgroup, and we have no chance to offline this page cgroup
> unless the memory block is onlined successfully again. It will cause
> that we can't hot-remove the memory device on that node, because some
> memory is used to store page cgroup. If onlining the memory block
> is failed, there is no need to stort page cgroup for this memory. So
> auto offline page_cgroup when onlining memory block failed.
>
> CC: David Rientjes <rientjes@google.com>
> CC: Jiang Liu <liuj97@gmail.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Paul Mackerras <paulus@samba.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  mm/page_cgroup.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
> index 5ddad0c..44db00e 100644
> --- a/mm/page_cgroup.c
> +++ b/mm/page_cgroup.c
> @@ -251,6 +251,9 @@ static int __meminit page_cgroup_callback(struct notifier_block *self,
>                                 mn->nr_pages, mn->status_change_nid);
>                 break;
>         case MEM_CANCEL_ONLINE:
> +               offline_page_cgroup(mn->start_pfn,
> +                               mn->nr_pages, mn->status_change_nid);
> +               break;
>         case MEM_GOING_OFFLINE:
>                 break;
>         case MEM_ONLINE:

Looks straight forward and reasonable.
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 0/4] bugfix for memory hotplug

[Andrew Morton]

I'll assume that you will be preparing a new revision of these patches.

We have another patch series today called "memory_hotplug: fix memory
hotplug bug", from another Fujitsu person.  But you people aren't
cc'ing each other.  Please do so and please review and if possible test
each others' work?

I'm assuming that the "memory_hotplug: fix memory hotplug bug" series
will also go through another revision?  I don't think I was cc'ed on
the second patch in that series btw.

Re: [PATCH 0/4] bugfix for memory hotplug

[Ni zhan Chen]

On 09/27/2012 01:45 PM, wency@cn.fujitsu.com wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
>
> Wen Congyang (2):
>    memory-hotplug: clear hwpoisoned flag when onlining pages
>    memory-hotplug: auto offline page_cgroup when onlining memory block
>      failed

Again, you should explain these two patches are the new version of 
memory-hotplug: hot-remove physical memory [20/21,21/21]

>
> Yasuaki Ishimatsu (2):
>    memory-hotplug: add memory_block_release
>    memory-hotplug: add node_device_release
>
>   drivers/base/memory.c |    9 ++++++++-
>   drivers/base/node.c   |   11 +++++++++++
>   mm/memory_hotplug.c   |    8 ++++++++
>   mm/page_cgroup.c      |    3 +++
>   4 files changed, 30 insertions(+), 1 deletions(-)
>
> --
> 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>
>