MCE: fix an error of mce_bad_pages statistics

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

Signed-off-by Xishi Qiu <[email protected]>

i>>?Signed-off-by: Jiang Liu <[email protected]>

---
 mm/memory-failure.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

mm/memory-failure.c +0 -0

index 8b20278..de760ca 100644

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

.

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

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

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 5 lines

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

.

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 9 lines

Quoted 5 lines

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

.

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

Quoted 85 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

Quoted 4 lines

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 86 lines · Simon Jeons

Quoted 85 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

Quoted 7 lines

Quoted 4 lines

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Hi Simon,

get_any_page() -> "else if(is_free_buddy_page(p))" branch is *not* redundancy.

It is another topic, I mean since the page is poisoned, so why not isolate it from page buddy alocator in soft_offline_page() rather than in check_new_page().

I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Quoted 10 lines

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 98 lines · Xishi Qiu

Quoted 86 lines · Simon Jeons

Quoted 85 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

Quoted 7 lines

Quoted 4 lines

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Hi Simon,

Quoted 8 lines

get_any_page() -> "else if(is_free_buddy_page(p))" branch is *not* redundancy.

It is another topic, I mean since the page is poisoned, so why not isolate it from page buddy alocator in soft_offline_page() rather than in check_new_page().

I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Hi Xishi,

HWPoison delays any action on buddy allocator pages, handling can be safely postponed until a later time when the page might be referenced. By delaying, some transient errors may not reoccur or may be irrelevant.

Regards,
Wanpeng Li

Quoted 11 lines

Quoted 10 lines

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

.

One more question, can we add a list_head to manager the poisoned pages?

HWPoison delays any action on buddy allocator pages, handling can be safely postponed until a later time when the page might be referenced. By delaying, some transient errors may not reoccur or may be irrelevant.

Quoted 86 lines · Simon Jeons

Quoted 85 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

Quoted 7 lines

Quoted 4 lines

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Hi Simon,

get_any_page() -> "else if(is_free_buddy_page(p))" branch is *not* redundancy.

It is another topic, I mean since the page is poisoned, so why not isolate it from page buddy alocator in soft_offline_page() rather than in check_new_page().

I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Quoted 10 lines

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 91 lines · Simon Jeons

Quoted 89 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Quoted 2 lines

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Hi Simon,

get_any_page() -> "else if(is_free_buddy_page(p))" branch is *not* redundancy.

It is another topic, I mean since the page is poisoned, so why not isolate it

from page buddy alocator in soft_offline_page() rather than in check_new_page().

I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Quoted 10 lines

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 97 lines · Simon Jeons

Quoted 96 lines · Xishi Qiu

Quoted 93 lines · Simon Jeons

Quoted 82 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

Quoted 7 lines

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Quoted 9 lines

Quoted 5 lines

Hi Simon,

get_any_page() -> "else if(is_free_buddy_page(p))" branch is *not* redundancy.

It is another topic, I mean since the page is poisoned, so why not isolate it

What topic? I still can't figure out when this branch can be executed since hwpoison inject path can't poison free buddy pages.

Hi Simon,

If we use "/sys/devices/system/memory/soft_offline_page" to offline a free page, the value of mce_bad_pages will be added. Then the page is marked HWPoison, but it is still managed by page buddy alocator.

So if we offline it again, the value of mce_bad_pages will be added again. Assume the page is not allocated during this short time.

soft_offline_page()

	get_any_page()
		"else if (is_free_buddy_page(p))" branch return 0
			"goto done";
				"atomic_long_add(1, &mce_bad_pages);"

I think it would be better to move "if(PageHWPoison(page))" at the beginning of soft_offline_page(). However I don't know what do these words mean, "Synchronized using the page lock with memory_failure()"

Quoted 22 lines

Quoted 17 lines

from page buddy alocator in soft_offline_page() rather than in check_new_page().

I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Quoted 10 lines

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

.

Cc other guys.

Quoted 127 lines · Xishi Qiu

Quoted 97 lines · Simon Jeons

Quoted 96 lines · Xishi Qiu

Quoted 93 lines · Simon Jeons

Quoted 82 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

Quoted 7 lines

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Quoted 9 lines

