[PATCH] memory hotplug: fix page_zone() calculation in test_pages_isolated()

Gerald Schaefer

author

2008-10-27 16:49 UTC #0 lore raw

PATCH 1/1 memory hotplug: fix page_zone() calculation in test_pages_isolated()

My last bugfix here (adding zone->lock) introduced a new problem: Using pfn_to_page(pfn) to get the zone after the for() loop is wrong. pfn then points to the first pfn after end_pfn, which may be in a different zone or not present at all. This may lead to an addressing exception in page_zone() or spin_lock_irqsave().

Using the last valid page that was found inside the for() loop, instead of pfn_to_page(), should fix this.

Signed-off-by Gerald Schaefer <[email protected]>

---
mm/page_isolation.c | 6 +++---

1 file changed, 3 insertions(+), 3 deletions(-)

mm/page_isolation.c +0 -0

Index: linux-2.6/mm/page_isolation.c
===================================================================
@@ -115,7 +115,7 @@ __test_page_isolated_in_pageblock(unsign

int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)

{
	unsigned long pfn, flags;
-	struct page *page;
+	struct page *page = NULL;
	struct zone *zone;
	int ret;

@@ -130,10 +130,10 @@ int test_pages_isolated(unsigned long st

		if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
			break;
	}
-	if (pfn < end_pfn)
+	if ((pfn < end_pfn) || !page)
		return -EBUSY;
	/* Check all pages are free or Marked as ISOLATED */
-	zone = page_zone(pfn_to_page(pfn));
+	zone = page_zone(page);
	spin_lock_irqsave(&zone->lock, flags);
	ret = __test_page_isolated_in_pageblock(start_pfn, end_pfn);
	spin_unlock_irqrestore(&zone->lock, flags);

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

Gerald Schaefer

author

2008-10-27 17:17 UTC #1 lore raw

Ouch, stupid Thunderbird broke the patch (or stupid me used Thunderbird...), will send a new one.

Gerald

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

Gerald Schaefer

author

2008-10-27 17:19 UTC #2 lore raw

PATCH 1/1 memory hotplug: fix page_zone() calculation in test_pages_isolated()

In reply to Gerald Schaefer

My last bugfix here (adding zone->lock) introduced a new problem: Using pfn_to_page(pfn) to get the zone after the for() loop is wrong. pfn then points to the first pfn after end_pfn, which may be in a different zone or not present at all. This may lead to an addressing exception in page_zone() or spin_lock_irqsave().

Using the last valid page that was found inside the for() loop, instead of pfn_to_page(), should fix this.

Signed-off-by Gerald Schaefer <[email protected]>

---
 mm/page_isolation.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

mm/page_isolation.c +3 -3

Index: linux-2.6/mm/page_isolation.c
===================================================================
@@ -115,7 +115,7 @@ __test_page_isolated_in_pageblock(unsign
 int test_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
 {
 	unsigned long pfn, flags;
-	struct page *page;
+	struct page *page = NULL;
 	struct zone *zone;
 	int ret;

@@ -130,10 +130,10 @@ int test_pages_isolated(unsigned long st
 		if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
 			break;
 	}
-	if (pfn < end_pfn)
+	if ((pfn < end_pfn) || !page)
 		return -EBUSY;
 	/* Check all pages are free or Marked as ISOLATED */
-	zone = page_zone(pfn_to_page(pfn));
+	zone = page_zone(page);
 	spin_lock_irqsave(&zone->lock, flags);
 	ret = __test_page_isolated_in_pageblock(start_pfn, end_pfn);
 	spin_unlock_irqrestore(&zone->lock, flags);

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

Dave Hansen

2008-10-27 17:22 UTC #3 lore raw

In reply to Gerald Schaefer

Quoted 5 lines · Gerald Schaefer

My last bugfix here (adding zone->lock) introduced a new problem: Using pfn_to_page(pfn) to get the zone after the for() loop is wrong. pfn then points to the first pfn after end_pfn, which may be in a different zone or not present at all. This may lead to an addressing exception in page_zone() or spin_lock_irqsave().

I'm not sure I follow. Let's look at the code, pre-patch:

Quoted 7 lines

	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
                page = __first_valid_page(pfn, pageblock_nr_pages);
                if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
                        break;
        }
        if (pfn < end_pfn)
                return -EBUSY;

We have two ways out of the loop:
1. 'page' is valid, and not isolated, so we did a 'break'
2. No page hit (1) in the range and we broke out of the loop because of the for() condition: (pfn < end_pfn).

So, when the condition happens that you mentioned in your changelog
above: "pfn then points to the first pfn after end_pfn", we jump out at the 'return -EBUSY;'. We don't ever do pfn_to_page() in that case since we've returned befoer.

Either 'page' is valid *OR* you return -EBUSY. I don't think you need to check both.

Review int test_pages_isolated(unsigned long st · line 136

Quoted 12 lines

Using the last valid page that was found inside the for() loop, instead of pfn_to_page(), should fix this.

(quoted diff) +2 -2

@@ -130,10 +130,10 @@ int test_pages_isolated(unsigned long st
 		if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
 			break;
 	}
-	if (pfn < end_pfn)
+	if ((pfn < end_pfn) || !page)
 		return -EBUSY;
 	/* Check all pages are free or Marked as ISOLATED */
-	zone = page_zone(pfn_to_page(pfn));
+	zone = page_zone(page);

I think this patch fixes the bug, but for reasons other than what you said. :)

The trouble here is that the 'pfn' could have been in the middle of a hole somewhere, which __first_valid_page() worked around. Since you saved off the result of __first_valid_page(), it ends up being OK with your patch.

Instead of using pfn_to_page() you could also have just called __first_valid_page() again. But, that would have duplicated a bit of work, even though not much in practice because the caches are still hot.

Technically, you wouldn't even need to check the return from __first_valid_page() since you know it has a valid result because you made the exact same call a moment before.

Anyway, can you remove the !page check, fix up the changelog and resend?

-- Dave

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

Gerald Schaefer

author

2008-10-27 17:59 UTC #4 lore raw

Quoted 15 lines · Dave Hansen

I'm not sure I follow. Let's look at the code, pre-patch:
Quoted 7 lines
	for (pfn = start_pfn; pfn < end_pfn; pfn += pageblock_nr_pages) {
                page = __first_valid_page(pfn, pageblock_nr_pages);
                if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
                        break;
        }
        if (pfn < end_pfn)
                return -EBUSY;
We have two ways out of the loop:
1. 'page' is valid, and not isolated, so we did a 'break'
2. No page hit (1) in the range and we broke out of the loop because of the for() condition: (pfn < end_pfn).

Quoted 7 lines

So, when the condition happens that you mentioned in your changelog
above: "pfn then points to the first pfn after end_pfn", we jump out at the 'return -EBUSY;'. We don't ever do pfn_to_page() in that case since we've returned befoer.
Either 'page' is valid *OR* you return -EBUSY. I don't think you need to check both.

We only return -EBUSY if pfn < end_pfn, but after completing the loop w/o a break pfn will be > end_pfn. Also, the last call to __first_valid_page() may return NULL w/o causing a break, so page may also be invalid after the loop.

Quoted 16 lines

Review int test_pages_isolated(unsigned long st · line 136
Quoted 12 lines
Using the last valid page that was found inside the for() loop, instead of pfn_to_page(), should fix this.
(quoted diff) +2 -2
@@ -130,10 +130,10 @@ int test_pages_isolated(unsigned long st
 		if (page && get_pageblock_migratetype(page) != MIGRATE_ISOLATE)
 			break;
 	}
-	if (pfn < end_pfn)
+	if ((pfn < end_pfn) || !page)
 		return -EBUSY;
 	/* Check all pages are free or Marked as ISOLATED */
-	zone = page_zone(pfn_to_page(pfn));
+	zone = page_zone(page);
I think this patch fixes the bug, but for reasons other than what you said. :)

Quoted 4 lines

The trouble here is that the 'pfn' could have been in the middle of a hole somewhere, which __first_valid_page() worked around. Since you saved off the result of __first_valid_page(), it ends up being OK with your patch.

I think pfn will always be > end_pfn if we complete the loop. And breaking out of the loop earlier will always return -EBUSY.

Quoted 9 lines

Instead of using pfn_to_page() you could also have just called __first_valid_page() again. But, that would have duplicated a bit of work, even though not much in practice because the caches are still hot.
Technically, you wouldn't even need to check the return from __first_valid_page() since you know it has a valid result because you made the exact same call a moment before.
Anyway, can you remove the !page check, fix up the changelog and resend?

Calling __first_valid_page() again might be a good idea. Thinking about it now, I guess there is still a problem left with my patch, but for reasons other than what you said :) If the loop is completed with page == NULL, we will return -EBUSY with the new patch. But there may have been valid pages before, and only some memory hole at the end. In this case, returning -EBUSY would probably be wrong.

Kamezawa, this loop/function was added by you, what do you think?

--
Thanks,
Gerald

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

KAMEZAWA Hiroyuki

2008-10-28 00:32 UTC #5 lore raw

Quoted 10 lines · On Mon, 27 Oct 2008 18:59:29 +0100 Gerald Schaefer…

Instead of using pfn_to_page() you could also have just called
Quoted 8 lines
__first_valid_page() again. But, that would have duplicated a bit of work, even though not much in practice because the caches are still hot.
Technically, you wouldn't even need to check the return from __first_valid_page() since you know it has a valid result because you made the exact same call a moment before.
Anyway, can you remove the !page check, fix up the changelog and resend?

Quoted 9 lines

Calling __first_valid_page() again might be a good idea. Thinking about it now, I guess there is still a problem left with my patch, but for reasons other than what you said :) If the loop is completed with page == NULL, we will return -EBUSY with the new patch. But there may have been valid pages before, and only some memory hole at the end. In this case, returning -EBUSY would probably be wrong.
Kamezawa, this loop/function was added by you, what do you think?

