memory_failure: remove redundant check for the PG_HWPoison flag of 'hpage'

PATCH In review Wang Xiaoqiang 2 participants 4 messages Jul 29, 2015 – Jul 31, 2015

Subsystems (2)

Memory management HWPoison memory failure handling

lore

Share & tools

lore

Wang Xiaoqiang

author

2015-07-29 07:58 UTC #0 lore raw

PATCH 1/1 memory_failure: remove redundant check for the PG_HWPoison flag of 'hpage'

Hi,

I find a little problem in the memory_failure function in mm/memory-failure.c . Please check it.

memory_failure: remove redundant check for the PG_HWPoison flag of `hpage'.

Since we have check the PG_HWPoison flag by PageHWPoison' before, so the later check by TestSetPageHWPoison' must return true, there is no need to check again!

Signed-off-by Wang Xiaoqiang <[email protected]>

---
 mm/memory-failure.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory-failure.c +1 -1

index 1cf7f29..7794fd8 100644
@@ -1115,7 +1115,7 @@ int memory_failure(unsigned long pfn, int trapno, int flags)
 			lock_page(hpage);
 			if (PageHWPoison(hpage)) {
 				if ((hwpoison_filter(p) && TestClearPageHWPoison(p))
-				    || (p != hpage && TestSetPageHWPoison(hpage))) {
+				    || p != hpage) {
 					atomic_long_sub(nr_pages, &num_poisoned_pages);
 					unlock_page(hpage);
 					return 0;

Signature · 7 lines

1.7.10.4



--
thx!
Wang Xiaoqiang

Naoya Horiguchi

2015-07-29 09:18 UTC #1 lore raw

# CC:ed linux-mm

Hi Xiaoqiang,

Quoted 11 lines · Wang Xiaoqiang

Hi,
I find a little problem in the memory_failure function in mm/memory-failure.c . Please check it.
memory_failure: remove redundant check for the PG_HWPoison flag of `hpage'.
Since we have check the PG_HWPoison flag by PageHWPoison' before, so the later check by TestSetPageHWPoison' must return true, there is no need to check again!

I'm afraid that this TestSetPageHWPoison is not redundant, because this code serializes the concurrent memory error events over the same hugetlb page (, where 'p' indicates the 4kB error page and 'hpage' indicates the head page.)

When an error hits a hugetlb page, set_page_hwpoison_huge_page() sets PageHWPoison flags over all subpages of the hugetlb page in the ascending order of pfn. So if we don't have this TestSet, memory error handler can run more than once on concurrent errors when the 1st memory error hits (for example) the 100th subpage and the 2nd memory error hits (for example) the 50th subpage.

Thanks,
Naoya Horiguchi

Quoted 27 lines

Signed-off-by Wang Xiaoqiang <[email protected]>

---
 mm/memory-failure.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory-failure.c +1 -1

