[PATCH 0/6] Optimise page alloc/free fast paths followup v2

[PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Mel Gorman]

This is a follow-up series based on Vlastimil Babka's review feedback.
The first change is that the second patch in the previous series was dropped
as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
is fine. The nodemask pointer is the same between cpuset retries. If the
zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
change then there is a second harmless pass through the page allocator.

Patches 1-3 are fixes for patches in mmotm. They should be taking into
account the changes in mmotm already made although that did involve
some guesswork. It should be relatively easy to merge into the correct
places. However, if there are major conflicts then let me know and I'll
respin the entire series.

Patches 4-6 are from Vlastimil with only minor modifications.

There is a marginal impact to the series but it's within the noise and
necessary to address the problems.

pagealloc
                                           4.6.0-rc4                  4.6.0-rc4
                                      mmotm-20150422              followup-v1r1
Min      alloc-odr0-1               317.00 (  0.00%)           319.00 ( -0.63%)
Min      alloc-odr0-2               232.00 (  0.00%)           231.00 (  0.43%)
Min      alloc-odr0-4               192.00 (  0.00%)           193.00 ( -0.52%)
Min      alloc-odr0-8               167.00 (  0.00%)           168.00 ( -0.60%)
Min      alloc-odr0-16              154.00 (  0.00%)           155.00 ( -0.65%)
Min      alloc-odr0-32              148.00 (  0.00%)           148.00 (  0.00%)
Min      alloc-odr0-64              145.00 (  0.00%)           145.00 (  0.00%)
Min      alloc-odr0-128             143.00 (  0.00%)           144.00 ( -0.70%)
Min      alloc-odr0-256             152.00 (  0.00%)           156.00 ( -2.63%)
Min      alloc-odr0-512             164.00 (  0.00%)           165.00 ( -0.61%)
Min      alloc-odr0-1024            172.00 (  0.00%)           175.00 ( -1.74%)
Min      alloc-odr0-2048            178.00 (  0.00%)           180.00 ( -1.12%)
Min      alloc-odr0-4096            184.00 (  0.00%)           186.00 ( -1.09%)
Min      alloc-odr0-8192            187.00 (  0.00%)           189.00 ( -1.07%)
Min      alloc-odr0-16384           188.00 (  0.00%)           189.00 ( -0.53%)
Min      free-odr0-1                178.00 (  0.00%)           177.00 (  0.56%)
Min      free-odr0-2                125.00 (  0.00%)           125.00 (  0.00%)
Min      free-odr0-4                 98.00 (  0.00%)            97.00 (  1.02%)
Min      free-odr0-8                 84.00 (  0.00%)            84.00 (  0.00%)
Min      free-odr0-16                79.00 (  0.00%)            80.00 ( -1.27%)
Min      free-odr0-32                75.00 (  0.00%)            75.00 (  0.00%)
Min      free-odr0-64                73.00 (  0.00%)            73.00 (  0.00%)
Min      free-odr0-128               72.00 (  0.00%)            72.00 (  0.00%)
Min      free-odr0-256               88.00 (  0.00%)            93.00 ( -5.68%)
Min      free-odr0-512              108.00 (  0.00%)           107.00 (  0.93%)
Min      free-odr0-1024             117.00 (  0.00%)           116.00 (  0.85%)
Min      free-odr0-2048             125.00 (  0.00%)           124.00 (  0.80%)
Min      free-odr0-4096             131.00 (  0.00%)           128.00 (  2.29%)
Min      free-odr0-8192             131.00 (  0.00%)           129.00 (  1.53%)
Min      free-odr0-16384            131.00 (  0.00%)           129.00 (  1.53%)
Min      total-odr0-1               495.00 (  0.00%)           496.00 ( -0.20%)
Min      total-odr0-2               357.00 (  0.00%)           356.00 (  0.28%)
Min      total-odr0-4               290.00 (  0.00%)           290.00 (  0.00%)
Min      total-odr0-8               251.00 (  0.00%)           252.00 ( -0.40%)
Min      total-odr0-16              233.00 (  0.00%)           235.00 ( -0.86%)
Min      total-odr0-32              223.00 (  0.00%)           223.00 (  0.00%)
Min      total-odr0-64              218.00 (  0.00%)           218.00 (  0.00%)
Min      total-odr0-128             215.00 (  0.00%)           216.00 ( -0.47%)
Min      total-odr0-256             240.00 (  0.00%)           249.00 ( -3.75%)
Min      total-odr0-512             272.00 (  0.00%)           272.00 (  0.00%)
Min      total-odr0-1024            289.00 (  0.00%)           291.00 ( -0.69%)
Min      total-odr0-2048            303.00 (  0.00%)           304.00 ( -0.33%)
Min      total-odr0-4096            315.00 (  0.00%)           314.00 (  0.32%)
Min      total-odr0-8192            318.00 (  0.00%)           318.00 (  0.00%)
Min      total-odr0-16384           319.00 (  0.00%)           318.00 (  0.31%)

 mm/page_alloc.c | 69 +++++++++++++++++++++++----------------------------------
 1 file changed, 28 insertions(+), 41 deletions(-)

-- 
2.6.4

[PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Mel Gorman]

This is a follow-up series based on Vlastimil Babka's review feedback.
The first change is that the second patch in the previous series was dropped
as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
is fine. The nodemask pointer is the same between cpuset retries. If the
zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
change then there is a second harmless pass through the page allocator.

Patches 1-3 are fixes for patches in mmotm. They should be taking into
account the changes in mmotm already made although that did involve
some guesswork. It should be relatively easy to merge into the correct
places. However, if there are major conflicts then let me know and I'll
respin the entire series.

Patches 4-6 are from Vlastimil with only minor modifications.

There is a marginal impact to the series but it's within the noise and
necessary to address the problems.

pagealloc
                                           4.6.0-rc4                  4.6.0-rc4
                                      mmotm-20150422              followup-v1r1
Min      alloc-odr0-1               317.00 (  0.00%)           319.00 ( -0.63%)
Min      alloc-odr0-2               232.00 (  0.00%)           231.00 (  0.43%)
Min      alloc-odr0-4               192.00 (  0.00%)           193.00 ( -0.52%)
Min      alloc-odr0-8               167.00 (  0.00%)           168.00 ( -0.60%)
Min      alloc-odr0-16              154.00 (  0.00%)           155.00 ( -0.65%)
Min      alloc-odr0-32              148.00 (  0.00%)           148.00 (  0.00%)
Min      alloc-odr0-64              145.00 (  0.00%)           145.00 (  0.00%)
Min      alloc-odr0-128             143.00 (  0.00%)           144.00 ( -0.70%)
Min      alloc-odr0-256             152.00 (  0.00%)           156.00 ( -2.63%)
Min      alloc-odr0-512             164.00 (  0.00%)           165.00 ( -0.61%)
Min      alloc-odr0-1024            172.00 (  0.00%)           175.00 ( -1.74%)
Min      alloc-odr0-2048            178.00 (  0.00%)           180.00 ( -1.12%)
Min      alloc-odr0-4096            184.00 (  0.00%)           186.00 ( -1.09%)
Min      alloc-odr0-8192            187.00 (  0.00%)           189.00 ( -1.07%)
Min      alloc-odr0-16384           188.00 (  0.00%)           189.00 ( -0.53%)
Min      free-odr0-1                178.00 (  0.00%)           177.00 (  0.56%)
Min      free-odr0-2                125.00 (  0.00%)           125.00 (  0.00%)
Min      free-odr0-4                 98.00 (  0.00%)            97.00 (  1.02%)
Min      free-odr0-8                 84.00 (  0.00%)            84.00 (  0.00%)
Min      free-odr0-16                79.00 (  0.00%)            80.00 ( -1.27%)
Min      free-odr0-32                75.00 (  0.00%)            75.00 (  0.00%)
Min      free-odr0-64                73.00 (  0.00%)            73.00 (  0.00%)
Min      free-odr0-128               72.00 (  0.00%)            72.00 (  0.00%)
Min      free-odr0-256               88.00 (  0.00%)            93.00 ( -5.68%)
Min      free-odr0-512              108.00 (  0.00%)           107.00 (  0.93%)
Min      free-odr0-1024             117.00 (  0.00%)           116.00 (  0.85%)
Min      free-odr0-2048             125.00 (  0.00%)           124.00 (  0.80%)
Min      free-odr0-4096             131.00 (  0.00%)           128.00 (  2.29%)
Min      free-odr0-8192             131.00 (  0.00%)           129.00 (  1.53%)
Min      free-odr0-16384            131.00 (  0.00%)           129.00 (  1.53%)
Min      total-odr0-1               495.00 (  0.00%)           496.00 ( -0.20%)
Min      total-odr0-2               357.00 (  0.00%)           356.00 (  0.28%)
Min      total-odr0-4               290.00 (  0.00%)           290.00 (  0.00%)
Min      total-odr0-8               251.00 (  0.00%)           252.00 ( -0.40%)
Min      total-odr0-16              233.00 (  0.00%)           235.00 ( -0.86%)
Min      total-odr0-32              223.00 (  0.00%)           223.00 (  0.00%)
Min      total-odr0-64              218.00 (  0.00%)           218.00 (  0.00%)
Min      total-odr0-128             215.00 (  0.00%)           216.00 ( -0.47%)
Min      total-odr0-256             240.00 (  0.00%)           249.00 ( -3.75%)
Min      total-odr0-512             272.00 (  0.00%)           272.00 (  0.00%)
Min      total-odr0-1024            289.00 (  0.00%)           291.00 ( -0.69%)
Min      total-odr0-2048            303.00 (  0.00%)           304.00 ( -0.33%)
Min      total-odr0-4096            315.00 (  0.00%)           314.00 (  0.32%)
Min      total-odr0-8192            318.00 (  0.00%)           318.00 (  0.00%)
Min      total-odr0-16384           319.00 (  0.00%)           318.00 (  0.31%)

 mm/page_alloc.c | 69 +++++++++++++++++++++++----------------------------------
 1 file changed, 28 insertions(+), 41 deletions(-)

-- 
2.6.4

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

