linux-kernel - Re: [RFC PATCH v7] sched: Fix performance regression introduced by mm

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: <5abacbf4-8101-7931-6dee-84f4f004240a@efficios.com>
Date:   Mon, 17 Apr 2023 09:28:54 -0400
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Aaron Lu <aaron.lu@...el.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, Olivier Dion <odion@...icios.com>,
        michael.christie@...cle.com
Subject: Re: [RFC PATCH v7] sched: Fix performance regression introduced by
 mm_cid

On 2023-04-17 06:18, Aaron Lu wrote:
> On Sun, Apr 16, 2023 at 06:32:17PM -0400, Mathieu Desnoyers wrote:
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 0d18c3969f90..9532cf1a2a44 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2084,8 +2084,10 @@ static inline void dequeue_task(struct rq *rq, struct task_struct *p, int flags)
>>   
>>   void activate_task(struct rq *rq, struct task_struct *p, int flags)
>>   {
>> -	if (task_on_rq_migrating(p))
>> +	if (task_on_rq_migrating(p)) {
>>   		flags |= ENQUEUE_MIGRATED;
>> +		sched_mm_cid_migrate_to(rq, p);
> 
> 
> I noticed you did this in previous version too but forgot to ask:
> is it your intention to only invoke sched_mm_cid_migrate_to() for queued
> tasks, i.e. tasks that are being migrated due to load/idle balance etc,
> but not those tasks that migrated on wakeup?

My intent is to also cover tasks migrated on wakeup.

> 
> I saw you did the corresonding sched_mm_cid_migrate_from() in
> set_task_cpu(), which on the other hand includes tasks that migrated due
> to wakeup, so it kind of feel weird.

I'm probably missing something here. AFAIU, when try_to_wake_up() moves 
the target process to a different cpu:

         cpu = select_task_rq(p, p->wake_cpu, wake_flags | WF_TTWU);
         if (task_cpu(p) != cpu) {

it ends up calling ttwu_queue() with wake_flags |= WF_MIGRATED bit set.

Then ttwu_queue() ends up calling ttwu_queue_wakelist(), which may 
initiate an IPI to the target cpu which executes sched_ttwu_pending().
This function will take the target cpu's runqueue lock and call 
ttwu_do_activate() with wake_flags=WF_MIGRATED.

The other path that ttwu_queue() can take is to issue ttwu_do_activate() 
with the target cpu's rq lock held.

ttwu_do_activate() calls activate_task() with flags having 
ENQUEUE_MIGRATED set.

OK I think I see what I missed here, I should change this to:

void activate_task(struct rq *rq, struct task_struct *p, int flags)
{
         if (task_on_rq_migrating(p))
                 flags |= ENQUEUE_MIGRATED;
	if (flags & ENQUEUE_MIGRATED)
                 sched_mm_cid_migrate_to(rq, p);
[...]

Because flags is received as input parameter as well.

Do I get your meaning correctly ?

Thanks!

Mathieu

> 
>> +	}
>>   
>>   	enqueue_task(rq, p, flags);
>>   
>> @@ -3195,6 +3197,7 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
>>   			p->sched_class->migrate_task_rq(p, new_cpu);
>>   		p->se.nr_migrations++;
>>   		rseq_migrate(p);
>> +		sched_mm_cid_migrate_from(p);
>>   		perf_event_task_migrate(p);
>>   	}
> 
> Thanks,
> Aaron

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com