Add populated_map to account for memoryless nodes

mm/page_alloc.c +1 -1

@@ -2161,7 +2164,7 @@ static int node_order[MAX_NUMNODES];
 static void build_zonelists_in_zone_order(pg_data_t *pgdat, int nr_nodes)
 {
 	enum zone_type i;
-	int pos, j, node;
+	int pos, j;
 	int zone_type;		/* needs to be signed */
 	struct zone *z;
 	struct zonelist *zonelist;

mm/page_alloc.c +5 -0

@@ -2244,6 +2247,22 @@ static void set_zonelist_order(void)
 		current_zonelist_order = user_zonelist_order;
 }
 
+/*
+ * setup_populate_map() - record nodes whose "policy_zone" is "on-node".
+ */
+static void setup_populated_map(int nid)
+{

mm/page_alloc.c +1 -0

+
 static void build_zonelists(pg_data_t *pgdat)
 {
 	int j, node, load;

@@ -2327,6 +2346,15 @@ static void set_zonelist_order(void)
 	current_zonelist_order = ZONELIST_ORDER_ZONE;

Quoted 12 lines · Nishanth Aravamudan

(quoted diff) +1 -1

@@ -2161,7 +2164,7 @@ static int node_order[MAX_NUMNODES];
 static void build_zonelists_in_zone_order(pg_data_t *pgdat, int nr_nodes)
 {
 	enum zone_type i;
-	int pos, j, node;
+	int pos, j;
 	int zone_type;		/* needs to be signed */
 	struct zone *z;
 	struct zonelist *zonelist;

@@ -2171,7 +2174,7 @@ static void build_zonelists_in_zone_order(pg_data_t *pgdat, int nr_nodes)
 		pos = 0;
 		for (zone_type = i; zone_type >= 0; zone_type--) {

Review static void build_zonelists_in_zone_order(pg_data_t *pgdat, int nr_nodes) · line 2180

Quoted 6 lines

(quoted diff) +1 -1

 			for (j = 0; j < nr_nodes; j++) {
-				node = node_order[j];
+				int node = node_order[j];
 				z = &NODE_DATA(node)->node_zones[zone_type];
 				if (populated_zone(z)) {
 					zonelist->zones[pos++] = z;

Unrelated modifications.

Quoted 9 lines

(quoted diff) +5 -0

@@ -2244,6 +2247,22 @@ static void set_zonelist_order(void)
 		current_zonelist_order = user_zonelist_order;
 }
 
+/*
+ * setup_populate_map() - record nodes whose "policy_zone" is "on-node".
+ */
+static void setup_populated_map(int nid)
+{

Review static void set_zonelist_order(void) · line 2264

Quoted 10 lines

(quoted diff) +10 -0

+	pg_data_t *pgdat = NODE_DATA(nid);
+	struct zonelist *zl = pgdat->node_zonelists + policy_zone;
+	struct zone *z = zl->zones[0];
+
+	VM_BUG_ON(!z);
+	if (z->zone_pgdat == pgdat)
+		node_set_populated(nid);
+	else
+		node_not_populated(nid);
+}

A node is only populated if it has memory in the policy zone? I would say a node is populated if it has any memory in any zone.

The above check may fail on x86_64 where only some nodes may have ZONE_NORMAL. Others only have ZONE_DMA32. Policy zone will be set to ZONE_NORMAL.

Quoted 7 lines

(quoted diff) +0 -0

 static void build_zonelists(pg_data_t *pgdat)
 {
 	int j, node, load;

@@ -2327,6 +2346,15 @@ static void set_zonelist_order(void)
 	current_zonelist_order = ZONELIST_ORDER_ZONE;
 }
 

Review static void set_zonelist_order(void) · line 2356

Quoted 8 lines

(quoted diff) +8 -0

+/*
+ * setup_populated_map - non-NUMA case
+ * Only node 0 should be on-line, and it MUST be populated!
+ */
+static void setup_populated_map(int nid)
+{
+	node_set_populated(nid);
+}

I'd say provide fallback functions so that node_populated() always returns true for !NUMA. That way it can be optimized out at compile time.

Review static int __build_all_zonelists(void *dummy) · line 2413

Quoted 9 lines

(quoted diff) +1 -0

 static void build_zonelists(pg_data_t *pgdat)
 {
 	int node, local_node;

@@ -2381,6 +2409,7 @@ static int __build_all_zonelists(void *dummy)
 	for_each_online_node(nid) {
 		build_zonelists(NODE_DATA(nid));
 		build_zonelist_cache(NODE_DATA(nid));
+		setup_populated_map(nid);
 	}

Is it possible to move the set_populated_node into build_zonelists somehow?

F.e. In build_zonelists_node you can check if nr_zones > 0 and then set it up?

Already done in the original patch (node_populated() returns (node == 0) if MAX_NUMODES <= 1), I think.

mm/page_alloc.c +5 -0

@@ -2299,6 +2303,18 @@ static void build_zonelists(pg_data_t *pgdat)
 		/* calculate node order -- i.e., DMA last! */
 		build_zonelists_in_zone_order(pgdat, j);
 	}
+
+	/*
+	 * record nodes whose first fallback zone is "on-node" as
+	 * populated
+	 */

Quoted 2 lines · Nishanth Aravamudan

Already done in the original patch (node_populated() returns (node == 0) if MAX_NUMODES <= 1), I think.

Ah good.

Quoted 18 lines

(quoted diff) +12 -0

@@ -2299,6 +2303,18 @@ static void build_zonelists(pg_data_t *pgdat)
 		/* calculate node order -- i.e., DMA last! */
 		build_zonelists_in_zone_order(pgdat, j);
 	}
+
+	/*
+	 * record nodes whose first fallback zone is "on-node" as
+	 * populated
+	 */
+	z = pgdat->node_zonelists->zones[0];
+
+	VM_BUG_ON(!z);
+	if (z->zone_pgdat == pgdat)
+		node_set_populated(local_node);
+	else
+		node_not_populated(local_node);
 }
 

Review static void build_zonelists(pg_data_t *pgdat) · line 2320

Quoted 2 lines

(quoted diff) +0 -0

 /* Construct the zonelist performance cache - see further mmzone.h */

Could be much simpler:

if (pgdat->node_present_pages)

	node_set_populated(local_node);

drivers/base/node.c static ssize_t node_read_distance(struct sys_device * dev, char * buf) · line 138

Quoted 8 lines · Nishanth Aravamudan

}
static SYSDEV_ATTR(distance, S_IRUGO, node_read_distance, NULL);

drivers/base/node.c +5 -0

+#ifdef CONFIG_HUGETLB_PAGE
+static SYSDEV_ATTR(nr_hugepages, S_IRUGO | S_IWUSR,
+				hugetlb_read_nr_hugepages_node,
+				hugetlb_write_nr_hugepages_node);
+#endif

Move the above to hugetlb.c?

Also so far there is nothing in the nodes directories that can be modified. This is the first one. Is that really the right location?

Also, I don't really see where else a per-node attribute should go other than /sys/devices/system/node/nodeX... Did you have a better location in mind?

Quoted 3 lines · Nishanth Aravamudan

}
static SYSDEV_ATTR(distance, S_IRUGO, node_read_distance, NULL);

drivers/base/node.c static ssize_t node_read_distance(struct sys_device * dev, char * buf) · line 138

Quoted 5 lines

drivers/base/node.c +5 -0

+#ifdef CONFIG_HUGETLB_PAGE
+static SYSDEV_ATTR(nr_hugepages, S_IRUGO | S_IWUSR,
+				hugetlb_read_nr_hugepages_node,
+				hugetlb_write_nr_hugepages_node);
+#endif

Move the above to hugetlb.c?

Ok, if I do that, would you prefer I just add callbacks into hugetlb code for register_node() and unregister_node() that are no-ops if !CONFIG_HUGETLB_PAGE? That is, rather than

	sysdev_remove_file(&node->sysdev, &attr_nr_hugepages);

just call something like

hugetlb_unregister_node()

? And similar for register? Otherwise, there are still going to be ifdefs for the remove and add calls.

Quoted 6 lines · Nishanth Aravamudan

Ok, if I do that, would you prefer I just add callbacks into hugetlb code for register_node() and unregister_node() that are no-ops if !CONFIG_HUGETLB_PAGE? That is, rather than

	sysdev_remove_file(&node->sysdev, &attr_nr_hugepages);

Quoted 6 lines

just call something like

hugetlb_unregister_node()

? And similar for register? Otherwise, there are still going to be ifdefs for the remove and add calls.

Sounds good. Lets see the patch.

Add node-parameterized helpers for dequeue_huge_page, alloc_fresh_huge_page and try_to_free_low. Also have update_and_free_page() take a nid parameter. This is necessary to add a per-node sysfs attribute to specify the number of hugepages on that node.

Review

Quoted 2 lines · Nishanth Aravamudan

(quoted diff) +2 -0

+	if (nid < 0)
+		nid = first_node(node_populated_map);

nid == 1 means local node? Or why do we check for nid < 0?

	if (nid == 1)
		 nid = numa_node_id();

?

mm/hugetlb.c static void free_huge_page(struct page *page) · line 123

Quoted 8 lines

mm/hugetlb.c +8 -0

+	do {
+		page = alloc_pages_node(nid,
+				GFP_HIGHUSER|__GFP_COMP|GFP_THISNODE,
+				HUGETLB_PAGE_ORDER);
+		nid = next_node(nid, node_populated_map);
+		if (nid >= nr_node_ids)
+			nid = first_node(node_populated_map);
+	} while (!page && nid != start_nid);

Looks good.

Quoted 10 lines · Christoph Lameter

Review

Quoted 2 lines · Nishanth Aravamudan

(quoted diff) +2 -0

+	if (nid < 0)
+		nid = first_node(node_populated_map);

nid == 1 means local node? Or why do we check for nid < 0?

	if (nid == 1)
		 nid = numa_node_id();

Quoted 1 line

?

No, nid is a static variable. So we initialize it to -1 to catch the first time we go through the loop.

IIRC, we can't just set it to first_node(node_populated_map), because it's a non-constant or something?

Quoted 13 lines · Nishanth Aravamudan

Quoted 11 lines · Christoph Lameter

Review

Quoted 2 lines · Nishanth Aravamudan

(quoted diff) +2 -0

+	if (nid < 0)
+		nid = first_node(node_populated_map);

nid == 1 means local node? Or why do we check for nid < 0?

	if (nid == 1)
		 nid = numa_node_id();

?

Quoted 5 lines

No, nid is a static variable. So we initialize it to -1 to catch the first time we go through the loop.

IIRC, we can't just set it to first_node(node_populated_map), because it's a non-constant or something?

Sure, you can initialize a c variable from another. So drop the -1.

static int nid = first_node(node_populated_map), I get:

mm/hugetlb.c:108: error: initializer element is not constant

Quoted 3 lines · Nishanth Aravamudan

static int nid = first_node(node_populated_map), I get:

mm/hugetlb.c:108: error: initializer element is not constant

Remove the static.

Quoted 6 lines · Christoph Lameter

Quoted 3 lines · Nishanth Aravamudan

static int nid = first_node(node_populated_map), I get:

mm/hugetlb.c:108: error: initializer element is not constant

Quoted 1 line

Remove the static.

Cutting down the CCs.

Removing static wont help if the variable is still global. You need to define a local variable. Then it can be initialized with a variable expression.

nid is static to alloc_fresh_huge_page().

Quoted 1 line · Nishanth Aravamudan

nid is static to alloc_fresh_huge_page().

Ahh did not see that. Can you not call simply into interleave() from mempolicy.c? It will get you the counter that you need.

Quoted 2 lines

Ahh did not see that. Can you not call simply into interleave() from mempolicy.c? It will get you the counter that you need.

You just told me that mempolicy.c is built conditionally on NUMA. alloc_fresh_huge_page() is not, it only depeonds on CONFIG_HUGETLB_PAGE!

The only interleave functions I see in mempolicy.c are:

interleave_nodes(), which takes a mempolicy, which I don't have in hugetlb.c

interleave_nid(), which also takes a mempolicy

I guess I could try and use huge_zonelist(), but I don't see the point?

Quoted 5 lines · Nishanth Aravamudan

Quoted 2 lines

Ahh did not see that. Can you not call simply into interleave() from mempolicy.c? It will get you the counter that you need.

You just told me that mempolicy.c is built conditionally on NUMA. alloc_fresh_huge_page() is not, it only depeonds on CONFIG_HUGETLB_PAGE!

Well you just need to have the appropriate fallbacks defined in mempolicy.h

Quoted 8 lines

The only interleave functions I see in mempolicy.c are:

interleave_nodes(), which takes a mempolicy, which I don't have in hugetlb.c

interleave_nid(), which also takes a mempolicy

I guess I could try and use huge_zonelist(), but I don't see the point?

Export a function for the interleave functionality so that we do not have to replicate the same thing in various locations in the kernel.

Quoted 2 lines

Export a function for the interleave functionality so that we do not have to replicate the same thing in various locations in the kernel.

But I don't understand this at all.

This is *not* generically available, unless every caller has its own private static variable. I don't know how to do that in C.

You're asking me to complicate patches that work just fine right now.

Quoted 3 lines · Nishanth Aravamudan

Quoted 2 lines

Export a function for the interleave functionality so that we do not have to replicate the same thing in various locations in the kernel.

Quoted 4 lines

But I don't understand this at all.

This is *not* generically available, unless every caller has its own private static variable. I don't know how to do that in C.

It is already there. Each task has a il_next field in its task struct for that purpose.

Quoted 1 line

You're asking me to complicate patches that work just fine right now.

I am trying to simplify your work.

Hrm, maybe that will work -- but then it means that if one is interleaving huge pages, it will interfere with the interleaving of small pages. Given that right now, huge pages are a rather precious commodity, do we want this?

Quoted 3 lines · Nishanth Aravamudan

Quoted 2 lines

Export a function for the interleave functionality so that we do not have to replicate the same thing in various locations in the kernel.

Quoted 4 lines

But I don't understand this at all.

This is *not* generically available, unless every caller has its own private static variable. I don't know how to do that in C.

It is already there. Each task has a il_next field in its task struct for that purpose.

Ok, I see that. And it represent the next node to use for an interleaved allocation. Makes sense to me, and I see how it's used in mempolicy.c to achieve that. But we're running at system boot time, or whenever some

invokes the sysctl /proc/sys/vm/nr_hugepages. Do we really want to muck with some arbitray bash shell's il_next field to achieve interleaving? What if it's a C process that is trying to achieve actual interleaving for other purposes and also allocates some hugepages on the system? It seems like il_next is very much a process-related field.

Quoted 3 lines · Nishanth Aravamudan

static int nid = first_node(node_populated_map), I get:

mm/hugetlb.c:108: error: initializer element is not constant

Remove the static.

Quoted 4 lines

nid == 1 means local node? Or why do we check for nid < 0?

	if (nid == 1)
		 nid = numa_node_id();

?

No, nid is a static variable. So we initialize it to -1 to catch the first time we go through the loop.
IIRC, we can't just set it to first_node(node_populated_map), because it's a non-constant or something?

I wrote that, so I figure I should chime in. The static variable can be killed off outright.

Initially filling the pool doesn't need the static affair. Refilling the pool from the page allocator can refill the node with the least memory first, and choose randomly otherwise. Using default mpolicies or defaulting to node-local memory instead of round-robin allocation will likely do for callers into the allocator.

Quoted 5 lines

Initially filling the pool doesn't need the static affair. Refilling the pool from the page allocator can refill the node with the least memory first, and choose randomly otherwise. Using default mpolicies or defaulting to node-local memory instead of round-robin allocation will likely do for callers into the allocator.

Each task already has a next node field. Just use that.

Quoted 1 line · Christoph Lameter

Each task already has a next node field. Just use that.

That's new. It sounds convenient.

Quoted 1 line

Quoted 1 line

Each task already has a next node field. Just use that.

Quoted 1 line

That's new. It sounds convenient.

No its ancient.

Quoted 4 lines · Christoph Lameter

Quoted 4 lines

nid == 1 means local node? Or why do we check for nid < 0?

	if (nid == 1)
		 nid = numa_node_id();

?

Quoted 4 lines · Nishanth Aravamudan

No, nid is a static variable. So we initialize it to -1 to catch the first time we go through the loop.
IIRC, we can't just set it to first_node(node_populated_map), because it's a non-constant or something?

I wrote that, so I figure I should chime in. The static variable can be killed off outright.

Initially filling the pool doesn't need the static affair. Refilling the pool from the page allocator can refill the node with the least memory first, and choose randomly otherwise. Using default mpolicies or defaulting to node-local memory instead of round-robin allocation will likely do for callers into the allocator.

It depends a bit on what SGI's app that originally wanted striping of hugetlb does.

Also, if one has such a large number of nodes that exhaustive search for the node with the least memory would be prohibitive, esp. when in a loop, it's always possible to keep node ID's in an array heap-ordered by the number of pages in the node's segment of the pool. In such a manner the inner loop's search is limited to O(lg(nr_online_nodes())).

Well, (presuming I understood everything you wrote :), don't we need the static 'affair' to guarantee the initial allocations are approximately round-robin? Or, if we aren't going to make that guarantee, than we should only change that once my sysfs allocator (or its equivalent) is available?
Just trying to get a handle on what you're suggesting without any historical context.

Quoted 7 lines · Nishanth Aravamudan

Well, (presuming I understood everything you wrote :), don't we need the static 'affair' to guarantee the initial allocations are approximately round-robin? Or, if we aren't going to make that guarantee, than we should only change that once my sysfs allocator (or its equivalent) is available?
Just trying to get a handle on what you're suggesting without any historical context.

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Quoted 2 lines

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Ok, I'll play with that a bit.

Quoted 7 lines · Nishanth Aravamudan

Well, (presuming I understood everything you wrote :), don't we need the static 'affair' to guarantee the initial allocations are approximately round-robin? Or, if we aren't going to make that guarantee, than we should only change that once my sysfs allocator (or its equivalent) is available?
Just trying to get a handle on what you're suggesting without any historical context.

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Quoted 2 lines

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

But how does one differentiate between "initally filling" the pool and a later attempt to add to the pool (or even just marginally later). I guess I don't see why folks are so against this static variable :) It does the job and removing it seems like it could be an independent cleanup?

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Quoted 5 lines · Nishanth Aravamudan

But how does one differentiate between "initally filling" the pool and a later attempt to add to the pool (or even just marginally later). I guess I don't see why folks are so against this static variable :) It does the job and removing it seems like it could be an independent cleanup?

Well, another approach is to just statically initialize it to something and then always check to make sure the node for the nid has memory, and if not, find the next nid with a node with memory from the populated map.

Quoted 11 lines · William Lee Irwin III

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Quoted 5 lines · Nishanth Aravamudan

But how does one differentiate between "initally filling" the pool and a later attempt to add to the pool (or even just marginally later). I guess I don't see why folks are so against this static variable :) It does the job and removing it seems like it could be an independent cleanup?

Quoted 3 lines

Well, another approach is to just statically initialize it to something and then always check to make sure the node for the nid has memory, and if not, find the next nid with a node with memory from the populated map.

How does something like this look? Or is it overkill?

Quoted 12 lines · Nishanth Aravamudan

Quoted 11 lines · William Lee Irwin III

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Quoted 5 lines · Nishanth Aravamudan

But how does one differentiate between "initally filling" the pool and a later attempt to add to the pool (or even just marginally later). I guess I don't see why folks are so against this static variable :) It does the job and removing it seems like it could be an independent cleanup?

Quoted 5 lines

Quoted 3 lines

Well, another approach is to just statically initialize it to something and then always check to make sure the node for the nid has memory, and if not, find the next nid with a node with memory from the populated map.

How does something like this look? Or is it overkill?

If that patch looks ok, then the other patches (numafy and sysfs) are relatively unchanged.

commit 041cb3d3c2fd3640aff50e2f701b8b5a670193de

Author: Nishanth Aravamudan <[email protected]>
Date:   Tue Jun 12 17:10:21 2007 -0700

hugetlb: numafy several functions

Add node-parameterized helpers for dequeue_huge_page, alloc_fresh_huge_page and try_to_free_low. Also have update_and_free_page() take a nid parameter. This is necessary to add a per-node sysfs attribute to specify the number of hugepages on that node.

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc William Lee Irwin III <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Andrew Morton <[email protected]>

mm/hugetlb.c +63 -34

index 1c13687..c4a966e 100644
@@ -66,11 +66,22 @@ static void enqueue_huge_page(struct page *page)
 	free_huge_pages_node[nid]++;
 }
 
