Date: Fri, 14 Jul 2017 18:06:48 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Andi Kleen <ak@linux.intel.com>
Cc: "Li, Aubrey" <aubrey.li@linux.intel.com>,
        Frederic Weisbecker <fweisbec@gmail.com>,
        Christoph Lameter <cl@linux.com>, Aubrey Li <aubrey.li@intel.com>,
        tglx@linutronix.de, len.brown@intel.com, rjw@rjwysocki.net,
        tim.c.chen@linux.intel.com, arjan@linux.intel.com,
        paulmck@linux.vnet.ibm.com, yang.zhang.wz@gmail.com, x86@kernel.org,
        linux-kernel@vger.kernel.org, daniel.lezcano@linaro.org
Subject: Re: [RFC PATCH v1 00/11] Create fast idle path for short idle periods
Message-ID: <20170714160648.tg2u6eo2id6gmnjz@hirez.programming.kicks-ass.net>
References: <20170712083410.ualmvnvzoohyami5@hirez.programming.kicks-ass.net>
 <20170712213240.GE3441@tassilo.jf.intel.com>
 <20170713083649.febfflfl5hafkko5@hirez.programming.kicks-ass.net>
 <16e12e23-6b28-f174-7c4b-4d719225cd3b@linux.intel.com>
 <20170713145311.z4zxlyd2dospeoqg@hirez.programming.kicks-ass.net>
 <4a577bd6-20b1-abb6-2153-f9870f0a721e@linux.intel.com>
 <20170713182820.sn3fjitnd3mca27p@hirez.programming.kicks-ass.net>
 <31170ac6-9db1-f0b8-4841-f1661c8ed6e1@linux.intel.com>
 <20170714153818.pjauqxebxyhs6ljp@hirez.programming.kicks-ass.net>
 <20170714155356.GH3441@tassilo.jf.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170714155356.GH3441@tassilo.jf.intel.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1730
Lines: 45

On Fri, Jul 14, 2017 at 08:53:56AM -0700, Andi Kleen wrote:
> > > -	if (cpuidle_not_available(drv, dev)) {
> > > +	if (cpuidle_not_available(drv, dev) || this_is_a_fast_idle) {
> > > 		default_idle_call();
> > > 		goto exit_idle;
> > > 	}
> > 
> > No, that's wrong. We want to fix the normal C state selection process to
> > pick the right C state.
> > 
> > The fast-idle criteria could cut off a whole bunch of available C
> > states. We need to understand why our current C state pick is wrong and
> > amend the algorithm to do better. Not just bolt something on the side.
> 
> Fast idle uses the same predictor as the current C state governor.
> 
> The only difference is that it uses a different threshold for C1.
> Likely that's the cause. If it was using the same threshold the
> decision would be the same.

Right, so its selecting C1 for longer. That in turn means we could now
never select C2; because the fast-idle threshold is longer than our C2
time.

Which I feel is wrong; because if we're getting C1 wrong, what says
we're then getting the rest right.

> The thresholds are coming either from the tables in intel idle,
> or from ACPI (let's assume the first)
> 
> That means either: the intel idle C1 threshold on the system Aubrey
> tested on is too high, or the fast idle threshold is too low.

Or our predictor is doing it wrong. It could be its over-estimating idle
duration. For example, suppose we have an idle distribution of:

	40% < C1
	60% > C2

And we end up selecting C2. Even though in many of our sleeps we really
wanted C1.

And as said; Daniel has been working on a better predictor -- now he's
probably not used it on the network workload you're looking at, so that
might be something to consider.