Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1499447 > unrolled thread

MPOL_BIND on memory only nodes

Back to article view | Back to linux.kernel

Contents

  MPOL_BIND on memory only nodes Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2016-10-12 11:30 +0200
    Re: MPOL_BIND on memory only nodes Michal Hocko <mhocko@kernel.org> - 2016-10-12 11:50 +0200
      Re: MPOL_BIND on memory only nodes Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2016-10-12 12:40 +0200
        Re: MPOL_BIND on memory only nodes Michal Hocko <mhocko@kernel.org> - 2016-10-12 13:10 +0200
      Re: MPOL_BIND on memory only nodes Michal Hocko <mhocko@kernel.org> - 2016-10-12 15:20 +0200

#1499447 — 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 ?
On any memory only node its likely that the local node "nd" might not be
part of the nodemask, hence does it make sense to pick up the first node of
the nodemask in those cases without looking for __GFP_THISNODE ?

/* Return a zonelist indicated by gfp for node representing a mempolicy */
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:
		/*
		 * 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.
		 */
		if (unlikely(gfp & __GFP_THISNODE) &&
				unlikely(!node_isset(nd, policy->v.nodes)))
			nd = first_node(policy->v.nodes);
		break;
	default:
		BUG();
	}
	return node_zonelist(nd, gfp);
}

- Anshuman

[toc] | [next] | [standalone]

#1499450

On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:
> 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?

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

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1499471

On 10/12/2016 03:13 PM, Michal Hocko wrote:
> On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:
>> 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.

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

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

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

Yeah, right.

Thanks for your reply.

[toc] | [prev] | [next] | [standalone]

#1499483

On Wed 12-10-16 16:08:48, Anshuman Khandual wrote:
> On 10/12/2016 03:13 PM, Michal Hocko wrote:
> > On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:
> >> 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.
> 
> > 
> > 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.

So do I understand this correctly that the issue was caused by
non-upstream changes?

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

In absence of __GFP_THISNODE we will use the zonelist for the given node
and that should contain even memoryless nodes for the fallback. The
nodemask from policy_nodemask() will then make sure that only nodes
relevant to the used policy is used.

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

It is not always a local node. Look at how do_huge_pmd_wp_page_fallback
tries to make all the pages into the same node. Also we have
alloc_pages_current() which tries to allocate from the local node which
should not fallback to the firs node in the policy nodemask.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1499670

On Wed 12-10-16 11:43:37, Michal Hocko wrote:
> On Wed 12-10-16 14:55:24, Anshuman Khandual wrote:
[...]
> > 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:
---
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index ad1c96ac313c..98beec47bba9 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -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) &&
-				unlikely(!node_isset(nd, policy->v.nodes)))
-			nd = first_node(policy->v.nodes);
-		break;
-	default:
-		BUG();
+		WARN_ON_ONCE(polic->mode == MPOL_BIND && (gfp && __GFP_THISNODE));
 	}
+
 	return node_zonelist(nd, gfp);
 }
 
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web