[PATCH v2 1/3] mm/hwpoison: fix bug triggered by unpoison empty zero page

[PATCH v2 1/3] mm/hwpoison: fix bug triggered by unpoison empty zero page

[Wanpeng Li]

v1 -> v2:
 * ignore empty zero page for madvise_hwpoison directly

[   57.579580] Injecting memory failure for page 0x19d0 at 0xb77d2000
[   57.579824] MCE 0x19d0: non LRU page recovery: Ignored
[   91.290453] MCE: Software-unpoisoned page 0x19d0
[   91.290456] BUG: Bad page state in process bash  pfn:019d0
[   91.290466] page:f3461a00 count:0 mapcount:0 mapping:  (null) index:0x0
[   91.290467] page flags: 0x40000404(referenced|reserved)
[   91.290469] Modules linked in: nfsd auth_rpcgss i915 nfs_acl nfs lockd video drm_kms_helper drm bnep rfcomm sunrpc bluetooth psmouse parport_pc ppdev lp serio_raw fscache parport gpio_ich lpc_ich mac_hid i2c_algo_bit tpm_tis wmi usb_storage hid_generic usbhid hid e1000e firewire_ohci firewire_core ahci ptp libahci pps_core crc_itu_t
[   91.290486] CPU: 3 PID: 2123 Comm: bash Not tainted 3.11.0-rc6+ #12
[   91.290487] Hardware name: LENOVO 7034DD7/        , BIOS 9HKT47AUS 01//2012
[   91.290488]  00000000 00000000 e9625ea0 c15ec49b f3461a00 e9625eb8 c15ea119 c17cbf18
[   91.290491]  ef084314 000019d0 f3461a00 e9625ed8 c110dc8a f3461a00 00000001 00000000
[   91.290494]  f3461a00 40000404 00000000 e9625ef8 c110dcc1 f3461a00 f3461a00 000019d0
[   91.290497] Call Trace:
[   91.290501]  [<c15ec49b>] dump_stack+0x41/0x52
[   91.290504]  [<c15ea119>] bad_page+0xcf/0xeb
[   91.290515]  [<c110dc8a>] free_pages_prepare+0x12a/0x140
[   91.290517]  [<c110dcc1>] free_hot_cold_page+0x21/0x110
[   91.290519]  [<c11123c1>] __put_single_page+0x21/0x30
[   91.290521]  [<c1112815>] put_page+0x25/0x40
[   91.290524]  [<c11544e7>] unpoison_memory+0x107/0x200
[   91.290526]  [<c104a537>] ? ns_capable+0x27/0x60
[   91.290528]  [<c1155720>] hwpoison_unpoison+0x20/0x30
[   91.290530]  [<c1178266>] simple_attr_write+0xb6/0xd0
[   91.290532]  [<c11781b0>] ? generic_fh_to_dentry+0x50/0x50
[   91.290535]  [<c1158c60>] vfs_write+0xa0/0x1b0
[   91.290537]  [<c11781b0>] ? generic_fh_to_dentry+0x50/0x50
[   91.290539]  [<c11590df>] SyS_write+0x4f/0x90
[   91.290549]  [<c15f9a81>] sysenter_do_call+0x12/0x22
[   91.290550] Disabling lock debugging due to kernel taint

Testcase:

#define _GNU_SOURCE
#include <stdlib.h>
#include <stdio.h>
#include <sys/mman.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h>
#include <errno.h>

#define PAGES_TO_TEST 1
#define PAGE_SIZE	4096

int main(void)
{
	char *mem;

	mem = mmap(NULL, PAGES_TO_TEST * PAGE_SIZE,
			PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);

	if (madvise(mem, PAGES_TO_TEST * PAGE_SIZE, MADV_HWPOISON) == -1)
		return -1;

	munmap(mem, PAGES_TO_TEST * PAGE_SIZE);

	return 0;
}

