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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 28 Apr 2022 10:06:26 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=aX9pNVCJbjAiUqScrtBfeRLTF+irJiIUFBKoTGvuesw=; b=OOPUhqrBPI6KYaqoIpgVwBkPzJT4wH6n3c5sC14lcxhCjLzFhSEr8OV7H2f9E0TewHhRll5YXpo61h0H7pU6RCQXzgNnT4KeoeFS6Y0c2T5Ctx2kh6MUdflcbXthEjJU2hNROK6yJNqXfBpSq4kjhzmCtOc/CyYFnT7llLp9b/InGGoNl97v28h0DZ3sD/gUUO7c2QsC9C/EYcbzn7Ryu8NAG8lm8V9vjmJdZ5Mlx7MEBzImD4yiN16KvHm4HMpQmyRu4EeUFlB31N6pkr5/hXJSgDT8t68LkqpBL2WvZmw7kwvivcO12PSYbrYaJi4t+2X0dfJMSIIBuRZ6ExvoLA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JWsfZAnvZwdFAF/+tRbvucOqG1DSQYT9cHROAJe4h2uuwWbSMcHU8S7/9YqsY7cMjf3p4ydMYZHsvKWLBkLG6uBRvHTWFdj2q8jmFUjZWEfa7dZhOr/UqpA88shNVqiXClN+NpftDDHGgI/Oi3gYXZg4MD2mont3VTCfwk64BF2zoEAaX8B9TqOEukSCtR2mDgr7pYuGLuV1DUvv0X6LWgzeaxrQAXeUCsrImNixEicYWbGvSMOfd1/+kYsnkoa1F4tUbfapRxOlDE6XUtFhyGP9WaG+FB7EWjOIL823Bjf0ZHgVivMuMTwjLGC0q+fORgt8RJc9QPGegzBKyzgJog==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Thu, 28 Apr 2022 08:06:43 +0000
  • Ironport-data: A9a23:/QVRYaALygs1wxVW/13iw5YqxClBgxIJ4kV8jS/XYbTApDIj3zcGz TYeWGiCPKyJMTPzKIglPtu09RxVuMfWzdI1QQY4rX1jcSlH+JHPbTi7wuYcHM8wwunrFh8PA xA2M4GYRCwMZiaA4E/raNANlFEkvU2ybuOU5NXsZ2YgHWeIdA970Ug5w7Jj0tYx6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPhxz I8clYGSaTwQP5DKpu8xdgdzTCZhaPguFL/veRBTsOS15mifKT7A5qsrC0s7e4oF5uxwHGdCs +QCLywAZQyCgOTwx6+nTu5rhYIoK8yD0IE34yk8i22GS6t3B8mcGs0m5vcBtNs0rtpJEvvEI dIQdBJkbQjaYg0JMVASYH47tLjw3yCjLGMCwL6TjaUdwm3i9RN96b3WHsDJX93NVZ5fj0nN8 woq+Ey8WHn2Lue3yzCI73atje/nhj7gVcQZE7jQ3u5nhhify3IeDDUSVECnur+ph0imQdVdJ kcIvC00osAa60iDXtT7GRqirxa5UgU0XtNRF6g27V+Lw6+NuQKBXDFbF3hGdcAss9IwSXoyz FiVktj1BDtp9rqIVXaa8bTSpjS3UcQIEVI/ieY/ZVNty7HeTEsb0nojkv4L/HaJs+DI
  • Ironport-hdrordr: A9a23:b3ATOKDjt2d+S97lHeg+sceALOsnbusQ8zAXPh9KJCC9I/bzqy nxpp8mPH/P5wr5lktQ/OxoHJPwOU80kqQFmrX5XI3SJTUO3VHFEGgM1+vfKlHbak7DH6tmpN 1dmstFeaLN5DpB/KHHCWCDer5PoeVvsprY49s2p00dMT2CAJsQizuRZDzrcHGfE2J9dOcE/d enl4N6jgvlXU5SQtWwB3EDUeSGj9rXlKj+aRpDIxI88gGBgR6h9ba/SnGjr1wjegIK5Y1n3X nOkgT/6Knmm/anyiXE32uWy5hNgtPuxvZKGcTJoMkILTfHjBquee1aKvW/lQFwhNvqxEchkd HKrRtlF8Nv60nJdmXwmhfp0xmI6kdb11bSjXujxVfzq83wQzw3T+Bbg5hCTxff4008+Plhza NixQuixtVqJCKFuB64y8nDVhlsmEbxi2Eli/Qvg3tWVpZbQKNNrLYY4FheHP47bW7HAbgcYa hT5fznlbZrmQvwVQGbgoAv+q3gYp0LJGbJfqBY0fblkQS/nxhCvj4lLYIk7zI9HakGOuh5Dt T/Q9pVfY51P78rhIJGdZA8qJiMexrwqSylChPgHX3XUIc6Blnql7nbpJ0I2cDCQu178HJ1ou WKbG9l
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

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