Use two zonelists per node instead of multiple zonelists v11r3

[PATCH 0/6] Use two zonelists per node instead of multiple zonelists v11r3

This is a rebase of the two-zonelist patchset to 2.6.25-rc2-mm1.

Mel, still on vacation last I checked, asked me to repost these as I'd already rebased them and I've been testing them continually on each -mm tree for months, hoping to see them in -mm for wider testing.

I have a series of mempolicy cleanup patches, including a rework of the reference counting that depend on this series. David R. has a series of mempolicy enhancements out for review that, IMO, will benefit from this series. In both cases, the removal of the custom zonelist for MPOL_BIND is the important feature.

Lee

---

Changelog since V11r2
o Rebase to 2.6.25-rc2-mm1

Changelog since V10
o Rebase to 2.6.24-rc4-mm1
o Clear up warnings in fs/buffer.c early in the patchset

Changelog since V9
o Rebase to 2.6.24-rc2-mm1
o Lookup the nodemask for each allocator callsite in mempolicy.c
o Update NUMA statistics based on preferred zone, not first zonelist entry
o When __GFP_THISNODE is specified with MPOL_BIND and the current node is not in the allowed nodemask, the first node in the mask will be used
o Stick with using two zonelists instead of one because of excessive complexity with corner cases

Changelog since V8
o Rebase to 2.6.24-rc2
o Added ack for the OOM changes
o Behave correctly when GFP_THISNODE and a node ID are specified
o Clear up warning over type of nodes_intersects() function

Changelog since V7
o Rebase to 2.6.23-rc8-mm2

Changelog since V6
o Fix build bug in relation to memory controller combined with one-zonelist
o Use while() instead of a stupid looking for()
o Instead of encoding zone index information in a pointer, this version introduces a structure that stores a zone pointer and its index

Changelog since V5
o Rebase to 2.6.23-rc4-mm1
o Drop patch that replaces inline functions with macros

Changelog since V4
o Rebase to -mm kernel. Host of memoryless patches collisions dealt with
o Do not call wakeup_kswapd() for every zone in a zonelist
o Dropped the FASTCALL removal
o Have cursor in iterator advance earlier
o Use nodes_and in cpuset_nodes_valid_mems_allowed()
o Use defines instead of inlines, noticably better performance on gcc-3.4 No difference on later compilers such as gcc 4.1
o Dropped gfp_skip patch until it is proven to be of benefit. Tests are currently inconclusive but it definitly consumes at least one cache line

Changelog since V3
o Fix compile error in the parisc change
o Calculate gfp_zone only once in __alloc_pages
o Calculate classzone_idx properly in get_page_from_freelist
o Alter check so that zone id embedded may still be used on UP
o Use Kamezawa-sans suggestion for skipping zones in zonelist
o Add __alloc_pages_nodemask() to filter zonelist based on a nodemask. This removes the need for MPOL_BIND to have a custom zonelist
o Move zonelist iterators and helpers to mm.h
o Change _zones from struct zone * to unsigned long

Changelog since V2
o shrink_zones() uses zonelist instead of zonelist->zones
o hugetlb uses zonelist iterator
o zone_idx information is embedded in zonelist pointers
o replace NODE_DATA(nid)->node_zonelist with node_zonelist(nid)

Changelog since V1
o Break up the patch into 3 patches
o Introduce iterators for zonelists
o Performance regression test

The following patches replace multiple zonelists per node with two zonelists that are filtered based on the GFP flags. The patches as a set fix a bug with regard to the use of MPOL_BIND and ZONE_MOVABLE. With this patchset, the MPOL_BIND will apply to the two highest zones when the highest zone is ZONE_MOVABLE. This should be considered as an alternative fix for the MPOL_BIND+ZONE_MOVABLE in 2.6.23 to the previously discussed hack that filters only custom zonelists.

The first patch cleans up an inconsistency where direct reclaim uses zonelist->zones where other places use zonelist.

The second patch introduces a helper function node_zonelist() for looking up the appropriate zonelist for a GFP mask which simplifies patches later in the set.

The third patch defines/remembers the "preferred zone" for numa statistics, as it is no longer always the first zone in a zonelist.

The forth patch replaces multiple zonelists with two zonelists that are filtered. The two zonelists are due to the fact that the memoryless patchset introduces a second set of zonelists for __GFP_THISNODE.

The fifth patch introduces helper macros for retrieving the zone and node indices of entries in a zonelist.

The final patch introduces filtering of the zonelists based on a nodemask. Two zonelists exist per node, one for normal allocations and one for __GFP_THISNODE.

Performance results varied depending on the machine configuration. In real workloads the gain/loss will depend on how much the userspace portion of the benchmark benefits from having more cache available due to reduced referencing of zonelists.

These are the range of performance losses/gains when running against 2.6.24-rc4-mm1. The set and these machines are a mix of i386, x86_64 and ppc64 both NUMA and non-NUMA.

loss to gain

Total CPU time on Kernbench: -0.86% to  1.13%
Elapsed   time on Kernbench: -0.79% to  0.76%
page_test from aim9:         -4.37% to  0.79%
brk_test  from aim9:         -0.71% to  4.07%
fork_test from aim9:         -1.84% to  4.60%
exec_test from aim9:         -0.71% to  1.08%

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

This patch records what the preferred zone is rather than assuming the first zone in the zonelist is it. This simplifies the reading of later patches in this set.

Quoted 3 lines

This patch records what the preferred zone is rather than assuming the first zone in the zonelist is it. This simplifies the reading of later patches in this set.

And is needed for correctness if GFP_THISNODE is used?

