Message-ID: <20230417225252.GA6156@ranerica-svr.sc.intel.com>
Date:   Mon, 17 Apr 2023 15:52:52 -0700
From:   Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Ricardo Neri <ricardo.neri@...el.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Ben Segall <bsegall@...gle.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Len Brown <len.brown@...el.com>, Mel Gorman <mgorman@...e.de>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Valentin Schneider <vschneid@...hat.com>,
        Lukasz Luba <lukasz.luba@....com>,
        Ionela Voinescu <ionela.voinescu@....com>, x86@...nel.org,
        "Joel Fernandes (Google)" <joel@...lfernandes.org>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        "Tim C . Chen" <tim.c.chen@...el.com>
Subject: Re: [PATCH v3 07/24] sched/fair: Compute IPC class scores for load
 balancing

On Fri, Mar 31, 2023 at 02:20:11PM +0200, Vincent Guittot wrote:
> On Thu, 30 Mar 2023 at 03:56, Ricardo Neri
> <ricardo.neri-calderon@...ux.intel.com> wrote:
> >
> > On Tue, Mar 28, 2023 at 12:00:58PM +0200, Vincent Guittot wrote:
> > > On Tue, 7 Feb 2023 at 06:01, Ricardo Neri
> > > <ricardo.neri-calderon@...ux.intel.com> wrote:
> > > >
> > > > Compute the joint total (both current and prospective) IPC class score of
> > > > a scheduling group and the local scheduling group.
> > > >
> > > > These IPCC statistics are used during idle load balancing. The candidate
> > > > scheduling group will have one fewer busy CPU after load balancing. This
> > > > observation is important for cores with SMT support.
> > > >
> > > > The IPCC score of scheduling groups composed of SMT siblings needs to
> > > > consider that the siblings share CPU resources. When computing the total
> > > > IPCC score of the scheduling group, divide score of each sibling by the
> > > > number of busy siblings.
> > > >
> > > > Collect IPCC statistics for asym_packing and fully_busy scheduling groups.
> > >
> > > IPCC statistics collect scores of current tasks, so they are
> > > meaningful only when trying to migrate one of those running tasks.
> > > Using such score when pulling other tasks is just meaningless. And I
> > > don't see how you ensure such correct use of ipcc score
> >
> > Thank you very much for your feedback Vincent!
> >
> > It is true that the task that is current when collecting statistics may be
> > different from the task that is current when we are ready to pluck tasks.
> >
> > Using IPCC scores for load balancing benefits large, long-running tasks
> > the most. For these tasks, the current task is likely to remain the same
> > at the two mentioned points in time.
> 
> My point was mainly about the fact that the current running task is
> the last one to be pulled. And this happens only when no other task
> was pulled otherwise.

(Thanks again for your feedback, Vincent. I am sorry for the late reply. I
needed some more time to think about it.)

Good point! It is smarter to compare and pull from the back of the queue,
rather than comparing curr and pulling from the back. We are more likely
to break the tie correctly without being too complex.

Here is an incremental patch with the update. I'll include this change in
my next version.

@@ -9281,24 +9281,42 @@ static void init_rq_ipcc_stats(struct sg_lb_stats *sgs)
 	sgs->min_score = ULONG_MAX;
 }
 
+static int rq_last_task_ipcc(int dst_cpu, struct rq *rq, unsigned short *ipcc)
+{
+	struct list_head *tasks = &rq->cfs_tasks;
+	struct task_struct *p;
+	struct rq_flags rf;
+	int ret = -EINVAL;
+
+	rq_lock_irqsave(rq, &rf);
+	if (list_empty(tasks))
+		goto out;
+
+	p = list_last_entry(tasks, struct task_struct, se.group_node);
+	if (p->flags & PF_EXITING || is_idle_task(p) ||
+	    !cpumask_test_cpu(dst_cpu, p->cpus_ptr))
+		goto out;
+
+	ret = 0;
+	*ipcc = p->ipcc;
+out:
+	rq_unlock(rq, &rf);
+	return ret;
+}
+
 /* Called only if cpu_of(@rq) is not idle and has tasks running. */
 static void update_sg_lb_ipcc_stats(int dst_cpu, struct sg_lb_stats *sgs,
 				    struct rq *rq)
 {
-	struct task_struct *curr;
 	unsigned short ipcc;
 	unsigned long score;
 
 	if (!sched_ipcc_enabled())
 		return;
 
-	curr = rcu_dereference(rq->curr);
-	if (!curr || (curr->flags & PF_EXITING) || is_idle_task(curr) ||
-	    task_is_realtime(curr) ||
-	    !cpumask_test_cpu(dst_cpu, curr->cpus_ptr))
+	if (rq_last_task_ipcc(dst_cpu, rq, &ipcc))
 		return;
 
-	ipcc = curr->ipcc;
 	score = arch_get_ipcc_score(ipcc, cpu_of(rq));
 
> 
> >
> > My patchset proposes to use IPCC clases to break ties between otherwise
> > identical sched groups in update_sd_pick_busiest(). Its use is limited to
> > asym_packing and fully_busy types. For these types, it is likely that there
> > will not be tasks wanting to run other than current. need_active_balance()
> > will return true and we will migrate the current task.
> 
> I disagree with your assumption above, asym_packing and fully_busy
> types doesn't put any mean on the number of running tasks

Agreed. What I stated was not correct.

o> 
> >
> > You are correct, by only looking at the current tasks we risk overlooking
> > other tasks in the queue and the statistics becoming meaningless. A fully
> > correct solution would need to keep track of the the types of tasks in
> > all runqueues as they come and go. IMO, the increased complexity of such
> > approach does not justify the benefit. We give the load balancer extra
> > information to decide between otherwise identical sched groups using the
> > IPCC statistics of big tasks.
> 
> because IPCC are meaningful only when there is only 1 running task and
> during active migration, you should collect them only for such
> situation

I think that if we compute the IPCC statistics using the tasks at the back
of the runqueue, then IPCC statistics remain meaningful for nr_running >= 1.

Powered by blists - more mailing lists