On 28/10/2020 19:41, Yun Hsiang wrote:
> Hi Patrick,
>
> On Wed, Oct 28, 2020 at 11:11:07AM +0100, Patrick Bellasi wrote:
[...]
>> On Tue, Oct 27, 2020 at 16:58:13 +0100, Yun Hsiang <hsiang023...@gmail.com>
>> wrote...
>>
>>> Hi Diet mar,
>>> On Mon, Oct 26, 2020 at 08:00:48PM +0100, Dietmar Eggemann wrote:
>>>> On 26/10/2020 16:45, Yun Hsiang wrote:
[...]
>>>> From: Dietmar Eggemann <dietmar.eggem...@arm.com>
[...]
>>>> +static bool uclamp_reset(enum uclamp_id clamp_id, unsigned long flags)
>>>> +{
>>
>> Maybe we can add in some comments?
>>
> I'll add these comment.
Yeah, why not.
>> /* No _UCLAMP_RESET flag set: do not reset */
>>>> + if (!(flags & SCHED_FLAG_UTIL_CLAMP_RESET))
>>>> + return false;
>>>> +
>>
>> /* Only _UCLAMP_RESET flag set: reset both clamps */
>>>> + if (!(flags & (SCHED_FLAG_UTIL_CLAMP_MIN | SCHED_FLAG_UTIL_CLAMP_MAX)))
>>>> + return true;
>>>> +
>> /* Both _UCLAMP_RESET and _UCLAMP_MIN flags are set: reset only min
>> */
>>>> + if ((flags & SCHED_FLAG_UTIL_CLAMP_MIN) && clamp_id == UCLAMP_MIN)
>>>> + return true;
>>>> +
>>
>> /* Both _UCLAMP_RESET and _UCLAMP_MAX flags are set: reset only max
>> */
>>>> + if ((flags & SCHED_FLAG_UTIL_CLAMP_MAX) && clamp_id == UCLAMP_MAX)
>>>> + return true;
>>
>> Since the evaluation ordering is important, do we have to better
>> _always_ use a READ_ONCE() for all flags accesses above, to ensure it is
>> preserved?
>>
>
> Is this mean that we want to use READ_ONCE to avoid compiler reordering these
> conditions?
Why would you need a READ_ONCE() on flags here?
[...]
>>>> /* Keep using defined clamps across class changes */
>>>> - if (uc_se->user_defined)
>>>> + if (!uclamp_reset(clamp_id, attr->sched_flags) &&
>>>> + uc_se->user_defined) {
>>>> continue;
>>>> + }
>>
>> I think we miss to reset the user_defined flag here.
>>
>> What about replacing the above chunk with:
>>
>> if (uclamp_reset(clamp_id, attr->sched_flags))
>> uc_se->user_defined = false;
>> if (uc-se->user_defined)
>> continue;
>>
>> ?
>
> user_defined flag will be reset later by uclamp_se_set(uc_se, value,
> false). But I agree to split it to two condition because it seems
> clearer.
IMHO it's more elegant to use uclamp_reset() in the condition next to
uc-se->user_defined and let uclamp_se_set() set uc-se->user_defined to
false later.
>>>> /*
>>>> * RT by default have a 100% boost value that could be modified
>>>> * at runtime.
>>>> */
>>>> if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
>>>> - __uclamp_update_util_min_rt_default(p);
>>>> + value = sysctl_sched_uclamp_util_min_rt_default;
>>
>> By removing this usage of __uclamp_updadate_util_min_rt_default(p),
>> the only other usage remaining is the call from:
>> uclamp_udpate_util_min_rt_default().
>>
>> What about an additional cleanup by in-lining the only surviving usage?
Don't see why not.
>>>> else
>>>> - uclamp_se_set(uc_se, uclamp_none(clamp_id), false);
>>>> + value = uclamp_none(clamp_id);
>>>>
>>>> + uclamp_se_set(uc_se, value, false);
>>>> }
>>>>
>>>> - if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
>>>> + if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)) ||
>>>> + attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_RESET) {
>>
>> The likely() above should not wrap both conditions to be effective?
>
> Got it.
I thought the likely is for no uclamp activities, i.e. policy change.
And a uclamp reset is different to a policy change. But is this likely too?
