Re: [PATCH 2/2 V7] intel_pstate: add kernel parameter to force loading on Sun X86 servers.

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Thursday, December 04, 2014 06:03:05 PM Linda Knippers wrote:
> On 12/4/2014 5:38 PM, Kristen Carlson Accardi wrote:
> > On Thu, 04 Dec 2014 23:10:58 +0100
> > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > 
> >> On Thursday, December 04, 2014 11:07:31 AM Ethan Zhao wrote:
> >>> To force loading on Oracle Sun X86 servers, provide one kernel command line
> >>> parameter
> >>>
> >>>   intel_pstate = ora_force
> >>
> >> I would suggest to change the name of the option to "oracle_force" or "sun_force"
> >> for clarity.
> >>
> >> Anyway, I need an ACK from Kristen if this patch is to be applied.
> >>
> >>> For those who be aware of the risk of no power capping capabily working and
> >>> try to get better performance with this driver.
> >>>
> >>> Signed-off-by: Ethan Zhao <ethan.zhao@oracle.com>
> >>> ---
> >>>  v2: change to hardware vendor specific naming parameter.
> >>>  v4: refine code and doc.
> >>>  v5&v6: fix a typo in doc.
> >>>  v7: change enum PCC to PPC.
> >>>
> >>>  Documentation/kernel-parameters.txt | 5 +++++
> >>>  drivers/cpufreq/intel_pstate.c      | 6 +++++-
> >>>  2 files changed, 10 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> >>> index 479f332..7d0983e 100644
> >>> --- a/Documentation/kernel-parameters.txt
> >>> +++ b/Documentation/kernel-parameters.txt
> >>> @@ -1446,6 +1446,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >>>  		       disable
> >>>  		         Do not enable intel_pstate as the default
> >>>  		         scaling driver for the supported processors
> >>> +		       ora_force
> >>> +			 Force loading intel_pstate on Oracle Sun Servers(X86).
> >>> +			 only for those who be aware of the risk of no power capping
> >>> +			 capability working and try to get better performance with this
> >>> +			 driver.
> >>
> >> That is not sufficiently clear.  What does "risk of no power capping capability
> >> working" mean, in particular?
> >>
> >>>  
> >>>  	intremap=	[X86-64, Intel-IOMMU]
> >>>  			on	enable Interrupt Remapping (default)
> >>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >>> index 1bb62ca..2654e13 100644
> >>> --- a/drivers/cpufreq/intel_pstate.c
> >>> +++ b/drivers/cpufreq/intel_pstate.c
> >>> @@ -866,6 +866,7 @@ static struct cpufreq_driver intel_pstate_driver = {
> >>>  };
> >>>  
> >>>  static int __initdata no_load;
> >>> +static unsigned int  ora_force;
> >>>  
> >>>  static int intel_pstate_msrs_not_valid(void)
> >>>  {
> >>> @@ -1003,7 +1004,8 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
> >>>  			case PSS:
> >>>  				return intel_pstate_no_acpi_pss();
> >>>  			case PPC:
> >>> -				return intel_pstate_has_acpi_ppc();
> >>> +				return intel_pstate_has_acpi_ppc() &&
> >>> +					(!ora_force);
> >>>  			}
> >>>  	}
> >>>  
> >>> @@ -1078,6 +1080,8 @@ static int __init intel_pstate_setup(char *str)
> >>>  
> >>>  	if (!strcmp(str, "disable"))
> >>>  		no_load = 1;
> >>> +	if (!strcmp(str, "ora_force"))
> >>> +		ora_force = 1;
> >>>  	return 0;
> >>>  }
> >>>  early_param("intel_pstate", intel_pstate_setup);
> >>
> >> And can anyone please remind me what was wrong with a "force" option that would
> >> work for everyone, not just Oracle/Sun?
> >>
> > 
> > That was my suggestion as well (i.e. a parameter to bypass the vendor
> > checks), but Linda didn't like it.  My personal opinion is that unless
> > it's generic, I don't really feel like having a force option solely for
> > oracle.  I'm not convinced you want this for production machines, and I
> > think for debug purposes I don't want a vendor specific param.
> 
> I'd be happy with it if it somehow disabled what the platform is doing,
> but it doesn't.  I don't see the point of forcing intel_pstate if you
> can't force the platform to stop doing power management at the same time.
> Even if it's for test/debug purposes, I'm not sure what you're testing
> when you have dueling power management.
> 
> The description would need to be different too since I think on
> ProLiant, power capping can happen at any time, even if the
> system is in OS control mode and the intel_pstate driver is
> loaded.
> 
> Can anyone suggest a description for a force option that would
> make sense generically?

What about:

	force
		Enable intel_pstate on systems where it may cause problems to
		happen due to conflicts with platform firmware attempting to
		drive P-states by itself in certain situations (for thermal
		control or power capping in general or other purposes).

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.