mm: skip the page buddy block instead of one page

PATCH NAK Xishi Qiu 7 participants 36 messages Aug 14, 2013 – Jan 21, 2014

Subsystems (3)

Memory management MM core Page allocator

lore

Share & tools

lore

Xishi Qiu

author

2013-08-14 04:46 UTC #0 lore raw

PATCH 1/1 mm: skip the page buddy block instead of one page

A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.

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

---
 mm/compaction.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

mm/compaction.c +3 -2

index 05ccb4c..874bae1 100644
@@ -520,9 +520,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 			goto next_pageblock;
 
 		/* Skip if free */
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+			low_pfn += (1 << page_order(page)) - 1;
 			continue;
-
+		}
 		/*
 		 * For async migration, also only scan in MOVABLE blocks. Async
 		 * migration is optimistic to see if the minimum amount of work

Signature · 7 lines

1.7.1

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

Minchan Kim

Acked-by on v1 1/1

2013-08-14 07:07 UTC #1 lore raw

Hello,

Quoted 4 lines · Xishi Qiu

A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Signed-off-by Xishi Qiu <[email protected]>

Nitpick is it could change nr_scanned's result so that COMPACMIGRATE_SCANNED of vmstat could be smaller than old. It means that compaction efficiency would pretend to be better than old and if something on userspace have been depends on it, it would be broken. But I don't know such usecase so I will pass the decision to others. Anyway, I suppose this patch.
If it's real concern, we can fix it with increasing nr_scanned by page_order.

Thanks.

Acked-by Minchan Kim <[email protected]>

Quoted 29 lines

---
 mm/compaction.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

mm/compaction.c +3 -2

index 05ccb4c..874bae1 100644
@@ -520,9 +520,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 			goto next_pageblock;
 
 		/* Skip if free */
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+			low_pfn += (1 << page_order(page)) - 1;
 			continue;
-
+		}
 		/*
 		 * For async migration, also only scan in MOVABLE blocks. Async
 		 * migration is optimistic to see if the minimum amount of work

Signature · 7 lines

1.7.1

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

Signature · 8 lines

Kind regards,
Minchan Kim

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

Mel Gorman

NAK

2013-08-14 08:57 UTC #2 lore raw

In reply to Xishi Qiu

Quoted 4 lines · Xishi Qiu

A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Signed-off-by Xishi Qiu <[email protected]>

page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the buddy allocator (per-cpu allocator would be ok except for refills). I expect it would not be a good tradeoff to acquire the lock just to use page_order.

Nak.

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Xishi Qiu

author

2013-08-14 09:15 UTC #3 lore raw

Quoted 4 lines · Mel Gorman

Quoted 3 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.

Quoted 9 lines

Quoted 1 line
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the buddy allocator (per-cpu allocator would be ok except for refills). I expect it would not be a good tradeoff to acquire the lock just to use page_order.
Nak.

Oh, you are right, we must hold zone->lock first.

Thanks,
Xishi Qiu

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

Minchan Kim

2013-08-14 15:52 UTC #4 lore raw

In reply to Mel Gorman

Hi Mel,

Quoted 8 lines · Mel Gorman

Quoted 4 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the

Argh, I missed that. And it seems you already pointed it out long time ago someone try to do same things if I remember correctly. :(
But let's think about it more.

It's always not right because CMA and memory-hotplug already isolated free pages in the range to MIGRATE_ISOLATE right before starting migration so we could use page_order safely in those contexts even if we don't hold zone->lock.

In addition, it's likely to have many free pages in case of CMA because CMA makes MIGRATE_CMA fallback of MIGRATE_MOVABLE to minimize number of migrations. Even CMA area was full, it could have many free pages once driver who is CMA area's owner releases the CMA area. So, the bigger CMA space is, the bigger patch's benefit is. And it could help memory-hotplug, too.

Only problem is normal compaction. The worst case is just skipping pageblock_nr_pages, for instace, 4M(of course, it depends on configuration). but we can make the race window very small by dobule checking PageBuddy. Still, it could make the race theoretically but I think it's really really unlikely and still enhance compaction overhead withtout holding the lock. Even if the race happens, normal compaction's customers(ex, THP) doesn't have critical result and just fallback. So I think it isn't not bad tradeoff.

Mel Gorman

2013-08-14 16:16 UTC #5 lore raw

Quoted 7 lines · Minchan Kim

Hi Mel,
Quoted 4 lines · Mel Gorman
Quoted 3 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the
Argh, I missed that. And it seems you already pointed it out long time ago someone try to do same things if I remember correctly. :(

It feels familiar but I do not remember why.

Quoted 7 lines

But let's think about it more.
It's always not right because CMA and memory-hotplug already isolated free pages in the range to MIGRATE_ISOLATE right before starting migration so we could use page_order safely in those contexts even if we don't hold zone->lock.

Both of those are teh corner cases. Neither operation happen frequently in comparison to something like THP allocations for example. I think an optimisation along those lines is marginal at best.

Quoted 26 lines

In addition, it's likely to have many free pages in case of CMA because CMA makes MIGRATE_CMA fallback of MIGRATE_MOVABLE to minimize number of migrations. Even CMA area was full, it could have many free pages once driver who is CMA area's owner releases the CMA area. So, the bigger CMA space is, the bigger patch's benefit is. And it could help memory-hotplug, too.
Only problem is normal compaction. The worst case is just skipping pageblock_nr_pages, for instace, 4M(of course, it depends on configuration). but we can make the race window very small by dobule checking PageBuddy. Still, it could make the race theoretically but I think it's really really unlikely and still enhance compaction overhead withtout holding the lock. Even if the race happens, normal compaction's customers(ex, THP) doesn't have critical result and just fallback. So I think it isn't not bad tradeoff.
mm/compaction.c +3 -1
index 05ccb4c..2341d52 100644
@@ -520,8 +520,18 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 			goto next_pageblock;
 
 		/* Skip if free */
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+			/*
+			 * page_order is racy without zone->lock but worst case

mm/compaction.c isolate_migratepages_range(struct zone *zone, struct compact_control *cc, · line 535

Quoted 10 lines

mm/compaction.c +8 -0

+			 * by the racing is just skipping pageblock_nr_pages.
+			 * but even the race is really unlikely by double
+			 * check of PageBuddy.
+			 */
+			unsigned long order = page_order(page);
+			if (PageBuddy(page))
+				low_pfn += (1 << order) - 1;
 			continue;
+		}

Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there from this patch?

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Minchan Kim

2013-08-14 16:39 UTC #6 lore raw

Quoted 26 lines · Mel Gorman

Quoted 7 lines · Minchan Kim
Hi Mel,
Quoted 4 lines · Mel Gorman
Quoted 3 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the
Argh, I missed that. And it seems you already pointed it out long time ago someone try to do same things if I remember correctly. :(
It feels familiar but I do not remember why.
Quoted 7 lines
But let's think about it more.
It's always not right because CMA and memory-hotplug already isolated free pages in the range to MIGRATE_ISOLATE right before starting migration so we could use page_order safely in those contexts even if we don't hold zone->lock.

Quoted 3 lines

Both of those are teh corner cases. Neither operation happen frequently in comparison to something like THP allocations for example. I think an optimisation along those lines is marginal at best.

In embedded side, we don't use THP yet but uses CMA and memory-hotplug so your claim isn't the case for the embedded world.
And as I said, CMA area is last fallback so it's likely to have many free pages so bigger CMA area is, the bigger the benefit(CPU and Power) is, I guess.

Quoted 36 lines

Quoted 36 lines
In addition, it's likely to have many free pages in case of CMA because CMA makes MIGRATE_CMA fallback of MIGRATE_MOVABLE to minimize number of migrations. Even CMA area was full, it could have many free pages once driver who is CMA area's owner releases the CMA area. So, the bigger CMA space is, the bigger patch's benefit is. And it could help memory-hotplug, too.
Only problem is normal compaction. The worst case is just skipping pageblock_nr_pages, for instace, 4M(of course, it depends on configuration). but we can make the race window very small by dobule checking PageBuddy. Still, it could make the race theoretically but I think it's really really unlikely and still enhance compaction overhead withtout holding the lock. Even if the race happens, normal compaction's customers(ex, THP) doesn't have critical result and just fallback. So I think it isn't not bad tradeoff.
mm/compaction.c +11 -1
index 05ccb4c..2341d52 100644
@@ -520,8 +520,18 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 			goto next_pageblock;
 
 		/* Skip if free */
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+			/*
+			 * page_order is racy without zone->lock but worst case
+			 * by the racing is just skipping pageblock_nr_pages.
+			 * but even the race is really unlikely by double
+			 * check of PageBuddy.
+			 */
+			unsigned long order = page_order(page);
+			if (PageBuddy(page))
+				low_pfn += (1 << order) - 1;
 			continue;
+		}

Quoted 8 lines

Quoted 1 line
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there

1. Just worst case is skipping pageblock_nr_pages
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.

If you really have a concern about that, we can add following.

Mel Gorman

2013-08-14 18:00 UTC #7 lore raw

Quoted 27 lines · Minchan Kim

Quoted 26 lines · Mel Gorman
Quoted 7 lines · Minchan Kim
Hi Mel,
Quoted 4 lines · Mel Gorman
Quoted 3 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the
Argh, I missed that. And it seems you already pointed it out long time ago someone try to do same things if I remember correctly. :(
It feels familiar but I do not remember why.
Quoted 7 lines
But let's think about it more.
It's always not right because CMA and memory-hotplug already isolated free pages in the range to MIGRATE_ISOLATE right before starting migration so we could use page_order safely in those contexts even if we don't hold zone->lock.

Quoted 6 lines

Quoted 3 lines
Both of those are teh corner cases. Neither operation happen frequently in comparison to something like THP allocations for example. I think an optimisation along those lines is marginal at best.
In embedded side, we don't use THP yet but uses CMA and memory-hotplug so your claim isn't the case for the embedded world.

I thought CMA was only used when certain devices require large contiguous ranges of memory. The expectation was that such allocations are relatively rare and the cost savings from this patch would not be measurable. Considering how heavy the memory hotplug operation is I also severely doubt that not being able to skip over PageBuddy pages in compaction is noticable.

Quoted 23 lines

Quoted 14 lines
Quoted 14 lines
(quoted diff) +11 -1
 		/* Skip if free */
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+			/*
+			 * page_order is racy without zone->lock but worst case
+			 * by the racing is just skipping pageblock_nr_pages.
+			 * but even the race is really unlikely by double
+			 * check of PageBuddy.
+			 */
+			unsigned long order = page_order(page);
+			if (PageBuddy(page))
+				low_pfn += (1 << order) - 1;
 			continue;
+		}
Quoted 8 lines
Quoted 1 line
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there

Quoted 1 line

1. Just worst case is skipping pageblock_nr_pages

No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone

Quoted 8 lines

2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
If you really have a concern about that, we can add following.
(quoted diff) +2 -1
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+#ifdef CONFIG_MEMORY_ISOLATION

Review

Quoted 10 lines

(quoted diff) +9 -0

+			/*
+			 * memory-hotplug and CMA already move free pages to
+			 * MIGRATE_ISOLATE so we can use page_order safely
+			 * without zone->lock.
+			 */
+			if (PageBuddy(page))
+				low_pfn += (1 << page_order(page)) - 1;
+#endif
 			continue;
+		}

How does that help anything? MEMORY_ISOLATION is set in distribution configs and there is no guarantee at all and the same race exists. If it was going to be anything it would be something like this untested hack

/*
* It is not safe to call page_order(page) for a PageBuddy page without
* holding the zone lock as the order can change or the page allocated.
* Check PageBuddy after reading page_order to minimise the race. As
* the value could still be stale, make sure that we do not accidentally
* skip past the end of the largest page the buddy allocator handles. */

if (PageBuddy(page)) {
	int nr_pages = (1 << page_order(page)) - 1;
	if (PageBuddy(page)) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}

It's still race-prone meaning that it really should be backed by some performance data justifying it.

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Minchan Kim

2013-08-14 19:11 UTC #8 lore raw

Quoted 32 lines · Mel Gorman

Quoted 31 lines · Minchan Kim
Quoted 29 lines · Mel Gorman
Quoted 7 lines · Minchan Kim
Hi Mel,
Quoted 4 lines · Mel Gorman
Quoted 3 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the
Argh, I missed that. And it seems you already pointed it out long time ago someone try to do same things if I remember correctly. :(
It feels familiar but I do not remember why.
Quoted 7 lines
But let's think about it more.
It's always not right because CMA and memory-hotplug already isolated free pages in the range to MIGRATE_ISOLATE right before starting migration so we could use page_order safely in those contexts even if we don't hold zone->lock.
Both of those are teh corner cases. Neither operation happen frequently in comparison to something like THP allocations for example. I think an optimisation along those lines is marginal at best.

Quoted 6 lines

Quoted 2 lines
In embedded side, we don't use THP yet but uses CMA and memory-hotplug so your claim isn't the case for the embedded world.
I thought CMA was only used when certain devices require large contiguous ranges of memory. The expectation was that such allocations are relatively rare and the cost savings from this patch would not be

It's not true. It depends on user activity.
For exmaple, if user opens the app like camera frequenlty, CMA allocation can happen frequently if we implement image buffer for camera as CMA.

Quoted 27 lines

measurable. Considering how heavy the memory hotplug operation is I also severely doubt that not being able to skip over PageBuddy pages in compaction is noticable.
Quoted 23 lines
Quoted 22 lines
Quoted 5 lines
(quoted diff) +2 -1
 		/* Skip if free */
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+			/*
Review
Quoted 10 lines
(quoted diff) +9 -0
+			 * page_order is racy without zone->lock but worst case
+			 * by the racing is just skipping pageblock_nr_pages.
+			 * but even the race is really unlikely by double
+			 * check of PageBuddy.
+			 */
+			unsigned long order = page_order(page);
+			if (PageBuddy(page))
+				low_pfn += (1 << order) - 1;
 			continue;
+		}
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there