Reviewed-by Christoph Lameter <[email protected]>

include/linux/mmzone.h +29 -0

+/* Returns the first zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **first_zones_zonelist(struct zonelist *zonelist,
+					enum zone_type highest_zoneidx)
+{
+	struct zone **z;
+
+	/* Find the first suitable zone to use for the allocation */
+	z = zonelist->zones;
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/* Returns the next zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **next_zones_zonelist(struct zone **z,
+					enum zone_type highest_zoneidx)
+{
+	/* Find the next suitable zone to use for the allocation */
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/**
+ * for_each_zone_zonelist - helper macro to iterate over valid zones in a zonelist at or below a given zone index
+ * @zone - The current zone in the iterator
+ * @z - The current pointer within zonelist->zones being iterated

Quoted 29 lines · On Wed, 27 Feb 2008 16:47:34 -0500 Lee Schermerhorn <[email protected]>…

include/linux/mmzone.h +29 -0

+/* Returns the first zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **first_zones_zonelist(struct zonelist *zonelist,
+					enum zone_type highest_zoneidx)
+{
+	struct zone **z;
+
+	/* Find the first suitable zone to use for the allocation */
+	z = zonelist->zones;
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/* Returns the next zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **next_zones_zonelist(struct zone **z,
+					enum zone_type highest_zoneidx)
+{
+	/* Find the next suitable zone to use for the allocation */
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/**
+ * for_each_zone_zonelist - helper macro to iterate over valid zones in a zonelist at or below a given zone index
+ * @zone - The current zone in the iterator
+ * @z - The current pointer within zonelist->zones being iterated

include/linux/mmzone.h extern struct zone *next_zone(struct zon · line 755

Quoted 10 lines

include/linux/mmzone.h +10 -0

+ * @zlist - The zonelist being iterated
+ * @highidx - The zone index of the highest zone to return
+ *
+ * This iterator iterates though all zones at or below a given zone index.
+ */
+#define for_each_zone_zonelist(zone, z, zlist, highidx) \
+	for (z = first_zones_zonelist(zlist, highidx), zone = *z++;	\
+		zone;							\
+		z = next_zones_zonelist(z, highidx), zone = *z++)
+

omygawd will that thing generate a lot of code!

It has four call sites in mm/oom_kill.c and the overall patchset increases mm/oom_kill.o's text section (x86_64 allmodconfig) from 3268 bytes to 3845.

vmscan.o and page_alloc.o also grew a lot. otoh total vmlinux bloat from the patchset is only around 700 bytes, so I expect that with a little less insanity we could actually get an aggregate improvement here.

Some of the inlining in mmzone.h is just comical. Some of it is obvious (first_zones_zonelist) and some of it is less obvious (pfn_present).

I applied these for testing but I really don't think we should be merging such easily-fixed regressions into mainline. Could someone please take a look at de-porking core MM?

Also, I switched all your Tested-by:s to Signed-off-by:s. You were on the delivery path, so s-o-b is the appropriate tag. I would like to believe that Signed-off-by: implies Tested-by: anyway (rofl).

From: Mel Gorman <[email protected]>
[PATCH 3/6] Remember what the preferred zone is for zone_statistics

V11r3 against 2.6.25-rc2-mm1

On NUMA, zone_statistics() is used to record events like numa hit, miss and foreign. It assumes that the first zone in a zonelist is the preferred zone. When multiple zonelists are replaced by one that is filtered, this is no longer the case.

This patch records what the preferred zone is rather than assuming the first zone in the zonelist is it. This simplifies the reading of later patches in this set.

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

Tested-by Lee Schermerhorn <[email protected]>

Quoted 11 lines

omygawd will that thing generate a lot of code!

It has four call sites in mm/oom_kill.c and the overall patchset increases mm/oom_kill.o's text section (x86_64 allmodconfig) from 3268 bytes to 3845.

vmscan.o and page_alloc.o also grew a lot. otoh total vmlinux bloat from the patchset is only around 700 bytes, so I expect that with a little less insanity we could actually get an aggregate improvement here.

Some of the inlining in mmzone.h is just comical. Some of it is obvious (first_zones_zonelist) and some of it is less obvious (pfn_present).

Yeah, Mel said he was really reaching to avoid performance regression in this set.

Quoted 1 line

Quoted 3 lines

I applied these for testing but I really don't think we should be merging such easily-fixed regressions into mainline. Could someone please take a look at de-porking core MM?

OK, Mel should be back real soon now, and I'll take a look as well. At this point, we just wanted to get some more testing in -mm.

[PATCH 6/6] Filter based on a nodemask as well as a gfp_mask

V11r3 against 2.6.25-rc2-mm1

The MPOL_BIND policy creates a zonelist that is used for allocations controlled by that mempolicy. As the per-node zonelist is already being filtered based on a zone id, this patch adds a version of __alloc_pages() that takes a nodemask for further filtering. This eliminates the need for MPOL_BIND to create a custom zonelist.

A positive benefit of this is that allocations using MPOL_BIND now use the local node's distance-ordered zonelist instead of a custom node-id-ordered zonelist. I.e., pages will be allocated from the closest allowed node with available memory.

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

Acked-by Christoph Lameter <[email protected]>

Tested-by Lee Schermerhorn <[email protected]>

mm/oom_kill.c +1 -1

