Received-SPF: pass (google.com: domain of linux-kernel+bounces-178133-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 13 May 2024 23:09:03 +0100
From: Qais Yousef <qyousef@layalina.io>
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Valentin Schneider <vschneid@redhat.com>,
	Christian Loehle <christian.loehle@arm.com>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] sched: Consolidate cpufreq updates
Message-ID: <20240513220903.no2j6zl4tk7lr6um@airbuntu>
References: <20240512190018.531820-1-qyousef@layalina.io>
 <9e845146-8a31-407c-a5ee-e2e32f1655e5@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <9e845146-8a31-407c-a5ee-e2e32f1655e5@arm.com>

On 05/13/24 14:43, Dietmar Eggemann wrote:
> On 12/05/2024 21:00, Qais Yousef wrote:
> 
> [...]
> 
> > @@ -4682,7 +4659,7 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
> >  
> >  	add_tg_cfs_propagate(cfs_rq, se->avg.load_sum);
> >  
> > -	cfs_rq_util_change(cfs_rq, 0);
> > +	cpufreq_update_util(rq_of(cfs_rq), 0);
> 
> Isn't this slighlty different now?
> 
> before:
> 
>    if (&rq->cfs == cfs_rq) {
>        cpufreq_update_util(rq, ....)
>    }
> 
> now:
> 
>    cpufreq_update_util(rq_of(cfs_rq), ...)
> 
> You should get way more updates from attach/detach now.

Yes, well spotted!

Looking at the path more closely, I can see this is called from
enqueue_task_fair() path when a task migrates to new CPU. And when
attach_task_cfs_rq() which is called when we switch_to_fair(), which I already
cover in the policy change for the RUNNING task, or when
task_change_group_fair() which what I originally understood Vincent was
referring to. I moved the update to this function after the detach/attach
operations with better guards to avoid unnecessary update.

I understood this will lead to big change and better apply immediately vs
wait for the next context switch. But I'll ask the question again, can we drop
this and defer to context switch?

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 43f6244ab0f9..e791969360d1 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4659,8 +4659,6 @@ static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s

        add_tg_cfs_propagate(cfs_rq, se->avg.load_sum);

-       cpufreq_update_util(rq_of(cfs_rq), 0);
-
        trace_pelt_cfs_tp(cfs_rq);
 }

@@ -4689,8 +4687,6 @@ static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s

        add_tg_cfs_propagate(cfs_rq, -se->avg.load_sum);

-       cpufreq_update_util(rq_of(cfs_rq), 0);
-
        trace_pelt_cfs_tp(cfs_rq);
 }

@@ -12856,6 +12852,7 @@ void init_cfs_rq(struct cfs_rq *cfs_rq)
 #ifdef CONFIG_FAIR_GROUP_SCHED
 static void task_change_group_fair(struct task_struct *p)
 {
+       struct rq *rq = task_rq(p);
        /*
         * We couldn't detach or attach a forked task which
         * hasn't been woken up by wake_up_new_task().
@@ -12871,6 +12868,10 @@ static void task_change_group_fair(struct task_struct *p)
 #endif
        set_task_rq(p, task_cpu(p));
        attach_task_cfs_rq(p);
+       if (task_on_rq_queued(p) && rq->cfs.decayed) {
+               rq->cfs.decayed = false;
+               cpufreq_update_util(rq, 0);
+       }
 }

 void free_fair_sched_group(struct task_group *tg)