There is one page reference count for default empty zero page, madvise_hwpoison
add another one by get_user_pages_fast. memory_hwpoison reduce one page reference
count since it's a non LRU page. unpoison_memory release the last page reference
count and free empty zero page to buddy system which is not correct since empty
zero page has PG_reserved flag. This patch fix it by ignore empty zero page for 
madvise_hwpoison directly. 

Suggested-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/madvise.c        | 2 ++
 mm/memory-failure.c | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/mm/madvise.c b/mm/madvise.c
index 212f5f1..a20764c 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -352,6 +352,8 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
 		int ret = get_user_pages_fast(start, 1, 0, &p);
 		if (ret != 1)
 			return ret;
+		if (page_to_pfn(p) == my_zero_pfn(0))
+			continue;
 		if (bhv == MADV_SOFT_OFFLINE) {
 			pr_info("Soft offlining page %#lx at %#lx\n",
 				page_to_pfn(p), start);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 7cdabc0..68cbca0 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1331,6 +1331,9 @@ int unpoison_memory(unsigned long pfn)
 	if (!pfn_valid(pfn))
 		return -ENXIO;
 
+	if (pfn == my_zero_pfn(0))
+		return 0;
+
 	p = pfn_to_page(pfn);
 	page = compound_head(p);
 
-- 
1.8.1.2

--
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 v2 2/3] mm/hwpoison: change permission of corrupt-pfn/unpoison-pfn to 0200

[Wanpeng Li]

Hwpoison inject doesn't implement read method for corrupt-pfn/unpoison-pfn
attributes:

# cat /sys/kernel/debug/hwpoison/corrupt-pfn
cat: /sys/kernel/debug/hwpoison/corrupt-pfn: Permission denied
# cat /sys/kernel/debug/hwpoison/unpoison-pfn
cat: /sys/kernel/debug/hwpoison/unpoison-pfn: Permission denied

This patch change the permission of corrupt-pfn/unpoison-pfn to 0200.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/hwpoison-inject.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
index 3a61efc..afc2daa 100644
--- a/mm/hwpoison-inject.c
+++ b/mm/hwpoison-inject.c
@@ -88,12 +88,12 @@ static int pfn_inject_init(void)
 	 * hardware status change, hence do not require hardware support.
 	 * They are mainly for testing hwpoison in software level.
 	 */
-	dentry = debugfs_create_file("corrupt-pfn", 0600, hwpoison_dir,
+	dentry = debugfs_create_file("corrupt-pfn", 0200, hwpoison_dir,
 					  NULL, &hwpoison_fops);
 	if (!dentry)
 		goto fail;
 
-	dentry = debugfs_create_file("unpoison-pfn", 0600, hwpoison_dir,
+	dentry = debugfs_create_file("unpoison-pfn", 0200, hwpoison_dir,
 				     NULL, &unpoison_fops);
 	if (!dentry)
 		goto fail;
-- 
1.8.1.2

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Wanpeng Li]

The return value outside for loop is always zero which means madvise_hwpoison 
return success, however, this is not truth for soft_offline_page w/ failure
return value.

Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
---
 mm/madvise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index a20764c..19b71e4 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -359,7 +359,7 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
 				page_to_pfn(p), start);
 			ret = soft_offline_page(p, MF_COUNT_INCREASED);
 			if (ret)
-				break;
+				return ret;
 			continue;
 		}
 		pr_info("Injecting memory failure for page %#lx at %#lx\n",
-- 
1.8.1.2

--
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 v2 1/3] mm/hwpoison: fix bug triggered by unpoison empty zero page

[Naoya Horiguchi]