Quoted 5 lines

Hi Simon,

get_any_page() -> "else if(is_free_buddy_page(p))" branch is *not* redundancy.

It is another topic, I mean since the page is poisoned, so why not isolate it

What topic? I still can't figure out when this branch can be executed since hwpoison inject path can't poison free buddy pages.

Hi Simon,

If we use "/sys/devices/system/memory/soft_offline_page" to offline a free page, the value of mce_bad_pages will be added. Then the page is marked HWPoison, but it is still managed by page buddy alocator.

So if we offline it again, the value of mce_bad_pages will be added again. Assume the page is not allocated during this short time.

soft_offline_page()

	get_any_page()
		"else if (is_free_buddy_page(p))" branch return 0
			"goto done";
				"atomic_long_add(1, &mce_bad_pages);"

I think it would be better to move "if(PageHWPoison(page))" at the beginning of soft_offline_page(). However I don't know what do these words mean, "Synchronized using the page lock with memory_failure()"

Quoted 21 lines

Quoted 21 lines

Quoted 18 lines

from page buddy alocator in soft_offline_page() rather than in check_new_page().

I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Quoted 10 lines

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 5 lines

Quoted 2 lines

.

Quoted 16 lines

Quoted 16 lines

Hi Simon,

If we use "/sys/devices/system/memory/soft_offline_page" to offline a free page, the value of mce_bad_pages will be added. Then the page is marked HWPoison, but it is still managed by page buddy alocator.

So if we offline it again, the value of mce_bad_pages will be added again. Assume the page is not allocated during this short time.

soft_offline_page()

	get_any_page()
		"else if (is_free_buddy_page(p))" branch return 0
			"goto done";
				"atomic_long_add(1, &mce_bad_pages);"

Quoted 3 lines

Quoted 3 lines

I think it would be better to move "if(PageHWPoison(page))" at the beginning of soft_offline_page(). However I don't know what do these words mean, "Synchronized using the page lock with memory_failure()"

Hi Xishi,

Unpoison will clear PG_hwpoison flag after hold page lock, memory_failure() and soft_offline_page() take the lock to avoid unpoison clear the flag behind them.

Regards,
Wanpeng Li

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Hi Simon,

If we use "/sys/devices/system/memory/soft_offline_page" to offline a free page, the value of mce_bad_pages will be added. Then the page is marked HWPoison, but it is still managed by page buddy alocator.

So if we offline it again, the value of mce_bad_pages will be added again. Assume the page is not allocated during this short time.

soft_offline_page()

	get_any_page()
		"else if (is_free_buddy_page(p))" branch return 0
			"goto done";
				"atomic_long_add(1, &mce_bad_pages);"

Quoted 3 lines

Quoted 3 lines

I think it would be better to move "if(PageHWPoison(page))" at the beginning of soft_offline_page(). However I don't know what do these words mean, "Synchronized using the page lock with memory_failure()"

Hi Xishi,

Unpoison will clear PG_hwpoison flag after hold page lock, memory_failure() and soft_offline_page() take the lock to avoid unpoison clear the flag behind them.

Quoted 3 lines

Regards,
Wanpeng Li

Hi Wanpeng,

As you mean, it is the necessary to get the page lock first when we check the HWPoison flag every time, this is in order to avoid conflict, right?

So why not use a globe lock here? For example lock_memory_hotplug() is used in online_pages() and offline_pages()?

Thanks,
Xishi Qiu

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Hi Simon,

If we use "/sys/devices/system/memory/soft_offline_page" to offline a free page, the value of mce_bad_pages will be added. Then the page is marked HWPoison, but it is still managed by page buddy alocator.

So if we offline it again, the value of mce_bad_pages will be added again. Assume the page is not allocated during this short time.

soft_offline_page()

	get_any_page()
		"else if (is_free_buddy_page(p))" branch return 0
			"goto done";
				"atomic_long_add(1, &mce_bad_pages);"

Quoted 3 lines

Quoted 3 lines

I think it would be better to move "if(PageHWPoison(page))" at the beginning of soft_offline_page(). However I don't know what do these words mean, "Synchronized using the page lock with memory_failure()"

