MPOL_BIND on memory only nodes

Hi,

We have the following function policy_zonelist() which selects a zonelist during various allocation paths. With this, general user space allocations (IIUC might not have __GFP_THISNODE) fails while trying to get memory from a memory only node without CPUs as the application runs some where else and that node is not part of the nodemask.

Why we insist on __GFP_THISNODE ?

Quoted 7 lines · Anshuman Khandual

Hi,

We have the following function policy_zonelist() which selects a zonelist during various allocation paths. With this, general user space allocations (IIUC might not have __GFP_THISNODE) fails while trying to get memory from a memory only node without CPUs as the application runs some where else and that node is not part of the nodemask.

I am not sure I understand. So you have a task with MPOL_BIND without a cpu less node in the mask and you are wondering why the memory is not allocated from that node?

Quoted 2 lines

Why we insist on __GFP_THISNODE ?

AFAIU __GFP_THISNODE just overrides the given node to the policy nodemask in case the current node is not part of that node mask. In other words we are ignoring the given node and use what the policy says.

I can see how this can be confusing especially when confronting the documentation:

* __GFP_THISNODE forces the allocation to be satisified from the requested
* node with no fallbacks or placement policy enforcements.

Quoted 8 lines · Michal Hocko

Quoted 7 lines · Anshuman Khandual

Hi,

We have the following function policy_zonelist() which selects a zonelist during various allocation paths. With this, general user space allocations (IIUC might not have __GFP_THISNODE) fails while trying to get memory from a memory only node without CPUs as the application runs some where else and that node is not part of the nodemask.

My bad. Was playing with some changes to the zonelists rebuild after a memory node hotplug and the order of various zones in them.

Quoted 1 line

Quoted 3 lines

I am not sure I understand. So you have a task with MPOL_BIND without a cpu less node in the mask and you are wondering why the memory is not allocated from that node?

In my experiment, there is a MPOL_BIND call with a CPU less node in the node mask and the memory is not allocated from that CPU less node. Thats because the zone of the CPU less node was absent from the FALLBACK zonelist of the local node.

Quoted 5 lines

Quoted 1 line

Why we insist on __GFP_THISNODE ?

AFAIU __GFP_THISNODE just overrides the given node to the policy nodemask in case the current node is not part of that node mask. In other words we are ignoring the given node and use what the policy says.

Right but provided the gfp flag has __GFP_THISNODE in it. In absence of __GFP_THISNODE, the node from the nodemask will not be selected.

I still wonder why ? Can we always go to the first node in the nodemask for MPOL_BIND interface calls ? Just curious to know why preference is given to the local node and it's FALLBACK zonelist.

On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:

Quoted 1 line

Why we insist on __GFP_THISNODE ?

AFAIU __GFP_THISNODE just overrides the given node to the policy nodemask in case the current node is not part of that node mask. In other words we are ignoring the given node and use what the policy says. I can see how this can be confusing especially when confronting the documentation:

* __GFP_THISNODE forces the allocation to be satisified from the requested
* node with no fallbacks or placement policy enforcements.

Quoted 1 line · Michal Hocko

On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:

[...]

Quoted 10 lines

Quoted 1 line

Why we insist on __GFP_THISNODE ?

AFAIU __GFP_THISNODE just overrides the given node to the policy nodemask in case the current node is not part of that node mask. In other words we are ignoring the given node and use what the policy says. I can see how this can be confusing especially when confronting the documentation:

* __GFP_THISNODE forces the allocation to be satisified from the requested
* node with no fallbacks or placement policy enforcements.

You made me think and look into this deeper. I came to the conclusion that this is actually a relict from the past. policy_zonelist is called only from 3 places:
- huge_zonelist - never should do __GFP_THISNODE when going this path
- alloc_pages_vma - which shouldn't depend on __GFP_THISNODE either
- alloc_pages_current - which uses default_policy id __GFP_THISNODE is used

So AFAICS this is essentially a dead code or I am missing something. Mel do you remember why we needed it in the past? I would be really tempted to just get rid of this confusing code and this instead:
---

mm/mempolicy.c +6 -11