On Tue, Aug 27, 2013 at 10:39:29AM +0800, Wanpeng Li wrote:
> v1 -> v2:
>  * ignore empty zero page for madvise_hwpoison directly
> 
> [   57.579580] Injecting memory failure for page 0x19d0 at 0xb77d2000
> [   57.579824] MCE 0x19d0: non LRU page recovery: Ignored
> [   91.290453] MCE: Software-unpoisoned page 0x19d0
> [   91.290456] BUG: Bad page state in process bash  pfn:019d0
> [   91.290466] page:f3461a00 count:0 mapcount:0 mapping:  (null) index:0x0
> [   91.290467] page flags: 0x40000404(referenced|reserved)
> [   91.290469] Modules linked in: nfsd auth_rpcgss i915 nfs_acl nfs lockd video drm_kms_helper drm bnep rfcomm sunrpc bluetooth psmouse parport_pc ppdev lp serio_raw fscache parport gpio_ich lpc_ich mac_hid i2c_algo_bit tpm_tis wmi usb_storage hid_generic usbhid hid e1000e firewire_ohci firewire_core ahci ptp libahci pps_core crc_itu_t
> [   91.290486] CPU: 3 PID: 2123 Comm: bash Not tainted 3.11.0-rc6+ #12
> [   91.290487] Hardware name: LENOVO 7034DD7/        , BIOS 9HKT47AUS 01//2012
> [   91.290488]  00000000 00000000 e9625ea0 c15ec49b f3461a00 e9625eb8 c15ea119 c17cbf18
> [   91.290491]  ef084314 000019d0 f3461a00 e9625ed8 c110dc8a f3461a00 00000001 00000000
> [   91.290494]  f3461a00 40000404 00000000 e9625ef8 c110dcc1 f3461a00 f3461a00 000019d0
> [   91.290497] Call Trace:
> [   91.290501]  [<c15ec49b>] dump_stack+0x41/0x52
> [   91.290504]  [<c15ea119>] bad_page+0xcf/0xeb
> [   91.290515]  [<c110dc8a>] free_pages_prepare+0x12a/0x140
> [   91.290517]  [<c110dcc1>] free_hot_cold_page+0x21/0x110
> [   91.290519]  [<c11123c1>] __put_single_page+0x21/0x30
> [   91.290521]  [<c1112815>] put_page+0x25/0x40
> [   91.290524]  [<c11544e7>] unpoison_memory+0x107/0x200
> [   91.290526]  [<c104a537>] ? ns_capable+0x27/0x60
> [   91.290528]  [<c1155720>] hwpoison_unpoison+0x20/0x30
> [   91.290530]  [<c1178266>] simple_attr_write+0xb6/0xd0
> [   91.290532]  [<c11781b0>] ? generic_fh_to_dentry+0x50/0x50
> [   91.290535]  [<c1158c60>] vfs_write+0xa0/0x1b0
> [   91.290537]  [<c11781b0>] ? generic_fh_to_dentry+0x50/0x50
> [   91.290539]  [<c11590df>] SyS_write+0x4f/0x90
> [   91.290549]  [<c15f9a81>] sysenter_do_call+0x12/0x22
> [   91.290550] Disabling lock debugging due to kernel taint
> 
> Testcase:
> 
> #define _GNU_SOURCE
> #include <stdlib.h>
> #include <stdio.h>
> #include <sys/mman.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <sys/types.h>
> #include <errno.h>
> 
> #define PAGES_TO_TEST 1
> #define PAGE_SIZE	4096
> 
> int main(void)
> {
> 	char *mem;
> 
> 	mem = mmap(NULL, PAGES_TO_TEST * PAGE_SIZE,
> 			PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> 
> 	if (madvise(mem, PAGES_TO_TEST * PAGE_SIZE, MADV_HWPOISON) == -1)
> 		return -1;
> 
> 	munmap(mem, PAGES_TO_TEST * PAGE_SIZE);
> 
> 	return 0;
> }
> 
> There is one page reference count for default empty zero page, madvise_hwpoison
> add another one by get_user_pages_fast. memory_hwpoison reduce one page reference
> count since it's a non LRU page. unpoison_memory release the last page reference
> count and free empty zero page to buddy system which is not correct since empty
> zero page has PG_reserved flag. This patch fix it by ignore empty zero page for 
> madvise_hwpoison directly. 
> 
> Suggested-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>

Thank you.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

> ---
>  mm/madvise.c        | 2 ++
>  mm/memory-failure.c | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 212f5f1..a20764c 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -352,6 +352,8 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
>  		int ret = get_user_pages_fast(start, 1, 0, &p);
>  		if (ret != 1)
>  			return ret;
> +		if (page_to_pfn(p) == my_zero_pfn(0))
> +			continue;
>  		if (bhv == MADV_SOFT_OFFLINE) {
>  			pr_info("Soft offlining page %#lx at %#lx\n",
>  				page_to_pfn(p), start);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7cdabc0..68cbca0 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1331,6 +1331,9 @@ int unpoison_memory(unsigned long pfn)
>  	if (!pfn_valid(pfn))
>  		return -ENXIO;
>  
> +	if (pfn == my_zero_pfn(0))
> +		return 0;
> +
>  	p = pfn_to_page(pfn);
>  	page = compound_head(p);

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Naoya Horiguchi]

On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
> The return value outside for loop is always zero which means madvise_hwpoison 
> return success, however, this is not truth for soft_offline_page w/ failure
> return value.

I don't understand what you want to do for what reason. Could you clarify
those?

> 
> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
> ---
>  mm/madvise.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index a20764c..19b71e4 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -359,7 +359,7 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
>  				page_to_pfn(p), start);
>  			ret = soft_offline_page(p, MF_COUNT_INCREASED);
>  			if (ret)
> -				break;
> +				return ret;
>  			continue;
>  		}
>  		pr_info("Injecting memory failure for page %#lx at %#lx\n",

This seems to introduce no behavioral change.

Thanks,
Naoya Horiguchi

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Wanpeng Li]

