Re: [PATCH 3/4] mwait-idle: add 'preferred_cstates' module argument

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2022 18:12, Roger Pau Monné wrote:
> On Wed, Apr 27, 2022 at 05:25:35PM +0200, Jan Beulich wrote:
>> On 27.04.2022 17:06, Roger Pau Monné wrote:
>>> On Wed, Apr 27, 2022 at 03:41:24PM +0200, Jan Beulich wrote:
>>>> On 27.04.2022 14:45, Roger Pau Monné wrote:
>>>>> On Tue, Apr 26, 2022 at 12:05:28PM +0200, Jan Beulich wrote:
>>>>>> --- unstable.orig/xen/arch/x86/cpu/mwait-idle.c
>>>>>> +++ unstable/xen/arch/x86/cpu/mwait-idle.c
>>>>>> @@ -82,6 +82,18 @@ boolean_param("mwait-idle", opt_mwait_id
>>>>>>  
>>>>>>  static unsigned int mwait_substates;
>>>>>>  
>>>>>> +/*
>>>>>> + * Some platforms come with mutually exclusive C-states, so that if one 
>>>>>> is
>>>>>> + * enabled, the other C-states must not be used. Example: C1 and C1E on
>>>>>> + * Sapphire Rapids platform. This parameter allows for selecting the
>>>>>> + * preferred C-states among the groups of mutually exclusive C-states - 
>>>>>> the
>>>>>> + * selected C-states will be registered, the other C-states from the 
>>>>>> mutually
>>>>>> + * exclusive group won't be registered. If the platform has no mutually
>>>>>> + * exclusive C-states, this parameter has no effect.
>>>>>> + */
>>>>>> +static unsigned int __ro_after_init preferred_states_mask;
>>>>>> +integer_param("preferred-cstates", preferred_states_mask);
>>>>>> +
>>>>>>  #define LAPIC_TIMER_ALWAYS_RELIABLE 0xFFFFFFFF
>>>>>>  /* Reliable LAPIC Timer States, bit 1 for C1 etc. Default to only C1. */
>>>>>>  static unsigned int lapic_timer_reliable_states = (1 << 1);
>>>>>> @@ -96,6 +108,7 @@ struct idle_cpu {
>>>>>>          unsigned long auto_demotion_disable_flags;
>>>>>>          bool byt_auto_demotion_disable_flag;
>>>>>>          bool disable_promotion_to_c1e;
>>>>>> +        bool enable_promotion_to_c1e;
>>>>>
>>>>> I'm confused by those fields, shouldn't we just have:
>>>>> promotion_to_c1e = true | false?
>>>>>
>>>>> As one field is the negation of the other:
>>>>> enable_promotion_to_c1e = !disable_promotion_to_c1e
>>>>>
>>>>> I know this is code from Linux, but would like to understand why two
>>>>> fields are needed.
>>>>
>>>> This really is a tristate; Linux is now changing their global variable
>>>> to an enum, but we don't have an equivalent of that global variable.
>>>
>>> So it would be: leave default, disable C1E promotion, enable C1E
>>> promotion.
>>>
>>> And Linux is leaving the {disable,enable}_promotion_to_c1e in
>>> idle_cpu?
>>
>> Iirc they only have disable_promotion_to_c1e there (as a struct field)
>> and keep it, but they convert the similarly named file-scope variable
>> to a tristate.
>>
>>> I guess there's not much we can do unless we want to diverge from
>>> upstream.
>>
>> We've diverged some from Linux here already - as said, for example we
>> don't have their file-scope variable. I could convert our struct field
>> to an enum, but that would be larger code churn for (I think) little
>> gain.
> 
> Hm, OK, could gaining the file scope variable would make sense in order
> to reduce divergences?  Or are the other roadblocks there?

I don't recall. It might have originated from a change I decided to not
port over, or I might have dropped it while porting. To be honest I'm
not keen on putting time into researching this, the more that I would
generally try to avoid static variables.

What I would be willing to put time in is making a more user friendly
command line option, but as said - I can't think of any good alternative
(except perhaps "preferred-cstates=c1e" or "cstates=preferred:c1e", with
internal translation of the strings into a bit mask, as long as (a) you
would think that's an improvement and (b) the further divergence from
Linux is not deemed a problem).

Jan

> I think this is ugly, but would make sense as long as it allows us to
> keep closer to upstream.
> 
> Thanks, Roger.
>