index ad1c96ac313c..98beec47bba9 100644
@@ -1679,25 +1679,17 @@ static nodemask_t *policy_nodemask(gfp_t gfp, struct mempolicy *policy)
 static struct zonelist *policy_zonelist(gfp_t gfp, struct mempolicy *policy,
 	int nd)
 {
-	switch (policy->mode) {
-	case MPOL_PREFERRED:
-		if (!(policy->flags & MPOL_F_LOCAL))
-			nd = policy->v.preferred_node;
-		break;
-	case MPOL_BIND:
+	if (policy->mode == MPOL_PREFERRED && !(policy->flags & MPOL_F_LOCAL))
+		nd = policy->v.preferred_node;
+	else {
 		/*
-		 * Normally, MPOL_BIND allocations are node-local within the
-		 * allowed nodemask.  However, if __GFP_THISNODE is set and the
-		 * current node isn't part of the mask, we use the zonelist for
-		 * the first node in the mask instead.
+		 * __GFP_THISNODE shouldn't even be used with the bind policy because
+		 * we might easily break the expectation to stay on the requested node
+		 * and not break the policy.
 		 */
-		if (unlikely(gfp & __GFP_THISNODE) &&

Which makes the function look like this. Even with these changes, MPOL_BIND is still going to pick up the local node's zonelist instead of the first node in policy->v.nodes nodemask. It completely ignores policy->v.nodes which it should not.

On Thu 13-10-16 15:24:54, Anshuman Khandual wrote:
[...]

Quoted 4 lines

Which makes the function look like this. Even with these changes, MPOL_BIND is still going to pick up the local node's zonelist instead of the first node in policy->v.nodes nodemask. It completely ignores policy->v.nodes which it should not.

Not really. I have tried to explain earlier. We do not ignore policy nodemask. This one comes from policy_nodemask. We start with the local node but fallback to some of the nodes from the nodemask defined by the policy.

Quoted 7 lines · Michal Hocko

On Thu 13-10-16 15:24:54, Anshuman Khandual wrote:
[...]

Quoted 4 lines

Which makes the function look like this. Even with these changes, MPOL_BIND is still going to pick up the local node's zonelist instead of the first node in policy->v.nodes nodemask. It completely ignores policy->v.nodes which it should not.

Quoted 5 lines

Not really. I have tried to explain earlier. We do not ignore policy nodemask. This one comes from policy_nodemask. We start with the local node but fallback to some of the nodes from the nodemask defined by the policy.

Yeah saw your response but did not get that exactly. We dont ignore policy nodemask while memory allocation, correct. But my point was we are ignoring policy nodemask while selecting zonelist which will be used during page allocation. Though the zone contents of both the zonelists are likely to be same, would not it be better to get the zone list from the nodemask as well ?

Or I am still missing something
here. The following change is what I am trying to propose.

mm/mempolicy.c +0 -0

index ad1c96a..f60ab80 100644
@@ -1685,14 +1685,7 @@ static struct zonelist *policy_zonelist(gfp_t gfp, struct mempolicy *policy,
                        nd = policy->v.preferred_node;
                break;
        case MPOL_BIND:

Quoted 1 line · Michal Hocko

On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:

[...]

Quoted 10 lines

Quoted 1 line

Why we insist on __GFP_THISNODE ?

AFAIU __GFP_THISNODE just overrides the given node to the policy nodemask in case the current node is not part of that node mask. In other words we are ignoring the given node and use what the policy says. I can see how this can be confusing especially when confronting the documentation:

* __GFP_THISNODE forces the allocation to be satisified from the requested
* node with no fallbacks or placement policy enforcements.

You made me think and look into this deeper. I came to the conclusion that this is actually a relict from the past. policy_zonelist is called only from 3 places:
- huge_zonelist - never should do __GFP_THISNODE when going this path
- alloc_pages_vma - which shouldn't depend on __GFP_THISNODE either
- alloc_pages_current - which uses default_policy id __GFP_THISNODE is used

So AFAICS this is essentially a dead code or I am missing something. Mel do you remember why we needed it in the past?

Quoted 22 lines · Michal Hocko

Quoted 1 line · Michal Hocko

On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:

[...]

Quoted 10 lines

Quoted 1 line

Why we insist on __GFP_THISNODE ?

AFAIU __GFP_THISNODE just overrides the given node to the policy nodemask in case the current node is not part of that node mask. In other words we are ignoring the given node and use what the policy says. I can see how this can be confusing especially when confronting the documentation:

* __GFP_THISNODE forces the allocation to be satisified from the requested
* node with no fallbacks or placement policy enforcements.

You made me think and look into this deeper. I came to the conclusion that this is actually a relict from the past. policy_zonelist is called only from 3 places:
- huge_zonelist - never should do __GFP_THISNODE when going this path
- alloc_pages_vma - which shouldn't depend on __GFP_THISNODE either
- alloc_pages_current - which uses default_policy id __GFP_THISNODE is used

Quoted 2 lines

So AFAICS this is essentially a dead code or I am missing something. Mel do you remember why we needed it in the past?

I don't recall a specific reason. It was likely due to confusion on my part at the time on the exact use of __GFP_THISNODE. The expectation is that flag is not used in fault paths or with policies. It's meant to enforce node-locality for kernel internal decisions such as the locality of slab pages and ensuring that a THP collapse from khugepaged is on the same node.