Quoted 4 lines

Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone

Sigh, I missed low_pfn is return value so you're right but we can simple fix it with min(low_pfn, end_pfn).

Quoted 15 lines

Quoted 15 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
If you really have a concern about that, we can add following.
(quoted diff) +8 -1
-		if (PageBuddy(page))
+		if (PageBuddy(page)) {
+#ifdef CONFIG_MEMORY_ISOLATION
+			/*
+			 * memory-hotplug and CMA already move free pages to
+			 * MIGRATE_ISOLATE so we can use page_order safely
+			 * without zone->lock.
+			 */
+			if (PageBuddy(page))

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +3 -0
+				low_pfn += (1 << page_order(page)) - 1;
+#endif
 			continue;
+		}
How does that help anything? MEMORY_ISOLATION is set in distribution configs and there is no guarantee at all and the same race exists. If it was going to be anything it would be something like this untested hack

Argh, I sent wrong versoin patch. Here is 4:06 am so maybe I need sleeping. :(

Quoted 8 lines

/*
* It is not safe to call page_order(page) for a PageBuddy page without
* holding the zone lock as the order can change or the page allocated.
* Check PageBuddy after reading page_order to minimise the race. As
* the value could still be stale, make sure that we do not accidentally
* skip past the end of the largest page the buddy allocator handles. */

Quoted 8 lines

if (PageBuddy(page)) {
	int nr_pages = (1 << page_order(page)) - 1;
	if (PageBuddy(page)) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}

My version is following as

	if (PageBuddy(page)) {
#ifdef CONFIG_MEMOR_ISOLATION
		unsigned long order = page_order(page);
		if (PageBuddy(page)) {
			low_pfn += (1 << order) - 1;
			low_pfn = min(low_pfn, end_pfn);
		}	
#endif
		continue;
	}

Quoted 3 lines

It's still race-prone meaning that it really should be backed by some performance data justifying it.

Okay.

Thanks for the input, Mel!

Quoted 10 lines

Signature · 9 lines

-- 
Mel Gorman
SUSE Labs

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

Signature · 8 lines

Kind regards,
Minchan Kim

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

Xishi Qiu

author

2013-08-15 02:33 UTC #9 lore raw

In reply to Mel Gorman

Quoted 7 lines · Mel Gorman

Quoted 7 lines
Quoted 6 lines
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there

Quoted 8 lines

Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone
Quoted 3 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.

Hi Minchan,
I think in this case, we may get the wrong value from page_order(page).

1. page is in page buddy

Quoted 1 line

if (PageBuddy(page)) {

2. someone allocated the page, and set page->private to another value

Quoted 1 line

	int nr_pages = (1 << page_order(page)) - 1;

3. someone freed the page

Quoted 1 line

	if (PageBuddy(page)) {

4. we will skip wrong pages

Quoted 9 lines

		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}
It's still race-prone meaning that it really should be backed by some performance data justifying it.

Minchan Kim

2013-08-15 02:44 UTC #10 lore raw

Hi Xishi,

Quoted 27 lines · Xishi Qiu

Quoted 7 lines · Mel Gorman
Quoted 7 lines
Quoted 6 lines
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there
Quoted 8 lines
Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone
Quoted 3 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
Hi Minchan,
I think in this case, we may get the wrong value from page_order(page).
1. page is in page buddy
Quoted 1 line
if (PageBuddy(page)) {
2. someone allocated the page, and set page->private to another value

Quoted 7 lines

Quoted 1 line
	int nr_pages = (1 << page_order(page)) - 1;
3. someone freed the page
Quoted 1 line
	if (PageBuddy(page)) {
4. we will skip wrong pages

So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case and the case you mentioned is right academically and I and Mel already pointed out that. But how often could that happen in real practice? I believe such is REALLY REALLY rare.
So, as Mel said, if you have some workloads to see the benefit from this patch, I think we could accept the patch.
Could you try and respin with the number?
I guess big contigous memory range or memory-hotplug which are full of free pages in embedded CPU which is rather slower than server or desktop side could have benefit.

Thanks.

Quoted 13 lines

Quoted 10 lines
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}
It's still race-prone meaning that it really should be backed by some performance data justifying it.

Signature · 8 lines

Kind regards,
Minchan Kim

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

Xishi Qiu

author

2013-08-15 03:46 UTC #11 lore raw

Quoted 32 lines · Minchan Kim

Hi Xishi,
Quoted 29 lines · Xishi Qiu
Quoted 7 lines · Mel Gorman
Quoted 7 lines
Quoted 6 lines
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there
Quoted 8 lines
Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone
Quoted 3 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
Hi Minchan,
I think in this case, we may get the wrong value from page_order(page).
1. page is in page buddy
Quoted 1 line
if (PageBuddy(page)) {
2. someone allocated the page, and set page->private to another value
Quoted 1 line
	int nr_pages = (1 << page_order(page)) - 1;

Quoted 8 lines

Quoted 5 lines
3. someone freed the page
Quoted 1 line
	if (PageBuddy(page)) {
4. we will skip wrong pages
So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case

Hi Minchan,
I mean if the private is set to a large number, it will skip 2^private pages, not (pageblock_nr_pages -1). I find somewhere will use page->private, such as fs. Here is the comment about parivate.
/* Mapping-private opaque data:
* usually used for buffer_heads
* if PagePrivate set; used for
* swp_entry_t if PageSwapCache;
* indicates order in the buddy
* system if PG_buddy is set.
*/
Thanks,
Xishi Qiu

Quoted 26 lines

and the case you mentioned is right academically and I and Mel already pointed out that. But how often could that happen in real practice? I believe such is REALLY REALLY rare.
So, as Mel said, if you have some workloads to see the benefit from this patch, I think we could accept the patch.
Could you try and respin with the number?
I guess big contigous memory range or memory-hotplug which are full of free pages in embedded CPU which is rather slower than server or desktop side could have benefit.
Thanks.
Quoted 13 lines
Quoted 10 lines
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}
It's still race-prone meaning that it really should be backed by some performance data justifying it.

Wanpeng Li

2013-08-15 04:00 UTC #12 lore raw

Quoted 40 lines · Xishi Qiu

Quoted 38 lines · Minchan Kim
Hi Xishi,
Quoted 34 lines · Xishi Qiu
Quoted 7 lines · Mel Gorman
Quoted 7 lines
Quoted 6 lines
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there
Quoted 8 lines
Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone
Quoted 3 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
Hi Minchan,
I think in this case, we may get the wrong value from page_order(page).
1. page is in page buddy
Quoted 1 line
if (PageBuddy(page)) {
2. someone allocated the page, and set page->private to another value
Quoted 1 line
	int nr_pages = (1 << page_order(page)) - 1;
3. someone freed the page
Quoted 1 line
	if (PageBuddy(page)) {
4. we will skip wrong pages

Quoted 7 lines

Quoted 2 lines
So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case
Hi Minchan,
I mean if the private is set to a large number, it will skip 2^private pages, not (pageblock_nr_pages -1). I find somewhere will use page->private, such as fs. Here is the comment about parivate.

There is PageBuddy() check.

Quoted 39 lines

/* Mapping-private opaque data:
* usually used for buffer_heads
* if PagePrivate set; used for
* swp_entry_t if PageSwapCache;
* indicates order in the buddy
* system if PG_buddy is set.
*/
Thanks,
Xishi Qiu
Quoted 10 lines
and the case you mentioned is right academically and I and Mel already pointed out that. But how often could that happen in real practice? I believe such is REALLY REALLY rare.
So, as Mel said, if you have some workloads to see the benefit from this patch, I think we could accept the patch.
Could you try and respin with the number?
I guess big contigous memory range or memory-hotplug which are full of free pages in embedded CPU which is rather slower than server or desktop side could have benefit.
Quoted 16 lines
Thanks.
Quoted 13 lines
Quoted 10 lines
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}
It's still race-prone meaning that it really should be backed by some performance data justifying it.

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>

Wanpeng Li

2013-08-15 04:00 UTC #13 lore raw

In reply to Xishi Qiu

Quoted 40 lines · Xishi Qiu

Quoted 38 lines · Minchan Kim
Hi Xishi,
Quoted 34 lines · Xishi Qiu
Quoted 7 lines · Mel Gorman
Quoted 7 lines
Quoted 6 lines
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there
Quoted 8 lines
Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone
Quoted 3 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
Hi Minchan,
I think in this case, we may get the wrong value from page_order(page).
1. page is in page buddy
Quoted 1 line
if (PageBuddy(page)) {
2. someone allocated the page, and set page->private to another value
Quoted 1 line
	int nr_pages = (1 << page_order(page)) - 1;
3. someone freed the page
Quoted 1 line
	if (PageBuddy(page)) {
4. we will skip wrong pages

Quoted 7 lines

Quoted 2 lines
So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case
Hi Minchan,
I mean if the private is set to a large number, it will skip 2^private pages, not (pageblock_nr_pages -1). I find somewhere will use page->private, such as fs. Here is the comment about parivate.

There is PageBuddy() check.

Quoted 39 lines

/* Mapping-private opaque data:
* usually used for buffer_heads
* if PagePrivate set; used for
* swp_entry_t if PageSwapCache;
* indicates order in the buddy
* system if PG_buddy is set.
*/
Thanks,
Xishi Qiu
Quoted 10 lines
and the case you mentioned is right academically and I and Mel already pointed out that. But how often could that happen in real practice? I believe such is REALLY REALLY rare.
So, as Mel said, if you have some workloads to see the benefit from this patch, I think we could accept the patch.
Could you try and respin with the number?
I guess big contigous memory range or memory-hotplug which are full of free pages in embedded CPU which is rather slower than server or desktop side could have benefit.
Quoted 16 lines
Thanks.
Quoted 13 lines
Quoted 10 lines
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}
It's still race-prone meaning that it really should be backed by some performance data justifying it.

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>

Minchan Kim

2013-08-15 04:18 UTC #14 lore raw

In reply to Xishi Qiu

Hello,

Quoted 43 lines · Xishi Qiu

Quoted 32 lines · Minchan Kim
Hi Xishi,
Quoted 29 lines · Xishi Qiu
Quoted 7 lines · Mel Gorman
Quoted 7 lines
Quoted 6 lines
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there
Quoted 8 lines
Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone
Quoted 3 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
Hi Minchan,
I think in this case, we may get the wrong value from page_order(page).
1. page is in page buddy
Quoted 1 line
if (PageBuddy(page)) {
2. someone allocated the page, and set page->private to another value
Quoted 1 line
	int nr_pages = (1 << page_order(page)) - 1;
Quoted 8 lines
Quoted 5 lines
3. someone freed the page
Quoted 1 line
	if (PageBuddy(page)) {
4. we will skip wrong pages
So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case

Quoted 11 lines

Hi Minchan,
I mean if the private is set to a large number, it will skip 2^private pages, not (pageblock_nr_pages -1). I find somewhere will use page->private, such as fs. Here is the comment about parivate.
/* Mapping-private opaque data:
* usually used for buffer_heads
* if PagePrivate set; used for
* swp_entry_t if PageSwapCache;
* indicates order in the buddy
* system if PG_buddy is set.
*/

Please read full thread in detail.

Mel suggested following as

if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}

min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.

if (PageBuddy(page)) {

#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}

so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.

if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
	continue;
}

Quoted 32 lines

Thanks,
Xishi Qiu
Quoted 26 lines
and the case you mentioned is right academically and I and Mel already pointed out that. But how often could that happen in real practice? I believe such is REALLY REALLY rare.
So, as Mel said, if you have some workloads to see the benefit from this patch, I think we could accept the patch.
Could you try and respin with the number?
I guess big contigous memory range or memory-hotplug which are full of free pages in embedded CPU which is rather slower than server or desktop side could have benefit.
Thanks.
Quoted 13 lines
Quoted 10 lines
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
		low_pfn += nr_pages;
		continue;
	}
}
It's still race-prone meaning that it really should be backed by some performance data justifying it.

Signature · 8 lines

Kind regards,
Minchan Kim

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

Minchan Kim

2013-08-15 04:24 UTC #15 lore raw

Quoted 90 lines · Minchan Kim

Hello,
Quoted 43 lines · Xishi Qiu
Quoted 32 lines · Minchan Kim
Hi Xishi,
Quoted 29 lines · Xishi Qiu
Quoted 7 lines · Mel Gorman
Quoted 7 lines
Quoted 6 lines
Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example. There is also a really small race that a page was freed, allocated with some number stuffed into page->private and freed again before the second PageBuddy check. It's a bit of a hand grenade. How much of a performance benefit is there
Quoted 8 lines
Quoted 1 line
1. Just worst case is skipping pageblock_nr_pages
No, the worst case is that page_order returns a number that is completely garbage and low_pfn goes off the end of the zone
Quoted 3 lines
2. Race is really small
3. Higher order page allocation customer always have graceful fallback.
Hi Minchan,
I think in this case, we may get the wrong value from page_order(page).
1. page is in page buddy
Quoted 1 line
if (PageBuddy(page)) {
2. someone allocated the page, and set page->private to another value
Quoted 1 line
	int nr_pages = (1 << page_order(page)) - 1;
Quoted 8 lines
Quoted 5 lines
3. someone freed the page
Quoted 1 line
	if (PageBuddy(page)) {
4. we will skip wrong pages
So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case
Quoted 11 lines
Hi Minchan,
I mean if the private is set to a large number, it will skip 2^private pages, not (pageblock_nr_pages -1). I find somewhere will use page->private, such as fs. Here is the comment about parivate.
/* Mapping-private opaque data:
* usually used for buffer_heads
* if PagePrivate set; used for
* swp_entry_t if PageSwapCache;
* indicates order in the buddy
* system if PG_buddy is set.
*/
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
if (PageBuddy(page)) {
#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.

Quoted 5 lines

if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);

Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).

Quoted 5 lines

	}
	continue;
}

Signature · 8 lines

Kind regards,
Minchan Kim

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

Xishi Qiu

author

2013-08-15 07:45 UTC #16 lore raw

Quoted 17 lines · Minchan Kim

Quoted 17 lines
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.

Quoted 11 lines

if (PageBuddy(page)) {

#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}

Hi Minchan,

I understand now, but why use "end_pfn" here?
Maybe like this:

if (PageBuddy(page)) {
	/*
	 * page_order is racy without zone->lock but worst case
	 * by the racing is just skipping pageblock_nr_pages.
	 */
	unsigned long nr_pages = 1 << page_order(page);
	if (likely(PageBuddy(page))) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);

		/* Align with pageblock boundary */
		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
		    pageblock_nr_pages)
			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
		else
			low_pfn += nr_pages - 1;
	}
	continue;
}

Thanks,
Xishi Qiu

Quoted 4 lines

Quoted 4 lines
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.

Quoted 15 lines

Quoted 5 lines
if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).
Quoted 5 lines
	}
	continue;
}

