Re: [PATCH v6 1/4] cpufreq: Remove per-CPU QoS constraint

[Viresh Kumar]

On 24-03-26, 12:39, Pierre Gondois wrote:
> I experimented a bit more and it seems the following happens:
> 
> 1. boost all CPUs: echo 1 > /sys/devices/system/cpu/cpufreq/boost
> 2. offline one CPU: echo 0 > /sys/devices/system/cpu/cpuX/online
> 3. deboost all CPUs: echo 0 > /sys/devices/system/cpu/cpufreq/boost
> 
> cpufreq_boost_trigger_state()
> \-for_each_active_policy()
>   \-cpufreq_driver->set_boost()
> doesn't act on the policy where there are no more online CPUs,
> so the max/cpuinfo.max/max_freq_req is left to the actual
> boost freq.

Right.

> 4. online CPUX: echo 1 > /sys/devices/system/cpu/cpuX/online
> 
> cpufreq_online()
> \-cpufreq_driver->init()
>   \-cppc_cpufreq_cpu_init()
> There:
> - policy->max
> - policy->cpuinfo.max_freq
> are set to the maximal non-boost freq., which is the correct value.

Right, but that is true only for CPPC. Not all drivers would set max freq from
there and depend on cpufreq_table_validate_and_sort() to fix this. Which was
broken earlier, but with my patch it should be fixed now.

So yes, we will have max/cpuinfo.max_freq set at this point (with my patch
applied) for all drivers.

> However, max_freq_req is left to the boosted frequency, so this
> is effectively an incorrect state.

Right.

> Also in cpufreq_set_policy(), policy->max is set to:
>   min(max_freq_req, cpuinfo.max_freq)
> (cf. verify() cb), so the incorrect state of max_freq_req is not
> visible.
> 
> 5. boost all CPUs again: echo 1 > /sys/devices/system/cpu/cpufreq/boost
> 
> As the max_freq_req value and the new boost value are equal,
> cpufreq_notifier_max() won't be called, which means that if the
> CPU needed to raise its freq., it won't be notified until another
> event trigger a re-evaluation of the freq. selection.

Right and max/cpuinfo.max_freq will be incorrect here. This was the issue the
commit 1608f0230510 was trying to fix.

> To observe that, I had to:
> - use the performance governor to be sure to select the max. available
>   freq.
> - observe scaling_cur_freq to see the last requested freq. for the CPU
> 
> So IMO the issue was actually fixed by:
> dd016f379ebc ("cpufreq: Introduce a more generic way to set default
> per-policy boost flag")
> which sets the correct max_freq_req value when putting back an
> inactive policy.

Right. This patch fixes the issue I think. Your patch is fine I guess, just
mention this reason and we are good to go.

-- 
viresh