linux-kernel - Re: [PATCH 2/2] sched: Extract the task putting code from pick_next

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: <jhjwo69lqcm.mognet@arm.com>
Date:   Mon, 20 Apr 2020 23:55:21 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Chen Yu <yu.c.chen@...el.com>, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>
Subject: Re: [PATCH 2/2] sched: Extract the task putting code from pick_next_task()


(There's a v2 at cover.1587393807.git.yu.c.chen@...el.com but I think this
still applies)

On 20/04/20 23:32, Steven Rostedt wrote:
>> @@ -3904,6 +3904,28 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
>>      schedstat_inc(this_rq()->sched_count);
>>  }
>>
>> +static void finish_prev_task(struct rq *rq, struct task_struct *prev,
>> +			     struct rq_flags *rf)
>> +{
>> +	const struct sched_class *class;
>> +#ifdef CONFIG_SMP
>> +	/*
>> +	 * We must do the balancing pass before put_next_task(), such
>
> I know this is just a cut and paste move, but I'm thinking that this
> comment is wrong. Shouldn't this be "put_prev_task()" as we have no
> "put_next_task()" function.
>

Oh, I think you're right.

>
>> +	 * that when we release the rq->lock the task is in the same
>> +	 * state as before we took rq->lock.
>> +	 *
>> +	 * We can terminate the balance pass as soon as we know there is
>> +	 * a runnable task of @class priority or higher.
>> +	 */
>> +	for_class_range(class, prev->sched_class, &idle_sched_class) {
>> +		if (class->balance(rq, prev, rf))
>> +			break;
>> +	}
>> +#endif
>> +
>> +	put_prev_task(rq, prev);
>> +}
>> +
>>  /*
>>   * Pick up the highest-prio task:
>>   */
>> @@ -3937,22 +3959,7 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>>      }
>>
>>  restart:
>> -#ifdef CONFIG_SMP
>> -	/*
>> -	 * We must do the balancing pass before put_next_task(), such
>> -	 * that when we release the rq->lock the task is in the same
>> -	 * state as before we took rq->lock.
>> -	 *
>> -	 * We can terminate the balance pass as soon as we know there is
>> -	 * a runnable task of @class priority or higher.
>> -	 */
>> -	for_class_range(class, prev->sched_class, &idle_sched_class) {
>> -		if (class->balance(rq, prev, rf))
>> -			break;
>> -	}
>> -#endif
>> -
>> -	put_prev_task(rq, prev);
>> +	finish_prev_task(rq, prev, rf);
>
> I'm not sure I like the name of this function. Perhaps
> "balance_and_put_prev_task()"? Something more in kind to what the function
> does.
>

The 'finish' thing isn't too far from the truth; it's the last thing we
need to do with the prev task (in terms of sched bookkeeping, I mean) -
and in Chen's defence ISTR Peter suggested that name.

Seeing as it's a "supercharged" put_prev_task(), I could live with the
marginally shorter "put_prev_task_balance()".

> -- Steve
>
>>
>>      for_each_class(class) {
>>              p = class->pick_next_task(rq);