linux-kernel - Re: [PATCH v2 1/4] sched/uclamp: Make uclamp_util

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Dec 2019 18:29:09 +0100
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Patrick Bellasi <patrick.bellasi@...bug.net>,
        Quentin Perret <qperret@...gle.com>,
        Qais Yousef <qais.yousef@....com>,
        Morten Rasmussen <morten.rasmussen@....com>
Subject: Re: [PATCH v2 1/4] sched/uclamp: Make uclamp_util_*() helpers use and
 return UL values

On Wed, 4 Dec 2019 at 18:15, Valentin Schneider
<valentin.schneider@....com> wrote:
>
> On 04/12/2019 16:12, Vincent Guittot wrote:
> > On Wed, 4 Dec 2019 at 17:03, Valentin Schneider
> > <valentin.schneider@....com> 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@...aro.org>

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