Hi Naoya,
On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
>On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
>> The return value outside for loop is always zero which means madvise_hwpoison 
>> return success, however, this is not truth for soft_offline_page w/ failure
>> return value.
>
>I don't understand what you want to do for what reason. Could you clarify
>those?

int ret is defined in two place in madvise_hwpoison. One is out of for
loop and its value is always zero(zero means success for madvise), the 
other one is in for loop. The soft_offline_page function maybe return 
-EBUSY and break, however, the ret out of for loop is return which means 
madvise_hwpoison success. 

Regards,
Wanpeng Li 

>
>> 
>> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
>> ---
>>  mm/madvise.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index a20764c..19b71e4 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -359,7 +359,7 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
>>  				page_to_pfn(p), start);
>>  			ret = soft_offline_page(p, MF_COUNT_INCREASED);
>>  			if (ret)
>> -				break;
>> +				return ret;
>>  			continue;
>>  		}
>>  		pr_info("Injecting memory failure for page %#lx at %#lx\n",
>
>This seems to introduce no behavioral change.
>
>Thanks,
>Naoya Horiguchi

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Wanpeng Li]

Hi Naoya,
On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
>On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
>> The return value outside for loop is always zero which means madvise_hwpoison 
>> return success, however, this is not truth for soft_offline_page w/ failure
>> return value.
>
>I don't understand what you want to do for what reason. Could you clarify
>those?

int ret is defined in two place in madvise_hwpoison. One is out of for
loop and its value is always zero(zero means success for madvise), the 
other one is in for loop. The soft_offline_page function maybe return 
-EBUSY and break, however, the ret out of for loop is return which means 
madvise_hwpoison success. 

Regards,
Wanpeng Li 

>
>> 
>> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
>> ---
>>  mm/madvise.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index a20764c..19b71e4 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -359,7 +359,7 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
>>  				page_to_pfn(p), start);
>>  			ret = soft_offline_page(p, MF_COUNT_INCREASED);
>>  			if (ret)
>> -				break;
>> +				return ret;
>>  			continue;
>>  		}
>>  		pr_info("Injecting memory failure for page %#lx at %#lx\n",
>
>This seems to introduce no behavioral change.
>
>Thanks,
>Naoya Horiguchi

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Naoya Horiguchi]