Wanpeng Li

2013-08-15 09:51 UTC #17 lore raw

Quoted 36 lines · Xishi Qiu

Quoted 17 lines · Minchan Kim
Quoted 17 lines
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
Quoted 11 lines
Quoted 11 lines
if (PageBuddy(page)) {
#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}
Hi Minchan,
I understand now, but why use "end_pfn" here?
Maybe like this:

Quoted 8 lines

if (PageBuddy(page)) {
	/*
	 * page_order is racy without zone->lock but worst case
	 * by the racing is just skipping pageblock_nr_pages.
	 */
	unsigned long nr_pages = 1 << page_order(page);
	if (likely(PageBuddy(page))) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);

How much sense it make? nr_pages is still equal to itself since nr_pages can't larger than MAX_ORDER_NR_PAGES.

Quoted 36 lines

		/* Align with pageblock boundary */
		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
		    pageblock_nr_pages)
			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
		else
			low_pfn += nr_pages - 1;
	}
	continue;
}
Thanks,
Xishi Qiu
Quoted 4 lines
Quoted 4 lines
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.
Quoted 15 lines
Quoted 5 lines
if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).
Quoted 5 lines
	}
	continue;
}

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>

Xishi Qiu

author

2013-08-15 11:16 UTC #18 lore raw

