Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751975AbdGEEuT (ORCPT ); Wed, 5 Jul 2017 00:50:19 -0400 Received: from mail-pf0-f178.google.com ([209.85.192.178]:34826 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750885AbdGEEuR (ORCPT ); Wed, 5 Jul 2017 00:50:17 -0400 Date: Wed, 5 Jul 2017 10:20:12 +0530 From: Viresh Kumar To: Patrick Bellasi Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Ingo Molnar , Peter Zijlstra , "Rafael J . Wysocki" , Vincent Guittot , Juri Lelli , Joel Fernandes , Andres Oportus , Todd Kjos , Morten Rasmussen , Dietmar Eggemann Subject: Re: [PATCH v2 2/6] cpufreq: schedutil: reset sg_cpus's flags at IDLE enter Message-ID: <20170705045012.GM3532@vireshk-i7> References: <1499189651-18797-1-git-send-email-patrick.bellasi@arm.com> <1499189651-18797-3-git-send-email-patrick.bellasi@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1499189651-18797-3-git-send-email-patrick.bellasi@arm.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2434 Lines: 68 On 04-07-17, 18:34, Patrick Bellasi wrote: > diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h > index d2be2cc..36ac8d2 100644 > --- a/include/linux/sched/cpufreq.h > +++ b/include/linux/sched/cpufreq.h > @@ -10,6 +10,7 @@ > #define SCHED_CPUFREQ_RT (1U << 0) > #define SCHED_CPUFREQ_DL (1U << 1) > #define SCHED_CPUFREQ_IOWAIT (1U << 2) > +#define SCHED_CPUFREQ_IDLE (1U << 3) > > #define SCHED_CPUFREQ_RT_DL (SCHED_CPUFREQ_RT | SCHED_CPUFREQ_DL) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index eaba6d6..004ae18 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -304,6 +304,12 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > > sg_cpu->util = util; > sg_cpu->max = max; > + > + /* CPU is entering IDLE, reset flags without triggering an update */ > + if (unlikely(flags & SCHED_CPUFREQ_IDLE)) { > + sg_cpu->flags = 0; > + goto done; > + } Why is it important to have the above diff at all ? For example we aren't doing similar stuff in sugov_update_single() and that will go on and try to change the frequency if rate_limit_us time is over since last update. And also why is it important to write 0 to sg_cpu->flags ? What wouldn't work if we set sg_cpu->flags to SCHED_CPUFREQ_IDLE in this case ? i.e. Just the below statement should be good for us. > sg_cpu->flags = flags; > > sugov_set_iowait_boost(sg_cpu, time, flags); > @@ -318,6 +324,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, > sugov_update_commit(sg_policy, time, next_f); > } > > +done: > raw_spin_unlock(&sg_policy->update_lock); > } > > diff --git a/kernel/sched/idle_task.c b/kernel/sched/idle_task.c > index 0c00172..a844c91 100644 > --- a/kernel/sched/idle_task.c > +++ b/kernel/sched/idle_task.c > @@ -29,6 +29,10 @@ pick_next_task_idle(struct rq *rq, struct task_struct *prev, struct rq_flags *rf > put_prev_task(rq, prev); > update_idle_core(rq); > schedstat_inc(rq->sched_goidle); > + > + /* kick cpufreq (see the comment in kernel/sched/sched.h). */ > + cpufreq_update_this_cpu(rq, SCHED_CPUFREQ_IDLE); > + This looks correct. Can we completely avoid the utilization contribution of the CPUs which have gone idle? Right now we avoid them with help of (delta_ns > TICK_NSEC). Can we instead check this SCHED_CPUFREQ_IDLE flag ? -- viresh