mm fix __alloc_pages cpuset ALLOC_* flags

PATCH In review Paul Jackson 4 participants 13 messages Nov 14, 2005 – Nov 15, 2005

Subsystems (3)

Memory management MM core Page allocator

lore

Share & tools

lore

Paul Jackson

author

2005-11-14 04:03 UTC #0 lore raw

PATCH 1/5 mm fix __alloc_pages cpuset ALLOC_* flags

Two changes to the setting of the ALLOC_CPUSET flag in mm/page_alloc.c:__alloc_pages()

1) A bug fix - the "ignoring mins" case should not be honoring ALLOC_CPUSET. This case of all cases, since it is handling a request that will free up more memory than is asked for (exiting tasks, e.g.) should be allowed to escape cpuset constraints when memory is tight.

2) A logic change to make it simpler. Honor cpusets even on GFP_ATOMIC (!wait) requests. With this, cpuset confinement applies to all requests except ALLOC_NO_WATERMARKS, so that in a subsequent cleanup patch, I can remove the ALLOC_CPUSET flag entirely. Since I don't know any real reason this logic has to be either way, I am choosing the path of the simplest code.

Signed-off-by Paul Jackson <[email protected]>

---

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

mm/page_alloc.c +2 -3

@@ -933,8 +933,7 @@ restart:
 		alloc_flags |= ALLOC_HARDER;
 	if (gfp_mask & __GFP_HIGH)
 		alloc_flags |= ALLOC_HIGH;
-	if (wait)
-		alloc_flags |= ALLOC_CPUSET;
+	alloc_flags |= ALLOC_CPUSET;
 
 	/*
 	 * Go through the zonelist again. Let __GFP_HIGH and allocations

@@ -956,7 +955,7 @@ restart:
 nofail_alloc:
 			/* go through the zonelist yet again, ignoring mins */
 			page = get_page_from_freelist(gfp_mask, order,
-				zonelist, ALLOC_NO_WATERMARKS|ALLOC_CPUSET);
+				zonelist, ALLOC_NO_WATERMARKS);
 			if (page)
 				goto got_pg;
 			if (gfp_mask & __GFP_NOFAIL) {

Signature · 9 lines

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

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

Paul Jackson

author

2005-11-14 04:03 UTC #1 lore raw

PATCH 2/5 mm simplify __alloc_pages cpuset ALLOC_* flags

Remove ALLOC_CPUSET flag from mm/page_alloc.c:__alloc_pages(). Thanks to the previous patch, it is equivalent to the setting of !ALLOC_NO_WATERMARKS, so redundant.

Signed-off-by Paul Jackson <[email protected]>

---

 mm/page_alloc.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

mm/page_alloc.c +4 -8

@@ -758,7 +758,6 @@ buffered_rmqueue(struct zone *zone, int 
 #define ALLOC_NO_WATERMARKS	0x01 /* don't check watermarks at all */
 #define ALLOC_HARDER		0x02 /* try to alloc harder */
 #define ALLOC_HIGH		0x04 /* __GFP_HIGH set */
-#define ALLOC_CPUSET		0x08 /* check for correct cpuset */
 
 /*
  * Return 1 if free pages are above 'mark'. This takes into account the order

@@ -814,11 +813,9 @@ get_page_from_freelist(gfp_t gfp_mask, u
 	 * See also cpuset_zone_allowed() comment in kernel/cpuset.c.
 	 */
 	do {
-		if ((alloc_flags & ALLOC_CPUSET) &&
-				!cpuset_zone_allowed(*z, gfp_mask))
-			continue;
-
 		if (!(alloc_flags & ALLOC_NO_WATERMARKS)) {
+			if (!cpuset_zone_allowed(*z, gfp_mask))
+				continue;
 			if (!zone_watermark_ok(*z, order, (*z)->pages_low,
 				    classzone_idx, alloc_flags))
 				continue;

@@ -911,7 +908,7 @@ __alloc_pages(gfp_t gfp_mask, unsigned i
 	}
 restart:
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order,
-				zonelist, ALLOC_CPUSET);
+				zonelist, 0);
 	if (page)
 		goto got_pg;

@@ -933,7 +930,6 @@ restart:
 		alloc_flags |= ALLOC_HARDER;
 	if (gfp_mask & __GFP_HIGH)
 		alloc_flags |= ALLOC_HIGH;
-	alloc_flags |= ALLOC_CPUSET;
 
 	/*
 	 * Go through the zonelist again. Let __GFP_HIGH and allocations

@@ -999,7 +995,7 @@ rebalance:
 		 * under heavy pressure.
 		 */
 		page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order,
-						zonelist, ALLOC_CPUSET);
+						zonelist, 0);
 		if (page)
 			goto got_pg;