On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
> Hi Naoya,
> On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
> >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
> >> The return value outside for loop is always zero which means madvise_hwpoison 
> >> return success, however, this is not truth for soft_offline_page w/ failure
> >> return value.
> >
> >I don't understand what you want to do for what reason. Could you clarify
> >those?
> 
> int ret is defined in two place in madvise_hwpoison. One is out of for
> loop and its value is always zero(zero means success for madvise), the 
> other one is in for loop. The soft_offline_page function maybe return 
> -EBUSY and break, however, the ret out of for loop is return which means 
> madvise_hwpoison success. 

Oh, I see. Thanks.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>


> Regards,
> Wanpeng Li 
> 
> >
> >> 
> >> Signed-off-by: Wanpeng Li <liwanp@linux.vnet.ibm.com>
> >> ---
> >>  mm/madvise.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/mm/madvise.c b/mm/madvise.c
> >> index a20764c..19b71e4 100644
> >> --- a/mm/madvise.c
> >> +++ b/mm/madvise.c
> >> @@ -359,7 +359,7 @@ static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
> >>  				page_to_pfn(p), start);
> >>  			ret = soft_offline_page(p, MF_COUNT_INCREASED);
> >>  			if (ret)
> >> -				break;
> >> +				return ret;
> >>  			continue;
> >>  		}
> >>  		pr_info("Injecting memory failure for page %#lx at %#lx\n",
> >
> >This seems to introduce no behavioral change.
> >
> >Thanks,
> >Naoya Horiguchi
> 
> --
> 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>
>

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Chen Gong]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

On Mon, Aug 26, 2013 at 11:41:36PM -0400, Naoya Horiguchi wrote:
> Date: Mon, 26 Aug 2013 23:41:36 -0400
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> To: Wanpeng Li <liwanp@linux.vnet.ibm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>, Andi Kleen
>  <andi@firstfloor.org>, Fengguang Wu <fengguang.wu@intel.com>, Tony Luck
>  <tony.luck@intel.com>, gong.chen@linux.intel.com, linux-mm@kvack.org,
>  linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] mm/hwpoison: fix return value of
>  madvise_hwpoison
> User-Agent: Mutt 1.5.21 (2010-09-15)
> 
> On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
> > Hi Naoya,
> > On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
> > >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
> > >> The return value outside for loop is always zero which means madvise_hwpoison 
> > >> return success, however, this is not truth for soft_offline_page w/ failure
> > >> return value.
> > >
> > >I don't understand what you want to do for what reason. Could you clarify
> > >those?
> > 
> > int ret is defined in two place in madvise_hwpoison. One is out of for
> > loop and its value is always zero(zero means success for madvise), the 
> > other one is in for loop. The soft_offline_page function maybe return 
> > -EBUSY and break, however, the ret out of for loop is return which means 
> > madvise_hwpoison success. 
> 
> Oh, I see. Thanks.
> 
I don't think such change is a good idea. The original code is obviously
easy to confuse people. Why not removing redundant local variable?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Wanpeng Li]

Hi Chen,
On Tue, Aug 27, 2013 at 03:37:01AM -0400, Chen Gong wrote:
>> On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
>> > Hi Naoya,
>> > On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
>> > >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
>> > >> The return value outside for loop is always zero which means madvise_hwpoison 
>> > >> return success, however, this is not truth for soft_offline_page w/ failure
>> > >> return value.
>> > >
>> > >I don't understand what you want to do for what reason. Could you clarify
>> > >those?
>> > 
>> > int ret is defined in two place in madvise_hwpoison. One is out of for
>> > loop and its value is always zero(zero means success for madvise), the 
>> > other one is in for loop. The soft_offline_page function maybe return 
>> > -EBUSY and break, however, the ret out of for loop is return which means 
>> > madvise_hwpoison success. 
>> 
>> Oh, I see. Thanks.
>> 
>I don't think such change is a good idea. The original code is obviously
>easy to confuse people. Why not removing redundant local variable?
>

