Received-SPF: pass (google.com: domain of linux-kernel+bounces-208060-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <33405fa7-7102-407e-8cc8-851c808cd47a@arm.com>
Date: Mon, 10 Jun 2024 12:06:08 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 6/6] cpuidle: teo: Don't count non-existent intercepts
To: Dietmar Eggemann <dietmar.eggemann@arm.com>, linux-pm@vger.kernel.org,
 linux-kernel@vger.kernel.org, rafael@kernel.org
Cc: vincent.guittot@linaro.org, qyousef@layalina.io, peterz@infradead.org,
 daniel.lezcano@linaro.org, anna-maria@linutronix.de,
 kajetan.puchalski@arm.com, lukasz.luba@arm.com
References: <20240606090050.327614-1-christian.loehle@arm.com>
 <20240606090050.327614-7-christian.loehle@arm.com>
 <a71314e6-b248-44a4-8a82-0a8dfde9d44f@arm.com>
Content-Language: en-US
From: Christian Loehle <christian.loehle@arm.com>
In-Reply-To: <a71314e6-b248-44a4-8a82-0a8dfde9d44f@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 6/7/24 11:17, Dietmar Eggemann wrote:
> On 06/06/2024 11:00, Christian Loehle wrote:
>> When bailing out early, teo will not query the sleep length anymore
>> since commit 6da8f9ba5a87 ("cpuidle: teo:
>> Skip tick_nohz_get_sleep_length() call in some cases") with an
>> expected sleep_length_ns value of KTIME_MAX.
>> This lead to state0 accumulating lots of 'intercepts' because
>> the actually measured sleep length was < KTIME_MAX, so count KTIME_MAX
>> as a hit (we have to count them as something otherwise we are stuck).
>>
>> Fundamentally we can only do one of the two:
>> 1. Skip sleep_length_ns query when we think intercept is likely
>> 2. Have accurate data if sleep_length_ns is actually intercepted when
>> we believe it is currently intercepted.
>>
>> This patch chooses that latter as I've found the additional time it
>> takes to query the sleep length to be negligible and the variants of
>> option 1 (count all unknowns as misses or count all unknown as hits)
>> had significant regressions (as misses had lots of too shallow idle
>> state selections and as hits had terrible performance in
>> intercept-heavy workloads).
> 
> So '2.' is the 'if (prev_intercept_idx != idx && !idx)' case ?
> 
> [...]

Yes, we allow the logic to bail out early, but not without querying the
expected sleep length.
(For idx > 0 the logic will continue to query the expected sleep length
later on.)

> 
>> @@ -514,6 +521,14 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>  			first_suitable_idx = i;
>>  		}
>>  	}
>> +	if (prev_intercept_idx != idx && !idx) {
> 
> if (!idx && prev_intercept_idx) ?
> 

Thanks! I picked that up for the next version.

>> +		/*
>> +		 * We have to query the sleep length here otherwise we don't
>> +		 * know after wakeup if our guess was correct.
>> +		 */
>> +		duration_ns = tick_nohz_get_sleep_length(&delta_tick);
>> +		cpu_data->sleep_length_ns = duration_ns;
>> +	}
>>  
>>  	/*
>>  	 * If there is a latency constraint, it may be necessary to select an
>