Hi Xishi,

Unpoison will clear PG_hwpoison flag after hold page lock, memory_failure() and soft_offline_page() take the lock to avoid unpoison clear the flag behind them.

Quoted 3 lines

Regards,
Wanpeng Li

Hi Wanpeng,

As you mean, it is the necessary to get the page lock first when we check the HWPoison flag every time, this is in order to avoid conflict, right?

So why not use a globe lock here? For example lock_memory_hotplug() is used in online_pages() and offline_pages()?

Thanks,
Xishi Qiu

Cc other guys.

Quoted 127 lines · Xishi Qiu

Quoted 97 lines · Simon Jeons

Quoted 96 lines · Xishi Qiu

Quoted 93 lines · Simon Jeons

Quoted 82 lines · Xishi Qiu

Quoted 70 lines · Wanpeng Li

Quoted 60 lines · Andrew Morton

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

Quoted 8 lines

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

Quoted 9 lines

Hi Andrew,

Since hwpoison bit for free buddy page has already be set in get_any_page, !TestSetPageHWPoison(page) will not increase mce_bad_pages count even for the first time.

Regards,
Wanpeng Li

Quoted 7 lines

The poisoned page is isolated in bad_page(), I wonder whether it could be isolated immediately in soft_offline_page() and memory_failure()?

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

Do you mean else if(is_free_buddy_page(p)) branch is redundancy?

Quoted 9 lines

Quoted 5 lines

Hi Simon,

get_any_page() -> "else if(is_free_buddy_page(p))" branch is *not* redundancy.

It is another topic, I mean since the page is poisoned, so why not isolate it

What topic? I still can't figure out when this branch can be executed since hwpoison inject path can't poison free buddy pages.

Hi Simon,

If we use "/sys/devices/system/memory/soft_offline_page" to offline a free page, the value of mce_bad_pages will be added. Then the page is marked HWPoison, but it is still managed by page buddy alocator.

So if we offline it again, the value of mce_bad_pages will be added again. Assume the page is not allocated during this short time.

soft_offline_page()

	get_any_page()
		"else if (is_free_buddy_page(p))" branch return 0
			"goto done";
				"atomic_long_add(1, &mce_bad_pages);"

I think it would be better to move "if(PageHWPoison(page))" at the beginning of soft_offline_page(). However I don't know what do these words mean, "Synchronized using the page lock with memory_failure()"

Quoted 21 lines

Quoted 21 lines

Quoted 18 lines

from page buddy alocator in soft_offline_page() rather than in check_new_page().

I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Quoted 10 lines

Quoted 10 lines

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 6 lines

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

Quoted 5 lines

Quoted 2 lines

.

It is another topic, I mean since the page is poisoned, so why not isolate it from page buddy alocator in soft_offline_page() rather than in check_new_page(). I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Quoted 4 lines

It is another topic, I mean since the page is poisoned, so why not isolate it from page buddy alocator in soft_offline_page() rather than in check_new_page(). I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Doing it in check_new_page is the only way if the page is currently allocated by someone. Since that's not uncommon it's simplest to always do it this way.

-Andi

IIUC, soft offlining will isolate and migrate hwpoisoned page, and this page will not be accessed by memory management subsystem until unpoison, correct?

Quoted 3 lines

IIUC, soft offlining will isolate and migrate hwpoisoned page, and this page will not be accessed by memory management subsystem until unpoison, correct?

No, soft offlining can still allow accesses for some time. It'll never kill anything.

Hard tries much harder and will kill.

In some cases (unshrinkable kernel allocation) they end up doing the same because there isn't any other alternative though. However these are expected to only apply to a small percentage of pages in a typical system.

-Andi

Oh, it will be putback to lru list during migration. So does your "some time" mean before call check_new_page?

Quoted 2 lines

Oh, it will be putback to lru list during migration. So does your "some time" mean before call check_new_page?

Yes until the next check_new_page() whenever that is. If the migration works it will be earlier, otherwise later.

-andi

Quoted 5 lines · Andi Kleen

Quoted 2 lines

Oh, it will be putback to lru list during migration. So does your "some time" mean before call check_new_page?