I think the trick here is get_user_pages_fast will return the number of
pages pinned. It is always 1 in madvise_hwpoison, the return value of 
memory_failure is ignored. Therefore we still need to reset ret to 0 
before return madvise_hwpoison. 

Regards,
Wanpeng Li 

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Wanpeng Li]

Hi Chen,
On Tue, Aug 27, 2013 at 03:37:01AM -0400, Chen Gong wrote:
>> On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
>> > Hi Naoya,
>> > On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
>> > >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
>> > >> The return value outside for loop is always zero which means madvise_hwpoison 
>> > >> return success, however, this is not truth for soft_offline_page w/ failure
>> > >> return value.
>> > >
>> > >I don't understand what you want to do for what reason. Could you clarify
>> > >those?
>> > 
>> > int ret is defined in two place in madvise_hwpoison. One is out of for
>> > loop and its value is always zero(zero means success for madvise), the 
>> > other one is in for loop. The soft_offline_page function maybe return 
>> > -EBUSY and break, however, the ret out of for loop is return which means 
>> > madvise_hwpoison success. 
>> 
>> Oh, I see. Thanks.
>> 
>I don't think such change is a good idea. The original code is obviously
>easy to confuse people. Why not removing redundant local variable?
>

I think the trick here is get_user_pages_fast will return the number of
pages pinned. It is always 1 in madvise_hwpoison, the return value of 
memory_failure is ignored. Therefore we still need to reset ret to 0 
before return madvise_hwpoison. 

Regards,
Wanpeng Li 

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Wanpeng Li]

Hi Chen,
On Tue, Aug 27, 2013 at 03:37:01AM -0400, Chen Gong wrote:
>On Mon, Aug 26, 2013 at 11:41:36PM -0400, Naoya Horiguchi wrote:
>> Date: Mon, 26 Aug 2013 23:41:36 -0400
>> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> To: Wanpeng Li <liwanp@linux.vnet.ibm.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>, Andi Kleen
>>  <andi@firstfloor.org>, Fengguang Wu <fengguang.wu@intel.com>, Tony Luck
>>  <tony.luck@intel.com>, gong.chen@linux.intel.com, linux-mm@kvack.org,
>>  linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v2 3/3] mm/hwpoison: fix return value of
>>  madvise_hwpoison
>> User-Agent: Mutt 1.5.21 (2010-09-15)
>> 
>> On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
>> > Hi Naoya,
>> > On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
>> > >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
>> > >> The return value outside for loop is always zero which means madvise_hwpoison 
>> > >> return success, however, this is not truth for soft_offline_page w/ failure
>> > >> return value.
>> > >
>> > >I don't understand what you want to do for what reason. Could you clarify
>> > >those?
>> > 
>> > int ret is defined in two place in madvise_hwpoison. One is out of for
>> > loop and its value is always zero(zero means success for madvise), the 
>> > other one is in for loop. The soft_offline_page function maybe return 
>> > -EBUSY and break, however, the ret out of for loop is return which means 
>> > madvise_hwpoison success. 
>> 
>> Oh, I see. Thanks.
>> 
>I don't think such change is a good idea. The original code is obviously
>easy to confuse people. Why not removing redundant local variable?
>

I think the trick here is get_user_pages_fast will return the number of
pages pinned. It is always 1 in madvise_hwpoison, the return value of
memory_failure is ignored. Therefore we still need to reset ret to 0
before return madvise_hwpoison.

Regards,
Wanpeng Li

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Wanpeng Li]