-int try_set_zone_oom(struct zonelist *zonelist)
+int try_set_zone_oom(struct zonelist *zonelist, gfp_t gfp_mask)
 {

The MPOL_BIND policy creates a zonelist that is used for allocations controlled by that mempolicy. As the per-node zonelist is already being filtered based on a zone id, this patch adds a version of __alloc_pages() that takes a nodemask for further filtering. This eliminates the need for MPOL_BIND to create a custom zonelist.

A positive benefit of this is that allocations using MPOL_BIND now use the local node's distance-ordered zonelist instead of a custom node-id-ordered zonelist. I.e., pages will be allocated from the closest allowed node with available memory.

mm/mempolicy.c +2 -2

-/* Generate a custom zonelist for the BIND policy. */
-static struct zonelist *bind_zonelist(nodemask_t *nodes)
+/* Check that the nodemask contains at least one populated zone */
+static int is_valid_nodemask(nodemask_t *nodemask)
 {

From: Mel Gorman <[email protected]>
[PATCH 0/6] Use two zonelists per node instead of multiple zonelists v11r3

This is a rebase of the two-zonelist patchset to 2.6.25-rc2-mm1.

Mel, still on vacation last I checked, asked me to repost these as I'd already rebased them and I've been testing them continually on each -mm tree for months, hoping to see them in -mm for wider testing.

These are the range of performance losses/gains when running against 2.6.24-rc4-mm1. The set and these machines are a mix of i386, x86_64 and ppc64 both NUMA and non-NUMA.

loss to gain

Total CPU time on Kernbench: -0.86% to  1.13%
Elapsed   time on Kernbench: -0.79% to  0.76%
page_test from aim9:         -4.37% to  0.79%
brk_test  from aim9:         -0.71% to  4.07%
fork_test from aim9:         -1.84% to  4.60%
exec_test from aim9:         -0.71% to  1.08%

Quoted 3 lines

This patch records what the preferred zone is rather than assuming the first zone in the zonelist is it. This simplifies the reading of later patches in this set.

And is needed for correctness if GFP_THISNODE is used?

Reviewed-by Christoph Lameter <[email protected]>

Quoted 10 lines · On Wed, 27 Feb 2008 16:47:28 -0500 Lee Schermerhorn <[email protected]>…

From: Mel Gorman <[email protected]>
[PATCH 3/6] Remember what the preferred zone is for zone_statistics

V11r3 against 2.6.25-rc2-mm1

On NUMA, zone_statistics() is used to record events like numa hit, miss and foreign. It assumes that the first zone in a zonelist is the preferred zone. When multiple zonelists are replaced by one that is filtered, this is no longer the case.

Quoted 7 lines

This patch records what the preferred zone is rather than assuming the first zone in the zonelist is it. This simplifies the reading of later patches in this set.

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

Tested-by Lee Schermerhorn <[email protected]>

I have no objection to the direction but

mm/page_alloc.c void split_page(struct page *page, unsig · line 1065

Quoted 3 lines

mm/page_alloc.c +1 -0

+static struct page *buffered_rmqueue(struct zone *preferred_zone,
 			struct zone *zone, int order, gfp_t gfp_flags)
 {

Can't this be written like this ?

struct page *
buffered_rmqueue(struct zone *zone, int order, gfp_t gfp_flags, bool numa_hit)

Can't caller itself set this bool value ?

Quoted 29 lines · On Wed, 27 Feb 2008 16:47:34 -0500 Lee Schermerhorn <[email protected]>…

include/linux/mmzone.h +29 -0

+/* Returns the first zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **first_zones_zonelist(struct zonelist *zonelist,
+					enum zone_type highest_zoneidx)
+{
+	struct zone **z;
+
+	/* Find the first suitable zone to use for the allocation */
+	z = zonelist->zones;
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/* Returns the next zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **next_zones_zonelist(struct zone **z,
+					enum zone_type highest_zoneidx)
+{
+	/* Find the next suitable zone to use for the allocation */
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/**
+ * for_each_zone_zonelist - helper macro to iterate over valid zones in a zonelist at or below a given zone index
+ * @zone - The current zone in the iterator
+ * @z - The current pointer within zonelist->zones being iterated

include/linux/mmzone.h extern struct zone *next_zone(struct zon · line 755

Quoted 10 lines

include/linux/mmzone.h +10 -0

+ * @zlist - The zonelist being iterated
+ * @highidx - The zone index of the highest zone to return
+ *
+ * This iterator iterates though all zones at or below a given zone index.
+ */
+#define for_each_zone_zonelist(zone, z, zlist, highidx) \
+	for (z = first_zones_zonelist(zlist, highidx), zone = *z++;	\
+		zone;							\
+		z = next_zones_zonelist(z, highidx), zone = *z++)
+

omygawd will that thing generate a lot of code!

It has four call sites in mm/oom_kill.c and the overall patchset increases mm/oom_kill.o's text section (x86_64 allmodconfig) from 3268 bytes to 3845.

vmscan.o and page_alloc.o also grew a lot. otoh total vmlinux bloat from the patchset is only around 700 bytes, so I expect that with a little less insanity we could actually get an aggregate improvement here.

Some of the inlining in mmzone.h is just comical. Some of it is obvious (first_zones_zonelist) and some of it is less obvious (pfn_present).

I applied these for testing but I really don't think we should be merging such easily-fixed regressions into mainline. Could someone please take a look at de-porking core MM?

Also, I switched all your Tested-by:s to Signed-off-by:s. You were on the delivery path, so s-o-b is the appropriate tag. I would like to believe that Signed-off-by: implies Tested-by: anyway (rofl).

On (28/02/08 13:32), Andrew Morton didst pronounce:

Quoted 45 lines

Quoted 29 lines · On Wed, 27 Feb 2008 16:47:34 -0500 Lee Schermerhorn <[email protected]>…

include/linux/mmzone.h +29 -0

+/* Returns the first zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **first_zones_zonelist(struct zonelist *zonelist,
+					enum zone_type highest_zoneidx)
+{
+	struct zone **z;
+
+	/* Find the first suitable zone to use for the allocation */
+	z = zonelist->zones;
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/* Returns the next zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **next_zones_zonelist(struct zone **z,
+					enum zone_type highest_zoneidx)
+{
+	/* Find the next suitable zone to use for the allocation */
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/**
+ * for_each_zone_zonelist - helper macro to iterate over valid zones in a zonelist at or below a given zone index
+ * @zone - The current zone in the iterator
+ * @z - The current pointer within zonelist->zones being iterated

include/linux/mmzone.h extern struct zone *next_zone(struct zon · line 755

Quoted 10 lines

include/linux/mmzone.h +10 -0

+ * @zlist - The zonelist being iterated
+ * @highidx - The zone index of the highest zone to return
+ *
+ * This iterator iterates though all zones at or below a given zone index.
+ */
+#define for_each_zone_zonelist(zone, z, zlist, highidx) \
+	for (z = first_zones_zonelist(zlist, highidx), zone = *z++;	\
+		zone;							\
+		z = next_zones_zonelist(z, highidx), zone = *z++)
+

omygawd will that thing generate a lot of code!

Quoted 3 lines

It has four call sites in mm/oom_kill.c and the overall patchset increases mm/oom_kill.o's text section (x86_64 allmodconfig) from 3268 bytes to 3845.

Yeah... that's pretty bad. They were inlined to avoid function call overhead when trying to avoid any additional performance overhead but the text overhead is not helping either. I'll start looking at things to uninline and see what can be gained text-reduction wise without mucking performance.

Quoted 3 lines

vmscan.o and page_alloc.o also grew a lot. otoh total vmlinux bloat from the patchset is only around 700 bytes, so I expect that with a little less insanity we could actually get an aggregate improvement here.

Quoted 9 lines

Some of the inlining in mmzone.h is just comical. Some of it is obvious (first_zones_zonelist) and some of it is less obvious (pfn_present).

I applied these for testing but I really don't think we should be merging such easily-fixed regressions into mainline. Could someone please take a look at de-porking core MM?

Quoted 4 lines

Also, I switched all your Tested-by:s to Signed-off-by:s. You were on the delivery path, so s-o-b is the appropriate tag. I would like to believe that Signed-off-by: implies Tested-by: anyway (rofl).

+ usage in slab.c and slub.c appears to be the fallback/slow path. Christoph can chime in, here, if he disagrees.

Quoted 46 lines

On (28/02/08 13:32), Andrew Morton didst pronounce:

Quoted 45 lines

Quoted 29 lines · On Wed, 27 Feb 2008 16:47:34 -0500 Lee Schermerhorn <[email protected]>…

include/linux/mmzone.h +29 -0

+/* Returns the first zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **first_zones_zonelist(struct zonelist *zonelist,
+					enum zone_type highest_zoneidx)
+{
+	struct zone **z;
+
+	/* Find the first suitable zone to use for the allocation */
+	z = zonelist->zones;
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/* Returns the next zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **next_zones_zonelist(struct zone **z,
+					enum zone_type highest_zoneidx)
+{
+	/* Find the next suitable zone to use for the allocation */
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/**
+ * for_each_zone_zonelist - helper macro to iterate over valid zones in a zonelist at or below a given zone index
+ * @zone - The current zone in the iterator
+ * @z - The current pointer within zonelist->zones being iterated

include/linux/mmzone.h extern struct zone *next_zone(struct zon · line 755

Quoted 10 lines

include/linux/mmzone.h +10 -0

+ * @zlist - The zonelist being iterated
+ * @highidx - The zone index of the highest zone to return
+ *
+ * This iterator iterates though all zones at or below a given zone index.
+ */
+#define for_each_zone_zonelist(zone, z, zlist, highidx) \
+	for (z = first_zones_zonelist(zlist, highidx), zone = *z++;	\
+		zone;							\
+		z = next_zones_zonelist(z, highidx), zone = *z++)
+

omygawd will that thing generate a lot of code!

Quoted 12 lines

Quoted 3 lines

It has four call sites in mm/oom_kill.c and the overall patchset increases mm/oom_kill.o's text section (x86_64 allmodconfig) from 3268 bytes to 3845.

Yeah... that's pretty bad. They were inlined to avoid function call overhead when trying to avoid any additional performance overhead but the text overhead is not helping either. I'll start looking at things to uninline and see what can be gained text-reduction wise without mucking performance.

Quoted 3 lines

vmscan.o and page_alloc.o also grew a lot. otoh total vmlinux bloat from the patchset is only around 700 bytes, so I expect that with a little less insanity we could actually get an aggregate improvement here.

Mel:

Thinking about this:

for_each_zone_zonelist():

Seems like the call sites to this macro are not hot paths, so maybe these can call out to a zonelist iterator func in page_alloc.c or, as Kame-san suggested, mmzone.c.

+ oom_kill and vmscan call sites: if these are hot, we're already in, uh..., slow mode.

+ usage in slab.c and slub.c appears to be the fallback/slow path. Christoph can chime in, here, if he disagrees.

+ in page_alloc.c: waking up of kswapd and counting free zone pages [mostly for init code] don't appear to be fast paths.

+ The call site in hugetlb.c is in the huge-page allocation path, which is under a global spinlock. So, any slowdown here could result in longer lock hold time and higher contention. But, I have to believe that in the grand scheme of things, huge-page allocation is not that hot. [Someone faulting in terabytes of hugepages might contest that.]

That leaves the call to for_each_zone_zonelist_nodemask() in get_page_from_freelist(). This might be deserving of inlining?

If this works out, we could end up with these macros being inlined in only 2 places: get_page_from_freelist() and a to-be-designed zonelist iterator function. [In fact, I believe that such an iterator need not expose the details of zonelists outside of page_alloc/mmzone, but that would require more rework of the call sites, and additional helper functions. Maybe someday...]

Comments?

Right now, I've got to build/test the latest reclaim scalability patches that Rik posted, and clean up the issues already pointed out. If you don't get to this, I can look at it further next week.

Lee

Quoted 14 lines

Quoted 13 lines

Some of the inlining in mmzone.h is just comical. Some of it is obvious (first_zones_zonelist) and some of it is less obvious (pfn_present).

I applied these for testing but I really don't think we should be merging such easily-fixed regressions into mainline. Could someone please take a look at de-porking core MM?

Also, I switched all your Tested-by:s to Signed-off-by:s. You were on the delivery path, so s-o-b is the appropriate tag. I would like to believe that Signed-off-by: implies Tested-by: anyway (rofl).

PATCH Mempolicy - make dequeue_huge_page_vma() obey MPOL_BIND nodemask

dequeue_huge_page_vma() is not obeying the MPOL_BIND nodemask with the zonelist rework. It needs to search only zones in the mempolicy nodemask for hugepages.

Use for_each_zone_zonelist_nodemask() instead of
for_each_zone_zonelist().

Note: this will bloat mm/hugetlb.o a bit until Mel reworks the inlining of the for_each_zone... macros and helpers.

Added mempolicy helper function mpol_bind_nodemask() to hide the details of mempolicy from hugetlb and to avoid #ifdef CONFIG_NUMA in dequeue_huge_page_vma().

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/mempolicy.h |   13 +++++++++++++
 mm/hugetlb.c              |    4 +++-
 2 files changed, 16 insertions(+), 1 deletion(-)

Index: linux-2.6.25-rc3-mm1/mm/hugetlb.c
===================================================================

Quoted 22 lines · Lee Schermerhorn

PATCH Mempolicy - make dequeue_huge_page_vma() obey MPOL_BIND nodemask

dequeue_huge_page_vma() is not obeying the MPOL_BIND nodemask with the zonelist rework. It needs to search only zones in the mempolicy nodemask for hugepages.

Use for_each_zone_zonelist_nodemask() instead of
for_each_zone_zonelist().

Note: this will bloat mm/hugetlb.o a bit until Mel reworks the inlining of the for_each_zone... macros and helpers.

Added mempolicy helper function mpol_bind_nodemask() to hide the details of mempolicy from hugetlb and to avoid #ifdef CONFIG_NUMA in dequeue_huge_page_vma().

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/mempolicy.h |   13 +++++++++++++
 mm/hugetlb.c              |    4 +++-
 2 files changed, 16 insertions(+), 1 deletion(-)

mm/hugetlb.c static struct page *dequeue_huge_page_vm · line 102

Quoted 9 lines

Index: linux-2.6.25-rc3-mm1/mm/hugetlb.c
===================================================================

mm/hugetlb.c +1 -0

@@ -99,8 +99,10 @@ static struct page *dequeue_huge_page_vm
 					htlb_alloc_mask, &mpol);
 	struct zone *zone;
 	struct zoneref *z;
+	nodemask_t *nodemask = mpol_bind_nodemask(mpol);

We get this mpol from huge_zonelist(). Would it perhaps make sense to pass the nodemask as a parameter, too, to huge_zonelist(), rather than adding mpol_bind_nodemask()? This is the only user of it in-tree.

Subject: [PATCH] 2.6.25-rc3-mm1 - Mempolicy: make dequeue_huge_page_vma() obey MPOL_BIND nodemask

Date: Wed, 05 Mar 2008 11:06:34 -0500
Organization: HP/OSLO
X-Mailer: Evolution 2.6.1

PATCH Mempolicy - make dequeue_huge_page_vma() obey MPOL_BIND nodemask

dequeue_huge_page_vma() is not obeying the MPOL_BIND nodemask with the zonelist rework. It needs to search only zones in the mempolicy nodemask for hugepages.

Use for_each_zone_zonelist_nodemask() instead of
for_each_zone_zonelist().

Note: this will bloat mm/hugetlb.o a bit until Mel reworks the inlining of the for_each_zone... macros and helpers.

Added mempolicy helper function mpol_bind_nodemask() to hide the details of mempolicy from hugetlb and to avoid #ifdef CONFIG_NUMA in dequeue_huge_page_vma().

Signed-off-by Lee Schermerhorn <[email protected]>

Quoted 23 lines · Nishanth Aravamudan

Quoted 22 lines · Lee Schermerhorn

PATCH Mempolicy - make dequeue_huge_page_vma() obey MPOL_BIND nodemask

dequeue_huge_page_vma() is not obeying the MPOL_BIND nodemask with the zonelist rework. It needs to search only zones in the mempolicy nodemask for hugepages.

Use for_each_zone_zonelist_nodemask() instead of
for_each_zone_zonelist().

Note: this will bloat mm/hugetlb.o a bit until Mel reworks the inlining of the for_each_zone... macros and helpers.

Added mempolicy helper function mpol_bind_nodemask() to hide the details of mempolicy from hugetlb and to avoid #ifdef CONFIG_NUMA in dequeue_huge_page_vma().

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/mempolicy.h |   13 +++++++++++++
 mm/hugetlb.c              |    4 +++-
 2 files changed, 16 insertions(+), 1 deletion(-)

Quoted 13 lines

mm/hugetlb.c static struct page *dequeue_huge_page_vm · line 102

Quoted 9 lines

Index: linux-2.6.25-rc3-mm1/mm/hugetlb.c
===================================================================

mm/hugetlb.c +1 -0

@@ -99,8 +99,10 @@ static struct page *dequeue_huge_page_vm
 					htlb_alloc_mask, &mpol);
 	struct zone *zone;
 	struct zoneref *z;
+	nodemask_t *nodemask = mpol_bind_nodemask(mpol);

We get this mpol from huge_zonelist(). Would it perhaps make sense to pass the nodemask as a parameter, too, to huge_zonelist(), rather than adding mpol_bind_nodemask()? This is the only user of it in-tree.

Nish:

I thought of that. I didn't go that way because I'd either need to pass a [pointer to a pointer to] a nodemask in addition to the [pointer to a pointer to] the mpol, so that I can release the reference on the mpol after the allocation is finished;

or I'd need to copy the nodemask [which can get pretty big] in the allocation path. I wanted to avoid both of those. I suppose I could be convinced that one or the other of those options is better than the single use helper function. What do you think?

Quoted 1 line · On Wed, 05 Mar 2008 11:06:34 -0500 Lee Schermerhorn <[email protected]>…

Subject: [PATCH] 2.6.25-rc3-mm1 - Mempolicy: make dequeue_huge_page_vma() obey MPOL_BIND nodemask

whine.

a) Please put the "2.6.25-rc3-mm1" stuff inside []. This tells the receiver that this is not-to-be-included metadata pertaining only to this email.

b) Sometimes (quite often) people send me patches advertised as being for "2.6.x-mm-y" whereas they actually fix problems which are in mainline (or even -stable).

