Received-SPF: pass (google.com: domain of linux-kernel+bounces-200900-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <8052b12a-98ac-4309-8286-a330315d531b@arm.com>
Date: Tue, 4 Jun 2024 15:49:59 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC][PATCH 08/10] sched/fair: Implement delayed dequeue
To: Luis Machado <luis.machado@arm.com>, Peter Zijlstra <peterz@infradead.org>
Cc: mingo@redhat.com, juri.lelli@redhat.com, vincent.guittot@linaro.org,
 dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
 mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
 linux-kernel@vger.kernel.org, kprateek.nayak@amd.com,
 wuyun.abel@bytedance.com, tglx@linutronix.de, efault@gmx.de, nd
 <nd@arm.com>, John Stultz <jstultz@google.com>
References: <20240405110010.631664251@infradead.org>
 <3888d7c8-660e-479c-8c10-8295204e5f36@arm.com>
 <1461277e-af68-41e7-947c-9178b55810b1@arm.com>
 <20240425104220.GE21980@noisy.programming.kicks-ass.net>
 <20240425114949.GH12673@noisy.programming.kicks-ass.net>
 <20240426093241.GI12673@noisy.programming.kicks-ass.net>
 <c6152855-ef92-4c24-a3f5-64d4256b6789@arm.com>
 <2fba04b0-e55e-41f4-8b7a-723734fe1ad2@arm.com>
 <20240529225036.GN40213@noisy.programming.kicks-ass.net>
 <7eac0774-0f9d-487c-97b6-ab0e85f0ae3a@arm.com>
 <20240604101107.GO26599@noisy.programming.kicks-ass.net>
 <24e09046-74ee-4ebb-ac1a-bdc84568e825@arm.com>
Content-Language: en-US
From: Hongyan Xia <hongyan.xia2@arm.com>
In-Reply-To: <24e09046-74ee-4ebb-ac1a-bdc84568e825@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 04/06/2024 15:23, Luis Machado wrote:
> On 6/4/24 11:11, Peter Zijlstra wrote:
>> On Mon, Jun 03, 2024 at 08:30:43PM +0100, Luis Machado wrote:
>>
>>> Exchanging some information with Hongyan today, he was a bit suspicious of the uclamp
>>> behavior with the eevdf complete series applied.
>>>
>>> Checking the uclamp code, I see we have some refcounting tied to enqueuing/dequeuing
>>> of tasks, and the uclamp values are organized in buckets.
>>>
>>> Just for fun I added a few trace_printk's in uclamp_eff_value, uclamp_rq_inc_id and
>>> uclamp_rq_dec_id.
>>>
>>> Booting up the system with delayed_dequeue disabled and running the benchmark, I
>>> see the uclamp bucket management pretty stable. Tasks get added to the uclamp
>>> buckets but then get removed. At the end of the benchmark, the uclamp buckets
>>> are (almost always) clean of tasks.
>>>
>>> Enabling delayed dequeue, I can see the uclamp buckets slowly filling up with
>>> tasks. At the end of the benchmark, I see uclamp buckets with 30, 40 or 50
>>> tasks still. If I do another run, I can see 80, 100 tasks still.
>>>
>>> I suspect refcounting might be going wrong somewhere due to delayed dequeue
>>> tasks, but that's more of a guess right now. Hopefully that is useful
>>> information. I'll resume investigation tomorrow.
>>
>> Thank you both!!
>>
>> Does the below help?
>>
>> Note how dequeue_task() does uclamp_rq_dec() unconditionally, which is
>> then not balanced in the case below.
>>
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -3664,6 +3664,7 @@ static int ttwu_runnable(struct task_str
>>   			/* mustn't run a delayed task */
>>   			SCHED_WARN_ON(task_on_cpu(rq, p));
>>   			enqueue_task(rq, p, ENQUEUE_DELAYED);
>> +			uclamp_rq_inc(rq, p);
>>   		}
>>   		if (!task_on_cpu(rq, p)) {
>>   			/*
> 
> As Hongyan pointed out in a separate message, the above makes things
> worse, as we end up with even more leftover tasks in the uclamp
> buckets.
> 
> I'm trying a fix in kernel/sched/core.c:enqueue_task that only
> calls uclamp_rq_inc if the task is not sched_delayed, so:
> 
> -       uclamp_rq_inc(rq, p);
> +       if (!p->se.sched_delayed)
> +         uclamp_rq_inc(rq, p);
> 
> I'm not entirely sure it is correct, but it seems to fix things,
> but I'm still running some tests.

This seems to so far not trigger any WARN or bad behavior in my tests.

> 
> With the current code, given uclamp_rq_inc and uclamp_rq_dec get
> called in enqueue_task and dequeue_task, the additional enqueue_task
> call from ttwu_runnable for a delayed_dequeue task may do an additional
> unconditional call to uclamp_rq_inc, no?