linux-kernel - Re: [PATCH] sched: Optimize pick_next_task for idle_sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170120111425.5afaad5c@gandalf.local.home>
Date:   Fri, 20 Jan 2017 11:14:25 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] sched: Optimize pick_next_task for idle_sched_class too

On Thu, 19 Jan 2017 18:44:08 +0100
Peter Zijlstra <peterz@...radead.org> wrote:
-	if (likely(prev->sched_class == class &&
> > +	if (likely((prev->sched_class == class ||
> > +		    prev->sched_class == idle_class) &&
> >  		   rq->nr_running == rq->cfs.h_nr_running)) {  
> 
> OK, so I hate this patch because it makes the condition more complex,
> and while staring at what it does for code generation I couldn't for the
> life of me figure out why we care about prev->sched_class to begin with.

I was thinking it would save on checking the rq at all, but rq is used
by pick_next_task_*() anyway, so I doubt it's much savings.

> 
> (we used to, but the current code not so much)
> 
> So I simply removed that entire clause, like below, and lo and behold,
> the system booted...

I thought about doing this too, but decided against it because I was
thinking that since class (and now idle_class) are constants, it would
help the non cfs case. Checking prev->sched_class against a constant I
thought would be quick. But yeah, I doubt it matters much in the grand
scale of things.


> 
> Could you give it a spin to see if anything comes apart?

Yeah this works. You can add:

Reported-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
Tested-by: Steven Rostedt (VMware) <rostedt@...dmis.org>

Thanks,

-- Steve

> 
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 49ce1cb..51ca21e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3321,15 +3321,14 @@ static inline void schedule_debug(struct task_struct *prev)
>  static inline struct task_struct *
>  pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>  {
> -	const struct sched_class *class = &fair_sched_class;
> +	const struct sched_class *class;
>  	struct task_struct *p;
>  
>  	/*
>  	 * Optimization: we know that if all tasks are in
>  	 * the fair class we can call that function directly:
>  	 */
> -	if (likely(prev->sched_class == class &&
> -		   rq->nr_running == rq->cfs.h_nr_running)) {
> +	if (likely(rq->nr_running == rq->cfs.h_nr_running)) {
>  		p = fair_sched_class.pick_next_task(rq, prev, rf);
>  		if (unlikely(p == RETRY_TASK))
>  			goto again;