So I tend to not believe it when people say that.

Quoted 13 lines

Date: Wed, 05 Mar 2008 11:06:34 -0500
Organization: HP/OSLO
X-Mailer: Evolution 2.6.1

PATCH Mempolicy - make dequeue_huge_page_vma() obey MPOL_BIND nodemask

dequeue_huge_page_vma() is not obeying the MPOL_BIND nodemask with the zonelist rework. It needs to search only zones in the mempolicy nodemask for hugepages.

Use for_each_zone_zonelist_nodemask() instead of
for_each_zone_zonelist().

Quoted 7 lines

Note: this will bloat mm/hugetlb.o a bit until Mel reworks the inlining of the for_each_zone... macros and helpers.

Added mempolicy helper function mpol_bind_nodemask() to hide the details of mempolicy from hugetlb and to avoid #ifdef CONFIG_NUMA in dequeue_huge_page_vma().

But this patch does indeed fix an only-in-mm problem. afacit it is a bugfix against mm-filter-based-on-a-nodemask-as-well-as-a-gfp_mask.patch. At least, that's how I've staged it.

If you have already worked that information out, please let me know.

Quoted 2 lines

Signed-off-by Lee Schermerhorn <[email protected]>

You're "Lee", not " Lee" ;)

Quoted 40 lines · Lee Schermerhorn