[PATCH 1/6] mm, page_alloc: Only check PageCompound for high-order pages -fix

[Mel Gorman]

Vlastimil Babka pointed out that an unlikely annotation in free_pages_prepare
shrinks stack usage by moving compound handling to the end of the function.

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-30 (-30)
function                                     old     new   delta
free_pages_prepare                           771     741     -30

It's also consistent with the buffered_rmqueue path.

This is a fix to the mmotm patch
mm-page_alloc-only-check-pagecompound-for-high-order-pages.patch.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1da56779f8fa..d8383750bd43 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1003,7 +1003,7 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	 * Check tail pages before head page information is cleared to
 	 * avoid checking PageCompound for order-0 pages.
 	 */
-	if (order) {
+	if (unlikely(order)) {
 		bool compound = PageCompound(page);
 		int i;
 
-- 
2.6.4

[PATCH 1/6] mm, page_alloc: Only check PageCompound for high-order pages -fix

[Mel Gorman]

Vlastimil Babka pointed out that an unlikely annotation in free_pages_prepare
shrinks stack usage by moving compound handling to the end of the function.

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-30 (-30)
function                                     old     new   delta
free_pages_prepare                           771     741     -30

It's also consistent with the buffered_rmqueue path.

This is a fix to the mmotm patch
mm-page_alloc-only-check-pagecompound-for-high-order-pages.patch.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1da56779f8fa..d8383750bd43 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1003,7 +1003,7 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	 * Check tail pages before head page information is cleared to
 	 * avoid checking PageCompound for order-0 pages.
 	 */
-	if (order) {
+	if (unlikely(order)) {
 		bool compound = PageCompound(page);
 		int i;
 
-- 
2.6.4

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

[PATCH 2/6] mm, page_alloc: move might_sleep_if check to the allocator slowpath -revert

[Mel Gorman]

Vlastimil Babka pointed out that a patch weakens a zone_reclaim test
which while "safe" defeats the purposes of the debugging check. As most
configurations eliminate this check anyway, I thought it was better to
simply revert the patch instead of adding a second check in zone_reclaim.

This is a revert of the mmotm patch
mm-page_alloc-move-might_sleep_if-check-to-the-allocator-slowpath.patch .

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d8383750bd43..9ad4e68486e9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3606,8 +3606,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		return NULL;
 	}
 
-	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
-
 	/*
 	 * We also sanity check to catch abuse of atomic reserves being used by
 	 * callers that are not in atomic context.
@@ -3806,6 +3804,8 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 
 	lockdep_trace_alloc(gfp_mask);
 
+	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
+
 	if (should_fail_alloc_page(gfp_mask, order))
 		return NULL;
 
-- 
2.6.4

[PATCH 2/6] mm, page_alloc: move might_sleep_if check to the allocator slowpath -revert

[Mel Gorman]

Vlastimil Babka pointed out that a patch weakens a zone_reclaim test
which while "safe" defeats the purposes of the debugging check. As most
configurations eliminate this check anyway, I thought it was better to
simply revert the patch instead of adding a second check in zone_reclaim.

This is a revert of the mmotm patch
mm-page_alloc-move-might_sleep_if-check-to-the-allocator-slowpath.patch .

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d8383750bd43..9ad4e68486e9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3606,8 +3606,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		return NULL;
 	}
 
-	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
-
 	/*
 	 * We also sanity check to catch abuse of atomic reserves being used by
 	 * callers that are not in atomic context.
@@ -3806,6 +3804,8 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 
 	lockdep_trace_alloc(gfp_mask);
 
+	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
+
 	if (should_fail_alloc_page(gfp_mask, order))
 		return NULL;
 
-- 
2.6.4

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

[PATCH 3/6] mm, page_alloc: Check once if a zone has isolated pageblocks -fix

[Mel Gorman]

Vlastimil Babka pointed out that the original code was protected by
the zone lock and provided a fix.

This is a fix to the mmotm patch
mm-page_alloc-check-once-if-a-zone-has-isolated-pageblocks.patch . Once
applied the following line should be removed from the changelog "Technically
this is race-prone but so is the existing code."

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9ad4e68486e9..c63e5e7e4864 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1098,9 +1098,10 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	int migratetype = 0;
 	int batch_free = 0;
 	unsigned long nr_scanned;
-	bool isolated_pageblocks = has_isolate_pageblock(zone);
+	bool isolated_pageblocks;
 
 	spin_lock(&zone->lock);
+	isolated_pageblocks = has_isolate_pageblock(zone);
 	nr_scanned = zone_page_state(zone, NR_PAGES_SCANNED);
 	if (nr_scanned)
 		__mod_zone_page_state(zone, NR_PAGES_SCANNED, -nr_scanned);
-- 
2.6.4

[PATCH 3/6] mm, page_alloc: Check once if a zone has isolated pageblocks -fix

[Mel Gorman]

Vlastimil Babka pointed out that the original code was protected by
the zone lock and provided a fix.

This is a fix to the mmotm patch
mm-page_alloc-check-once-if-a-zone-has-isolated-pageblocks.patch . Once
applied the following line should be removed from the changelog "Technically
this is race-prone but so is the existing code."

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9ad4e68486e9..c63e5e7e4864 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1098,9 +1098,10 @@ static void free_pcppages_bulk(struct zone *zone, int count,
 	int migratetype = 0;
 	int batch_free = 0;
 	unsigned long nr_scanned;
-	bool isolated_pageblocks = has_isolate_pageblock(zone);
+	bool isolated_pageblocks;
 
 	spin_lock(&zone->lock);
+	isolated_pageblocks = has_isolate_pageblock(zone);
 	nr_scanned = zone_page_state(zone, NR_PAGES_SCANNED);
 	if (nr_scanned)
 		__mod_zone_page_state(zone, NR_PAGES_SCANNED, -nr_scanned);
-- 
2.6.4

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

[PATCH 4/6] mm, page_alloc: un-inline the bad part of free_pages_check

[Mel Gorman]

From: Vlastimil Babka <vbabka@suse.cz>

!DEBUG_VM size and bloat-o-meter:

add/remove: 1/0 grow/shrink: 0/2 up/down: 124/-370 (-246)
function                                     old     new   delta
free_pages_check_bad                           -     124    +124
free_pcppages_bulk                          1288    1171    -117
__free_pages_ok                              948     695    -253

DEBUG_VM:

add/remove: 1/0 grow/shrink: 0/1 up/down: 124/-214 (-90)
function                                     old     new   delta
free_pages_check_bad                           -     124    +124
free_pages_prepare                          1112     898    -214

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c63e5e7e4864..97894cbe2fa3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -906,18 +906,11 @@ static inline bool page_expected_state(struct page *page,
 	return true;
 }
 
-static inline int free_pages_check(struct page *page)
+static void free_pages_check_bad(struct page *page)
 {
 	const char *bad_reason;
 	unsigned long bad_flags;
 
-	if (page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE)) {
-		page_cpupid_reset_last(page);
-		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
-		return 0;
-	}
-
-	/* Something has gone sideways, find it */
 	bad_reason = NULL;
 	bad_flags = 0;
 
@@ -936,6 +929,17 @@ static inline int free_pages_check(struct page *page)
 		bad_reason = "page still charged to cgroup";
 #endif
 	bad_page(page, bad_reason, bad_flags);
+}
+static inline int free_pages_check(struct page *page)
+{
+	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE))) {
+		page_cpupid_reset_last(page);
+		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
+		return 0;
+	}
+
+	/* Something has gone sideways, find it */
+	free_pages_check_bad(page);
 	return 1;
 }
 
