mbox series

[0/7] Minor cleanup for thermal gov power allocator

Message ID 20231025192225.468228-1-lukasz.luba@arm.com
Headers show
Series Minor cleanup for thermal gov power allocator | expand

Message

Lukasz Luba Oct. 25, 2023, 7:22 p.m. UTC
Hi all,

The patch set does some small clean up for Intelligent Power Allocator.
Those changes are not expected to alter the general functionality. They just
improve the code reading. Only patch 3/7 might improve the use case for
binding the governor to thermal zone (very unlikely in real products, but
it's needed for correctness).

The changes are based on top of current PM thermal branch, so with the
new trip points.

Regards,
Lukasz

Lukasz Luba (7):
  thermal: gov_power_allocator: Rename trip_max_desired_temperature
  thermal: gov_power_allocator: Setup trip points earlier
  thermal: gov_power_allocator: Check the cooling devices only for
    trip_max
  thermal: gov_power_allocator: Rearrange the order of variables
  thermal: gov_power_allocator: Use shorter variable when possible
  thermal: gov_power_allocator: Remove unneeded local variables
  thermal: gov_power_allocator: Clean needed variables at the beginning

 drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
 1 file changed, 64 insertions(+), 59 deletions(-)

Comments

Rafael J. Wysocki Oct. 26, 2023, 8:54 a.m. UTC | #1
On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi all,
>
> The patch set does some small clean up for Intelligent Power Allocator.
> Those changes are not expected to alter the general functionality. They just
> improve the code reading. Only patch 3/7 might improve the use case for
> binding the governor to thermal zone (very unlikely in real products, but
> it's needed for correctness).
>
> The changes are based on top of current PM thermal branch, so with the
> new trip points.
>
> Regards,
> Lukasz
>
> Lukasz Luba (7):
>   thermal: gov_power_allocator: Rename trip_max_desired_temperature
>   thermal: gov_power_allocator: Setup trip points earlier
>   thermal: gov_power_allocator: Check the cooling devices only for
>     trip_max
>   thermal: gov_power_allocator: Rearrange the order of variables
>   thermal: gov_power_allocator: Use shorter variable when possible
>   thermal: gov_power_allocator: Remove unneeded local variables
>   thermal: gov_power_allocator: Clean needed variables at the beginning
>
>  drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>  1 file changed, 64 insertions(+), 59 deletions(-)
>
> --

The series looks good to me overall, but I'd prefer to make these
changes in the 6.8 cycle, because the 6.7 merge window is around the
corner and there is quite a bit of thermal material in this cycle
already.
Lukasz Luba Oct. 26, 2023, 12:22 p.m. UTC | #2
On 10/26/23 09:54, Rafael J. Wysocki wrote:
> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi all,
>>
>> The patch set does some small clean up for Intelligent Power Allocator.
>> Those changes are not expected to alter the general functionality. They just
>> improve the code reading. Only patch 3/7 might improve the use case for
>> binding the governor to thermal zone (very unlikely in real products, but
>> it's needed for correctness).
>>
>> The changes are based on top of current PM thermal branch, so with the
>> new trip points.
>>
>> Regards,
>> Lukasz
>>
>> Lukasz Luba (7):
>>    thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>    thermal: gov_power_allocator: Setup trip points earlier
>>    thermal: gov_power_allocator: Check the cooling devices only for
>>      trip_max
>>    thermal: gov_power_allocator: Rearrange the order of variables
>>    thermal: gov_power_allocator: Use shorter variable when possible
>>    thermal: gov_power_allocator: Remove unneeded local variables
>>    thermal: gov_power_allocator: Clean needed variables at the beginning
>>
>>   drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>   1 file changed, 64 insertions(+), 59 deletions(-)
>>
>> --
> 
> The series looks good to me overall, but I'd prefer to make these
> changes in the 6.8 cycle, because the 6.7 merge window is around the
> corner and there is quite a bit of thermal material in this cycle
> already.

Thanks for having a look! Yes, I agree, we can wait after the
merge window. It just have to be cleaned one day a bit and I postponed
this a few times, so no rush ;)
Lukasz Luba Nov. 23, 2023, 3:19 p.m. UTC | #3
Hi Rafael,

Gentle ping

On 10/26/23 13:22, Lukasz Luba wrote:
> 
> 
> On 10/26/23 09:54, Rafael J. Wysocki wrote:
>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>
>>> Hi all,
>>>
>>> The patch set does some small clean up for Intelligent Power Allocator.
>>> Those changes are not expected to alter the general functionality. 
>>> They just
>>> improve the code reading. Only patch 3/7 might improve the use case for
>>> binding the governor to thermal zone (very unlikely in real products, 
>>> but
>>> it's needed for correctness).
>>>
>>> The changes are based on top of current PM thermal branch, so with the
>>> new trip points.
>>>
>>> Regards,
>>> Lukasz
>>>
>>> Lukasz Luba (7):
>>>    thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>>    thermal: gov_power_allocator: Setup trip points earlier
>>>    thermal: gov_power_allocator: Check the cooling devices only for
>>>      trip_max
>>>    thermal: gov_power_allocator: Rearrange the order of variables
>>>    thermal: gov_power_allocator: Use shorter variable when possible
>>>    thermal: gov_power_allocator: Remove unneeded local variables
>>>    thermal: gov_power_allocator: Clean needed variables at the beginning
>>>
>>>   drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>>   1 file changed, 64 insertions(+), 59 deletions(-)
>>>
>>> -- 
>>
>> The series looks good to me overall, but I'd prefer to make these
>> changes in the 6.8 cycle, because the 6.7 merge window is around the
>> corner and there is quite a bit of thermal material in this cycle
>> already.
> 
> Thanks for having a look! Yes, I agree, we can wait after the
> merge window. It just have to be cleaned one day a bit and I postponed
> this a few times, so no rush ;)

I've seen you've created the new pm/thermal. Could you consider to take
those in, please?

I would send some RFC on top showing the issue with reading back the CPU
max frequency from the PM_QoS chain.

Regards,
Lukasz
Rafael J. Wysocki Nov. 23, 2023, 7:50 p.m. UTC | #4
Hi Lukasz,

On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Rafael,
>
> Gentle ping
>
> On 10/26/23 13:22, Lukasz Luba wrote:
> >
> >
> > On 10/26/23 09:54, Rafael J. Wysocki wrote:
> >> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>
> >>> Hi all,
> >>>
> >>> The patch set does some small clean up for Intelligent Power Allocator.
> >>> Those changes are not expected to alter the general functionality.
> >>> They just
> >>> improve the code reading. Only patch 3/7 might improve the use case for
> >>> binding the governor to thermal zone (very unlikely in real products,
> >>> but
> >>> it's needed for correctness).
> >>>
> >>> The changes are based on top of current PM thermal branch, so with the
> >>> new trip points.
> >>>
> >>> Regards,
> >>> Lukasz
> >>>
> >>> Lukasz Luba (7):
> >>>    thermal: gov_power_allocator: Rename trip_max_desired_temperature
> >>>    thermal: gov_power_allocator: Setup trip points earlier
> >>>    thermal: gov_power_allocator: Check the cooling devices only for
> >>>      trip_max
> >>>    thermal: gov_power_allocator: Rearrange the order of variables
> >>>    thermal: gov_power_allocator: Use shorter variable when possible
> >>>    thermal: gov_power_allocator: Remove unneeded local variables
> >>>    thermal: gov_power_allocator: Clean needed variables at the beginning
> >>>
> >>>   drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
> >>>   1 file changed, 64 insertions(+), 59 deletions(-)
> >>>
> >>> --
> >>
> >> The series looks good to me overall, but I'd prefer to make these
> >> changes in the 6.8 cycle, because the 6.7 merge window is around the
> >> corner and there is quite a bit of thermal material in this cycle
> >> already.
> >
> > Thanks for having a look! Yes, I agree, we can wait after the
> > merge window. It just have to be cleaned one day a bit and I postponed
> > this a few times, so no rush ;)
>
> I've seen you've created the new pm/thermal. Could you consider to take
> those in, please?

Sure, I'll get to them presumably tomorrow and if not then early next week.

> I would send some RFC on top showing the issue with reading back the CPU
> max frequency from the PM_QoS chain.

Sounds good.
Lukasz Luba Nov. 24, 2023, 7:45 a.m. UTC | #5
On 11/23/23 19:50, Rafael J. Wysocki wrote:
> Hi Lukasz,
> 
> On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Rafael,
>>
>> Gentle ping
>>
>> On 10/26/23 13:22, Lukasz Luba wrote:
>>>
>>>
>>> On 10/26/23 09:54, Rafael J. Wysocki wrote:
>>>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> The patch set does some small clean up for Intelligent Power Allocator.
>>>>> Those changes are not expected to alter the general functionality.
>>>>> They just
>>>>> improve the code reading. Only patch 3/7 might improve the use case for
>>>>> binding the governor to thermal zone (very unlikely in real products,
>>>>> but
>>>>> it's needed for correctness).
>>>>>
>>>>> The changes are based on top of current PM thermal branch, so with the
>>>>> new trip points.
>>>>>
>>>>> Regards,
>>>>> Lukasz
>>>>>
>>>>> Lukasz Luba (7):
>>>>>     thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>>>>     thermal: gov_power_allocator: Setup trip points earlier
>>>>>     thermal: gov_power_allocator: Check the cooling devices only for
>>>>>       trip_max
>>>>>     thermal: gov_power_allocator: Rearrange the order of variables
>>>>>     thermal: gov_power_allocator: Use shorter variable when possible
>>>>>     thermal: gov_power_allocator: Remove unneeded local variables
>>>>>     thermal: gov_power_allocator: Clean needed variables at the beginning
>>>>>
>>>>>    drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>>>>    1 file changed, 64 insertions(+), 59 deletions(-)
>>>>>
>>>>> --
>>>>
>>>> The series looks good to me overall, but I'd prefer to make these
>>>> changes in the 6.8 cycle, because the 6.7 merge window is around the
>>>> corner and there is quite a bit of thermal material in this cycle
>>>> already.
>>>
>>> Thanks for having a look! Yes, I agree, we can wait after the
>>> merge window. It just have to be cleaned one day a bit and I postponed
>>> this a few times, so no rush ;)
>>
>> I've seen you've created the new pm/thermal. Could you consider to take
>> those in, please?
> 
> Sure, I'll get to them presumably tomorrow and if not then early next week.

OK, thank you Rafael!

> 
>> I would send some RFC on top showing the issue with reading back the CPU
>> max frequency from the PM_QoS chain.
> 
> Sounds good.
Rafael J. Wysocki Nov. 28, 2023, 3:17 p.m. UTC | #6
On Fri, Nov 24, 2023 at 8:44 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 11/23/23 19:50, Rafael J. Wysocki wrote:
> > Hi Lukasz,
> >
> > On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> Gentle ping
> >>
> >> On 10/26/23 13:22, Lukasz Luba wrote:
> >>>
> >>>
> >>> On 10/26/23 09:54, Rafael J. Wysocki wrote:
> >>>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>>
> >>>>> Hi all,
> >>>>>
> >>>>> The patch set does some small clean up for Intelligent Power Allocator.
> >>>>> Those changes are not expected to alter the general functionality.
> >>>>> They just
> >>>>> improve the code reading. Only patch 3/7 might improve the use case for
> >>>>> binding the governor to thermal zone (very unlikely in real products,
> >>>>> but
> >>>>> it's needed for correctness).
> >>>>>
> >>>>> The changes are based on top of current PM thermal branch, so with the
> >>>>> new trip points.
> >>>>>
> >>>>> Regards,
> >>>>> Lukasz
> >>>>>
> >>>>> Lukasz Luba (7):
> >>>>>     thermal: gov_power_allocator: Rename trip_max_desired_temperature
> >>>>>     thermal: gov_power_allocator: Setup trip points earlier
> >>>>>     thermal: gov_power_allocator: Check the cooling devices only for
> >>>>>       trip_max
> >>>>>     thermal: gov_power_allocator: Rearrange the order of variables
> >>>>>     thermal: gov_power_allocator: Use shorter variable when possible
> >>>>>     thermal: gov_power_allocator: Remove unneeded local variables
> >>>>>     thermal: gov_power_allocator: Clean needed variables at the beginning
> >>>>>
> >>>>>    drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
> >>>>>    1 file changed, 64 insertions(+), 59 deletions(-)
> >>>>>
> >>>>> --
> >>>>
> >>>> The series looks good to me overall, but I'd prefer to make these
> >>>> changes in the 6.8 cycle, because the 6.7 merge window is around the
> >>>> corner and there is quite a bit of thermal material in this cycle
> >>>> already.
> >>>
> >>> Thanks for having a look! Yes, I agree, we can wait after the
> >>> merge window. It just have to be cleaned one day a bit and I postponed
> >>> this a few times, so no rush ;)
> >>
> >> I've seen you've created the new pm/thermal. Could you consider to take
> >> those in, please?
> >
> > Sure, I'll get to them presumably tomorrow and if not then early next week.
>
> OK, thank you Rafael!

I've queued up the whole lot for 6.8 and in the process I've edited
all of the changelogs and some subjects for clarity and English
grammar improvements.

Thanks!
Lukasz Luba Nov. 28, 2023, 3:32 p.m. UTC | #7
On 11/28/23 15:17, Rafael J. Wysocki wrote:
> On Fri, Nov 24, 2023 at 8:44 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 11/23/23 19:50, Rafael J. Wysocki wrote:
>>> Hi Lukasz,
>>>
>>> On Thu, Nov 23, 2023 at 4:19 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> Gentle ping
>>>>
>>>> On 10/26/23 13:22, Lukasz Luba wrote:
>>>>>
>>>>>
>>>>> On 10/26/23 09:54, Rafael J. Wysocki wrote:
>>>>>> On Wed, Oct 25, 2023 at 9:21 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> The patch set does some small clean up for Intelligent Power Allocator.
>>>>>>> Those changes are not expected to alter the general functionality.
>>>>>>> They just
>>>>>>> improve the code reading. Only patch 3/7 might improve the use case for
>>>>>>> binding the governor to thermal zone (very unlikely in real products,
>>>>>>> but
>>>>>>> it's needed for correctness).
>>>>>>>
>>>>>>> The changes are based on top of current PM thermal branch, so with the
>>>>>>> new trip points.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Lukasz
>>>>>>>
>>>>>>> Lukasz Luba (7):
>>>>>>>      thermal: gov_power_allocator: Rename trip_max_desired_temperature
>>>>>>>      thermal: gov_power_allocator: Setup trip points earlier
>>>>>>>      thermal: gov_power_allocator: Check the cooling devices only for
>>>>>>>        trip_max
>>>>>>>      thermal: gov_power_allocator: Rearrange the order of variables
>>>>>>>      thermal: gov_power_allocator: Use shorter variable when possible
>>>>>>>      thermal: gov_power_allocator: Remove unneeded local variables
>>>>>>>      thermal: gov_power_allocator: Clean needed variables at the beginning
>>>>>>>
>>>>>>>     drivers/thermal/gov_power_allocator.c | 123 ++++++++++++++------------
>>>>>>>     1 file changed, 64 insertions(+), 59 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>
>>>>>> The series looks good to me overall, but I'd prefer to make these
>>>>>> changes in the 6.8 cycle, because the 6.7 merge window is around the
>>>>>> corner and there is quite a bit of thermal material in this cycle
>>>>>> already.
>>>>>
>>>>> Thanks for having a look! Yes, I agree, we can wait after the
>>>>> merge window. It just have to be cleaned one day a bit and I postponed
>>>>> this a few times, so no rush ;)
>>>>
>>>> I've seen you've created the new pm/thermal. Could you consider to take
>>>> those in, please?
>>>
>>> Sure, I'll get to them presumably tomorrow and if not then early next week.
>>
>> OK, thank you Rafael!
> 
> I've queued up the whole lot for 6.8 and in the process I've edited
> all of the changelogs and some subjects for clarity and English
> grammar improvements.

Thank you and I really appreciate the improvements!