Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: "Doug Smythies" <dsmythies@telus.net>
To: "'Rafael J. Wysocki'" <rjw@rjwysocki.net>
Cc: "'Peter Zijlstra'" <peterz@infradead.org>,
	"'Daniel Lezcano'" <daniel.lezcano@linaro.org>,
	"'LKML'" <linux-kernel@vger.kernel.org>,
	"'Giovanni Gherdovich'" <ggherdovich@suse.cz>,
	"'Linux PM'" <linux-pm@vger.kernel.org>
Subject: RE: [PATCH v2] cpuidle: Use nanoseconds as the unit of time
Date: Sun, 10 Nov 2019 14:12:20 -0800
Message-ID: <002301d59813$ee18c920$ca4a5b60$@net> (raw)
In-Reply-To: <000b01d597f2$06403a50$12c0aef0$@net>

On 2019.11.10 10:10 Doug Smythies wrote:
> On 2019.11.10 09:24 Rafael J. Wysocki wrote:
>> On Sunday, November 10, 2019 5:48:21 PM CET Rafael J. Wysocki wrote:
>>
>> I have found a bug, which should be addressed by the patch below.
>>
>> If it still doesn't reduce the discrepancy, we'll need to look further.
>>
>> ---
>> drivers/cpuidle/governors/menu.c |    4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/cpuidle/governors/menu.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
>> +++ linux-pm/drivers/cpuidle/governors/menu.c
>> @@ -516,8 +516,8 @@ static void menu_update(struct cpuidle_d
>> 	new_factor -= new_factor / DECAY;
>> 
>> 	if (data->next_timer_ns > 0 && measured_ns < MAX_INTERESTING)
>> -		new_factor += RESOLUTION * div64_u64(measured_ns,
>> -						     data->next_timer_ns);
>> +		new_factor += div64_u64(RESOLUTION * measured_ns,
>> +					data->next_timer_ns);
>> 	else
>> 		/*
>> 		 * we were idle so long that we count it as a perfect
>
> Yes, that was the exact bit of code I focused on yesterday.
> However, my attempt to fix was different, and made no difference,
> with the new graph being exactly on top of the old bad one.
> I had defined new_factor as u64 and RESOLUTION as ULL.

Your patch does fix the problem.
I now also understand why my attempt did not.

New data added to previous graphs. For those that don't
want to go to the graphs, the nanosecond menu graphs are now
almost identical to the microsecond based one.

http://www.smythies.com/~doug/linux/idle/nano-second-conversion/sweep/index.html

Legend:
teo-base : linux-next 2019.11.07
menu-base: linux-next 2019.11.07
teo-v2   : linux-next 2019.11.07 + cpuidle: Consolidate disabled state checks + this v2
menu-v2  : linux-next 2019.11.07 + cpuidle: Consolidate disabled state checks + this v2
rjw1     : menu-v2 + above patch.

Acked-by and tested-by Doug Smythies <dsmythies@telus.net>

Disclaimer: Only teo and menu, not ladder or haltpoll governors.

Additional suggestions:

Header comments:

> microseconds provided by drivers.  In addition to that, change
> cpuidle_governor_latency_req() to return the idle state exit
> latency constraint in nanoseconds.

Suggest:

microseconds provided by drivers.  Additionally, change
cpuidle_governor_latency_req() to return the idle state exit
latency constraint in nanoseconds.

> With that, meeasure idle state residency (last_residency_ns in
             ^^^^^^^^
Suggest:

Also measure idle state residency (last_residency_ns in

Code:
Suggest deletion of this note:

/*
 * Please note when changing the tuning values:
 * If (MAX_INTERESTING-1) * RESOLUTION > UINT_MAX, the result of
 * a scaling operation multiplication may overflow on 32 bit platforms.
 * In that case, #define RESOLUTION as ULL to get 64 bit result:
 * #define RESOLUTION 1024ULL
 *
 * The default values do not overflow.
 */

Because you have managed the extra bit requirements as part of the patch.

... Doug



  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-06 10:30 [PATCH] " Rafael J. Wysocki
2019-11-07 14:25 ` [PATCH v2] " Rafael J. Wysocki
2019-11-08  1:44   ` Rafael J. Wysocki
2019-11-08  8:45     ` Doug Smythies
2019-11-08  9:45       ` Rafael J. Wysocki
2019-11-08 17:04         ` Doug Smythies
2019-11-10 16:48           ` Rafael J. Wysocki
2019-11-10 17:24             ` Rafael J. Wysocki
2019-11-10 18:09               ` Doug Smythies
2019-11-10 22:12                 ` Doug Smythies [this message]
2019-11-11 21:29                   ` Rafael J. Wysocki
2019-11-08  9:39     ` Peter Zijlstra

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='002301d59813$ee18c920$ca4a5b60$@net' \
    --to=dsmythies@telus.net \
    --cc=daniel.lezcano@linaro.org \
    --cc=ggherdovich@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    /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

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

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

Example config snippet for mirrors

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


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