[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87mso0ym8q.fsf@oracle.com>
Date: Tue, 04 Jun 2024 22:47:17 -0700
From: Ankur Arora <ankur.a.arora@...cle.com>
To: Joao Martins <joao.m.martins@...cle.com>
Cc: Ankur Arora <ankur.a.arora@...cle.com>, catalin.marinas@....com,
will@...nel.org, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
x86@...nel.org, hpa@...or.com, pbonzini@...hat.com,
wanpengli@...cent.com, vkuznets@...hat.com, rafael@...nel.org,
daniel.lezcano@...aro.org, peterz@...radead.org, arnd@...db.de,
lenb@...nel.org, mark.rutland@....com, harisokn@...zon.com,
boris.ostrovsky@...cle.com, konrad.wilk@...cle.com,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
kvm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 4/9] cpuidle-haltpoll: define arch_haltpoll_supported()
Joao Martins <joao.m.martins@...cle.com> writes:
> On 30/04/2024 19:37, Ankur Arora wrote:
>> From: Joao Martins <joao.m.martins@...cle.com>
>>
>> Right now kvm_para_has_hint(KVM_HINTS_REALTIME) is x86 only. In
>> pursuit of making cpuidle-haltpoll architecture independent, define
>> arch_haltpoll_supported() which handles the architectural check for
>> enabling haltpoll.
>>
>> Move the (kvm_para_available() && kvm_para_has_hint(KVM_HINTS_REALTIME))
>> check to the x86 specific arch_haltpoll_supported().
>>
>> Signed-off-by: Joao Martins <joao.m.martins@...cle.com>
>> Signed-off-by: Mihai Carabas <mihai.carabas@...cle.com>
>> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.com>
>>
>> ---
>> Changelog:
>>
>> - s/arch_haltpoll_want/arch_haltpoll_supported/
>
>
> I am not sure it's correct to call supported() considering that it's supposed to
> always supported (via WFE or cpu_relax()) and it's not exactly what it is doing.
> The function you were changing is more about whether it's default enabled or
> not. So I think the old name in v4 is more appropriate i.e. arch_haltpoll_want()
>
> Alternatively you could have it called arch_haltpoll_default_enabled() though
> it's longer/verbose.
So, I thought about it some and the driver loading decision tree
should be:
1. bail out based on the value of boot_option_idle_override.
2. if arch_haltpoll_supported(), enable haltpoll
3. if cpuidle-haltpoll.force=1, enable haltpoll,
Note: in the posted versions, cpuidle-haltpoll.force is allowed to
override boot_option_idle_override, which is wrong. With that fixed
the x86 check should be:
bool arch_haltpoll_supported(void)
{
return kvm_para_available() && kvm_para_has_hint(KVM_HINTS_REALTIME);
}
and arm64:
static inline bool arch_haltpoll_supported(void)
{
/*
* Ensure the event stream is available to provide a terminating
* condition to the WFE in the poll loop.
*/
return arch_timer_evtstrm_available();
}
Now, both of these fit reasonably well with arch_haltpoll_supported().
My personal preference for that is because it seems to me that the
architecture code should just deal with mechanism and not policy.
However, as you imply arch_haltpoll_supported() is a more loaded name
and given that the KVM side of arm64 haltpoll is not done yet, it's
best to have a more neutral label like arch_haltpoll_want() or
arch_haltpoll_do_enable().
> Though if you want a true supported() arch helper *I think* you need to make a
> bigger change into introducing arch_haltpoll_supported() separate from
> arch_haltpoll_want() where the former would ignore the .force=y modparam and
> never be able to load if a given feature wasn't present e.g. prevent arm64
> haltpoll loading be conditioned to arch_timer_evtstrm_available() being present.
>
> Though I don't think that you want this AIUI
Yeah I don't. I think the cpuidle-haltpoll.force=1, should be allowed to
override arch_haltpoll_supported(), so long as smp_cond_load_relaxed()
is well defined (as it is here).
It shouldn't, however, override the user's choice of boot_option_idle_override.
--
ankur
Powered by blists - more mailing lists