linux-kernel - Re: [PATCH] sched/fair: Fix task utilization accountability in cpu_util

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YDPUwKKYgZfzzCJm@google.com>
Date:   Mon, 22 Feb 2021 15:58:56 +0000
From:   Quentin Perret <qperret@...gle.com>
To:     Vincent Donnefort <vincent.donnefort@....com>
Cc:     peterz@...radead.org, mingo@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, linux-kernel@...r.kernel.org,
        patrick.bellasi@...bug.net, valentin.schneider@....com
Subject: Re: [PATCH] sched/fair: Fix task utilization accountability in
 cpu_util_next()

On Monday 22 Feb 2021 at 15:01:51 (+0000), Vincent Donnefort wrote:
> You mean that it could lead to a wrong frequency estimation when doing
> freq = map_util_freq() in em_cpu_energy()?

I'm not too worried about the map_util_freq() part, I'm worried about
the schedutil aggregation. Specifically, when a task is enqueued on a
rq, we sum its util_avg to the rq's util_avg, and the _task_util_est()
to the rq's util_est.enqueue member (as per util_est_enqueue()).

Now, in schedutil, sugov_get_util() calls cpu_util_cfs(), which does the
following:

	static inline unsigned long cpu_util_cfs(struct rq *rq)
	{
		unsigned long util = READ_ONCE(rq->cfs.avg.util_avg);

		if (sched_feat(UTIL_EST)) {
			util = max_t(unsigned long, util,
				     READ_ONCE(rq->cfs.avg.util_est.enqueued));
		}

		return util;
	}

And that value will be the base for frequency selection. cpu_util_next()
tries to mimic this as accurately as possible, by doing the sums
independently, and then computing the max, exactly as we will do when
the task is enqueued and a freq update is generated.

> But in any case, the computed energy, being the product of sum_util with the
> OPP's cost, it is directly affected by this util_avg/util_est difference.

Sure, but we're not going to fix it by messing up the OPP selection part ;-)

> In the case where the task placement doesn't change the OPP, which is often the
> case, we can simplify the comparison and end-up with the following:
> 
>   delta_energy(CPU-3): OPP3 cost * (cpu_util_avg + task_util_avg - cpu_util_avg)
>   delta_energy(CPU-2): OPP2 cost * (cpu_util_est + task_util_est - cpu_util_est)
> 
>   => OPP3 cost * task_util_avg < task_util_est * OPP2 cost
> 
> With the same example I described previously, if you add the scaled OPP cost of
> 0.76 for CPU-3 and 0.65 for CPU-2 (real life OPP scaled costs), we have:
> 
>   2.3 (CPU-3) < 7.15 (CPU-2)
> 
> The task is placed on CPU-3, while it would have been much more efficient to use
> CPU-2.

That should really be a transient state: having a util_avg much larger
than util_est.enqueued is indicative of either a new task or a workload
changing behaviour. And so, chances are all the estimations are wrong
anyways -- it's hard to do good estimations when the present doesn't
look like the recent past.

But in any case, if we're going to address this, I'm still not sure this
patch will be what we want. As per my first comment we need to keep the
frequency estimation right.

> > > When computing the energy
> > > deltas, pd0's is likely to be higher than pd1's, only because the task
> > > contribution is higher for one comparison than the other.
> > 
> > You mean the contribution to sum_util right? I think I see what you mean
> > but I'm still not sure if this really is an issue. This is how util_est
> > works, and the EM stuff is just consistent with that.
> > 
> > The issue you describe can only happen (I think) when a rq's util_avg is
> > larger than its util-est emwa by some margin (that has to do with the
> > ewma-util_avg delta for the task?). But that means the ewma is not to be
> > trusted to begin with, so ...
> 
> cfs_rq->avg.util_est.ewma is not used. cpu_util() will only return the max
> between ue.enqueued and util_avg.

Right, my bad, it was the 'enqueued' member. But the rest of the
argument is still valid I think, but with s/ewma/enqueued :-)

Thanks,
Quentin