+static struct page *dequeue_huge_page_node(int nid)
+{
+	struct page *page;
+
+	page = list_entry(hugepage_freelists[nid].next,
+					  struct page, lru);
+	list_del(&page->lru);
+	free_huge_pages--;
+	free_huge_pages_node[nid]--;
+	return page;
+}
+
 static struct page *dequeue_huge_page(struct vm_area_struct *vma,
 				unsigned long address)
 {
 	int nid;
-	struct page *page = NULL;
 	struct zonelist *zonelist = huge_zonelist(vma, address,
 						htlb_alloc_mask);
 	struct zone **z;

@@ -82,14 +93,9 @@ static struct page *dequeue_huge_page(struct vm_area_struct *vma,
 			break;
 	}
 
-	if (*z) {
-		page = list_entry(hugepage_freelists[nid].next,
-				  struct page, lru);
-		list_del(&page->lru);
-		free_huge_pages--;
-		free_huge_pages_node[nid]--;
-	}
-	return page;
+	if (*z)
+		return dequeue_huge_page_node(nid);
+	return NULL;
 }
 
 static void free_huge_page(struct page *page)

@@ -103,6 +109,25 @@ static void free_huge_page(struct page *page)
 	spin_unlock(&hugetlb_lock);
 }
 
+static struct page *alloc_fresh_huge_page_node(int nid)
+{
+	struct page *page;
+
+	page = alloc_pages_node(nid,
+			GFP_HIGHUSER|__GFP_COMP|GFP_THISNODE,
+			HUGETLB_PAGE_ORDER);
+	if (page) {
+		set_compound_page_dtor(page, free_huge_page);
+		spin_lock(&hugetlb_lock);
+		nr_huge_pages++;
+		nr_huge_pages_node[nid]++;
+		spin_unlock(&hugetlb_lock);
+		put_page(page); /* free it into the hugepage allocator */
+	}
+
+	return page;
+}
+
 static int alloc_fresh_huge_page(struct mempolicy *policy)
 {
 	int nid;

@@ -112,20 +137,12 @@ static int alloc_fresh_huge_page(struct mempolicy *policy)
 	nid = start_nid;
 
 	do {
-		page = alloc_pages_node(nid,
-				htlb_alloc_mask|__GFP_COMP|GFP_THISNODE,
-				HUGETLB_PAGE_ORDER);
+		page = alloc_fresh_huge_page_node(nid);
 		nid = interleave_nodes(policy);
 	} while (!page && nid != start_nid);
-	if (page) {
-		set_compound_page_dtor(page, free_huge_page);
-		spin_lock(&hugetlb_lock);
-		nr_huge_pages++;
-		nr_huge_pages_node[page_to_nid(page)]++;
-		spin_unlock(&hugetlb_lock);
-		put_page(page); /* free it into the hugepage allocator */
+
+	if (page)
 		return 1;
-	}
 	return 0;
 }
 

