Re: [PATCH v6 07/14] sched/topology: Introduce sched_energy_present static key

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

On 09/06/2018 02:29 AM, Quentin Perret wrote:
> Hi Dietmar,
> 
> On Wednesday 05 Sep 2018 at 23:06:38 (-0700), Dietmar Eggemann wrote:
>> On 08/20/2018 02:44 AM, Quentin Perret wrote:
>>> In order to ensure a minimal performance impact on non-energy-aware
>>> systems, introduce a static_key guarding the access to Energy-Aware
>>> Scheduling (EAS) code.
>>>
>>> The static key is set iff all the following conditions are met for at
>>> least one root domain:
>>>     1. all online CPUs of the root domain are covered by the Energy
>>>        Model (EM);
>>>     2. the complexity of the root domain's EM is low enough to keep
>>>        scheduling overheads low;
>>>     3. the root domain has an asymmetric CPU capacity topology (detected
>>>        by looking for the SD_ASYM_CPUCAPACITY flag in the sched_domain
>>>        hierarchy).
>>
>> This is pretty much the list (+ is schedutil running) of conditions to set
>> rd->pd != NULL in build_perf_domains().
> 
> Yes, exactly. I actually loop over the rds to check if one of them has a
> pd != NULL in order to enable/disable the static key. So the check for
> those conditions is always done at the rd level.
> 
>> So when testing 'static_branch_unlikely(&sched_energy_present) &&
>> rcu_dereference(rd->pd)' don't you test two times the same thing?
> 
> Well, not exactly. You could have two rds in your system, and only one
> of the two has an Energy Model. The static key is just a performance
> optimization for !EAS systems really. But I must admit it sort of lost a

Ah, that's correct. But the static key could be exchanged by a sched 
feature, that's the important bit here.

> bit of its interest over the versions. I mean, it's not that clear now
> that a static key is a better option than a sched_feat as you suggest
> below.
> 
>> Also, if let's say somebody wants to run another EM user (e.g. a thermal
>> governor, like IPA) but not EAS on a asymmetric CPU capacity system. This
>> can't be achieved with the current static branch approach
> 
> I assume you're talking about IPA once migrated to using the EM
> framework ? In this case, I think you're right, we won't have a single

Right, in case we will have multiple user of the EM in the future.

> tunable left to enable/disable EAS. On a big.LITTLE platform, if you
> want IPA, EAS will always be enabled by side effect ...
> 
> That's a very good point actually. I think some people will not be happy
> with that. There are big.LITTLE users (not very many of them, but still)
> who don't care that much about energy, but do about performance. And
> those guys might want to use IPA without EAS. So I guess we really need
> a new knob.

I guess so too.

>> So what about using a (disabled by default ?) sched_feature + rd->pd != NULL
>> instead?
> 
> Right, that's an option. I could remove the static key and
> sched_energy_start() altogether and replace all the
> "if (static_branch_unlikely(&sched_energy_present))" by
> "if (sched_feat(ENERGY_AWARE))" for example. That should be equivalent
> to what I did with the static key from a performance standpoint. But that
> would mean users have to manually flip switches to get EAS up and
> running ... I assume it's the price to pay for configurability.
> 
> Another option would be a KConfig option + static key. I could keep all
> of the ifdefery inside an accessor function like the following:
> 
> 	static inline bool sched_energy_aware(void)
> 	{
> 	#ifdef CONFIG_SCHED_ENERGY
> 		return static_branch_likely(&sched_energy_present);
> 	#else
> 		return false;
> 	#endif
> 	}
> 
> Now, I understand that scheduler-related KConfig options aren't welcome
> in general, so I tend to prefer the sched_feat option.
> 
> Thoughts ?

I would prefer a sched_feature. I guess it has to be disabled by default 
so that other systems don't have to check rcu_dereference(rd->pd) in the 
wakeup path.

But since at the beginning EAS will be the only user of the EM there is 
no need to change the static key sched_energy_present right now.