From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932358AbbHGPaF (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Aug 2015 11:30:05 -0400
Received: from casper.infradead.org ([85.118.1.10]:51174 "EHLO
	casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S932217AbbHGPaA (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Aug 2015 11:30:00 -0400
Date: Fri, 7 Aug 2015 17:29:56 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Tejun Heo <tj@kernel.org>
Cc: mingo@kernel.org, riel@redhat.com, dedekind1@gmail.com,
        linux-kernel@vger.kernel.org, mgorman@suse.de, rostedt@goodmis.org,
        juri.lelli@arm.com, Oleg Nesterov <oleg@redhat.com>
Subject: Re: [RFC][PATCH 1/4] sched: Fix a race between __kthread_bind() and
 sched_setaffinity()
Message-ID: <20150807152956.GN16853@twins.programming.kicks-ass.net>
References: <20150515154333.712161952@infradead.org>
 <20150515154833.545640346@infradead.org>
 <20150515155653.GA23692@htj.duckdns.org>
 <20150807142708.GK16853@twins.programming.kicks-ass.net>
 <20150807151608.GD14626@mtj.duckdns.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150807151608.GD14626@mtj.duckdns.org>
User-Agent: Mutt/1.5.21 (2012-12-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Aug 07, 2015 at 11:16:08AM -0400, Tejun Heo wrote:
> On Fri, Aug 07, 2015 at 04:27:08PM +0200, Peter Zijlstra wrote:
> > Which is the rescue thread attaching itself to a pool that needs help,
> > and obviously the rescue thread isn't new so kthread_bind doesn't work
> > right.
> > 
> > The best I could come up with is something like the below on top; does
> > that work for you? I'll go give it some runtime.
> > 
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -1622,11 +1622,15 @@ static struct worker *alloc_worker(int n
> >   * cpu-[un]hotplugs.
> >   */
> >  static void worker_attach_to_pool(struct worker *worker,
> > -				   struct worker_pool *pool)
> > +				   struct worker_pool *pool,
> > +				   bool new)
> >  {
> >  	mutex_lock(&pool->attach_mutex);
> >  
> > -	kthread_bind_mask(worker->task, pool->attrs->cpumask);
> > +	if (new)
> > +		kthread_bind_mask(worker->task, pool->attrs->cpumask);
> > +	else
> > +		set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
> >  
> >  	/*
> >  	 * The pool->attach_mutex ensures %POOL_DISASSOCIATED remains
> > @@ -1712,7 +1716,7 @@ static struct worker *create_worker(stru
> >  	set_user_nice(worker->task, pool->attrs->nice);
> >  
> >  	/* successful, attach the worker to the pool */
> > -	worker_attach_to_pool(worker, pool);
> > +	worker_attach_to_pool(worker, pool, true);
> >  
> >  	/* start the newly created worker */
> >  	spin_lock_irq(&pool->lock);
> > @@ -2241,7 +2245,7 @@ static int rescuer_thread(void *__rescue
> >  
> >  		spin_unlock_irq(&wq_mayday_lock);
> >  
> > -		worker_attach_to_pool(rescuer, pool);
> > +		worker_attach_to_pool(rescuer, pool, false);
> 
> Hmmm... the race condition didn't exist for workqueue in the first
> place, right?

No, I think workqueues are susceptible just the same as everybody else.
By the time we call __kthread_bind() the task exists and is visible to
userspace.

        __kthread_bind()
          do_set_cpus_allowed()
                                                <SYSCALL>
                                                  sched_setaffinity()
                                                    if (p->flags & PF_NO_SETAFFINITIY) /* false-not-taken */
                                                    set_cpus_allowed_ptr()
          p->flags |= PF_NO_SETAFFINITY

> As long as the flag is set before the affinity is configured, there's
> no race condition.

Even if we were to strictly order those stores you could have (note
there is no matching barrier, as there is only the one load, so ordering
cannot help):

	__kthread_bind()
						<SYSCALL>
						  sched_setaffinity()
						    if (p->flags & PF_NO_SETAFFINITY) /* false-not-taken */
	  p->flags |= PF_NO_SETAFFINITY;
	  smp_wmb();
	  do_set_cpus_allowed();
						    set_cpus_allowed_ptr()

> I think the code was better before.  Can't we just revert workqueue.c
> part?

I agree that the new argument isn't pretty, but I cannot see how
workqueues would not be affected by this.