Quoted 23 lines · Nishanth Aravamudan

Quoted 22 lines · Lee Schermerhorn

PATCH Mempolicy - make dequeue_huge_page_vma() obey MPOL_BIND nodemask

dequeue_huge_page_vma() is not obeying the MPOL_BIND nodemask with the zonelist rework. It needs to search only zones in the mempolicy nodemask for hugepages.

Use for_each_zone_zonelist_nodemask() instead of
for_each_zone_zonelist().

Note: this will bloat mm/hugetlb.o a bit until Mel reworks the inlining of the for_each_zone... macros and helpers.

Added mempolicy helper function mpol_bind_nodemask() to hide the details of mempolicy from hugetlb and to avoid #ifdef CONFIG_NUMA in dequeue_huge_page_vma().

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/mempolicy.h |   13 +++++++++++++
 mm/hugetlb.c              |    4 +++-
 2 files changed, 16 insertions(+), 1 deletion(-)

Quoted 13 lines

mm/hugetlb.c static struct page *dequeue_huge_page_vm · line 102

Quoted 9 lines

Index: linux-2.6.25-rc3-mm1/mm/hugetlb.c
===================================================================

mm/hugetlb.c +1 -0

@@ -99,8 +99,10 @@ static struct page *dequeue_huge_page_vm
 					htlb_alloc_mask, &mpol);
 	struct zone *zone;
 	struct zoneref *z;