-- 
2.6.4

[PATCH 4/6] mm, page_alloc: un-inline the bad part of free_pages_check

[Mel Gorman]

From: Vlastimil Babka <vbabka@suse.cz>

!DEBUG_VM size and bloat-o-meter:

add/remove: 1/0 grow/shrink: 0/2 up/down: 124/-370 (-246)
function                                     old     new   delta
free_pages_check_bad                           -     124    +124
free_pcppages_bulk                          1288    1171    -117
__free_pages_ok                              948     695    -253

DEBUG_VM:

add/remove: 1/0 grow/shrink: 0/1 up/down: 124/-214 (-90)
function                                     old     new   delta
free_pages_check_bad                           -     124    +124
free_pages_prepare                          1112     898    -214

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c63e5e7e4864..97894cbe2fa3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -906,18 +906,11 @@ static inline bool page_expected_state(struct page *page,
 	return true;
 }
 
-static inline int free_pages_check(struct page *page)
+static void free_pages_check_bad(struct page *page)
 {
 	const char *bad_reason;
 	unsigned long bad_flags;
 
-	if (page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE)) {
-		page_cpupid_reset_last(page);
-		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
-		return 0;
-	}
-
-	/* Something has gone sideways, find it */
 	bad_reason = NULL;
 	bad_flags = 0;
 
@@ -936,6 +929,17 @@ static inline int free_pages_check(struct page *page)
 		bad_reason = "page still charged to cgroup";
 #endif
 	bad_page(page, bad_reason, bad_flags);
+}
+static inline int free_pages_check(struct page *page)
+{
+	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE))) {
+		page_cpupid_reset_last(page);
+		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
+		return 0;
+	}
+
+	/* Something has gone sideways, find it */
+	free_pages_check_bad(page);
 	return 1;
 }
 
-- 
2.6.4

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

[PATCH 5/6] mm, page_alloc: pull out side effects from free_pages_check

[Mel Gorman]

Check without side-effects should be easier to maintain. It also removes the
duplicated cpupid and flags reset done in !DEBUG_VM variant of both
free_pcp_prepare() and then bulkfree_pcp_prepare(). Finally, it enables
the next patch.

It shouldn't result in new branches, thanks to inlining of the check.

!DEBUG_VM bloat-o-meter:

add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-27 (-27)
function                                     old     new   delta
__free_pages_ok                              748     739      -9
free_pcppages_bulk                          1403    1385     -18

DEBUG_VM:

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-28 (-28)
function                                     old     new   delta
free_pages_prepare                           806     778     -28