Hi Chen,
On Tue, Aug 27, 2013 at 03:37:01AM -0400, Chen Gong wrote:
>On Mon, Aug 26, 2013 at 11:41:36PM -0400, Naoya Horiguchi wrote:
>> Date: Mon, 26 Aug 2013 23:41:36 -0400
>> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> To: Wanpeng Li <liwanp@linux.vnet.ibm.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>, Andi Kleen
>>  <andi@firstfloor.org>, Fengguang Wu <fengguang.wu@intel.com>, Tony Luck
>>  <tony.luck@intel.com>, gong.chen@linux.intel.com, linux-mm@kvack.org,
>>  linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v2 3/3] mm/hwpoison: fix return value of
>>  madvise_hwpoison
>> User-Agent: Mutt 1.5.21 (2010-09-15)
>> 
>> On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
>> > Hi Naoya,
>> > On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
>> > >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
>> > >> The return value outside for loop is always zero which means madvise_hwpoison 
>> > >> return success, however, this is not truth for soft_offline_page w/ failure
>> > >> return value.
>> > >
>> > >I don't understand what you want to do for what reason. Could you clarify
>> > >those?
>> > 
>> > int ret is defined in two place in madvise_hwpoison. One is out of for
>> > loop and its value is always zero(zero means success for madvise), the 
>> > other one is in for loop. The soft_offline_page function maybe return 
>> > -EBUSY and break, however, the ret out of for loop is return which means 
>> > madvise_hwpoison success. 
>> 
>> Oh, I see. Thanks.
>> 
>I don't think such change is a good idea. The original code is obviously
>easy to confuse people. Why not removing redundant local variable?
>

I think the trick here is get_user_pages_fast will return the number of
pages pinned. It is always 1 in madvise_hwpoison, the return value of
memory_failure is ignored. Therefore we still need to reset ret to 0
before return madvise_hwpoison.

Regards,
Wanpeng Li

--
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 v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Chen Gong]

[-- Attachment #1: Type: text/plain, Size: 2796 bytes --]

On Tue, Aug 27, 2013 at 04:05:23PM +0800, Wanpeng Li wrote:
> Date: Tue, 27 Aug 2013 16:05:23 +0800
> From: Wanpeng Li <liwanp@linux.vnet.ibm.com>
> To: Chen Gong <gong.chen@linux.intel.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>, Andrew Morton
>  <akpm@linux-foundation.org>, Andi Kleen <andi@firstfloor.org>, Fengguang
>  Wu <fengguang.wu@intel.com>, Tony Luck <tony.luck@intel.com>,
>  linux-mm@kvack.org, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] mm/hwpoison: fix return value of
>  madvise_hwpoison
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> Hi Chen,
> On Tue, Aug 27, 2013 at 03:37:01AM -0400, Chen Gong wrote:
> >On Mon, Aug 26, 2013 at 11:41:36PM -0400, Naoya Horiguchi wrote:
> >> Date: Mon, 26 Aug 2013 23:41:36 -0400
> >> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> >> To: Wanpeng Li <liwanp@linux.vnet.ibm.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>, Andi Kleen
> >>  <andi@firstfloor.org>, Fengguang Wu <fengguang.wu@intel.com>, Tony Luck
> >>  <tony.luck@intel.com>, gong.chen@linux.intel.com, linux-mm@kvack.org,
> >>  linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v2 3/3] mm/hwpoison: fix return value of
> >>  madvise_hwpoison
> >> User-Agent: Mutt 1.5.21 (2010-09-15)
> >> 
> >> On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
> >> > Hi Naoya,
> >> > On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
> >> > >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
> >> > >> The return value outside for loop is always zero which means madvise_hwpoison 
> >> > >> return success, however, this is not truth for soft_offline_page w/ failure
> >> > >> return value.
> >> > >
> >> > >I don't understand what you want to do for what reason. Could you clarify
> >> > >those?
> >> > 
> >> > int ret is defined in two place in madvise_hwpoison. One is out of for
> >> > loop and its value is always zero(zero means success for madvise), the 
> >> > other one is in for loop. The soft_offline_page function maybe return 
> >> > -EBUSY and break, however, the ret out of for loop is return which means 
> >> > madvise_hwpoison success. 
> >> 
> >> Oh, I see. Thanks.
> >> 
> >I don't think such change is a good idea. The original code is obviously
> >easy to confuse people. Why not removing redundant local variable?
> >
> 
> I think the trick here is get_user_pages_fast will return the number of
> pages pinned. It is always 1 in madvise_hwpoison, the return value of
> memory_failure is ignored. Therefore we still need to reset ret to 0
> before return madvise_hwpoison.
> 
> Regards,
> Wanpeng Li
> 
It looks like the original author wrote in that way deliberately but
botching it. FWIW, I just think its harmness is more than good.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 3/3] mm/hwpoison: fix return value of madvise_hwpoison