+	nodemask_t *nodemask = mpol_bind_nodemask(mpol);

We get this mpol from huge_zonelist(). Would it perhaps make sense to pass the nodemask as a parameter, too, to huge_zonelist(), rather than adding mpol_bind_nodemask()? This is the only user of it in-tree.

Nish:

Quoted 4 lines

I thought of that. I didn't go that way because I'd either need to pass a [pointer to a pointer to] a nodemask in addition to the [pointer to a pointer to] the mpol, so that I can release the reference on the mpol after the allocation is finished;

See I looked at that and thought: "We're already passing a pointer to a pointer to mpol, so a pointer to a pointer to a nodemask shouldn't be that big of deal.

This is the one call-site, as well. The idea being, we've pushed as much of the zonelist/nodemask knowledge into huge_zonelist(), keeping hugetlb.c relatively clear of it. Maybe it doesn't matter, was really just a question.

Not sure what other folks
think.

Quoted 4 lines

or I'd need to copy the nodemask [which can get pretty big] in the allocation path. I wanted to avoid both of those. I suppose I could be convinced that one or the other of those options is better than the single use helper function. What do you think?

What you have is fine, I guess -- and has been picked up by Andrew.

Quoted 29 lines · On Wed, 27 Feb 2008 16:47:34 -0500 Lee Schermerhorn <[email protected]>…