This is also slightly faster because cpupid information is not set on tail
pages so we can avoid resets there.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 97894cbe2fa3..b823f00c275b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -932,11 +932,8 @@ static void free_pages_check_bad(struct page *page)
 }
 static inline int free_pages_check(struct page *page)
 {
-	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE))) {
-		page_cpupid_reset_last(page);
-		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
+	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE)))
 		return 0;
-	}
 
 	/* Something has gone sideways, find it */
 	free_pages_check_bad(page);
@@ -1016,7 +1013,11 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 		for (i = 1; i < (1 << order); i++) {
 			if (compound)
 				bad += free_tail_pages_check(page, page + i);
-			bad += free_pages_check(page + i);
+			if (unlikely(free_pages_check(page + i))) {
+				bad++;
+				continue;
+			}
+			(page + i)->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
 		}
 	}
 	if (PageAnonHead(page))
@@ -1025,6 +1026,8 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	if (bad)
 		return false;
 
+	page_cpupid_reset_last(page);
+	page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
 	reset_page_owner(page, order);
 
 	if (!PageHighMem(page)) {
-- 
2.6.4

[PATCH 5/6] mm, page_alloc: pull out side effects from free_pages_check

[Mel Gorman]

Check without side-effects should be easier to maintain. It also removes the
duplicated cpupid and flags reset done in !DEBUG_VM variant of both
free_pcp_prepare() and then bulkfree_pcp_prepare(). Finally, it enables
the next patch.

It shouldn't result in new branches, thanks to inlining of the check.

!DEBUG_VM bloat-o-meter:

add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-27 (-27)
function                                     old     new   delta
__free_pages_ok                              748     739      -9
free_pcppages_bulk                          1403    1385     -18

DEBUG_VM:

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-28 (-28)
function                                     old     new   delta
free_pages_prepare                           806     778     -28

This is also slightly faster because cpupid information is not set on tail
pages so we can avoid resets there.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 97894cbe2fa3..b823f00c275b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -932,11 +932,8 @@ static void free_pages_check_bad(struct page *page)
 }
 static inline int free_pages_check(struct page *page)
 {
-	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE))) {
-		page_cpupid_reset_last(page);
-		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
+	if (likely(page_expected_state(page, PAGE_FLAGS_CHECK_AT_FREE)))
 		return 0;
-	}
 
 	/* Something has gone sideways, find it */
 	free_pages_check_bad(page);
@@ -1016,7 +1013,11 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 		for (i = 1; i < (1 << order); i++) {
 			if (compound)
 				bad += free_tail_pages_check(page, page + i);
-			bad += free_pages_check(page + i);
+			if (unlikely(free_pages_check(page + i))) {
+				bad++;
+				continue;
+			}
+			(page + i)->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
 		}
 	}
 	if (PageAnonHead(page))
@@ -1025,6 +1026,8 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	if (bad)
 		return false;
 
+	page_cpupid_reset_last(page);
+	page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
 	reset_page_owner(page, order);
 
 	if (!PageHighMem(page)) {
-- 
2.6.4

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

[PATCH 6/6] mm, page_alloc: don't duplicate code in free_pcp_prepare

[Mel Gorman]

The new free_pcp_prepare() function shares a lot of code with
free_pages_prepare(), which makes this a maintenance risk when some future
patch modifies only one of them. We should be able to achieve the same effect
(skipping free_pages_check() from !DEBUG_VM configs) by adding a parameter to
free_pages_prepare() and making it inline, so the checks (and the order != 0
parts) are eliminated from the call from free_pcp_prepare().

!DEBUG_VM: bloat-o-meter reports no difference, as my gcc was already inlining
free_pages_prepare() and the elimination seems to work as expected

DEBUG_VM bloat-o-meter:

add/remove: 0/1 grow/shrink: 2/0 up/down: 1035/-778 (257)
function                                     old     new   delta
__free_pages_ok                              297    1060    +763
free_hot_cold_page                           480     752    +272
free_pages_prepare                           778       -    -778

Here inlining didn't occur before, and added some code, but it's ok for a debug
option.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b823f00c275b..bc4160bfb36b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -990,7 +990,8 @@ static int free_tail_pages_check(struct page *head_page, struct page *page)
 	return ret;
 }
 
-static bool free_pages_prepare(struct page *page, unsigned int order)
+static __always_inline bool free_pages_prepare(struct page *page, unsigned int order,
+						bool check_free)
 {
 	int bad = 0;
 
@@ -1022,7 +1023,8 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	}
 	if (PageAnonHead(page))
 		page->mapping = NULL;
-	bad += free_pages_check(page);
+	if (check_free)
+		bad += free_pages_check(page);
 	if (bad)
 		return false;
 
@@ -1046,7 +1048,7 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 #ifdef CONFIG_DEBUG_VM
 static inline bool free_pcp_prepare(struct page *page)
 {
-	return free_pages_prepare(page, 0);
+	return free_pages_prepare(page, 0, true);
 }
 
 static inline bool bulkfree_pcp_prepare(struct page *page)