Signature · 9 lines

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

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

Paul Jackson

author

2005-11-14 04:03 UTC #2 lore raw

PATCH 3/5 mm rationalize __alloc_pages ALLOC_* flag names

In reply to Paul Jackson

Rationalize mm/page_alloc.c:__alloc_pages() ALLOC flag names.

The case where we should be nice and not dip into memory reserves had been hiding behind the ALLOC_CPUSET flag, but with that gone, had no name and was a naked "0" value.

It was not clear which of HARDER or HIGH was harder, and it was not clear how ALLOC_HIGH related to the allocation policy for dipping into reserves.

Finally, the names and numeric order did not reflect the natural order of increasing urgency (increasing willingness to dip into reserves.)

Rename and renumber these flags to fix above.

A comment "Go through the zonelist again ..." explaining some of this was obsolete, and adequately covered by a modestly modified earlier comment, so bye bye obsolete comment.

Signed-off-by Paul Jackson <[email protected]>

---

 mm/page_alloc.c |   33 +++++++++++++--------------------
 1 files changed, 13 insertions(+), 20 deletions(-)

mm/page_alloc.c +13 ~~-20~~

@@ -755,9 +755,10 @@ buffered_rmqueue(struct zone *zone, int 
 	return page;
 }
 
-#define ALLOC_NO_WATERMARKS	0x01 /* don't check watermarks at all */
-#define ALLOC_HARDER		0x02 /* try to alloc harder */
-#define ALLOC_HIGH		0x04 /* __GFP_HIGH set */
+#define ALLOC_DONT_DIP	0x01 	/* don't dip into memory reserves */
+#define ALLOC_DIP_SOME	0x02 	/* dip into reserves some */
+#define ALLOC_DIP_ALOT	0x04 	/* dip into reserves further */
+#define ALLOC_MUSTHAVE	0x08 	/* ignore all constraints */
 
 /*
  * Return 1 if free pages are above 'mark'. This takes into account the order

@@ -770,9 +771,9 @@ int zone_watermark_ok(struct zone *z, in
 	long min = mark, free_pages = z->free_pages - (1 << order) + 1;
 	int o;
 
-	if (alloc_flags & ALLOC_HIGH)
+	if (alloc_flags & ALLOC_DIP_SOME)
 		min -= min / 2;
-	if (alloc_flags & ALLOC_HARDER)
+	if (alloc_flags & ALLOC_DIP_ALOT)
 		min -= min / 4;
 
 	if (free_pages <= min + z->lowmem_reserve[classzone_idx])

@@ -813,7 +814,7 @@ get_page_from_freelist(gfp_t gfp_mask, u
 	 * See also cpuset_zone_allowed() comment in kernel/cpuset.c.
 	 */
 	do {
-		if (!(alloc_flags & ALLOC_NO_WATERMARKS)) {
+		if (!(alloc_flags & ALLOC_MUSTHAVE)) {
 			if (!cpuset_zone_allowed(*z, gfp_mask))
 				continue;
 			if (!zone_watermark_ok(*z, order, (*z)->pages_low,

@@ -908,7 +909,7 @@ __alloc_pages(gfp_t gfp_mask, unsigned i
 	}
 restart:
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order,
-				zonelist, 0);
+				zonelist, ALLOC_DONT_DIP);
 	if (page)
 		goto got_pg;

@@ -923,22 +924,14 @@ restart:
 	 *
 	 * The caller may dip into page reserves a bit more if the caller
 	 * cannot run direct reclaim, or if the caller has realtime scheduling
-	 * policy.
+	 * policy or is asking for __GFP_HIGH memory.
 	 */
 	alloc_flags = 0;
 	if ((unlikely(rt_task(p)) && !in_interrupt()) || !wait)
-		alloc_flags |= ALLOC_HARDER;
+		alloc_flags |= ALLOC_DIP_ALOT;
 	if (gfp_mask & __GFP_HIGH)
-		alloc_flags |= ALLOC_HIGH;
+		alloc_flags |= ALLOC_DIP_SOME;
 
-	/*
-	 * Go through the zonelist again. Let __GFP_HIGH and allocations
-	 * coming from realtime tasks go deeper into reserves.
-	 *
-	 * This is the last chance, in general, before the goto nopage.
-	 * Ignore cpuset if GFP_ATOMIC (!wait) rather than fail alloc.
-	 * See also cpuset_zone_allowed() comment in kernel/cpuset.c.
-	 */
 	page = get_page_from_freelist(gfp_mask, order, zonelist, alloc_flags);
 	if (page)
 		goto got_pg;

@@ -951,7 +944,7 @@ restart:
 nofail_alloc:
 			/* go through the zonelist yet again, ignoring mins */
 			page = get_page_from_freelist(gfp_mask, order,
-				zonelist, ALLOC_NO_WATERMARKS);
+				zonelist, ALLOC_MUSTHAVE);
 			if (page)
 				goto got_pg;
 			if (gfp_mask & __GFP_NOFAIL) {

@@ -995,7 +988,7 @@ rebalance:
 		 * under heavy pressure.
 		 */
 		page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order,
-						zonelist, 0);
+						zonelist, ALLOC_DONT_DIP);
 		if (page)
 			goto got_pg;

Signature · 9 lines

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

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

Nick Piggin

2005-11-15 08:58 UTC #3 lore raw

Re: [PATCH 03/05] mm rationalize __alloc_pages ALLOC_* flag names

Quoted 2 lines · Paul Jackson

Rationalize mm/page_alloc.c:__alloc_pages() ALLOC flag names.

I don't really see the need for this. The names aren't clearly better, and the downside is that they move away from the terminlogy we've been using in the page allocator for the past few years.

Signature · 3 lines

SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com

Andrew Morton

2005-11-15 09:03 UTC #4 lore raw

Re: [PATCH 03/05] mm rationalize __alloc_pages ALLOC_* flag names

Quoted 9 lines · Nick Piggin

Quoted 2 lines · Paul Jackson
Rationalize mm/page_alloc.c:__alloc_pages() ALLOC flag names.
I don't really see the need for this. The names aren't clearly better, and the downside is that they move away from the terminlogy we've been using in the page allocator for the past few years.

I thought they were heaps better, actually.

-#define ALLOC_NO_WATERMARKS 0x01 /* don't check watermarks at all */
-#define ALLOC_HARDER 0x02 /* try to alloc harder */
-#define ALLOC_HIGH 0x04 /* __GFP_HIGH set */
+#define ALLOC_DONT_DIP 0x01 /* don't dip into memory reserves */
+#define ALLOC_DIP_SOME 0x02 /* dip into reserves some */
+#define ALLOC_DIP_ALOT 0x04 /* dip into reserves further */
+#define ALLOC_MUSTHAVE 0x08 /* ignore all constraints */

very explicit.

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

Nick Piggin

2005-11-15 09:53 UTC #5 lore raw

Re: [PATCH 03/05] mm rationalize __alloc_pages ALLOC_* flag names

Quoted 7 lines · Andrew Morton

Quoted 5 lines · Nick Piggin
Quoted 2 lines · Paul Jackson
Rationalize mm/page_alloc.c:__alloc_pages() ALLOC flag names.

Quoted 8 lines

Quoted 4 lines
I don't really see the need for this. The names aren't clearly better, and the downside is that they move away from the terminlogy we've been using in the page allocator for the past few years.
I thought they were heaps better, actually.

Some? Alot? Musthave?

To me it just changed the manner in which the hands are waving. Actually, I like the current names because ALLOC_HIGH explicitly is used for __GFP_HIGH allocations, and MUSTHAVE is not really an improvement on NO_WATERMARKS.

However if you'd really like to change the names, I'd prefer them to be more consistent, eg:

ALLOC_DIP_NONE
ALLOC_DIP_LESS
ALLOC_DIP_MORE
ALLOC_DIP_FULL

Signature · 3 lines

SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com

Paul Jackson

author

2005-11-15 19:20 UTC #6 lore raw

Re: [PATCH 03/05] mm rationalize __alloc_pages ALLOC_* flag names

Nick suggested:

Quoted 4 lines

ALLOC_DIP_NONE
ALLOC_DIP_LESS
ALLOC_DIP_MORE
ALLOC_DIP_FULL

Sweet. PATCH coming soon.

Signature · 9 lines

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.925.600.0401

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

Arjan van de Ven

2005-11-15 09:59 UTC #7 lore raw

Re: [PATCH 03/05] mm rationalize __alloc_pages ALLOC_* flag names

In reply to Andrew Morton

Quoted 21 lines · Andrew Morton

Quoted 9 lines · Nick Piggin
Quoted 2 lines · Paul Jackson
Rationalize mm/page_alloc.c:__alloc_pages() ALLOC flag names.
I don't really see the need for this. The names aren't clearly better, and the downside is that they move away from the terminlogy we've been using in the page allocator for the past few years.
I thought they were heaps better, actually.
mm/page_alloc.c +4 -3
-#define ALLOC_NO_WATERMARKS	0x01 /* don't check watermarks at all */
-#define ALLOC_HARDER		0x02 /* try to alloc harder */
-#define ALLOC_HIGH		0x04 /* __GFP_HIGH set */
+#define ALLOC_DONT_DIP	0x01 	/* don't dip into memory reserves */
+#define ALLOC_DIP_SOME	0x02 	/* dip into reserves some */
+#define ALLOC_DIP_ALOT	0x04 	/* dip into reserves further */
+#define ALLOC_MUSTHAVE	0x08 	/* ignore all constraints */

Quoted 1 line

very explicit.

maybe.
however... if names get changed anyway, maybe name them based on intent?

ALLOC_NORMAL
ALLOC_KERNELTHREAD
ALLOC_VMCAUSED
ALLOC_WOULDDEADLOCK

or something.. yes these are lame

perhaps both are needed.. bitflags for the implementation, and defines based on usage that are compounded bitflags..

Paul Jackson

author

2005-11-15 09:18 UTC #8 lore raw

Re: [PATCH 03/05] mm rationalize __alloc_pages ALLOC_* flag names

In reply to Nick Piggin

Quoted 3 lines · Nick

the downside is that they move away
from the terminlogy we've been using in the page allocator for the past few years.

I was trying to make the names more readable for the rest of us ;).