(quoted diff) +29 -0

+/* Returns the first zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **first_zones_zonelist(struct zonelist *zonelist,
+					enum zone_type highest_zoneidx)
+{
+	struct zone **z;
+
+	/* Find the first suitable zone to use for the allocation */
+	z = zonelist->zones;
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/* Returns the next zone at or below highest_zoneidx in a zonelist */
+static inline struct zone **next_zones_zonelist(struct zone **z,
+					enum zone_type highest_zoneidx)
+{
+	/* Find the next suitable zone to use for the allocation */
+	while (*z && zone_idx(*z) > highest_zoneidx)
+		z++;
+
+	return z;
+}
+
+/**
+ * for_each_zone_zonelist - helper macro to iterate over valid zones in a zonelist at or below a given zone index
+ * @zone - The current zone in the iterator
+ * @z - The current pointer within zonelist->zones being iterated

Review

Quoted 10 lines

(quoted diff) +10 -0

+ * @zlist - The zonelist being iterated
+ * @highidx - The zone index of the highest zone to return
+ *
+ * This iterator iterates though all zones at or below a given zone index.
+ */
+#define for_each_zone_zonelist(zone, z, zlist, highidx) \
+	for (z = first_zones_zonelist(zlist, highidx), zone = *z++;	\
+		zone;							\
+		z = next_zones_zonelist(z, highidx), zone = *z++)
+

omygawd will that thing generate a lot of code!

It has four call sites in mm/oom_kill.c and the overall patchset increases mm/oom_kill.o's text section (x86_64 allmodconfig) from 3268 bytes to 3845.

vmscan.o and page_alloc.o also grew a lot. otoh total vmlinux bloat from the patchset is only around 700 bytes, so I expect that with a little less insanity we could actually get an aggregate improvement here.

Some of the inlining in mmzone.h is just comical. Some of it is obvious (first_zones_zonelist) and some of it is less obvious (pfn_present).

I applied these for testing but I really don't think we should be merging such easily-fixed regressions into mainline. Could someone please take a look at de-porking core MM?

Quoted 15 lines · Mel Gorman

[PATCH 6/6] Filter based on a nodemask as well as a gfp_mask

V11r3 against 2.6.25-rc2-mm1

The MPOL_BIND policy creates a zonelist that is used for allocations controlled by that mempolicy. As the per-node zonelist is already being filtered based on a zone id, this patch adds a version of __alloc_pages() that takes a nodemask for further filtering. This eliminates the need for MPOL_BIND to create a custom zonelist.

A positive benefit of this is that allocations using MPOL_BIND now use the local node's distance-ordered zonelist instead of a custom node-id-ordered zonelist. I.e., pages will be allocated from the closest allowed node with available memory.

Quoted 4 lines

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

Acked-by Christoph Lameter <[email protected]>

Tested-by Lee Schermerhorn <[email protected]>

Thank you! I like this very much.
Next step is maybe to pass nodemask to try_to_free_pages().

BTW, cpuset memory limitation by nodemask has the same kind of feature. But it seems cpuset_zone_allowed_soft/hardwall() has extra checks for system sanity.

Could you import them ? maybe like this
==
void __alloc_pages_internal(gfp_t gfp_mask, unsigned int order,

(quoted diff) +1 -0

+			struct zonelist *zonelist, nodemask_t *nodemask))

{
	if (nodemask) {
		if (unlikely(test_thread_flag(TIF_MEMDIE)))
	                nodemask = NULL;
		if ((gfp_mask & __GFP_HARDWALL)
                     && (unlikely(test_thread_flag(TIF_MEMDIE)))
			nodemask = NULL;
	}
}

==
(I don't think above is clean.)

Fix for earlier patch:
"mempolicy-make-dequeue_huge_page_vma-obey-bind-policy"

Against: 2.6.25-rc3-mm1 atop the above patch.

As suggested by Nish Aravamudan, remove the mpol_bind_nodemask() helper and return a pointer to the policy node mask from huge_zonelist for MPOL_BIND. This hides more of the mempolicy quirks from hugetlb.

In making this change, I noticed that the huge_zonelist() stub for !NUMA wasn't nulling out the mpol. Added that as well.

Quoted 6 lines · Lee Schermerhorn

Fix for earlier patch:
"mempolicy-make-dequeue_huge_page_vma-obey-bind-policy"

Against: 2.6.25-rc3-mm1 atop the above patch.

Quoted 7 lines

As suggested by Nish Aravamudan, remove the mpol_bind_nodemask() helper and return a pointer to the policy node mask from huge_zonelist for MPOL_BIND. This hides more of the mempolicy quirks from hugetlb.

In making this change, I noticed that the huge_zonelist() stub for !NUMA wasn't nulling out the mpol. Added that as well.

Hrm, I was thinking more of the following (on top of this patch):

mm/mempolicy.c +5 -5

index 4c5d41d..3790f5a 100644
@@ -1298,9 +1298,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	*mpol = NULL;		/* probably no unref needed */
 	*nodemask = NULL;	/* assume !MPOL_BIND */
-	if (pol->policy == MPOL_BIND) {
-			*nodemask = &pol->v.nodes;
-	} else if (pol->policy == MPOL_INTERLEAVE) {
+	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);

@@ -1310,10 +1308,12 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	zl = zonelist_policy(GFP_HIGHUSER, pol);
 	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
-		if (pol->policy != MPOL_BIND)
+		if (pol->policy != MPOL_BIND) {
 			__mpol_free(pol);	/* finished with pol */
-		else
+		} else {
 			*mpol = pol;	/* unref needed after allocation */
+			*nodemask = &pol->v.nodes;
+		}
 	}
 	return zl;
 }