@@ -1056,30 +1058,7 @@ static inline bool bulkfree_pcp_prepare(struct page *page)
 #else
 static bool free_pcp_prepare(struct page *page)
 {
-	VM_BUG_ON_PAGE(PageTail(page), page);
-
-	trace_mm_page_free(page, 0);
-	kmemcheck_free_shadow(page, 0);
-	kasan_poison_free_pages(page, 0);
-
-	if (PageAnonHead(page))
-		page->mapping = NULL;
-
-	reset_page_owner(page, 0);
-
-	if (!PageHighMem(page)) {
-		debug_check_no_locks_freed(page_address(page),
-					   PAGE_SIZE);
-		debug_check_no_obj_freed(page_address(page),
-					   PAGE_SIZE);
-	}
-	arch_free_page(page, 0);
-	kernel_poison_pages(page, 0, 0);
-	kernel_map_pages(page, 0, 0);
-
-	page_cpupid_reset_last(page);
-	page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
-	return true;
+	return free_pages_prepare(page, 0, false);
 }
 
 static bool bulkfree_pcp_prepare(struct page *page)
@@ -1257,7 +1236,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	int migratetype;
 	unsigned long pfn = page_to_pfn(page);
 
-	if (!free_pages_prepare(page, order))
+	if (!free_pages_prepare(page, order, true))
 		return;
 
 	migratetype = get_pfnblock_migratetype(page, pfn);
-- 
2.6.4

[PATCH 6/6] mm, page_alloc: don't duplicate code in free_pcp_prepare

[Mel Gorman]

The new free_pcp_prepare() function shares a lot of code with
free_pages_prepare(), which makes this a maintenance risk when some future
patch modifies only one of them. We should be able to achieve the same effect
(skipping free_pages_check() from !DEBUG_VM configs) by adding a parameter to
free_pages_prepare() and making it inline, so the checks (and the order != 0
parts) are eliminated from the call from free_pcp_prepare().

!DEBUG_VM: bloat-o-meter reports no difference, as my gcc was already inlining
free_pages_prepare() and the elimination seems to work as expected

DEBUG_VM bloat-o-meter:

add/remove: 0/1 grow/shrink: 2/0 up/down: 1035/-778 (257)
function                                     old     new   delta
__free_pages_ok                              297    1060    +763
free_hot_cold_page                           480     752    +272
free_pages_prepare                           778       -    -778

Here inlining didn't occur before, and added some code, but it's ok for a debug
option.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 mm/page_alloc.c | 35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b823f00c275b..bc4160bfb36b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -990,7 +990,8 @@ static int free_tail_pages_check(struct page *head_page, struct page *page)
 	return ret;
 }
 
-static bool free_pages_prepare(struct page *page, unsigned int order)
+static __always_inline bool free_pages_prepare(struct page *page, unsigned int order,
+						bool check_free)
 {
 	int bad = 0;
 
@@ -1022,7 +1023,8 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 	}
 	if (PageAnonHead(page))
 		page->mapping = NULL;
-	bad += free_pages_check(page);
+	if (check_free)
+		bad += free_pages_check(page);
 	if (bad)
 		return false;
 
@@ -1046,7 +1048,7 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
 #ifdef CONFIG_DEBUG_VM
 static inline bool free_pcp_prepare(struct page *page)
 {
-	return free_pages_prepare(page, 0);
+	return free_pages_prepare(page, 0, true);
 }
 
 static inline bool bulkfree_pcp_prepare(struct page *page)
@@ -1056,30 +1058,7 @@ static inline bool bulkfree_pcp_prepare(struct page *page)
 #else
 static bool free_pcp_prepare(struct page *page)
 {
-	VM_BUG_ON_PAGE(PageTail(page), page);
-
-	trace_mm_page_free(page, 0);
-	kmemcheck_free_shadow(page, 0);
-	kasan_poison_free_pages(page, 0);
-
-	if (PageAnonHead(page))
-		page->mapping = NULL;
-
-	reset_page_owner(page, 0);
-
-	if (!PageHighMem(page)) {
-		debug_check_no_locks_freed(page_address(page),
-					   PAGE_SIZE);
-		debug_check_no_obj_freed(page_address(page),
-					   PAGE_SIZE);
-	}
-	arch_free_page(page, 0);
-	kernel_poison_pages(page, 0, 0);
-	kernel_map_pages(page, 0, 0);
-
-	page_cpupid_reset_last(page);
-	page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
-	return true;
+	return free_pages_prepare(page, 0, false);
 }
 
 static bool bulkfree_pcp_prepare(struct page *page)
@@ -1257,7 +1236,7 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	int migratetype;
 	unsigned long pfn = page_to_pfn(page);
 
-	if (!free_pages_prepare(page, order))
+	if (!free_pages_prepare(page, order, true))
 		return;
 
 	migratetype = get_pfnblock_migratetype(page, pfn);
-- 
2.6.4

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

Re: [PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Vlastimil Babka]

On 04/27/2016 04:57 PM, Mel Gorman wrote:
> as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
> is fine. The nodemask pointer is the same between cpuset retries. If the
> zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
> change then there is a second harmless pass through the page allocator.

True. But I just realized (while working on direct compaction priorities)
that there's another subtle issue with the ALLOC_NO_WATERMARKS part.
According to the comment it should be ignoring mempolicies, but it still
honours ac.nodemask, and your patch is replacing NULL ac.nodemask with the
mempolicy one.

