[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/4] mwait-idle: add 'preferred_cstates' module argument
On 28.04.2022 10:06, Roger Pau Monné wrote: > 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> Yes, I would have been planning to accept both (but probably reject mixing of both). > 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. Okay, I can certainly do that. The more that I did consider doing so already, breaking ties simply based on the "less code churn" argument. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |