Re: Commit 554c8aa8ecad causing severe performance degression with pcc-cpufreq

[Andreas Herrmann <aherrmann@suse.com>]

On Tue, Jul 17, 2018 at 12:21:36PM +0200, Andreas Herrmann wrote:
> On Tue, Jul 17, 2018 at 12:09:21PM +0200, Rafael J. Wysocki wrote:

  ---8<---

> > OK, the patch is below.
> > 
> > First, I hope that if "Collaborative Power Control" is disabled, it will
> > simply hide the PCCH object and so intel_pstate will still not load then.
> 
> PCCH is hidden in that case.
> 
> > The main question basically is what the OS is expected to do if
> > "Dynamic Power Savings Mode" is set.  If we are *expected* to use
> > the PCC interface then, intel_pstate may not work in that case, but
> > I suspect that the PCC interface allows extra energy to be saved
> > over what is possible without it.
> 
> I'll test it and see what happens.

I've tested it on top of v4.18-rc5-36-g30b06abfb92b. intel_pstate now
loads instead of pcc-cpufreq and system looks stable.

When disabling "Collaborative Power Control" no cpufreq driver is loaded
(as expected).

Performance (with kernbench) is as expected (always better than any
brew of pcc-cpufreq + misc modifications to this driver + partial
rollback of commit 554c8aa8ecad).

If you like you can add either Tested-by or
Reviewed-by: Andreas Herrmann <aherrmann@suse.com>

I think this patch should be tagged for 4.17-stable.


Thanks,

Andreas

> > ---
> >  drivers/cpufreq/intel_pstate.c |   17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > Index: linux-pm/drivers/cpufreq/intel_pstate.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> > +++ linux-pm/drivers/cpufreq/intel_pstate.c
> > @@ -2391,6 +2391,18 @@ static bool __init intel_pstate_no_acpi_
> >  	return true;
> >  }
> >  
> > +static bool __init intel_pstate_no_acpi_pcch(void)
> > +{
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +
> > +	status = acpi_get_handle(NULL, "\\_SB", &handle);
> > +	if (ACPI_FAILURE(status))
> > +		return true;
> > +
> > +	return !acpi_has_method(handle, "PCCH");
> > +}
> > +
> >  static bool __init intel_pstate_has_acpi_ppc(void)
> >  {
> >  	int i;
> > @@ -2450,7 +2462,10 @@ static bool __init intel_pstate_platform
> >  
> >  	switch (plat_info[idx].data) {
> >  	case PSS:
> > -		return intel_pstate_no_acpi_pss();
> > +		if (!intel_pstate_no_acpi_pss())
> > +			return false;
> > +
> > +		return intel_pstate_no_acpi_pcch();
> >  	case PPC:
> >  		return intel_pstate_has_acpi_ppc() && !force_load;
> >  	}
> > 
> >