Re: [PATCH v10 1/3] cpufreq: Add mechanism for registering utilization update callbacks

[Juri Lelli]

Hi Peter,

On 22/02/16 11:52, Peter Zijlstra wrote:
> On Fri, Feb 19, 2016 at 09:28:23AM -0800, Steve Muckle wrote:
> > On 02/19/2016 08:42 AM, Srinivas Pandruvada wrote:
> > > We did experiments using util/max in intel_pstate. For some benchmarks
> > > there were regression of 4 to 5%, for some benchmarks it performed at
> > > par with getting utilization from the processor. Further optimization
> > > in the algorithm is possible and still in progress. Idea is that we can
> > > change P-State fast enough and be more reactive. Once I have good data,
> > > I will send to this list. The algorithm can be part of the cpufreq
> > > governor too.
> > 
> > There has been a lot of work in the area of scheduler-driven CPU
> > frequency selection by Linaro and ARM as well. It was posted most
> > recently a couple months ago:
> > 
> > http://thread.gmane.org/gmane.linux.power-management.general/69176
> > 
> > It was also posted as part of the energy-aware scheduling series last
> > July. There's a new RFC series forthcoming which I had hoped (and
> > failed) to post prior to my business travel this week; it should be out
> > next week. It will address the feedback received thus far along with
> > locking and other things.
> 
> Right, so I had a wee look at that again, and had a quick chat with Juri
> on IRC. So the main difference seems to be that you guys want to know
> why the utilization changed, as opposed to purely _that_ it changed.
> 
> And hence you have callbacks all over the place.
> 
> I'm not too sure I really like that too much, it bloats the code and
> somewhat obfuscates the point.
> 
> So I would really like there to be just the one callback when we
> actually compute a new number, and that is update_load_avg().
> 
> Now I think we can 'easily' propagate the information you want into
> update_load_avg() (see below), but I would like to see actual arguments
> for why you would need this.
> 

Right. The information we propagate with your patch might be all we
need, but I'll have to play with it on top of Rafael's or Steve's
changes to fully convince myself. :-)

> For one, the migration bits don't really make sense. We typically do not
> call migration code local on both cpus, typically just one, but possibly
> neither. That means you cannot actually update the relevant CPU state
> from these sites anyway.
> 

I might actually have one point regarding migrations. See below. And I'm
not sure I understand why you are saying that we can't update the
relevant CPU state on migrations; we do know src and dst cpus, don't we?

[...]

> @@ -4320,7 +4333,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>  		if (cfs_rq_throttled(cfs_rq))
>  			break;
>  
> -		update_load_avg(se, 1);
> +		update_load_avg(se, 1, LOAD_ENQUEUE + (p->on_rq & TASK_ON_RQ_MIGRATING));
>  		update_cfs_shares(cfs_rq);
>  	}
>  
> @@ -4380,7 +4393,7 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>  		if (cfs_rq_throttled(cfs_rq))
>  			break;
>  
> -		update_load_avg(se, 1);
> +		update_load_avg(se, 1, LOAD_DEQUEUE + (p->on_rq & TASK_ON_RQ_MIGRATING));
>  		update_cfs_shares(cfs_rq);
>  	}
>  

What we are trying to do with the sched-freq approach (and maybe that is
just broken :-/) is to wait until all tasks are detached from src cpu
and attached to dst cpu to trigger updates on such cpus. I fear that if
don't do that we might have problems with any sort of rate limiting for
freq transitions we might need to put in place.

Best,

- Juri