[Andrew Morton]

On Tue, 27 Aug 2013 16:02:29 +0800 Wanpeng Li <liwanp@linux.vnet.ibm.com> wrote:

> Hi Chen,
> On Tue, Aug 27, 2013 at 03:37:01AM -0400, Chen Gong wrote:
> >> On Tue, Aug 27, 2013 at 11:38:27AM +0800, Wanpeng Li wrote:
> >> > Hi Naoya,
> >> > On Mon, Aug 26, 2013 at 11:28:16PM -0400, Naoya Horiguchi wrote:
> >> > >On Tue, Aug 27, 2013 at 10:39:31AM +0800, Wanpeng Li wrote:
> >> > >> The return value outside for loop is always zero which means madvise_hwpoison 
> >> > >> return success, however, this is not truth for soft_offline_page w/ failure
> >> > >> return value.
> >> > >
> >> > >I don't understand what you want to do for what reason. Could you clarify
> >> > >those?
> >> > 
> >> > int ret is defined in two place in madvise_hwpoison. One is out of for
> >> > loop and its value is always zero(zero means success for madvise), the 
> >> > other one is in for loop. The soft_offline_page function maybe return 
> >> > -EBUSY and break, however, the ret out of for loop is return which means 
> >> > madvise_hwpoison success. 
> >> 
> >> Oh, I see. Thanks.
> >> 
> >I don't think such change is a good idea. The original code is obviously
> >easy to confuse people. Why not removing redundant local variable?
> >
> 
> I think the trick here is get_user_pages_fast will return the number of
> pages pinned. It is always 1 in madvise_hwpoison, the return value of 
> memory_failure is ignored. Therefore we still need to reset ret to 0 
> before return madvise_hwpoison. 

erk, madvise_hwpoison() has two locals with the same name.   Bad.


From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm/madvise.c:madvise_hwpoison(): remove local `ret'

madvise_hwpoison() has two locals called "ret".  Fix it all up.

Cc: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/madvise.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff -puN mm/madvise.c~a mm/madvise.c
--- a/mm/madvise.c~a
+++ a/mm/madvise.c
@@ -343,15 +343,16 @@ static long madvise_remove(struct vm_are
  */
 static int madvise_hwpoison(int bhv, unsigned long start, unsigned long end)
 {
-	int ret = 0;
-
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	for (; start < end; start += PAGE_SIZE) {
 		struct page *p;
-		int ret = get_user_pages_fast(start, 1, 0, &p);
+		int ret;
+		
+		ret = get_user_pages_fast(start, 1, 0, &p);
 		if (ret != 1)
 			return ret;
+
 		if (PageHWPoison(p)) {
 			put_page(p);
 			continue;
@@ -369,7 +370,7 @@ static int madvise_hwpoison(int bhv, uns
 		/* Ignore return value for now */
 		memory_failure(page_to_pfn(p), 0, MF_COUNT_INCREASED);
 	}
-	return ret;
+	return 0;
 }
 #endif
 
_

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