Re: [PATCH 5/6] sched/fair: Get rid of scaling utilization by capacity_orig

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Sep 08, 2015 at 05:53:31PM +0100, Morten Rasmussen wrote:
> On Tue, Sep 08, 2015 at 03:31:58PM +0100, Morten Rasmussen wrote:

> > On Tue, Sep 08, 2015 at 02:52:05PM +0200, Peter Zijlstra wrote:
> > But if we apply the scaling to the weight instead of time, we would only
> > have to apply it once and not three times like it is now? So maybe we
> > can end up with almost the same number of multiplications.
> > 
> > We might be loosing bits for low priority task running on cpus at a low
> > frequency though.
> 
> Something like the below. We should be saving one multiplication.

> @@ -2577,8 +2575,13 @@ __update_load_avg(u64 now, int cpu, struct sched_avg *sa,
>  		return 0;
>  	sa->last_update_time = now;
>  
> -	scale_freq = arch_scale_freq_capacity(NULL, cpu);
> -	scale_cpu = arch_scale_cpu_capacity(NULL, cpu);
> +	if (weight || running)
> +		scale_freq = arch_scale_freq_capacity(NULL, cpu);
> +	if (weight)
> +		scaled_weight = weight * scale_freq >> SCHED_CAPACITY_SHIFT;
> +	if (running)
> +		scale_freq_cpu = scale_freq * arch_scale_cpu_capacity(NULL, cpu)
> +							>> SCHED_CAPACITY_SHIFT;
>  
>  	/* delta_w is the amount already accumulated against our next period */
>  	delta_w = sa->period_contrib;
> @@ -2594,16 +2597,15 @@ __update_load_avg(u64 now, int cpu, struct sched_avg *sa,
>  		 * period and accrue it.
>  		 */
>  		delta_w = 1024 - delta_w;
> -		scaled_delta_w = cap_scale(delta_w, scale_freq);
>  		if (weight) {
> -			sa->load_sum += weight * scaled_delta_w;
> +			sa->load_sum += scaled_weight * delta_w;
>  			if (cfs_rq) {
>  				cfs_rq->runnable_load_sum +=
> -						weight * scaled_delta_w;
> +						scaled_weight * delta_w;
>  			}
>  		}
>  		if (running)
> -			sa->util_sum += scaled_delta_w * scale_cpu;
> +			sa->util_sum += delta_w * scale_freq_cpu;
>  
>  		delta -= delta_w;
>  

Sadly that makes the code worse; I get 14 mul instructions where
previously I had 11.

What happens is that GCC gets confused and cannot constant propagate the
new variables, so what used to be shifts now end up being actual
multiplications.

With this, I get back to 11. Can you see what happens on ARM where you
have both functions defined to non constants?

---
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2551,10 +2551,10 @@ static __always_inline int
 __update_load_avg(u64 now, int cpu, struct sched_avg *sa,
 		  unsigned long weight, int running, struct cfs_rq *cfs_rq)
 {
+	unsigned long scaled_weight, scale_freq, scale_freq_cpu;
+	unsigned int delta_w, decayed = 0;
 	u64 delta, periods;
 	u32 contrib;
-	unsigned int delta_w, decayed = 0;
-	unsigned long scaled_weight = 0, scale_freq, scale_freq_cpu = 0;
 
 	delta = now - sa->last_update_time;
 	/*
@@ -2575,13 +2575,10 @@ __update_load_avg(u64 now, int cpu, stru
 		return 0;
 	sa->last_update_time = now;
 
-	if (weight || running)
-		scale_freq = arch_scale_freq_capacity(NULL, cpu);
-	if (weight)
-		scaled_weight = weight * scale_freq >> SCHED_CAPACITY_SHIFT;
-	if (running)
-		scale_freq_cpu = scale_freq * arch_scale_cpu_capacity(NULL, cpu)
-							>> SCHED_CAPACITY_SHIFT;
+	scale_freq = arch_scale_freq_capacity(NULL, cpu);
+
+	scaled_weight = weight * scale_freq >> SCHED_CAPACITY_SHIFT;
+	scale_freq_cpu = scale_freq * arch_scale_cpu_capacity(NULL, cpu) >> SCHED_CAPACITY_SHIFT;
 
 	/* delta_w is the amount already accumulated against our next period */
 	delta_w = sa->period_contrib;