Re: [PATCH v2 3/8] thermal: gov_power_allocator: Move memory allocation out of throttle()

[Lukasz Luba]

On 12/20/23 14:35, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> > The new thermal callback allows to react to the change of cooling
> > instances in the thermal zone. Move the memory allocation to that new
> > callback and save CPU cycles in the throttle() code path.
> >
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   drivers/thermal/gov_power_allocator.c | 144 ++++++++++++++++++++------
> >   1 file changed, 113 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 38e1e89ba10c..3328c3ec71f2 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -61,6 +61,13 @@ static inline s64 div_frac(s64 x, s64 y)
> >    *                     @trip_switch_on should be NULL.
> >    * @trip_max:          last passive trip point of the thermal zone. The
> >    *                     temperature we are controlling for.
> > + * @num_actors:                number of cooling devices supporting IPA callbacks
> > + * @buffer_size:       IPA internal buffer size
> > + * @req_power:         IPA buffer for requested power
> > + * @max_power:         IPA buffer for max allocatable power
> > + * @granted_power:     IPA buffer for granted power
> > + * @extra_actor_power: IPA buffer for extra power
> > + * @weighted_req_power:        IPA buffer for weighted requested power
> >    */
> >   struct power_allocator_params {
> >          bool allocated_tzp;
> > @@ -69,6 +76,13 @@ struct power_allocator_params {
> >          u32 sustainable_power;
> >          const struct thermal_trip *trip_switch_on;
> >          const struct thermal_trip *trip_max;
> > +       int num_actors;
> > +       int buffer_size;
>
> None of the above can be negative, so maybe consider using unsigned int?

True, I'll change them to unsigned.

> > +       u32 *req_power;
> > +       u32 *max_power;
> > +       u32 *granted_power;
> > +       u32 *extra_actor_power;
> > +       u32 *weighted_req_power;
> >   };
> >
> >   /**
> > @@ -387,39 +401,24 @@ static int allocate_power(struct thermal_zone_device *tz, int control_temp)
> >          u32 *weighted_req_power;
> >          u32 power_range, weight;
> >          int total_weight = 0;
> > -       int num_actors = 0;
>
> You could retain this local var and set it to params->num_actors.  It
> is kind of inconsistent to drop it and still use the local variables
> above.

OK, I'll do that.

[snip]

> > +
> > +       req_power = kcalloc(num_actors * 5, sizeof(u32), GFP_KERNEL);
>
> I'd use sizeof(*req_power) instead of sizeof(u32) here and below.

OK

> > +       if (!req_power) {
> > +               ret = -ENOMEM;
> > +               goto clean_buffers;
> > +       }
> > +
> > +       params->num_actors = num_actors;
> > +       params->buffer_size = num_actors * 5 * sizeof(u32);
> > +
> > +       _power_buffers_init(params, req_power, &req_power[params->num_actors],
> > +                           &req_power[2 * params->num_actors],
> > +                           &req_power[3 * params->num_actors],
> > +                           &req_power[4 * params->num_actors]);
>
> Why don't you use the local var in this instead of the struct member?
> It would be easier to read then IMO.
>
> Also, I would rather use pointer arithmetic, so it would become
>
> _power_buffers_init(params, req_power, req_power + num_actors,
> req_power + 2 * num_actors, req_power + 3 * num_actors, req_power + 4
> * num_actors);
>
> And note that you could introduce something like
>
> struct power_actor {
>          u32 req_power;
>          u32 max_power;
>          u32 granted_power;
>          u32 extra_actor_power;
>          u32 weighted_req_power;
> };
>
> and use a single array of these instead of 5 different arrays of u32,
> which would result in more straightforward code if I'm not mistaken.

That sounds like a good idea. Let me implement it and see - but it
should be a better way. Thanks!