In the short term, there is seldom a reason to change names, as it impacts the current experts more than it helps others.

Over a sufficiently long term, everyone is an 'other'. Most of the people who will have reason to want to understand this code over the next five years are not experts in it now.

Signature · 9 lines

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.925.600.0401

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

Paul Jackson

author

2005-11-14 04:04 UTC #9 lore raw

PATCH 4/5 mm simplify __alloc_pages cpuset hardwall logic

In reply to Paul Jackson

The __GFP_HARDWALL flag logic is the same in mm/page_alloc.c for all calls to get_page_from_freelist(). Save a couple of lines of source code and a few bytes of text by only computing it once, inside get_page_from_freelist(), instead of passing it in each time. The key improvement is that it reduces the logic in __alloc_pages() by another detail. There is now -no- cpuset specific confinement logic in __alloc_pages(). It is all more generically stated in get_page_from_freelist().

Signed-off-by Paul Jackson <[email protected]>

---

 mm/page_alloc.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

mm/page_alloc.c +4 -2

@@ -815,6 +815,8 @@ get_page_from_freelist(gfp_t gfp_mask, u
 	 */
 	do {
 		if (!(alloc_flags & ALLOC_MUSTHAVE)) {
+			if (alloc_flags == ALLOC_DONT_DIP)
+				gfp_mask |= __GFP_HARDWALL;
 			if (!cpuset_zone_allowed(*z, gfp_mask))
 				continue;
 			if (!zone_watermark_ok(*z, order, (*z)->pages_low,

@@ -908,7 +910,7 @@ __alloc_pages(gfp_t gfp_mask, unsigned i
 		return NULL;
 	}
 restart:
-	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order,
+	page = get_page_from_freelist(gfp_mask, order,
 				zonelist, ALLOC_DONT_DIP);
 	if (page)
 		goto got_pg;

@@ -987,7 +989,7 @@ rebalance:
 		 * a parallel oom killing, we must fail if we're still
 		 * under heavy pressure.
 		 */
-		page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order,
+		page = get_page_from_freelist(gfp_mask, order,
 						zonelist, ALLOC_DONT_DIP);
 		if (page)
 			goto got_pg;

Signature · 9 lines

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

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

Paul Jackson

author

2005-11-14 04:04 UTC #10 lore raw

PATCH 5/5 mm GFP_ATOMIC comment

In reply to Paul Jackson

Clarify in comments that GFP_ATOMIC means both "don't sleep" and "use emergency pools", hence both ALLOC_DIP_ALOT and ALLOC_DIP_SOME.

Signed-off-by Paul Jackson <[email protected]>

---

 include/linux/gfp.h |    1 +
 mm/page_alloc.c     |    3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

include/linux/gfp.h +1 -0

@@ -58,6 +58,7 @@ struct vm_area_struct;
 			__GFP_NOFAIL|__GFP_NORETRY|__GFP_NO_GROW|__GFP_COMP| \
 			__GFP_NOMEMALLOC|__GFP_HARDWALL)
 
+/* GFP_ATOMIC means both !wait (__GFP_WAIT not set) and use emergency pool */
 #define GFP_ATOMIC	(__GFP_VALID | __GFP_HIGH)
 #define GFP_NOIO	(__GFP_VALID | __GFP_WAIT)
 #define GFP_NOFS	(__GFP_VALID | __GFP_WAIT | __GFP_IO)

mm/page_alloc.c +2 -1

@@ -926,7 +926,8 @@ restart:
 	 *
 	 * The caller may dip into page reserves a bit more if the caller
 	 * cannot run direct reclaim, or if the caller has realtime scheduling
-	 * policy or is asking for __GFP_HIGH memory.
+	 * policy or is asking for __GFP_HIGH memory.  GFP_ATOMIC requests will
+	 * set both ALLOC_DIP_ALOT (!wait) and ALLOC_DIP_SOME (__GFP_HIGH).
 	 */
 	alloc_flags = 0;
 	if ((unlikely(rt_task(p)) && !in_interrupt()) || !wait)

Signature · 9 lines

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

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

Nick Piggin

2005-11-15 08:50 UTC #11 lore raw

In reply to Paul Jackson

Quoted 9 lines · Paul Jackson

Two changes to the setting of the ALLOC_CPUSET flag in mm/page_alloc.c:__alloc_pages()
1) A bug fix - the "ignoring mins" case should not be honoring ALLOC_CPUSET. This case of all cases, since it is handling a request that will free up more memory than is asked for (exiting tasks, e.g.) should be allowed to escape cpuset constraints when memory is tight.

Quoted 8 lines

2) A logic change to make it simpler. Honor cpusets even on GFP_ATOMIC (!wait) requests. With this, cpuset confinement applies to all requests except ALLOC_NO_WATERMARKS, so that in a subsequent cleanup patch, I can remove the ALLOC_CPUSET flag entirely. Since I don't know any real reason this logic has to be either way, I am choosing the path of the simplest code.

Hi,

I think #1 is OK, however I was under the impression that you introduced the exception reverted in #2 due to seeing atomic allocation failures?!

Signature · 3 lines

SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com

Paul Jackson

author

2005-11-15 09:50 UTC #12 lore raw

Quoted 3 lines · Nick

I was under the impression that you
introduced the exception reverted in #2 due to seeing atomic allocation failures?!

For the record, the discussion Nick is recalling starts here:

http://www.ussg.iu.edu/hypermail/linux/kernel/0503.3/0763.html

My motivation for letting GFP_ATOMIC requests escape cpuset confinement was not based on seeing real world events, but based on code reading.

If some GFP_ATOMIC requests fail, the system can panic. Apparently these allocations are in init and setup code, where only a really sick system could fail a kmalloc() anyway. But, back then in March 2005, I concluded that GFP_ATOMIC requests were the absolute most essential allocations to satisfy, at all costs, cpusets be damned.

This time around, when reading __alloc_pages() again, I realized that GFP_ATOMIC requests did not get the highest priority in setting watermarks. Even they had to leave some reserves behind. The only allocations allowed to ignore all mins were the special case of allocations that promised to free more memory than they were consuming, really soon now (such as an exiting task).

I figured this time that what's good for watermark setting is good for cpuset setting.

Signature · 9 lines

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.925.600.0401

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