but perhaps that won't do the right thing if pol == current->mempolicy and pol->policy == MPOL_BIND.

So something like:

mm/mempolicy.c +6 -7

index 4c5d41d..7eb77e0 100644
@@ -1298,9 +1298,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	*mpol = NULL;		/* probably no unref needed */
 	*nodemask = NULL;	/* assume !MPOL_BIND */
-	if (pol->policy == MPOL_BIND) {
-			*nodemask = &pol->v.nodes;
-	} else if (pol->policy == MPOL_INTERLEAVE) {
+	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);

@@ -1309,11 +1307,12 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 	}
 
 	zl = zonelist_policy(GFP_HIGHUSER, pol);
-	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
-		if (pol->policy != MPOL_BIND)
-			__mpol_free(pol);	/* finished with pol */
-		else
+	if (unlikely(pol != &default_policy && pol != current->mempolicy
+						&& pol->policy != MPOL_BIND))
+		__mpol_free(pol);	/* finished with pol */
+	if (pol->policy == MPOL_BIND) {
 			*mpol = pol;	/* unref needed after allocation */
+			*nodemask = &pol->v.nodes;
 	}
 	return zl;
 }

Still not quite as clean, but I think it's best to keep the *mpol and *nodemask assignments together, as if *mpol is being assigned, that's the only time we should need to set *nodemask, right?

Quoted 48 lines · Nishanth Aravamudan

Quoted 6 lines · Lee Schermerhorn

Fix for earlier patch:
"mempolicy-make-dequeue_huge_page_vma-obey-bind-policy"

Against: 2.6.25-rc3-mm1 atop the above patch.

Quoted 7 lines

As suggested by Nish Aravamudan, remove the mpol_bind_nodemask() helper and return a pointer to the policy node mask from huge_zonelist for MPOL_BIND. This hides more of the mempolicy quirks from hugetlb.

In making this change, I noticed that the huge_zonelist() stub for !NUMA wasn't nulling out the mpol. Added that as well.

Hrm, I was thinking more of the following (on top of this patch):

mm/mempolicy.c +5 -5

index 4c5d41d..3790f5a 100644
@@ -1298,9 +1298,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	*mpol = NULL;		/* probably no unref needed */
 	*nodemask = NULL;	/* assume !MPOL_BIND */
-	if (pol->policy == MPOL_BIND) {
-			*nodemask = &pol->v.nodes;
-	} else if (pol->policy == MPOL_INTERLEAVE) {
+	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);

@@ -1310,10 +1308,12 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	zl = zonelist_policy(GFP_HIGHUSER, pol);
 	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
-		if (pol->policy != MPOL_BIND)
+		if (pol->policy != MPOL_BIND) {
 			__mpol_free(pol);	/* finished with pol */
-		else
+		} else {
 			*mpol = pol;	/* unref needed after allocation */
+			*nodemask = &pol->v.nodes;
+		}
 	}
 	return zl;
 }

Quoted 2 lines

but perhaps that won't do the right thing if pol == current->mempolicy and pol->policy == MPOL_BIND.

Right, you won't return the nodemask for current task policy == MBIND.

Quoted 36 lines

So something like:

mm/mempolicy.c +6 -7

index 4c5d41d..7eb77e0 100644
@@ -1298,9 +1298,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	*mpol = NULL;		/* probably no unref needed */
 	*nodemask = NULL;	/* assume !MPOL_BIND */
-	if (pol->policy == MPOL_BIND) {
-			*nodemask = &pol->v.nodes;
-	} else if (pol->policy == MPOL_INTERLEAVE) {
+	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);

@@ -1309,11 +1307,12 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 	}
 
 	zl = zonelist_policy(GFP_HIGHUSER, pol);
-	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
-		if (pol->policy != MPOL_BIND)
-			__mpol_free(pol);	/* finished with pol */
-		else
+	if (unlikely(pol != &default_policy && pol != current->mempolicy
+						&& pol->policy != MPOL_BIND))
+		__mpol_free(pol);	/* finished with pol */
+	if (pol->policy == MPOL_BIND) {
 			*mpol = pol;	/* unref needed after allocation */
+			*nodemask = &pol->v.nodes;
 	}
 	return zl;
 }

Quoted 3 lines

Still not quite as clean, but I think it's best to keep the *mpol and *nodemask assignments together, as if *mpol is being assigned, that's the only time we should need to set *nodemask, right?

Well, as you've noted, we do have to test MPOL_BIND twice: once to return the nodemask for any 'BIND policy and once to return a non-NULL mpol ONLY if it's MPOL_BIND and we need an unref. However, I wanted to avoid checking the policies twice as well, or storing *nodemask 3rd time.

I think that your second change above is not quite right, either. You're unconditionally returning the policy when the 'mode' == MBIND, even if it does not need a deref. This could result in prematurely freeing the task policy, causing a "use after free" error on next allocation; or even decrementing the reference on the system_default policy, which is probably benign, but not "nice". [Also, check your parentheses...]

Anyway you slice it, it's pretty ugly.

So, for now, I'd like to keep it the way I have it. I'll be sending out a set of patches to rework the reference counting after mempolicy settles down--i.e., Mel's and David's patches, which I'm testing now. That will clean this area up quite a bit, IMO.