Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 8 Nov 2019 13:00:35 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Quentin Perret <qperret@google.com>
Cc:     Kirill Tkhai <ktkhai@virtuozzo.com>, linux-kernel@vger.kernel.org,
        aaron.lwe@gmail.com, valentin.schneider@arm.com, mingo@kernel.org,
        pauld@redhat.com, jdesfossez@digitalocean.com,
        naravamudan@digitalocean.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, juri.lelli@redhat.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        kernel-team@android.com, john.stultz@linaro.org
Subject: Re: NULL pointer dereference in pick_next_task_fair
Message-ID: <20191108120034.GK4131@hirez.programming.kicks-ass.net>
References: <20191106120525.GX4131@hirez.programming.kicks-ass.net>
 <33643a5b-1b83-8605-2347-acd1aea04f93@virtuozzo.com>
 <20191106165437.GX4114@hirez.programming.kicks-ass.net>
 <20191106172737.GM5671@hirez.programming.kicks-ass.net>
 <831c2cd4-40a4-31b2-c0aa-b5f579e770d6@virtuozzo.com>
 <20191107132628.GZ4114@hirez.programming.kicks-ass.net>
 <20191107153848.GA31774@google.com>
 <20191107184356.GF4114@hirez.programming.kicks-ass.net>
 <20191107192907.GA30258@worktop.programming.kicks-ass.net>
 <20191108110212.GA204618@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191108110212.GA204618@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 08, 2019 at 11:02:12AM +0000, Quentin Perret wrote:
> On Thursday 07 Nov 2019 at 20:29:07 (+0100), Peter Zijlstra wrote:
> > I still havne't had food, but this here compiles...
> 
> And it seems to work, too :)

Excellent!

> > @@ -3929,13 +3929,17 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> >  	}
> > 
> >  restart:
> > -	/*
> > -	 * Ensure that we put DL/RT tasks before the pick loop, such that they
> > -	 * can PULL higher prio tasks when we lower the RQ 'priority'.
> > -	 */
> > -	prev->sched_class->put_prev_task(rq, prev, rf);
> > -	if (!rq->nr_running)
> > -		newidle_balance(rq, rf);
> > +#ifdef CONFIG_SMP
> > +	for (class = prev->sched_class;
> > +	     class != &idle_sched_class;
> > +	     class = class->next) {
> > +
> > +		if (class->balance(rq, prev, rf))
> > +			break;
> > +	}
> > +#endif
> > +
> > +	put_prev_task(rq, prev);
> 
> Right, that looks much cleaner IMO. I'm thinking if we killed the
> special case for CFS above we could do with a single loop to iterate the
> classes, and you could fold ->balance() in ->pick_next_task() ...

No, you can't, because then you're back to having to restart the pick
when something happens when we drop the rq halfway down the pick.  Which
is something I really wanted to get rid of.

> That would remove one call site to newidle_balance() too, which I think
> is good. Hackbench probably won't like that, though.

Yeah, that fast path really is important. I've got a few patches pending
there, fixing a few things and that gets me 2% extra on a sched-yield
benchmark.

> > +static int balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> > +{
> > +	if (rq->cfs.nr_running)

FWIW that must test rq->nr_running.

> > +		return 1;
> > +
> > +	return newidle_balance(rq, rf) != 0;
> 
> And you can ignore the RETRY_TASK case here under the assumption that
> we must have tried to pull from RT/DL before ending up here ?

Well, the balance callback can always return 0 and be correct. The point
is mostly to avoid more work.

In this case, since fair is the last class and we've already excluded
idle for balancing, the win is minimal. But since we have the code,
might as well dtrt.