Yes until the next check_new_page() whenever that is. If the migration works it will be earlier, otherwise later.

But I can't figure out any page reclaim path check if the page is set PG_hwpoison, can poisoned pages be rclaimed?

Quoted 4 lines · Simon Jeons

Quoted 3 lines · Andi Kleen

Quoted 2 lines

Oh, it will be putback to lru list during migration. So does your "some time" mean before call check_new_page?

Quoted 5 lines

Quoted 2 lines

Yes until the next check_new_page() whenever that is. If the migration works it will be earlier, otherwise later.

But I can't figure out any page reclaim path check if the page is set PG_hwpoison, can poisoned pages be rclaimed?

The only way to reclaim a page is to free and reallocate it.

-Andi

Quoted 11 lines · Andi Kleen

Quoted 9 lines · Simon Jeons

Quoted 5 lines · Andi Kleen

Quoted 2 lines

Oh, it will be putback to lru list during migration. So does your "some time" mean before call check_new_page?

Yes until the next check_new_page() whenever that is. If the migration works it will be earlier, otherwise later.

But I can't figure out any page reclaim path check if the page is set PG_hwpoison, can poisoned pages be rclaimed?

Quoted 1 line

The only way to reclaim a page is to free and reallocate it.

Then why there doesn't have check in reclaim path to avoid relcaim poisoned page?

-Simon

Quoted 2 lines

-Andi

Quoted 12 lines · Simon Jeons

Quoted 11 lines · Andi Kleen

Quoted 9 lines · Simon Jeons

Quoted 5 lines · Andi Kleen

Quoted 2 lines

Oh, it will be putback to lru list during migration. So does your "some time" mean before call check_new_page?

Yes until the next check_new_page() whenever that is. If the migration works it will be earlier, otherwise later.

But I can't figure out any page reclaim path check if the page is set PG_hwpoison, can poisoned pages be rclaimed?

Quoted 6 lines

Quoted 1 line

The only way to reclaim a page is to free and reallocate it.

Then why there doesn't have check in reclaim path to avoid relcaim poisoned page?

-Simon

Hi Simon,

If the page is free, it will be set PG_hwpoison, and soft_offline_page() is done. When the page is alocated later, check_new_page() will find the poisoned page and isolate the whole buddy block(just drop the block).

If the page is not free, soft_offline_page() try to free it first, if this is failed, it will migrate the page, but the page is still in LRU list after migration, migrate_pages()

	unmap_and_move()
		if (rc != -EAGAIN) {
			...
			putback_lru_page(page);
		}

We can use lru_add_drain_all() to drain lru pagevec, at last free_hot_cold_page()

will be called, and free_pages_prepare() check the poisoned pages. free_pages_prepare()

	free_pages_check()
		bad_page()

Is this right, Andi?

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 2 lines

-Andi

Quoted 12 lines · Simon Jeons

Quoted 11 lines · Andi Kleen

Quoted 9 lines · Simon Jeons

Quoted 5 lines · Andi Kleen

Quoted 2 lines

Oh, it will be putback to lru list during migration. So does your "some time" mean before call check_new_page?

Yes until the next check_new_page() whenever that is. If the migration works it will be earlier, otherwise later.

But I can't figure out any page reclaim path check if the page is set PG_hwpoison, can poisoned pages be rclaimed?

Quoted 6 lines

Quoted 1 line

The only way to reclaim a page is to free and reallocate it.

Then why there doesn't have check in reclaim path to avoid relcaim poisoned page?

-Simon

Hi Simon,

If the page is free, it will be set PG_hwpoison, and soft_offline_page() is done. When the page is alocated later, check_new_page() will find the poisoned page and isolate the whole buddy block(just drop the block).

If the page is not free, soft_offline_page() try to free it first, if this is failed, it will migrate the page, but the page is still in LRU list after migration, migrate_pages()

	unmap_and_move()
		if (rc != -EAGAIN) {
			...
			putback_lru_page(page);
		}

We can use lru_add_drain_all() to drain lru pagevec, at last free_hot_cold_page()

will be called, and free_pages_prepare() check the poisoned pages. free_pages_prepare()

	free_pages_check()
		bad_page()

Is this right, Andi?