I think it's possibly easily fixed outside the fast path like this. If
you agree, consider it has my s-o-b:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3f052bbca41d..7ccaa6e023f3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3834,6 +3834,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 	alloc_mask = memalloc_noio_flags(gfp_mask);
 	ac.spread_dirty_pages = false;
 
+	/*
+	 * Restore the original nodemask, which might have been replaced with
+	 * &cpuset_current_mems_allowed to optimize the fast-path attempt.
+	 */
+	ac.nodemask = nodemask;
 	page = __alloc_pages_slowpath(alloc_mask, order, &ac);
 
 no_zone:

Re: [PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Vlastimil Babka]

On 04/27/2016 04:57 PM, Mel Gorman wrote:
> as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
> is fine. The nodemask pointer is the same between cpuset retries. If the
> zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
> change then there is a second harmless pass through the page allocator.

True. But I just realized (while working on direct compaction priorities)
that there's another subtle issue with the ALLOC_NO_WATERMARKS part.
According to the comment it should be ignoring mempolicies, but it still
honours ac.nodemask, and your patch is replacing NULL ac.nodemask with the
mempolicy one.

I think it's possibly easily fixed outside the fast path like this. If
you agree, consider it has my s-o-b:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3f052bbca41d..7ccaa6e023f3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3834,6 +3834,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 	alloc_mask = memalloc_noio_flags(gfp_mask);
 	ac.spread_dirty_pages = false;
 
+	/*
+	 * Restore the original nodemask, which might have been replaced with
+	 * &cpuset_current_mems_allowed to optimize the fast-path attempt.
+	 */
+	ac.nodemask = nodemask;
 	page = __alloc_pages_slowpath(alloc_mask, order, &ac);
 
 no_zone:

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

Re: [PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Mel Gorman]

On Mon, May 02, 2016 at 10:54:23AM +0200, Vlastimil Babka wrote:
> On 04/27/2016 04:57 PM, Mel Gorman wrote:
> > as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
> > is fine. The nodemask pointer is the same between cpuset retries. If the
> > zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
> > change then there is a second harmless pass through the page allocator.
> 
> True. But I just realized (while working on direct compaction priorities)
> that there's another subtle issue with the ALLOC_NO_WATERMARKS part.
> According to the comment it should be ignoring mempolicies, but it still
> honours ac.nodemask, and your patch is replacing NULL ac.nodemask with the
> mempolicy one.
> 
> I think it's possibly easily fixed outside the fast path like this. If
> you agree, consider it has my s-o-b:
> 

While I see your point, I don't necessarily see why this fixes it as the
original nodemask may also be a restricted set that ALLOC_NO_WATERMARKS
should ignore. How about this?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 79100583b9de..dbb08d102d41 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3432,9 +3432,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		/*
 		 * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
 		 * the allocation is high priority and these type of
-		 * allocations are system rather than user orientated
+		 * allocations are system rather than user orientated. If a
+		 * cpuset retry occurs then these values persist across the
+		 * retry but that's ok for a context ignoring watermarks.
 		 */
 		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
+		ac->high_zoneidx = MAX_NR_ZONES - 1;
+		ac->nodemask = NULL;
 		page = get_page_from_freelist(gfp_mask, order,
 						ALLOC_NO_WATERMARKS, ac);
 		if (page)

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Mel Gorman]

On Mon, May 02, 2016 at 10:54:23AM +0200, Vlastimil Babka wrote:
> On 04/27/2016 04:57 PM, Mel Gorman wrote:
> > as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
> > is fine. The nodemask pointer is the same between cpuset retries. If the
> > zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
> > change then there is a second harmless pass through the page allocator.
> 
> True. But I just realized (while working on direct compaction priorities)
> that there's another subtle issue with the ALLOC_NO_WATERMARKS part.
> According to the comment it should be ignoring mempolicies, but it still
> honours ac.nodemask, and your patch is replacing NULL ac.nodemask with the
> mempolicy one.
> 
> I think it's possibly easily fixed outside the fast path like this. If
> you agree, consider it has my s-o-b:
> 

While I see your point, I don't necessarily see why this fixes it as the
original nodemask may also be a restricted set that ALLOC_NO_WATERMARKS
should ignore. How about this?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 79100583b9de..dbb08d102d41 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3432,9 +3432,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		/*
 		 * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
 		 * the allocation is high priority and these type of
-		 * allocations are system rather than user orientated
+		 * allocations are system rather than user orientated. If a
+		 * cpuset retry occurs then these values persist across the
+		 * retry but that's ok for a context ignoring watermarks.
 		 */
 		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
+		ac->high_zoneidx = MAX_NR_ZONES - 1;
+		ac->nodemask = NULL;
 		page = get_page_from_freelist(gfp_mask, order,
 						ALLOC_NO_WATERMARKS, ac);
 		if (page)

-- 
Mel Gorman
SUSE Labs

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

Re: [PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Vlastimil Babka]