index 1cf7f29..7794fd8 100644
@@ -1115,7 +1115,7 @@ int memory_failure(unsigned long pfn, int trapno, int flags)
 			lock_page(hpage);
 			if (PageHWPoison(hpage)) {
 				if ((hwpoison_filter(p) && TestClearPageHWPoison(p))
-				    || (p != hpage && TestSetPageHWPoison(hpage))) {
+				    || p != hpage) {
 					atomic_long_sub(nr_pages, &num_poisoned_pages);
 					unlock_page(hpage);
 					return 0;

Signature · 7 lines

1.7.10.4



--
thx!
Wang Xiaoqiang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Wang Xiaoqiang

author

2015-07-30 02:52 UTC #2 lore raw

Quoted 22 lines · On Wed, 29 Jul 2015 09:17:32 +0000 Naoya Horiguchi <[email protected]>…

# CC:ed linux-mm
Hi Xiaoqiang,
Quoted 11 lines · Wang Xiaoqiang
Hi,
I find a little problem in the memory_failure function in mm/memory-failure.c . Please check it.
memory_failure: remove redundant check for the PG_HWPoison flag of `hpage'.
Since we have check the PG_HWPoison flag by PageHWPoison' before, so the later check by TestSetPageHWPoison' must return true, there is no need to check again!
I'm afraid that this TestSetPageHWPoison is not redundant, because this code serializes the concurrent memory error events over the same hugetlb page (, where 'p' indicates the 4kB error page and 'hpage' indicates the head page.)

Quoted 6 lines

When an error hits a hugetlb page, set_page_hwpoison_huge_page() sets PageHWPoison flags over all subpages of the hugetlb page in the ascending order of pfn. So if we don't have this TestSet, memory error handler can run more than once on concurrent errors when the 1st memory error hits (for example) the 100th subpage and the 2nd memory error hits (for example) the 50th subpage.

In your example, the 100th subage enter the memory error handler firstly, and then it uses the set_page_hwpoison_huge_page to set all subpages with PG_HWPoison flag, the 50th page handler waits for grab the lock_page(hpage) now.

When the 100th page handler unlock the 'hpage', the 50th grab it, and now the 'hapge' has been set with PG_HWPosison. So PageHWPoison micro will return true, and the following code will be executed:

if (PageHWPoison(hpage)) {
    if ((hwpoison_filter(p) && TestClearPageHWPoison(p))
        || (p != hpage && TestSetPageHWPoison(hpage))) {
        atomic_long_sub(nr_pages, &num_poisoned_pages);
        unlock_page(hpage);
        return 0;
    }   
}

Now 'p' is 50th subpage, it doesn't equal the 'hpage' obviously, so if we don't have TestSetPageHWPoison here, it still will ignore the 50th error.
Why the memory error handler can run more than once? Hope to receive from you!

thx,
Wang Xiaoqiang

Quoted 33 lines

Thanks,
Naoya Horiguchi

Quoted 29 lines

Signed-off-by Wang Xiaoqiang <[email protected]>

---
 mm/memory-failure.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memory-failure.c +1 -1

index 1cf7f29..7794fd8 100644
@@ -1115,7 +1115,7 @@ int memory_failure(unsigned long pfn, int
 trapno, int flags) lock_page(hpage);
 			if (PageHWPoison(hpage)) {
 				if ((hwpoison_filter(p) &&
 TestClearPageHWPoison(p))
-				    || (p != hpage &&
 TestSetPageHWPoison(hpage))) {
+				    || p != hpage) {
 					atomic_long_sub(nr_pages,

&num_poisoned_pages); unlock_page(hpage);
return 0;

Signature · 7 lines

1.7.10.4



--
thx!
Wang Xiaoqiang

Naoya Horiguchi

Acked-by

2015-07-31 08:13 UTC #3 lore raw

On Thu, Jul 30, 2015 at 10:52:46AM +0800, Wang Xiaoqiang wrote:
...

Quoted 21 lines

In your example, the 100th subage enter the memory error handler firstly, and then it uses the set_page_hwpoison_huge_page to set all subpages with PG_HWPoison flag, the 50th page handler waits for grab the lock_page(hpage) now.
When the 100th page handler unlock the 'hpage', the 50th grab it, and now the 'hapge' has been set with PG_HWPosison. So PageHWPoison micro
will return true, and the following code will
be executed:
if (PageHWPoison(hpage)) {
    if ((hwpoison_filter(p) && TestClearPageHWPoison(p))
        || (p != hpage && TestSetPageHWPoison(hpage))) {
        atomic_long_sub(nr_pages, &num_poisoned_pages);
        unlock_page(hpage);
        return 0;
    }   
}

Quoted 3 lines

Now 'p' is 50th subpage, it doesn't equal the 'hpage' obviously, so if we don't have TestSetPageHWPoison here, it still will ignore the 50th error.

Ah, you're right, thanks for the explanation, Xiaoqiang!

Acked-by Naoya Horiguchi <[email protected]>