LKML Archive on lore.kernel.org
 help / Atom feed
From: leo.yan@linaro.org
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [RESEND PATCH v1 1/2] cpuidle: menu: Correct the criteria for stopping tick
Date: Fri, 10 Aug 2018 16:49:06 +0800
Message-ID: <20180810084906.GD11817@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <CAJZ5v0iyGFFDO83KGcET9a+UpPCDQ=_sxZeGxuG6fL4O+hcw+w@mail.gmail.com>

On Fri, Aug 10, 2018 at 09:22:10AM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 10, 2018 at 9:13 AM,  <leo.yan@linaro.org> wrote:
> > On Thu, Aug 09, 2018 at 10:47:17PM +0200, Rafael J. Wysocki wrote:
> >> On Thu, Aug 9, 2018 at 7:20 PM, Leo Yan <leo.yan@linaro.org> wrote:
> 
> [cut]
> 
> >> And that will cause the tick to be stopped unnecessarily in certain
> >> situations, so why is this better?
> >
> > Let's see below two cases, the first one case we configure
> > TICK_USEC=1000 (1ms) and the second case we configure TICK_USEC=4000
> > (4ms).
> >
> > Let's assume we do the testing one the same platform and have two runs,
> > in the Case 1 we configure HZ=1000 so TICK_USEC=1ms, expected_interval
> > is 3ms and deepest idle state target residency is 2ms, finally the idle
> > governor will choose the deepest state and skip to calibrate to shallow
> > state caused by 'expected_interval' > TICK_USEC;
> >
> > In the Case 2 we configure HZ=250 so TICK_USE=4ms, expected_interval
> > (3ms) and deepest idle state target residency (2ms) are same with the
> > Case 1; but because expected_interval < TICK_USEC so the idle governor
> > will do calibration to select a shallower state.  If we image on one
> > platform, the deepest idle state's target residency is smaller value,
> > then it has bigger gap with TICK_USEC, the deepest idle state is harder
> > to be selected due 'expected_interval' can be easily hit the range
> > [Deepest target residency..TICK_USEC).
> >
> > This patch has no any change for Case 1 and it wants to optimize for
> > Case 2 so Case 2 has chance to stay in deepest idle state.  I
> > understand from the performance pespective, we need to avoid to stop
> > tick for shallow states; on the other hand we cannot prevent CPU run
> > into deepest idle state just only we want to keep the tick running,
> > especially the expected interval is longer than the deepest state
> > target residency.
> >
> > Case 1:
> >       Deepest idle state's target residency=2ms
> >                      |
> >                      V
> > |--------------------------------------------------------> time (ms)
> >       ^                              ^
> >       |                              |
> > TICK_USEC=1ms           expected_interval=3ms
> >
> >
> > Case 2:
> >       Deepest idle state's target residency = 2ms
> >                      |
> >                      V
> > |--------------------------------------------------------> time (ms)
> >                                      ^                  ^
> >                                      |                  |
> >                           expected_interval = 3ms   TICK_USEC = 4ms
> >
> >
> >
> >> >                 unsigned int delta_next_us = ktime_to_us(delta_next);
> >> >
> >> >                 *stop_tick = false;
> >> > --
> 
> Well, I don't quite agree with the approach here, then.
> 
> As I said in the previous reply, IMO restarting the stopped tick
> before leaving the loop in do_idle() is pointless overhead.  It is not
> necessary to do that to avoid leaving CPUs in shallow idle states for
> too long (I'll send an alternative patch to fix this issue shortly).
> 
> While you may think that pointless overhead is not a problem, I don't
> quite agree with that.

I disagree this patch will introduce any extra overhead.

Firstly, the idle loop doesn't support restarting tick even this patch
tells idle loop to restart the tick; secondly this patch is mainly to
resolve issue for the CPU cannot stay in deepest state in Case 2, as a
side effect it also can tell idle loop to restart the tick for case 3
in below, actually IMHO this makes sense to tell the idle loop to
enable the tick but idle loop can ignore this info.

Furthermore, we have another thread for the patch to always stop
tick after the the tick has been stopped in the idle loop.

So this patch is still valid.

Case 3:
      Deepest idle state's target residency = 2ms
                     |
                     V
|--------------------------------------------------------> time (ms)
      ^        `
      |         \
TICK_USEC=1ms   expected_interval = 1.5ms



  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-09 17:20 [RESEND PATCH v1 0/2] Optimization CPU idle state impacted by tick Leo Yan
2018-08-09 17:20 ` [RESEND PATCH v1 1/2] cpuidle: menu: Correct the criteria for stopping tick Leo Yan
2018-08-09 20:47   ` Rafael J. Wysocki
2018-08-10  7:13     ` leo.yan
2018-08-10  7:22       ` Rafael J. Wysocki
2018-08-10  8:49         ` leo.yan [this message]
2018-08-10  9:03           ` leo.yan
2018-08-12 11:12             ` Rafael J. Wysocki
2018-08-12 16:07               ` leo.yan
2018-08-13  8:01                 ` Rafael J. Wysocki
2018-08-13  9:58                   ` leo.yan
2018-08-09 17:20 ` [RESEND PATCH v1 2/2] cpuidle: menu: Dismiss tick impaction on correction factors Leo Yan
2018-08-09 21:06   ` Rafael J. Wysocki

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180810084906.GD11817@leoy-ThinkPad-X240s \
    --to=leo.yan@linaro.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=rafael@kernel.org \
    --cc=vincent.guittot@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox