linux-kernel - Re: [LKP] [cpuidle] 259231a045: will-it-scale.per_process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200106155212.GB10439@shbuild999.sh.intel.com>
Date:   Mon, 6 Jan 2020 23:52:12 +0800
From:   Feng Tang <feng.tang@...el.com>
To:     Marcelo Tosatti <mtosatti@...hat.com>
Cc:     kernel test robot <rong.a.chen@...el.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>, lkp@...org,
        LKML <linux-kernel@...r.kernel.org>, ying.huang@...el.com
Subject: Re: [LKP] [cpuidle] 259231a045: will-it-scale.per_process_ops -12.6%
 regression

Hi Marcelo,

On Fri, Jan 03, 2020 at 10:36:14AM -0300, Marcelo Tosatti wrote:
> Hi Feng,
> 
> > Anyway, I found commit 259231a04 lost one "break" when moving
> > the original code, thus the semantics is changed to the last
> > enabled state's target_residency instead of the first enabled
> > one's.
> > 
> > I don't know if it's intentional, and I guess no, so here 
> > is a fix patch, please review, thanks
> 
> Not intentional.
> 
> > But even with this patch, the regression is still not recovered.
> > 
> > - Feng
> 
> This has been fixed upstream already, should be on Rafael's GIT tree.

Glad to hear that.

> 
> > >From cddd6b409e18ce97a8d7b851db4400396f71d857 Mon Sep 17 00:00:00 2001
> > From: Feng Tang <feng.tang@...el.com>
> > Date: Thu, 2 Jan 2020 16:58:31 +0800
> > Subject: [PATCH] cpuidle: Add back the lost break in cpuidle_poll_time
> > 
> > Commit c4cbb8b649b5 move the poll time calculation into a
> > new function cpuidle_poll_time(), during which one "break"
> > get lost, and the semantic is changed from the last enabled
> > state's target_residency instead of the first enabled one's.
> > 
> > So add it back.
> > 
> > Fixes: c4cbb8b649b5 "cpuidle: add poll_limit_ns to cpuidle_device structure"
> > Signed-off-by: Feng Tang <feng.tang@...el.com>
> > Cc: Marcelo Tosatti <mtosatti@...hat.com>
> > ---
> >  drivers/cpuidle/cpuidle.c | 1 +
> >  1 file changed, 1 insertion(+)
> 
> About the regression... if you only revert the 
> 
> drivers/cpuidle/poll_state.c
> 
> changes from 
> 
> 259231a045616c4101d023a8f4dcc8379af265a6
> 
> Is the performance regression gone?

Aha, I did tried a similar patch, which can NOT cure the regression.

(You can check if the below patch complies with your idea) 

Another thing I've tried is to move the "poll_limit_ns" to the end of
struct cpuidle_device which could reduce the regression from 12.6%
to about 7.5%

Thanks,
Feng

commit 18260c38f4a802592e1cb6e82eb71ddca7709def
Author: Feng Tang <feng.tang@...el.com>
Date:   Fri Jan 3 09:21:54 2020 +0800

    make the poll_time inline
    
    Signed-off-by: Feng Tang <feng.tang@...el.com>

diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
index c8fa5f4..e278d9e 100644
--- a/drivers/cpuidle/poll_state.c
+++ b/drivers/cpuidle/poll_state.c
@@ -22,7 +22,24 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
 		unsigned int loop_count = 0;
 		u64 limit;
 
-		limit = cpuidle_poll_time(drv, dev);
+		
+		if (likely(dev->poll_limit_ns))
+			limit = dev->poll_limit_ns;
+		else {
+			int i;
+
+			limit = TICK_NSEC;
+			for (i = 1; i < drv->state_count; i++) {
+				if (drv->states[i].disabled || dev->states_usage[i].disable)
+					continue;
+
+				limit = (u64)drv->states[i].target_residency * NSEC_PER_USEC;
+				break;
+			}
+
+			dev->poll_limit_ns = limit;
+		}
+
 
 		while (!need_resched()) {
 			cpu_relax();