Quoted 46 lines · Wanpeng Li

Quoted 36 lines · Xishi Qiu
Quoted 17 lines · Minchan Kim
Quoted 17 lines
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
Quoted 11 lines
Quoted 11 lines
if (PageBuddy(page)) {
#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}
Hi Minchan,
I understand now, but why use "end_pfn" here?
Maybe like this:
Quoted 8 lines
if (PageBuddy(page)) {
	/*
	 * page_order is racy without zone->lock but worst case
	 * by the racing is just skipping pageblock_nr_pages.
	 */
	unsigned long nr_pages = 1 << page_order(page);
	if (likely(PageBuddy(page))) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);

Quoted 3 lines

How much sense it make? nr_pages is still equal to itself since nr_pages can't larger than MAX_ORDER_NR_PAGES.

Hi Wanpeng,

Mel pointed "page_order cannot be used unless zone->lock is held". "Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example."

If someone use the page during the double PageBuddy check, the value of private may be wrong. In my opinion, just keep the code unchanged.

Thanks,
Xishi Qiu

Quoted 24 lines

Quoted 24 lines
		/* Align with pageblock boundary */
		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
		    pageblock_nr_pages)
			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
		else
			low_pfn += nr_pages - 1;
	}
	continue;
}
Thanks,
Xishi Qiu
Quoted 10 lines
Quoted 9 lines
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.
if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).
Quoted 4 lines
	}
	continue;
}

Wanpeng Li

2013-08-15 11:23 UTC #19 lore raw

Quoted 54 lines · Xishi Qiu

Quoted 46 lines · Wanpeng Li
Quoted 36 lines · Xishi Qiu
Quoted 17 lines · Minchan Kim
Quoted 17 lines
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
Quoted 11 lines
Quoted 11 lines
if (PageBuddy(page)) {
#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}
Hi Minchan,
I understand now, but why use "end_pfn" here?
Maybe like this:
Quoted 8 lines
if (PageBuddy(page)) {
	/*
	 * page_order is racy without zone->lock but worst case
	 * by the racing is just skipping pageblock_nr_pages.
	 */
	unsigned long nr_pages = 1 << page_order(page);
	if (likely(PageBuddy(page))) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);
Quoted 3 lines
How much sense it make? nr_pages is still equal to itself since nr_pages can't larger than MAX_ORDER_NR_PAGES.
Hi Wanpeng,

Quoted 8 lines

Mel pointed "page_order cannot be used unless zone->lock is held". "Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example."
If someone use the page during the double PageBuddy check, the value of private may be wrong. In my opinion, just keep the code unchanged.

Hi Xishi,

You miss my point, I mean nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES); not make sense since nr_pages can't larger then MAX_ORDER_NR_PAGES.

Regards,
Wanpeng Li

Quoted 36 lines

Thanks,
Xishi Qiu
Quoted 31 lines
Quoted 31 lines
		/* Align with pageblock boundary */
		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
		    pageblock_nr_pages)
			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
		else
			low_pfn += nr_pages - 1;
	}
	continue;
}
Thanks,
Xishi Qiu
Quoted 17 lines
Quoted 9 lines
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.
if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).
Quoted 4 lines
	}
	continue;
}

Wanpeng Li

2013-08-15 11:23 UTC #20 lore raw

In reply to Xishi Qiu

Quoted 54 lines · Xishi Qiu

Quoted 46 lines · Wanpeng Li
Quoted 36 lines · Xishi Qiu
Quoted 17 lines · Minchan Kim
Quoted 17 lines
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
Quoted 11 lines
Quoted 11 lines
if (PageBuddy(page)) {
#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}
Hi Minchan,
I understand now, but why use "end_pfn" here?
Maybe like this:
Quoted 8 lines
if (PageBuddy(page)) {
	/*
	 * page_order is racy without zone->lock but worst case
	 * by the racing is just skipping pageblock_nr_pages.
	 */
	unsigned long nr_pages = 1 << page_order(page);
	if (likely(PageBuddy(page))) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);
Quoted 3 lines
How much sense it make? nr_pages is still equal to itself since nr_pages can't larger than MAX_ORDER_NR_PAGES.
Hi Wanpeng,

Quoted 8 lines

Mel pointed "page_order cannot be used unless zone->lock is held". "Even if the page is still page buddy, there is no guarantee that it's the same page order as the first read. It could have be currently merging with adjacent buddies for example."
If someone use the page during the double PageBuddy check, the value of private may be wrong. In my opinion, just keep the code unchanged.

Hi Xishi,

You miss my point, I mean nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES); not make sense since nr_pages can't larger then MAX_ORDER_NR_PAGES.

Regards,
Wanpeng Li

Quoted 36 lines

Thanks,
Xishi Qiu
Quoted 31 lines
Quoted 31 lines
		/* Align with pageblock boundary */
		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
		    pageblock_nr_pages)
			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
		else
			low_pfn += nr_pages - 1;
	}
	continue;
}
Thanks,
Xishi Qiu
Quoted 17 lines
Quoted 9 lines
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.
if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).
Quoted 4 lines
	}
	continue;
}

Xishi Qiu

author

2013-08-15 11:18 UTC #21 lore raw

In reply to Wanpeng Li

Quoted 46 lines · Wanpeng Li

Quoted 36 lines · Xishi Qiu
Quoted 17 lines · Minchan Kim
Quoted 17 lines
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
Quoted 11 lines
Quoted 11 lines
if (PageBuddy(page)) {
#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}
Hi Minchan,
I understand now, but why use "end_pfn" here?
Maybe like this:
Quoted 8 lines
if (PageBuddy(page)) {
	/*
	 * page_order is racy without zone->lock but worst case
	 * by the racing is just skipping pageblock_nr_pages.
	 */
	unsigned long nr_pages = 1 << page_order(page);
	if (likely(PageBuddy(page))) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);

Quoted 3 lines

How much sense it make? nr_pages is still equal to itself since nr_pages can't larger than MAX_ORDER_NR_PAGES.

Hi Wanpeng,

If someone use the page during the double PageBuddy check, the value of private may be wrong. In my opinion, just keep the code unchanged.

Thanks,
Xishi Qiu

Quoted 11 lines

		/* Align with pageblock boundary */
		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
		    pageblock_nr_pages)
			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
		else
			low_pfn += nr_pages - 1;
	}
	continue;
}

Quoted 3 lines

Quoted 3 lines
Thanks,
Xishi Qiu

Wanpeng Li

2013-08-15 09:51 UTC #22 lore raw

In reply to Xishi Qiu

Quoted 36 lines · Xishi Qiu

Quoted 17 lines · Minchan Kim
Quoted 17 lines
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
Quoted 11 lines
Quoted 11 lines
if (PageBuddy(page)) {
#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}
Hi Minchan,
I understand now, but why use "end_pfn" here?
Maybe like this:

Quoted 8 lines

if (PageBuddy(page)) {
	/*
	 * page_order is racy without zone->lock but worst case
	 * by the racing is just skipping pageblock_nr_pages.
	 */
	unsigned long nr_pages = 1 << page_order(page);
	if (likely(PageBuddy(page))) {
		nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES);

How much sense it make? nr_pages is still equal to itself since nr_pages can't larger than MAX_ORDER_NR_PAGES.

Quoted 36 lines

		/* Align with pageblock boundary */
		if ((low_pfn & (pageblock_nr_pages - 1)) + nr_pages >
		    pageblock_nr_pages)
			low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
		else
			low_pfn += nr_pages - 1;
	}
	continue;
}
Thanks,
Xishi Qiu
Quoted 4 lines
Quoted 4 lines
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.
Quoted 15 lines
Quoted 5 lines
if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
Maybe it should be low_pfn = min(low_pfn, end_pfn - 1).
Quoted 5 lines
	}
	continue;
}

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>

Xishi Qiu

author

2013-08-15 06:39 UTC #23 lore raw

In reply to Minchan Kim

Quoted 18 lines · Minchan Kim

Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.

Quoted 6 lines

if (PageBuddy(page)) {

#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);

Hi Minchan,

I understand now, but why use "end_pfn" here?
Do you mean we should use pageblock_nr_pages instead of MAX_ORDER_NR_PAGES? Just like this:

if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	order = min(order, pageblock_order);
	if (PageBuddy(page))
		low_pfn += (1 << order) - 1;
	continue;
}

Thanks,
Xishi Qiu

Quoted 9 lines

	}
#endif
	continue;
}
so worst case is (pageblock_nr_pages - 1).
but we don't need to add CONFIG_MEMORY_ISOLATION so my suggestion is following as.

Quoted 10 lines

if (PageBuddy(page)) {
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
	continue;
}

Mel Gorman

2013-08-15 11:30 UTC #24 lore raw

In reply to Minchan Kim

Quoted 2 lines · Minchan Kim

Hello,

Well, this thread managed to get out of control for no good reason!

Quoted 29 lines

Quoted 4 lines
Quoted 3 lines
<SNIP>
So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case
Quoted 11 lines
Hi Minchan,
I mean if the private is set to a large number, it will skip 2^private pages, not (pageblock_nr_pages -1). I find somewhere will use page->private, such as fs. Here is the comment about parivate.
/* Mapping-private opaque data:
* usually used for buffer_heads
* if PagePrivate set; used for
* swp_entry_t if PageSwapCache;
* indicates order in the buddy
* system if PG_buddy is set.
*/
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}

Quoted 4 lines

min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.

Why? We're looking for pages to migrate. If the page is free and at the maximum order then there is no point searching in the middle of a free page.

Quoted 11 lines

