Re: [PATCH v2 1/4] sched/uclamp: Make uclamp_util_*() helpers use and return UL values

[Vincent Guittot]

On Wed, 4 Dec 2019 at 18:15, Valentin Schneider
<valentin.schneider@xxxxxxx> wrote:
>
> On 04/12/2019 16:12, Vincent Guittot wrote:
> > On Wed, 4 Dec 2019 at 17:03, Valentin Schneider
> > <valentin.schneider@xxxxxxx> wrote:
> >>
> >> On 04/12/2019 15:22, Vincent Guittot wrote:
> >>>> @@ -2303,15 +2303,15 @@ static inline void cpufreq_update_util(struct rq *rq, unsigned int flags) {}
> >>>>  unsigned int uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
> >>>
> >>> Why not changing uclamp_eff_value to return unsigned long too ? The
> >>> returned value represents a utilization to be compared with other
> >>> utilization value
> >>>
> >>
> >> IMO uclamp_eff_value() is a simple accessor to uclamp_se.value
> >> (unsigned int), which is why I didn't want to change its return type.
> >> I see it as being the task equivalent of rq->uclamp[clamp_id].value, IOW
> >> "give me the uclamp value for that clamp index". It just happens to be a
> >> bit more intricate for tasks than for rqs.
> >
> > But then you have to take care of casting the returned value in
> > several places here and in patch 3
> >
>
> True. I'm not too hot on having to handle rq clamp values
> (rq->uclamp[x].value) and task clamp values (uclamp_eff_value())
> differently (cast one but not the other), but I don't feel *too* strongly
> about this, so if you want I can do that change for v3.

Yes please. This makes the code easier to read and understand.

The rest of the patch series looks good to me. So feel free to add my
reviewed by on the next version
Reviewed-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>

>
> >>
> >> uclamp_util() & uclamp_util_with() do explicitly return a utilization,
> >> so here it makes sense (in my mind, that is) to return UL.