@@ -201,11 +218,11 @@ static unsigned int cpuset_mems_nr(unsigned int *array)
 }
 
 #ifdef CONFIG_SYSCTL
-static void update_and_free_page(struct page *page)
+static void update_and_free_page(int nid, struct page *page)
 {
 	int i;
 	nr_huge_pages--;
-	nr_huge_pages_node[page_to_nid(page)]--;
+	nr_huge_pages_node[nid]--;
 	for (i = 0; i < (HPAGE_SIZE / PAGE_SIZE); i++) {
 		page[i].flags &= ~(1 << PG_locked | 1 << PG_error | 1 << PG_referenced |
 				1 << PG_dirty | 1 << PG_active | 1 << PG_reserved |

@@ -217,25 +234,37 @@ static void update_and_free_page(struct page *page)
 }
 
 #ifdef CONFIG_HIGHMEM
+static void try_to_free_low_node(int nid, unsigned long count)
+{
+	struct page *page, *next;
+
+	list_for_each_entry_safe(page, next,
+				&hugepage_freelists[nid], lru) {
+		if (PageHighMem(page))
+			continue;
+		list_del(&page->lru);
+		update_and_free_page(nid, page);
+		free_huge_pages--;
+		free_huge_pages_node[nid]--;
+		if (count >= nr_huge_pages_node[nid])
+			return;
+	}
+}
+
 static void try_to_free_low(unsigned long count)
 {
 	int i;
 
 	for (i = 0; i < MAX_NUMNODES; ++i) {
-		struct page *page, *next;
-		list_for_each_entry_safe(page, next, &hugepage_freelists[i], lru) {
-			if (PageHighMem(page))
-				continue;
-			list_del(&page->lru);
-			update_and_free_page(page);
-			free_huge_pages--;
-			free_huge_pages_node[page_to_nid(page)]--;
-			if (count >= nr_huge_pages)
-				return;
-		}
+		try_to_free_low_node(i, count);
+		if (count >= nr_huge_pages)
+			break;
 	}
 }
 #else
+static inline void try_to_free_low_node(int nid, unsigned long count)
+{
+}
 static inline void try_to_free_low(unsigned long count)
 {
 }

@@ -263,7 +292,7 @@ static unsigned long set_max_huge_pages(unsigned long count)
 		struct page *page = dequeue_huge_page(NULL, 0);
 		if (!page)
 			break;
-		update_and_free_page(page);
+		update_and_free_page(page_to_nid(page), page);
 	}
 	spin_unlock(&hugetlb_lock);
 	return nr_huge_pages;

Signature · 3 lines

-- 
Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

commit 05a7edb8c909c674cdefb0323348825cf3e2d1d0

Author: Nishanth Aravamudan <[email protected]>
Date:   Thu Jun 7 08:54:48 2007 -0700

hugetlb: add per-node nr_hugepages sysfs attribute

Allow specifying the number of hugepages to allocate on a particular node. Our current global sysctl will try its best to put hugepages equally on each node, but htat may not always be desired. This allows the admin to control the layout of hugepage allocation at a finer level (while not breaking the existing interface). Add callbacks in the sysfs node registration and unregistration functions into hugetlb to add the nr_hugepages attribute, which is a no-op if !NUMA or !HUGETLB.

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc William Lee Irwin III <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Andrew Morton <[email protected]>

---
Do the dummy function definitions need to be (void)0?

include/linux/hugetlb.h +1 -0

index aa0dc9b..e9f5928 100644
@@ -5,6 +5,7 @@
 
 #include <linux/mempolicy.h>
 #include <linux/shm.h>
+#include <linux/sysdev.h>
 #include <asm/tlbflush.h>
 
 struct ctl_table;

On Wed, 2007-06-13 at 08:28 -0700, Nishanth Aravamudan wrote:
<snip>

Quoted 25 lines

commit 05a7edb8c909c674cdefb0323348825cf3e2d1d0

Author: Nishanth Aravamudan <[email protected]>
Date:   Thu Jun 7 08:54:48 2007 -0700

hugetlb: add per-node nr_hugepages sysfs attribute

Allow specifying the number of hugepages to allocate on a particular node. Our current global sysctl will try its best to put hugepages equally on each node, but htat may not always be desired. This allows the admin to control the layout of hugepage allocation at a finer level (while not breaking the existing interface). Add callbacks in the sysfs node registration and unregistration functions into hugetlb to add the nr_hugepages attribute, which is a no-op if !NUMA or !HUGETLB.

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc William Lee Irwin III <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Andrew Morton <[email protected]>

---
Do the dummy function definitions need to be (void)0?

<snip>

Quoted 11 lines

include/linux/hugetlb.h +1 -0

index aa0dc9b..e9f5928 100644
@@ -5,6 +5,7 @@
 
 #include <linux/mempolicy.h>
 #include <linux/shm.h>
+#include <linux/sysdev.h>
 #include <asm/tlbflush.h>
 

include/linux/hugetlb.h void __unmap_hugepage_range(struct vm_area_struct *, unsigned long, unsigned lon · line 28

Quoted 7 lines

include/linux/hugetlb.h +2 -0

 struct ctl_table;

@@ -23,6 +24,11 @@ void __unmap_hugepage_range(struct vm_area_struct *, unsigned long, unsigned lon
 int hugetlb_prefault(struct address_space *, struct vm_area_struct *);
 int hugetlb_report_meminfo(char *);
 int hugetlb_report_node_meminfo(int, char *);
+int hugetlb_register_node(struct sys_device *);
+void hugetlb_unregister_node(struct sys_device *);

The parameter type for the two functions above need to be "struct node". You'll need to include <linux/node.h> after <linux/sysdev.h>, as well. Otherwise, doesn't build.

Quoted 24 lines · Lee Schermerhorn

On Wed, 2007-06-13 at 08:28 -0700, Nishanth Aravamudan wrote:
<snip>

Quoted 22 lines

commit 05a7edb8c909c674cdefb0323348825cf3e2d1d0

Author: Nishanth Aravamudan <[email protected]>
Date:   Thu Jun 7 08:54:48 2007 -0700

hugetlb: add per-node nr_hugepages sysfs attribute

Allow specifying the number of hugepages to allocate on a particular node. Our current global sysctl will try its best to put hugepages equally on each node, but htat may not always be desired. This allows the admin to control the layout of hugepage allocation at a finer level (while not breaking the existing interface). Add callbacks in the sysfs node registration and unregistration functions into hugetlb to add the nr_hugepages attribute, which is a no-op if !NUMA or !HUGETLB.

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc William Lee Irwin III <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Andrew Morton <[email protected]>

Quoted 5 lines

Quoted 3 lines

---
Do the dummy function definitions need to be (void)0?

<snip>

Quoted 29 lines · Nishanth Aravamudan

Quoted 28 lines · Lee Schermerhorn

On Wed, 2007-06-13 at 08:28 -0700, Nishanth Aravamudan wrote:
<snip>

Quoted 25 lines

commit 05a7edb8c909c674cdefb0323348825cf3e2d1d0

Author: Nishanth Aravamudan <[email protected]>
Date:   Thu Jun 7 08:54:48 2007 -0700

hugetlb: add per-node nr_hugepages sysfs attribute

Allow specifying the number of hugepages to allocate on a particular node. Our current global sysctl will try its best to put hugepages equally on each node, but htat may not always be desired. This allows the admin to control the layout of hugepage allocation at a finer level (while not breaking the existing interface). Add callbacks in the sysfs node registration and unregistration functions into hugetlb to add the nr_hugepages attribute, which is a no-op if !NUMA or !HUGETLB.

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc William Lee Irwin III <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Andrew Morton <[email protected]>

---
Do the dummy function definitions need to be (void)0?

Quoted 1 line

Quoted 1 line

<snip>

I tested hugepage allocation on my HP rx8620 platform [16 cpu ia64, 32GB in 4 "real" nodes and one pseudo-node containing only DMA memory]. As expected, I don't get a balanced distribution across the real nodes. Here's what I see:

Quoted 30 lines · Lee Schermerhorn

Quoted 29 lines · Nishanth Aravamudan

Quoted 28 lines · Lee Schermerhorn

On Wed, 2007-06-13 at 08:28 -0700, Nishanth Aravamudan wrote:
<snip>

Quoted 25 lines

commit 05a7edb8c909c674cdefb0323348825cf3e2d1d0

Author: Nishanth Aravamudan <[email protected]>
Date:   Thu Jun 7 08:54:48 2007 -0700

hugetlb: add per-node nr_hugepages sysfs attribute

Allow specifying the number of hugepages to allocate on a particular node. Our current global sysctl will try its best to put hugepages equally on each node, but htat may not always be desired. This allows the admin to control the layout of hugepage allocation at a finer level (while not breaking the existing interface). Add callbacks in the sysfs node registration and unregistration functions into hugetlb to add the nr_hugepages attribute, which is a no-op if !NUMA or !HUGETLB.

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc William Lee Irwin III <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Andrew Morton <[email protected]>

---
Do the dummy function definitions need to be (void)0?

Quoted 6 lines

Quoted 1 line

Quoted 1 line

<snip>

I tested hugepage allocation on my HP rx8620 platform [16 cpu ia64, 32GB in 4 "real" nodes and one pseudo-node containing only DMA memory]. As expected, I don't get a balanced distribution across the real nodes. Here's what I see:

Hrm, not good.

Can you try without any of my add-on patches, but just the original set from Christoph?

I tried to "tighten up" alloc_pages_node() to check the location of the first zone in the selected zonelist, as discussed in previous exchange. When I do this, I hit a BUG() in slub.c in early_kmem_cache_node_alloc(), as it apparently can't handle new_slab() returning a NULL page, even tho' it calls it with GFP_THISNODE. Slub should be able to handle memoryless nodes, right? I'm looking for a work around to this now.

Quoted 7 lines · On Wed, 13 Jun 2007 16:05:10 -0400 Lee Schermerhorn <[email protected]>…

I tried to "tighten up" alloc_pages_node() to check the location of the first zone in the selected zonelist, as discussed in previous exchange. When I do this, I hit a BUG() in slub.c in early_kmem_cache_node_alloc(), as it apparently can't handle new_slab() returning a NULL page, even tho' it calls it with GFP_THISNODE. Slub should be able to handle memoryless nodes, right? I'm looking for a work around to this now.

The memoryless node patchset results in SLUB not attempting to allocate on memoryless nodes during bootstrap.

Quoted 11 lines · William Lee Irwin III

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Quoted 5 lines · Nishanth Aravamudan

But how does one differentiate between "initally filling" the pool and a later attempt to add to the pool (or even just marginally later). I guess I don't see why folks are so against this static variable :) It does the job and removing it seems like it could be an independent cleanup?

Quoted 3 lines

Well, another approach is to just statically initialize it to something and then always check to make sure the node for the nid has memory, and if not, find the next nid with a node with memory from the populated map.

How does something like this look? Or is it overkill?

[PATCH 2.6.22-rc4-mm2] Fix hugetlb pool allocation with empty nodes V7

Anton found a problem with the hugetlb pool allocation when some nodes have no memory (http://marc.info/?l=linux-mm&m=118133042025995&w=2). Lee worked on versions that tried to fix it, but none were accepted. Christoph has created a set of patches which allow for GFP_THISNODE allocations to fail if the node has no memory and for exporting a node_memory_map indicating which nodes have memory. Since mempolicy.c already has a number of functions which support interleaving, create a mempolicy when we invoke alloc_fresh_huge_page() that specifies interleaving across all the nodes in node_memory_map, rather than custom interleaving code in hugetlb.c. This requires adding some dummy functions, and some declarations, in mempolicy.h to compile with NUMA or !NUMA.

Signed-off-by Nishanth Aravamudan <[email protected]>

Cc Anton Blanchard <[email protected]>

Cc Lee Schermerhorn <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc William Lee Irwin III <[email protected]>

Cc Andrew Morton <[email protected]>

include/linux/mempolicy.h +14 -0

index 22b668c..c8a68b8 100644
@@ -76,6 +76,8 @@ struct mempolicy {
  * The default fast path of a NULL MPOL_DEFAULT policy is always inlined.
  */
 
+extern struct mempolicy *mpol_new(int mode, nodemask_t *nodes);
+
 extern void __mpol_free(struct mempolicy *pol);
 static inline void mpol_free(struct mempolicy *pol)
 {

@@ -164,6 +166,8 @@ static inline void check_highest_zone(enum zone_type k)
 		policy_zone = k;
 }
 
+extern unsigned interleave_nodes(struct mempolicy *policy);
+
 int do_migrate_pages(struct mm_struct *mm,
 	const nodemask_t *from_nodes, const nodemask_t *to_nodes, int flags);
 

@@ -179,6 +183,11 @@ static inline int mpol_equal(struct mempolicy *a, struct mempolicy *b)
 
 #define mpol_set_vma_default(vma) do {} while(0)
 
+static inline struct mempolicy *mpol_new(int mode, nodemask_t *nodes)
+{
+	return NULL;
+}
+
 static inline void mpol_free(struct mempolicy *p)
 {
 }

@@ -267,6 +276,11 @@ static inline int do_migrate_pages(struct mm_struct *mm,
 static inline void check_highest_zone(int k)
 {
 }
+
+static inline unsigned interleave_nodes(struct mempolicy *policy)
+{
+	return 0;
+}
 #endif /* CONFIG_NUMA */
 #endif /* __KERNEL__ */
 

mm/hugetlb.c +2 -6

index 858c0b3..1c13687 100644
@@ -103,15 +103,20 @@ static void free_huge_page(struct page *page)
 	spin_unlock(&hugetlb_lock);
 }
 
-static int alloc_fresh_huge_page(void)
+static int alloc_fresh_huge_page(struct mempolicy *policy)
 {
-	static int nid = 0;
+	int nid;
 	struct page *page;
-	page = alloc_pages_node(nid, htlb_alloc_mask|__GFP_COMP|__GFP_NOWARN,
-					HUGETLB_PAGE_ORDER);
-	nid = next_node(nid, node_online_map);
-	if (nid == MAX_NUMNODES)

Quoted 19 lines · Nishanth Aravamudan

Quoted 11 lines · William Lee Irwin III

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

For initially filling the pool one can just loop over nid's modulo the number of populated nodes and pass down a stack-allocated variable.

Quoted 5 lines · Nishanth Aravamudan

But how does one differentiate between "initally filling" the pool and a later attempt to add to the pool (or even just marginally later). I guess I don't see why folks are so against this static variable :) It does the job and removing it seems like it could be an independent cleanup?

Quoted 3 lines

Well, another approach is to just statically initialize it to something and then always check to make sure the node for the nid has memory, and if not, find the next nid with a node with memory from the populated map.

How does something like this look? Or is it overkill?

[PATCH 2.6.22-rc4-mm2] Fix hugetlb pool allocation with empty nodes V7

Review

Quoted 5 lines

(quoted diff) +5 -0

+	do {
+		page = alloc_pages_node(nid,
+				htlb_alloc_mask|__GFP_COMP|GFP_THISNODE,
+				HUGETLB_PAGE_ORDER);
+		nid = interleave_nodes(policy);

This needs to be:

		if (!page)
			nid = interleave_nodes(policy);

Otherwise, you skip every other populated node, because you call interleave_nodes() at the top when you initialize start_nid. You only want to advance to the next node here if the allocation failed.

Review

Quoted 2 lines · Nishanth Aravamudan

(quoted diff) +2 -0

+	if (nid < 0)
+		nid = first_node(node_populated_map);

nid == 1 means local node? Or why do we check for nid < 0?

	if (nid == 1)
		 nid = numa_node_id();

?

mm/hugetlb.c static void free_huge_page(struct page *page) · line 123

Quoted 8 lines

mm/hugetlb.c +8 -0

+	do {
+		page = alloc_pages_node(nid,
+				GFP_HIGHUSER|__GFP_COMP|GFP_THISNODE,
+				HUGETLB_PAGE_ORDER);
+		nid = next_node(nid, node_populated_map);
+		if (nid >= nr_node_ids)
+			nid = first_node(node_populated_map);
+	} while (!page && nid != start_nid);

Looks good.

Christoph was also worried about the performance impact on these paths, so, as he suggested, uninline alloc_pages_node() and move it to mempolicy.c.

mm/mempolicy.c +0 -0

@@ -184,6 +185,16 @@ static struct mempolicy *mpol_new(int mode, nodemask_t *nodes)
 	atomic_set(&policy->refcnt, 1);
 	switch (mode) {
 	case MPOL_INTERLEAVE:

Quoted 3 lines · Nishanth Aravamudan

Christoph was also worried about the performance impact on these paths, so, as he suggested, uninline alloc_pages_node() and move it to mempolicy.c.

uninlining does not address performance issues.

Quoted 4 lines

(quoted diff) +0 -0

@@ -184,6 +185,16 @@ static struct mempolicy *mpol_new(int mode, nodemask_t *nodes)
 	atomic_set(&policy->refcnt, 1);
 	switch (mode) {
 	case MPOL_INTERLEAVE:

Review static struct mempolicy *mpol_new(int mode, nodemask_t *nodes) · line 197

Quoted 10 lines

(quoted diff) +10 -0

+		/*
+		 * Clear any memoryless nodes here so that v.nodes can be used
+		 * without extra checks
+		 */
+		nid = first_node(*nodes);
+		while (nid < MAX_NUMNODES) {
+			if (!node_populated(nid))
+				node_clear(nid, *nodes);
+			nid = next_node(nid, *nodes);
+		}

There is a "nodes_and" function for this.

Review static unsigned interleave_nodes(struct mempolicy *policy) · line 1160

Quoted 12 lines

(quoted diff) +5 -3

@@ -1126,9 +1153,11 @@ static unsigned interleave_nodes(struct mempolicy *policy)
 	struct task_struct *me = current;
 
 	nid = me->il_next;
-	next = next_node(nid, policy->v.nodes);
-	if (next >= MAX_NUMNODES)
-		next = first_node(policy->v.nodes);
+	do {
+		next = next_node(nid, policy->v.nodes);
+		if (next >= MAX_NUMNODES)
+			next = first_node(policy->v.nodes);
+	} while (!node_populated(next));

Is there a case where nodes has no node set?

Well, if the only place to get a "new" policy is mpol_new(), no, as just after the above nodes_and(), we check the weight of the nodemask. Is that sufficient?

Christoph said:
"This does not work for the address based interleaving for anonymous vmas. I am not sure what to do there. We could change the calculation of the node to be based only on nodes with memory and then skip the memoryless ones. I have only added a comment to describe its brokennes for now."

I have copied his draft's comment.

Change alloc_pages_node() to fail __GFP_THISNODE allocations if the node is not populated.

Again, Christoph said:
"This will fix the alloc_pages_node case but not the alloc_pages() case. In the alloc_pages() case we do not specify a node. Implicitly it is understood that we (in the case of no memory policy / cpuset options) allocate from the nearest node. So it may be argued there that the GFP_THISNODE behavior of taking the first node from the zonelist is okay."

Christoph was also worried about the performance impact on these paths, as am I.

Finally, as he suggested, uninline alloc_pages_node() and move it to mempolicy.c.

include/linux/gfp.h +1 -13

index 49dcc2f..c83e56a 100644
@@ -165,19 +165,7 @@ static inline void arch_alloc_page(struct page *page, int order) { }
 extern struct page *
 FASTCALL(__alloc_pages(gfp_t, unsigned int, struct zonelist *));
 
-static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
-						unsigned int order)
-{
-	if (unlikely(order >= MAX_ORDER))
-		return NULL;
-
-	/* Unknown node is current node */
-	if (nid < 0)
-		nid = numa_node_id();
-
-	return __alloc_pages(gfp_mask, order,
-		NODE_DATA(nid)->node_zonelists + gfp_zone(gfp_mask));
-}
+extern struct page * alloc_pages_node(int, gfp_t, unsigned int);
 
 #ifdef CONFIG_NUMA
 extern struct page *alloc_pages_current(gfp_t gfp_mask, unsigned order);

mm/mempolicy.c +0 -0

index 144805c..abadbf4 100644