[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 28 Apr 2022 10:10:56 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=8wSzMA2p0/jtSzHmJbmsc7/g1XLR1wYP8xbxa9wmVh4=; b=CUW4Oh1X9Nne/4tnxmg3JpDVdtOsSQBkolJnDA5qMomRCb/Aw9pBhj7oLBJtf1F+WgMkQuGNOM49yEwJwd2xCgd756YIGgOntUQ6mk17TQWF59CICydSOBS5+GYLMGV/lCa+mXqBI6OCT1by+HMBINPmL5mCLHZjIfoYJSJC8c8SH2e/gHiEMMEELOG5MTSUHBBUL0jfCqa7Pk4Tgw2KS2cSu3pvFXSlQOIK7dagrTD2M2bH13RLv9RtX1n3dKzJlL87E9T2qMQpW56XTNOzgQb4p4No6AA+cTup/HB+6Foc4hZr+ZwP2t9PFdiiQlHrA8AmWPZBixq7FDjs5sCK+Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NsYdjTRmD0iGmaIShC3AeUa2jkR2ZvM0B/638BzpuiN5woGE2z9jvRaGRs8BqWsZ0+kZryVfxnZlDhOBDr9+AqycWY1vDDshJzn62cBKEVms/3v0sr/8PGDNhk/3hzOcEruT/pJqwBT3bAoafkaWdf2cNjXWia3e8otHGhrXwQ3Te1RPElrtHk3C9quiB2JLdaatvSC8SIb5sNgfQS/pIuurilC6NLeFvmEA2s3ggnV78Y+5MDnWxrTCzsMSzikD+zNy4e+0+lL6lSh8zZHKw3YRtJ60eNNPe45CpnZYC/PaZ5WdoW9eFpl2Rn7kZ5aQ9hskq1/sp/icsp06qUv4gQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Thu, 28 Apr 2022 08:11:05 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.