mm: fix bugs of mpol_rebind_nodemask()

- local variable might be an empty nodemask, so must be checked before setting pol->v.nodes to it.

- nodes_remap() may cause the weight of pol->v.nodes being monotonic decreasing. and never become large even we pass a nodemask with large weight after ->v.nodes become little.

this patch fixes these two problem.

Signed-off-by Miao Xie <[email protected]>

---
 mm/mempolicy.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

mm/mempolicy.c +1 -2

index 08f40a2..03ba9fc 100644
@@ -291,12 +291,15 @@ static void mpol_rebind_nodemask(struct mempolicy *pol,
 	else if (pol->flags & MPOL_F_RELATIVE_NODES)
 		mpol_relative_nodemask(&tmp, &pol->w.user_nodemask, nodes);
 	else {
-		nodes_remap(tmp, pol->v.nodes, pol->w.cpuset_mems_allowed,
-			    *nodes);
+		tmp = *nodes;
 		pol->w.cpuset_mems_allowed = *nodes;
 	}

Quoted 3 lines · Miao Xie

- local variable might be an empty nodemask, so must be checked before setting pol->v.nodes to it.

Quoted 4 lines

- nodes_remap() may cause the weight of pol->v.nodes being monotonic decreasing. and never become large even we pass a nodemask with large weight after ->v.nodes become little.

That's always been the intention of rebinding a mempolicy nodemask: we remap the current mempolicy nodes over the new nodemask given the set of allowed nodes. The nodes_remap() shouldn't be removed.

Quoted 22 lines

this patch fixes these two problem.

Signed-off-by Miao Xie <[email protected]>

---
 mm/mempolicy.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

mm/mempolicy.c +1 -2

index 08f40a2..03ba9fc 100644
@@ -291,12 +291,15 @@ static void mpol_rebind_nodemask(struct mempolicy *pol,
 	else if (pol->flags & MPOL_F_RELATIVE_NODES)
 		mpol_relative_nodemask(&tmp, &pol->w.user_nodemask, nodes);
 	else {
-		nodes_remap(tmp, pol->v.nodes, pol->w.cpuset_mems_allowed,
-			    *nodes);
+		tmp = *nodes;
 		pol->w.cpuset_mems_allowed = *nodes;
 	}
 

mm/mempolicy.c static void mpol_rebind_nodemask(struct mempolicy *pol, · line 305

Quoted 9 lines

mm/mempolicy.c +5 -1

-	pol->v.nodes = tmp;
+	if (nodes_empty(tmp))
+		pol->v.nodes = *nodes;
+	else
+		pol->v.nodes = tmp;
+
 	if (!node_isset(current->il_next, tmp)) {
 		current->il_next = next_node(current->il_next, tmp);
 		if (current->il_next >= MAX_NUMNODES)

Suppose the current mempolicy nodes is 0-2, we can remap it from 0-2 to 2, then we can remap it from 2 to 1, but we can't remap it from 2 to 0-2.

that is to say it can't be remaped to a large set of allowed nodes, and the task just can use the small set of nodes for ever, even the large set of nodes is allowed, I think it is unreasonable.

Quoted 7 lines · Miao Xie

Suppose the current mempolicy nodes is 0-2, we can remap it from 0-2 to 2, then we can remap it from 2 to 1, but we can't remap it from 2 to 0-2.

that is to say it can't be remaped to a large set of allowed nodes, and the task just can use the small set of nodes for ever, even the large set of nodes is allowed, I think it is unreasonable.

That's been the behavior for at least three years so changing it from under the applications isn't acceptable, see Documentation/vm/numa_memory_policy.txt regarding mempolicy rebinds and the two flags that are defined that can be used to adjust the behavior.

The pol->v.nodes = nodes_empty(tmp) ? *nodes : tmp fix is welcome, however, as a standalone patch.

Quoted 4 lines

That's been the behavior for at least three years so changing it from under the applications isn't acceptable, see Documentation/vm/numa_memory_policy.txt regarding mempolicy rebinds and the two flags that are defined that can be used to adjust the behavior.

Is the flags what you said MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES? But the codes that I changed isn't under MPOL_F_STATIC_NODES or MPOL_F_RELATIVE_NODES. The documentation doesn't say what we should do if either of these two flags is not set.

Furthermore, in order to fix no node to alloc memory, when we want to update mempolicy and mems_allowed, we expand the set of nodes first (set all the newly nodes) and shrink the set of nodes lazily(clean disallowed nodes).

But remap() breaks the expanding, so if we don't remove remap(), the problem can't be fixed. Otherwise, cpuset has to do the rebinding by itself and the code is ugly. Like this:

static void cpuset_change_task_nodemask(struct task_struct *tsk, nodemask_t *newmems)

{
	nodemask_t tmp;
	...
	/* expand the set of nodes */
	if (!mpol_store_user_nodemask(tsk->mempolicy)) {
		nodes_remap(tmp, ...);
		nodes_or(tsk->mempolicy->v.nodes, tsk->mempolicy->v.nodes, tmp);
	}
	...

	/* shrink the set of nodes */
	if (!mpol_store_user_nodemask(tsk->mempolicy))
		tsk->mempolicy->v.nodes = tmp;
}

Quoted 9 lines · Miao Xie

Quoted 4 lines

That's been the behavior for at least three years so changing it from under the applications isn't acceptable, see Documentation/vm/numa_memory_policy.txt regarding mempolicy rebinds and the two flags that are defined that can be used to adjust the behavior.

Is the flags what you said MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES? But the codes that I changed isn't under MPOL_F_STATIC_NODES or MPOL_F_RELATIVE_NODES. The documentation doesn't say what we should do if either of these two flags is not set.

MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES allow you to adjust the behavior of the rebind: the former requires specific nodes to be assigned to the mempolicy and could suppress the rebind completely, if necessary; the latter ensures the mempolicy nodemask has a certain weight as nodes are assigned in a round-robin manner. The behavior that you're referring to is provided via MPOL_F_RELATIVE_NODES, which guarantees whatever weight is passed via set_mempolicy() will be preserved when mems are added to a cpuset.

Regardless of whether the behavior is documented when either flag is passed, we can't change the long-standing default behavior that people use when their cpuset mems are rebound: we can only extend the functionality and the behavior you're seeking is already available with a MPOL_F_RELATIVE_NODES flag modifier.

Quoted 3 lines

Furthermore, in order to fix no node to alloc memory, when we want to update mempolicy and mems_allowed, we expand the set of nodes first (set all the newly nodes) and shrink the set of nodes lazily(clean disallowed nodes).

That's a cpuset implementation choice, not a mempolicy one; mempolicies have nothing to do with an empty current->mems_allowed.

Quoted 5 lines

But remap() breaks the expanding, so if we don't remove remap(), the problem can't be fixed. Otherwise, cpuset has to do the rebinding by itself and the code is ugly. Like this:

static void cpuset_change_task_nodemask(struct task_struct *tsk, nodemask_t *newmems)

Quoted 15 lines

{
	nodemask_t tmp;
	...
	/* expand the set of nodes */
	if (!mpol_store_user_nodemask(tsk->mempolicy)) {
		nodes_remap(tmp, ...);
		nodes_or(tsk->mempolicy->v.nodes, tsk->mempolicy->v.nodes, tmp);
	}
	...

	/* shrink the set of nodes */
	if (!mpol_store_user_nodemask(tsk->mempolicy))
		tsk->mempolicy->v.nodes = tmp;
}

I don't see why this is even necessary, the mempolicy code could simply return numa_node_id() when nodes_empty(current->mempolicy->v.nodes) to close the race.

[ Your pseudo-code is also lacking task_lock(tsk), which is required to
safely dereference tsk->mempolicy, and this is only available so far in -mm since the oom killer rewrite. ]