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

Groups > linux.kernel > #1621309 > unrolled thread

[RFC 5/6] mm, cpuset: always use seqlock when changing task's nodemask

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC 5/6] mm, cpuset: always use seqlock when changing task's nodemask Vlastimil Babka <vbabka@suse.cz> - 2017-04-11 16:10 +0200
    Re: [RFC 5/6] mm, cpuset: always use seqlock when changing task's nodemask "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-04-12 10:20 +0200
      Re: [RFC 5/6] mm, cpuset: always use seqlock when changing task's  nodemask Vlastimil Babka <vbabka@suse.cz> - 2017-04-12 10:20 +0200

#1621309 — [RFC 5/6] mm, cpuset: always use seqlock when changing task's nodemask

When updating task's mems_allowed and rebinding its mempolicy due to cpuset's
mems being changed, we currently only take the seqlock for writing when either
the task has a mempolicy, or the new mems has no intersection with the old
mems. This should be enough to prevent a parallel allocation seeing no
available nodes, but the optimization is IMHO unnecessary (cpuset updates
should not be frequent), and we still potentially risk issues if the
intersection of new and old nodes has limited amount of free/reclaimable
memory. Let's just use the seqlock for all tasks.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 kernel/cgroup/cpuset.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index b0159f8f8c89..e76d18daf085 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1038,38 +1038,23 @@ static void cpuset_post_attach(void)
  * @tsk: the task to change
  * @newmems: new nodes that the task will be set
  *
- * In order to avoid seeing no nodes if the old and new nodes are disjoint,
- * we structure updates as setting all new allowed nodes, then clearing newly
- * disallowed ones.
+ * We use the mems_allowed_seq seqlock to safely update both tsk->mems_allowed
+ * and rebind an eventual tasks' mempolicy. If the task is allocating in
+ * parallel, it might temporarily see an empty intersection, which results in
+ * a seqlock check and retry before OOM or allocation failure.
  */
 static void cpuset_change_task_nodemask(struct task_struct *tsk,
 					nodemask_t *newmems)
 {
-	bool need_loop;
-
 	task_lock(tsk);
-	/*
-	 * Determine if a loop is necessary if another thread is doing
-	 * read_mems_allowed_begin().  If at least one node remains unchanged and
-	 * tsk does not have a mempolicy, then an empty nodemask will not be
-	 * possible when mems_allowed is larger than a word.
-	 */
-	need_loop = task_has_mempolicy(tsk) ||
-			!nodes_intersects(*newmems, tsk->mems_allowed);
 
-	if (need_loop) {
-		local_irq_disable();
-		write_seqcount_begin(&tsk->mems_allowed_seq);
-	}
+	local_irq_disable();
+	write_seqcount_begin(&tsk->mems_allowed_seq);
 
-	nodes_or(tsk->mems_allowed, tsk->mems_allowed, *newmems);
 	mpol_rebind_task(tsk, newmems);
 	tsk->mems_allowed = *newmems;
 
-	if (need_loop) {
-		write_seqcount_end(&tsk->mems_allowed_seq);
-		local_irq_enable();
-	}
+	write_seqcount_end(&tsk->mems_allowed_seq);
 
 	task_unlock(tsk);
 }
-- 
2.12.2

[toc] | [next] | [standalone]

#1621908

On April 11, 2017 10:06 PM Vlastimil Babka wrote: 
> 
>  static void cpuset_change_task_nodemask(struct task_struct *tsk,
>  					nodemask_t *newmems)
>  {
> -	bool need_loop;
> -
>  	task_lock(tsk);
> -	/*
> -	 * Determine if a loop is necessary if another thread is doing
> -	 * read_mems_allowed_begin().  If at least one node remains unchanged and
> -	 * tsk does not have a mempolicy, then an empty nodemask will not be
> -	 * possible when mems_allowed is larger than a word.
> -	 */
> -	need_loop = task_has_mempolicy(tsk) ||
> -			!nodes_intersects(*newmems, tsk->mems_allowed);
> 
> -	if (need_loop) {
> -		local_irq_disable();
> -		write_seqcount_begin(&tsk->mems_allowed_seq);
> -	}
> +	local_irq_disable();
> +	write_seqcount_begin(&tsk->mems_allowed_seq);
> 
> -	nodes_or(tsk->mems_allowed, tsk->mems_allowed, *newmems);
>  	mpol_rebind_task(tsk, newmems);
>  	tsk->mems_allowed = *newmems;
> 
> -	if (need_loop) {
> -		write_seqcount_end(&tsk->mems_allowed_seq);
> -		local_irq_enable();
> -	}
> +	write_seqcount_end(&tsk->mems_allowed_seq);
> 
Doubt if we'd listen irq again.

>  	task_unlock(tsk);
>  }
> --
> 2.12.2

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

#1621911 — Re: [RFC 5/6] mm, cpuset: always use seqlock when changing task's nodemask

On 04/12/2017 10:10 AM, Hillf Danton wrote:
> On April 11, 2017 10:06 PM Vlastimil Babka wrote: 
>>
>>  static void cpuset_change_task_nodemask(struct task_struct *tsk,
>>  					nodemask_t *newmems)
>>  {
>> -	bool need_loop;
>> -
>>  	task_lock(tsk);
>> -	/*
>> -	 * Determine if a loop is necessary if another thread is doing
>> -	 * read_mems_allowed_begin().  If at least one node remains unchanged and
>> -	 * tsk does not have a mempolicy, then an empty nodemask will not be
>> -	 * possible when mems_allowed is larger than a word.
>> -	 */
>> -	need_loop = task_has_mempolicy(tsk) ||
>> -			!nodes_intersects(*newmems, tsk->mems_allowed);
>>
>> -	if (need_loop) {
>> -		local_irq_disable();
>> -		write_seqcount_begin(&tsk->mems_allowed_seq);
>> -	}
>> +	local_irq_disable();
>> +	write_seqcount_begin(&tsk->mems_allowed_seq);
>>
>> -	nodes_or(tsk->mems_allowed, tsk->mems_allowed, *newmems);
>>  	mpol_rebind_task(tsk, newmems);
>>  	tsk->mems_allowed = *newmems;
>>
>> -	if (need_loop) {
>> -		write_seqcount_end(&tsk->mems_allowed_seq);
>> -		local_irq_enable();
>> -	}
>> +	write_seqcount_end(&tsk->mems_allowed_seq);
>>
> Doubt if we'd listen irq again.

Ugh, thanks for catching this. Looks like my testing config didn't have
lockup detectors enabled.

>>  	task_unlock(tsk);
>>  }
>> --
>> 2.12.2
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web