On 05/03/2016 10:50 AM, Mel Gorman wrote:
> On Mon, May 02, 2016 at 10:54:23AM +0200, Vlastimil Babka wrote:
>> On 04/27/2016 04:57 PM, Mel Gorman wrote:
>>> as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
>>> is fine. The nodemask pointer is the same between cpuset retries. If the
>>> zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
>>> change then there is a second harmless pass through the page allocator.
>>
>> True. But I just realized (while working on direct compaction priorities)
>> that there's another subtle issue with the ALLOC_NO_WATERMARKS part.
>> According to the comment it should be ignoring mempolicies, but it still
>> honours ac.nodemask, and your patch is replacing NULL ac.nodemask with the
>> mempolicy one.
>>
>> I think it's possibly easily fixed outside the fast path like this. If
>> you agree, consider it has my s-o-b:
>>
>
> While I see your point, I don't necessarily see why this fixes it as the
> original nodemask may also be a restricted set that ALLOC_NO_WATERMARKS
> should ignore.

I wasn't so sure about that, so I defensively went with just restoring 
the pre-patch behavior. I expect that it's safe to ignore mempolicies 
imposed on the process via e.g. taskset/numactl, for a a critical kernel 
allocation that happens to be done within the process context. But 
ignoring nodemask that's imposed by the kernel allocation itself might 
be breaking some expectations. I guess vma policies can also result in a 
restricted nodemask, but those should be for userspace allocations and 
never ALLOC_NO_WATERMARKS?

> How about this?
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 79100583b9de..dbb08d102d41 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3432,9 +3432,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   		/*
>   		 * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
>   		 * the allocation is high priority and these type of
> -		 * allocations are system rather than user orientated
> +		 * allocations are system rather than user orientated. If a
> +		 * cpuset retry occurs then these values persist across the
> +		 * retry but that's ok for a context ignoring watermarks.
>   		 */
>   		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
> +		ac->high_zoneidx = MAX_NR_ZONES - 1;

Wouldn't altering high_zoneidx like this result in e.g. DMA allocation 
ending up in a NORMAL zone? Also high_zoneidx doesn't seem to get 
restricted by mempolicy/nodemask anyway. Maybe you wanted to re-set 
preferred_zoneref? That would make sense, yeah.

> +		ac->nodemask = NULL;
>   		page = get_page_from_freelist(gfp_mask, order,
>   						ALLOC_NO_WATERMARKS, ac);
>   		if (page)
>

Re: [PATCH 0/6] Optimise page alloc/free fast paths followup v2

[Vlastimil Babka]

On 05/03/2016 10:50 AM, Mel Gorman wrote:
> On Mon, May 02, 2016 at 10:54:23AM +0200, Vlastimil Babka wrote:
>> On 04/27/2016 04:57 PM, Mel Gorman wrote:
>>> as the patch "mm, page_alloc: inline the fast path of the zonelist iterator"
>>> is fine. The nodemask pointer is the same between cpuset retries. If the
>>> zonelist changes due to ALLOC_NO_WATERMARKS *and* it races with a cpuset
>>> change then there is a second harmless pass through the page allocator.
>>
>> True. But I just realized (while working on direct compaction priorities)
>> that there's another subtle issue with the ALLOC_NO_WATERMARKS part.
>> According to the comment it should be ignoring mempolicies, but it still
>> honours ac.nodemask, and your patch is replacing NULL ac.nodemask with the
>> mempolicy one.
>>
>> I think it's possibly easily fixed outside the fast path like this. If
>> you agree, consider it has my s-o-b:
>>
>
> While I see your point, I don't necessarily see why this fixes it as the
> original nodemask may also be a restricted set that ALLOC_NO_WATERMARKS
> should ignore.

I wasn't so sure about that, so I defensively went with just restoring 
the pre-patch behavior. I expect that it's safe to ignore mempolicies 
imposed on the process via e.g. taskset/numactl, for a a critical kernel 
allocation that happens to be done within the process context. But 
ignoring nodemask that's imposed by the kernel allocation itself might 
be breaking some expectations. I guess vma policies can also result in a 
restricted nodemask, but those should be for userspace allocations and 
never ALLOC_NO_WATERMARKS?

> How about this?
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 79100583b9de..dbb08d102d41 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3432,9 +3432,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   		/*
>   		 * Ignore mempolicies if ALLOC_NO_WATERMARKS on the grounds
>   		 * the allocation is high priority and these type of
> -		 * allocations are system rather than user orientated
> +		 * allocations are system rather than user orientated. If a
> +		 * cpuset retry occurs then these values persist across the
> +		 * retry but that's ok for a context ignoring watermarks.
>   		 */
>   		ac->zonelist = node_zonelist(numa_node_id(), gfp_mask);
> +		ac->high_zoneidx = MAX_NR_ZONES - 1;

Wouldn't altering high_zoneidx like this result in e.g. DMA allocation 
ending up in a NORMAL zone? Also high_zoneidx doesn't seem to get 
restricted by mempolicy/nodemask anyway. Maybe you wanted to re-set 
preferred_zoneref? That would make sense, yeah.

> +		ac->nodemask = NULL;
>   		page = get_page_from_freelist(gfp_mask, order,
>   						ALLOC_NO_WATERMARKS, ac);
>   		if (page)
>

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