I think there is a bug, as you wrote.
But
- "pfn" and "end_pfn" (and pfn in the middle of them) can be in different zone on strange machine.

Now: test_pages_isolated() is called in following sequence.

check_page_isolated()

     walk_memory_resource()			# read resource range and get start/end of pfn
         -> chcek_page_isolated_cb()
		-> test_page_isolated().

I think all pages within [start, end) passed to test_pages_isolated() should be in the same zone.

please change this to
check_page_isolated()

     walk_memory_resource()
         -> check_page_isolated_cb()
                 -> walk_page_range_in_same_zone()  # get page range in the same zone.
                        -> test_page_isolated().

Could you try ?

Thanks,
-Kame

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

Gerald Schaefer

author

2008-10-28 13:00 UTC #6 lore raw

Quoted 12 lines · KAMEZAWA Hiroyuki

But
- "pfn" and "end_pfn" (and pfn in the middle of them) can be in different zone on strange machine.
Now: test_pages_isolated() is called in following sequence.
check_page_isolated()
     walk_memory_resource()			# read resource range and get start/end of pfn
         -> chcek_page_isolated_cb()
		-> test_page_isolated().
I think all pages within [start, end) passed to test_pages_isolated() should be in the same zone.

Quoted 8 lines

please change this to
check_page_isolated()

     walk_memory_resource()
         -> check_page_isolated_cb()
                 -> walk_page_range_in_same_zone()  # get page range in the same zone.
                        -> test_page_isolated().

Could you try ?

There is already a "same zone" check at the beginning of offline_pages():

Quoted 2 lines

	if (!test_pages_in_a_zone(start_pfn, end_pfn))
		return -EINVAL;

So we should be safe here, the only problem that I see is that my zone->lock patch in test_pages_isolated() is broken. As explained, the pfn used in my page_zone(pfn_to_page(pfn)) is >= end_pfn.

I'll send a new patch to fix this, using __first_valid_page() again, as described in my reply to Daves mail. The only other solution that I see would be to remember the first/last !NULL page that was found inside the for() loop. Not sure which is better, but I think I like the first one more. Any other ideas?

Thanks,
Gerald

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