Thanks
Xishi Qiu

Quoted 6 lines

Quoted 2 lines

-Andi

Quoted 4 lines

It is another topic, I mean since the page is poisoned, so why not isolate it from page buddy alocator in soft_offline_page() rather than in check_new_page(). I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Doing it in check_new_page is the only way if the page is currently allocated by someone. Since that's not uncommon it's simplest to always do it this way.

-Andi

Quoted 11 lines · Andi Kleen

Quoted 4 lines

It is another topic, I mean since the page is poisoned, so why not isolate it from page buddy alocator in soft_offline_page() rather than in check_new_page(). I find soft_offline_page() only migrate the page and mark HWPoison, the poisoned page is still managed by page buddy alocator.

Doing it in check_new_page is the only way if the page is currently allocated by someone. Since that's not uncommon it's simplest to always do it this way.

-Andi

Hi Andi,

The poisoned page is isolated in check_new_page, however the whole buddy block will be dropped, it seems to be a waste of memory.

Can we separate the poisoned page from the buddy block, then *only* drop the poisoned page?

That sounds like overkill. There are not so many free pages in a typical server system.

Quoted 2 lines

That sounds like overkill. There are not so many free pages in a typical server system.

As Fengguang said -- memory error handling is tricky. Lots of things could be done in theory, but they all have a cost in testing and maintenance.

In general they are only worth doing if the situation is common and represents a significant percentage of the total pages of a relevant server workload.

-Andi

"There are not so many free pages in a typical server system", sorry I don't quite understand it.

buffered_rmqueue()

	prep_new_page()
		check_new_page()
			bad_page()

If we alloc 2^10 pages and one of them is a poisoned page, then the whole 4M memory will be dropped.

Quoted 19 lines · On Fri, 7 Dec 2012 16:48:45 +0800 Xishi Qiu <[email protected]> wrote:

On x86 platform, if we use "/sys/devices/system/memory/soft_offline_page" to offline a free page twice, the value of mce_bad_pages will be added twice. So this is an error, since the page was already marked HWPoison, we should skip the page and don't add the value of mce_bad_pages.

$ cat /proc/meminfo | grep HardwareCorrupted

soft_offline_page()

	get_any_page()
		atomic_long_add(1, &mce_bad_pages)

...

mm/memory-failure.c +0 -0

@@ -1582,8 +1582,11 @@ int soft_offline_page(struct page *page, int flags)
 		return ret;
 
 done:

mm/memory-failure.c int soft_offline_page(struct page *page, int flags) · line 1592

Quoted 10 lines

mm/memory-failure.c +5 -2

-	atomic_long_add(1, &mce_bad_pages);
-	SetPageHWPoison(page);
 	/* keep elevated page count for bad page */
+	if (!PageHWPoison(page)) {
+		atomic_long_add(1, &mce_bad_pages);
+		SetPageHWPoison(page);
+	}
+
 	return ret;
 }

A few things:

- soft_offline_page() already checks for this case:

	if (PageHWPoison(page)) {
		unlock_page(page);
		put_page(page);
		pr_info("soft offline: %#lx page already poisoned\n", pfn);
		return -EBUSY;
	}

so why didn't this check work for you?

Presumably because one of the earlier "goto done" branches was taken. Which one, any why?

This function is an utter mess. It contains six return points randomly intermingled with three "goto done" return points.

This mess is probably the cause of the bug you have observed. Can we please fix it up somehow? It *seems* that the design (lol) of this function is "for errors, return immediately. For success, goto done". In which case "done" should have been called "success". But if you just look at the function you'll see that this approach didn't work. I suggest it be converted to have two return points - one for the success path, one for the failure path. Or something.

- soft_offline_huge_page() is a miniature copy of soft_offline_page() and might suffer the same bug.

- A cleaner, shorter and possibly faster implementation is

	if (!TestSetPageHWPoison(page))
		atomic_long_add(1, &mce_bad_pages);

- We have atomic_long_inc(). Use it?

- Why do we have a variable called "mce_bad_pages"? MCE is an x86 concept, and this code is in mm/. Lights are flashing, bells are ringing and a loudspeaker is blaring "layering violation" at us!

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