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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 28, 2022 at 08:37:58AM +0200, Jan Beulich wrote:
> 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).

I think (b) won't be a problem as long as internally the user option
is translated into a bitmask.

Regarding (a) I do think it would be helpful to express this in a more
user friendly way, I'm not sure whether it would make sense to keep
Linux format also for compatibility reasons if users already have a
bitmask and want to use the same parameter for Xen and Linux, ie:

preferred-cstates = <string of c1e,c1,...> | <integer bitmask>

What I think we should fix is the naming of the two booleans:

bool disable_promotion_to_c1e;
bool enable_promotion_to_c1e;

I would rather translated this into an enum, as right now it's
confusing IMO.

Thanks, Roger.