if (PageBuddy(page)) {

#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}

Quoted 1 line

so worst case is (pageblock_nr_pages - 1).

No it isn't. The worst case it that the whole region being searched is skipped. For THP allocations, it would happen to work as being the pageblock boundary but it is not required by the API. I expect that end_pfn is not necessarily the next pageblock boundary for CMA allocations.

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Minchan Kim

2013-08-15 13:19 UTC #25 lore raw

Hi Mel,

Quoted 35 lines · Mel Gorman

Quoted 2 lines · Minchan Kim
Hello,
Well, this thread managed to get out of control for no good reason!
Quoted 29 lines
Quoted 4 lines
Quoted 3 lines
<SNIP>
So, what's the result by that?
As I said, it's just skipping (pageblock_nr_pages -1) at worst case
Quoted 11 lines
Hi Minchan,
I mean if the private is set to a large number, it will skip 2^private pages, not (pageblock_nr_pages -1). I find somewhere will use page->private, such as fs. Here is the comment about parivate.
/* Mapping-private opaque data:
* usually used for buffer_heads
* if PagePrivate set; used for
* swp_entry_t if PageSwapCache;
* indicates order in the buddy
* system if PG_buddy is set.
*/
Please read full thread in detail.
Mel suggested following as
if (PageBuddy(page)) {
        int nr_pages = (1 << page_order(page)) - 1;
        if (PageBuddy(page)) {
                nr_pages = min(nr_pages, MAX_ORDER_NR_PAGES - 1);
                low_pfn += nr_pages;
                continue;
        }
}

Quoted 8 lines

Quoted 4 lines
min(nr_pages, xxx) removes your concern but I think Mel's version isn't right. It should be aligned with pageblock boundary so I suggested following.
Why? We're looking for pages to migrate. If the page is free and at the maximum order then there is no point searching in the middle of a free page.

isolate_migratepages_range API works with [low_pfn, end_pfn) and we can't guarantee page_order in normal compaction path so I'd like to limit the skipping by end_pfn conservatively.

Quoted 12 lines

if (PageBuddy(page)) {

#ifdef CONFIG_MEMORY_ISOLATION
	unsigned long order = page_order(page);
	if (PageBuddy(page)) {
		low_pfn += (1 << order) - 1;
		low_pfn = min(low_pfn, end_pfn);
	}
#endif
	continue;
}

Quoted 7 lines

Quoted 1 line
so worst case is (pageblock_nr_pages - 1).
No it isn't. The worst case it that the whole region being searched is skipped. For THP allocations, it would happen to work as being the pageblock boundary but it is not required by the API. I expect that end_pfn is not necessarily the next pageblock boundary for CMA allocations.

Mel, as I said eariler, CMA and memory-hotplug don't have a race problem of page_order so we can consider only normal compaction path like high order allocation(ex, THP). So, about this race problem, worst case is the number of (pageblock_nr_pages - 1) skipping.

Quoted 10 lines

Signature · 9 lines

-- 
Mel Gorman
SUSE Labs

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

Signature · 8 lines

Kind regards,
Minchan Kim

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

Mel Gorman

2013-08-15 13:42 UTC #26 lore raw

Quoted 9 lines · Minchan Kim

Quoted 4 lines
Why? We're looking for pages to migrate. If the page is free and at the maximum order then there is no point searching in the middle of a free page.
isolate_migratepages_range API works with [low_pfn, end_pfn) and we can't guarantee page_order in normal compaction path so I'd like to limit the skipping by end_pfn conservatively.

Fine

s/MAX_ORDER_NR_PAGES/pageblock_nr_pages/

and take the min of it and

low_pfn = min(low_pfn, end_pfn - 1)

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Minchan Kim

2013-08-15 14:17 UTC #27 lore raw

Quoted 11 lines · Mel Gorman

Quoted 9 lines · Minchan Kim
Quoted 4 lines
Why? We're looking for pages to migrate. If the page is free and at the maximum order then there is no point searching in the middle of a free page.
isolate_migratepages_range API works with [low_pfn, end_pfn) and we can't guarantee page_order in normal compaction path so I'd like to limit the skipping by end_pfn conservatively.

Quoted 7 lines

Fine
s/MAX_ORDER_NR_PAGES/pageblock_nr_pages/
and take the min of it and
low_pfn = min(low_pfn, end_pfn - 1)

True. That was really what I suggested to Xishi.
Only thing we need it is just number to justify for proving the benefit.

Thanks.

Signature · 8 lines

Kind regards,
Minchan Kim

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

Andrew Morton

maintainer

2013-08-14 20:26 UTC #28 lore raw

In reply to Minchan Kim

Quoted 11 lines · On Thu, 15 Aug 2013 00:52:29 +0900 Minchan Kim <[email protected]> wrote:

Quoted 8 lines · Mel Gorman
Quoted 4 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the
Argh, I missed that.

I missed it as well. And so did Xishi Qiu.

Mel, we have a problem. What can we do to make this code more maintainable?

Mel Gorman

2013-08-14 22:22 UTC #29 lore raw

Quoted 12 lines · Andrew Morton

Quoted 10 lines · On Thu, 15 Aug 2013 00:52:29 +0900 Minchan Kim <[email protected]> wrote:
Quoted 8 lines · Mel Gorman
Quoted 4 lines · Xishi Qiu
A large free page buddy block will continue many times, so if the page is free, skip the whole page buddy block instead of one page.
Signed-off-by Xishi Qiu <[email protected]>
page_order cannot be used unless zone->lock is held which is not held in this path. Acquiring the lock would prevent parallel allocations from the

Quoted 6 lines

Quoted 1 line
Argh, I missed that.
I missed it as well. And so did Xishi Qiu.
Mel, we have a problem. What can we do to make this code more maintainable?

I sit in the bad man corner until I write a comment patch :/

page_order already has a comment but obviously the call site on compaction.c could do with a hint. As I think the consequences of this race can be dealt with I'm hoping Xishi Qiu will take the example I posted, fix it if it needs fixing, turn it into a real patch and run it through whatever test case led him to find this problem in the first place (HINT HINT). If that happens, great! If not, I might do it myself and failing that, I'll post a patch adding a comment explaining why page_order is not used there.

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Mel Gorman

2014-01-17 14:32 UTC #30 lore raw

PATCH 1/1 mm: Improve documentation of page_order

Developers occasionally try and optimise PFN scanners by using page_order but miss that in general it requires zone->lock. This has happened twice for compaction.c and rejected both times. This patch clarifies the documentation of page_order and adds a note to compaction.c why page_order is not used.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/compaction.c | 5 ++++-
 mm/internal.h   | 8 +++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

mm/compaction.c +4 -1

index f58bcd0..f91d26b 100644
@@ -522,7 +522,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 		if (!isolation_suitable(cc, page))
 			goto next_pageblock;
 
-		/* Skip if free */
+		/*
+		 * Skip if free. page_order cannot be used without zone->lock
+		 * as nothing prevents parallel allocations or buddy merging.
+		 */
 		if (PageBuddy(page))
 			continue;

mm/internal.h +5 -3

index 684f7aa..09cd8be 100644
@@ -144,9 +144,11 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 #endif
 
 /*
- * function for dealing with page's order in buddy system.
- * zone->lock is already acquired when we use these.
- * So, we don't need atomic page->flags operations here.
+ * This functions returns the order of a free page in the buddy system.
+ * In general, page_zone(page)->lock must be held by the caller to prevent
+ * the page being allocated in parallel and returning garbage as the order.
+ * If the caller does not hold page_zone(page), they must guarantee that
+ * the page cannot be allocated or merged in parallel.
  */
 static inline unsigned long page_order(struct page *page)
 {

Rafael Aquini

Acked-by on v1 1/1

2014-01-17 18:40 UTC #31 lore raw

Re: [PATCH] mm: Improve documentation of page_order

Quoted 36 lines · Mel Gorman

Developers occasionally try and optimise PFN scanners by using page_order but miss that in general it requires zone->lock. This has happened twice for compaction.c and rejected both times. This patch clarifies the documentation of page_order and adds a note to compaction.c why page_order is not used.
Signed-off-by Mel Gorman <[email protected]>
---
 mm/compaction.c | 5 ++++-
 mm/internal.h   | 8 +++++---
 2 files changed, 9 insertions(+), 4 deletions(-)
mm/compaction.c +4 -1
index f58bcd0..f91d26b 100644
@@ -522,7 +522,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 		if (!isolation_suitable(cc, page))
 			goto next_pageblock;
 
-		/* Skip if free */
+		/*
+		 * Skip if free. page_order cannot be used without zone->lock
+		 * as nothing prevents parallel allocations or buddy merging.
+		 */
 		if (PageBuddy(page))
 			continue;
 
mm/internal.h +0 -1
index 684f7aa..09cd8be 100644
@@ -144,9 +144,11 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 #endif
 
 /*
- * function for dealing with page's order in buddy system.

mm/internal.h isolate_migratepages_range(struct zone *zone, struct compact_control *cc, · line 154

Quoted 10 lines

mm/internal.h +5 -2

- * zone->lock is already acquired when we use these.
- * So, we don't need atomic page->flags operations here.
+ * This functions returns the order of a free page in the buddy system.
+ * In general, page_zone(page)->lock must be held by the caller to prevent
+ * the page being allocated in parallel and returning garbage as the order.
+ * If the caller does not hold page_zone(page), they must guarantee that
+ * the page cannot be allocated or merged in parallel.
  */
 static inline unsigned long page_order(struct page *page)
 {

Acked-by Rafael Aquini <[email protected]>

Laura Abbott

2014-01-17 18:53 UTC #32 lore raw

Re: [PATCH] mm: Improve documentation of page_order

In reply to Mel Gorman

Quoted 32 lines · Mel Gorman

Developers occasionally try and optimise PFN scanners by using page_order but miss that in general it requires zone->lock. This has happened twice for compaction.c and rejected both times. This patch clarifies the documentation of page_order and adds a note to compaction.c why page_order is not used.
Signed-off-by Mel Gorman <[email protected]>
---
  mm/compaction.c | 5 ++++-
  mm/internal.h   | 8 +++++---
  2 files changed, 9 insertions(+), 4 deletions(-)
mm/compaction.c +4 -1
index f58bcd0..f91d26b 100644
@@ -522,7 +522,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
  		if (!isolation_suitable(cc, page))
  			goto next_pageblock;
 
-		/* Skip if free */
+		/*
+		 * Skip if free. page_order cannot be used without zone->lock
+		 * as nothing prevents parallel allocations or buddy merging.
+		 */
  		if (PageBuddy(page))
  			continue;
 
mm/internal.h +0 -0
index 684f7aa..09cd8be 100644

mm/internal.h isolate_migratepages_range(struct zone *zone, struct compact_control *cc, · line 150

Quoted 10 lines

mm/internal.h +4 -3

@@ -144,9 +144,11 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
  #endif
 
  /*
- * function for dealing with page's order in buddy system.
- * zone->lock is already acquired when we use these.
- * So, we don't need atomic page->flags operations here.
+ * This functions returns the order of a free page in the buddy system.
+ * In general, page_zone(page)->lock must be held by the caller to prevent
+ * the page being allocated in parallel and returning garbage as the order.
+ * If the caller does not hold page_zone(page), they must guarantee that

                                  page_zone(page)->lock here?

Quoted 11 lines

mm/internal.h +1 -1
+ * the page cannot be allocated or merged in parallel.
   */
  static inline unsigned long page_order(struct page *page)
  {
 
--
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>

Signature · 8 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

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

Mel Gorman

2014-01-17 20:00 UTC #33 lore raw

Re: [PATCH] mm: Improve documentation of page_order

Quoted 35 lines · Laura Abbott

Quoted 34 lines · Mel Gorman
Developers occasionally try and optimise PFN scanners by using page_order but miss that in general it requires zone->lock. This has happened twice for compaction.c and rejected both times. This patch clarifies the documentation of page_order and adds a note to compaction.c why page_order is not used.
Signed-off-by Mel Gorman <[email protected]>
---
 mm/compaction.c | 5 ++++-
 mm/internal.h   | 8 +++++---
 2 files changed, 9 insertions(+), 4 deletions(-)
mm/compaction.c +4 -1
index f58bcd0..f91d26b 100644
@@ -522,7 +522,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 		if (!isolation_suitable(cc, page))
 			goto next_pageblock;
 
-		/* Skip if free */
+		/*
+		 * Skip if free. page_order cannot be used without zone->lock
+		 * as nothing prevents parallel allocations or buddy merging.
+		 */
 		if (PageBuddy(page))
 			continue;
 
mm/internal.h +0 -0
index 684f7aa..09cd8be 100644
@@ -144,9 +144,11 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 #endif
 

Quoted 9 lines

mm/internal.h isolate_migratepages_range(struct zone *zone, struct compact_control *cc, · line 150

Quoted 8 lines

mm/internal.h +4 -3

 /*
- * function for dealing with page's order in buddy system.
- * zone->lock is already acquired when we use these.
- * So, we don't need atomic page->flags operations here.
+ * This functions returns the order of a free page in the buddy system.
+ * In general, page_zone(page)->lock must be held by the caller to prevent
+ * the page being allocated in parallel and returning garbage as the order.
+ * If the caller does not hold page_zone(page), they must guarantee that

                                  page_zone(page)->lock here?

*sigh*

Yes, thanks.

Signature · 8 lines

Mel Gorman
SUSE Labs

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

Mel Gorman

2014-01-21 11:05 UTC #34 lore raw

PATCH 1/1 mm: Improve documentation of page_order v2

[[email protected]: Corrected a page_zone(page)->lock reference]

Signed-off-by Mel Gorman <[email protected]>

Acked-by Rafael Aquini <[email protected]>

Acked-by Minchan Kim <[email protected]>

---
 mm/compaction.c | 5 ++++-
 mm/internal.h   | 8 +++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

mm/compaction.c +4 -1

index f58bcd0..f91d26b 100644
@@ -522,7 +522,10 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 		if (!isolation_suitable(cc, page))
 			goto next_pageblock;
 
-		/* Skip if free */
+		/*
+		 * Skip if free. page_order cannot be used without zone->lock
+		 * as nothing prevents parallel allocations or buddy merging.
+		 */
 		if (PageBuddy(page))
 			continue;

mm/internal.h +5 -3

index 684f7aa..09cd8be 100644
@@ -144,9 +144,11 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 #endif
 
 /*
- * function for dealing with page's order in buddy system.
- * zone->lock is already acquired when we use these.
- * So, we don't need atomic page->flags operations here.
+ * This functions returns the order of a free page in the buddy system. In
+ * general, page_zone(page)->lock must be held by the caller to prevent the
+ * page being allocated in parallel and returning garbage as the order. If the
+ * caller does not hold page_zone(page)->lock, they must guarantee that the
+ * page cannot be allocated or merged in parallel.
  */
 static inline unsigned long page_order(struct page *page)
 {

Minchan Kim

Acked-by on v1 1/1

2014-01-20 06:10 UTC #35 lore raw

Re: [PATCH] mm: Improve documentation of page_order

In reply to Mel Gorman

Quoted 6 lines · Mel Gorman

Developers occasionally try and optimise PFN scanners by using page_order but miss that in general it requires zone->lock. This has happened twice for compaction.c and rejected both times. This patch clarifies the documentation of page_order and adds a note to compaction.c why page_order is not used.
Signed-off-by Mel Gorman <[email protected]>

Except Laura pointed out,

Acked-by Minchan Kim <[email protected]>

Thanks for following up this issue without forgetting.

Signature · 2 lines

Kind regards,
Minchan Kim