v5.7: new core kernel option missing help text

v5.7: new core kernel option missing help text

[Russell King - ARM Linux admin]

Hi,

A new kernel configuration option ("SCHED_THERMAL_PRESSURE") was
recently added, but has no help text. This is most unhelpful when
trying to configure the kernel, since one does not know what the
effect of answering yes or no to this option would be.

Please supply a proper help text when adding core kernel options
so that people can make an informed decision when answering the
prompt, rather than just guessing.

One suggestion on IRC has been that this option has something to
do with climate change.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: v5.7: new core kernel option missing help text

[Valentin Schneider]

On 03/06/20 18:31, Russell King - ARM Linux admin wrote:
> Hi,
>
> A new kernel configuration option ("SCHED_THERMAL_PRESSURE") was
> recently added, but has no help text. This is most unhelpful when
> trying to configure the kernel, since one does not know what the
> effect of answering yes or no to this option would be.
>
> Please supply a proper help text when adding core kernel options
> so that people can make an informed decision when answering the
> prompt, rather than just guessing.
>

Right; does the below look good enough?

---
diff --git a/init/Kconfig b/init/Kconfig
index 74a5ac65644f..f40cf852d00a 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -441,6 +441,10 @@ config HAVE_SCHED_AVG_IRQ
 config SCHED_THERMAL_PRESSURE
        bool "Enable periodic averaging of thermal pressure"
        depends on SMP
+	help
+	  This option allows the scheduler to be aware of CPU thermal throttling
+	  (i.e. thermal pressure), providing arch_scale_thermal_pressure() is
+	  implemented.

 config BSD_PROCESS_ACCT
        bool "BSD Process Accounting"
---

Re: v5.7: new core kernel option missing help text

[Russell King - ARM Linux admin]

On Wed, Jun 03, 2020 at 07:00:26PM +0100, Valentin Schneider wrote:
> 
> On 03/06/20 18:31, Russell King - ARM Linux admin wrote:
> > Hi,
> >
> > A new kernel configuration option ("SCHED_THERMAL_PRESSURE") was
> > recently added, but has no help text. This is most unhelpful when
> > trying to configure the kernel, since one does not know what the
> > effect of answering yes or no to this option would be.
> >
> > Please supply a proper help text when adding core kernel options
> > so that people can make an informed decision when answering the
> > prompt, rather than just guessing.
> >
> 
> Right; does the below look good enough?

It's a start.  I'm still wondering whether I should answer yes or no
for the platforms I'm building for.

So far, all I've found is:

arch/arm/include/asm/topology.h:#define arch_scale_thermal_pressure topology_get_thermal_pressure

which really doesn't tell me anything about this.  So I'm still in
the dark.

I guess topology_get_thermal_pressure is provided by something in
drivers/ which will be conditional on some driver or something.

> 
> ---
> diff --git a/init/Kconfig b/init/Kconfig
> index 74a5ac65644f..f40cf852d00a 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -441,6 +441,10 @@ config HAVE_SCHED_AVG_IRQ
>  config SCHED_THERMAL_PRESSURE
>         bool "Enable periodic averaging of thermal pressure"
>         depends on SMP
> +	help
> +	  This option allows the scheduler to be aware of CPU thermal throttling
> +	  (i.e. thermal pressure), providing arch_scale_thermal_pressure() is
> +	  implemented.
> 
>  config BSD_PROCESS_ACCT
>         bool "BSD Process Accounting"
> ---
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: v5.7: new core kernel option missing help text

[Vincent Guittot]

On Wed, 3 Jun 2020 at 20:45, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 03, 2020 at 07:00:26PM +0100, Valentin Schneider wrote:
> >
> > On 03/06/20 18:31, Russell King - ARM Linux admin wrote:
> > > Hi,
> > >
> > > A new kernel configuration option ("SCHED_THERMAL_PRESSURE") was
> > > recently added, but has no help text. This is most unhelpful when
> > > trying to configure the kernel, since one does not know what the
> > > effect of answering yes or no to this option would be.
> > >
> > > Please supply a proper help text when adding core kernel options
> > > so that people can make an informed decision when answering the
> > > prompt, rather than just guessing.
> > >
> >
> > Right; does the below look good enough?
>
> It's a start.  I'm still wondering whether I should answer yes or no
> for the platforms I'm building for.
>
> So far, all I've found is:
>
> arch/arm/include/asm/topology.h:#define arch_scale_thermal_pressure topology_get_thermal_pressure
>
> which really doesn't tell me anything about this.  So I'm still in
> the dark.
>
> I guess topology_get_thermal_pressure is provided by something in
> drivers/ which will be conditional on some driver or something.

You need cpufreq_cooling device to make it useful and only for SMP
I don't think that this should not be user configurable because even
with the description above, it is not easy to choose.
This should be set by the driver that implement the feature which is
only cpufreq cooling device for now it

>
> >
> > ---
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 74a5ac65644f..f40cf852d00a 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -441,6 +441,10 @@ config HAVE_SCHED_AVG_IRQ
> >  config SCHED_THERMAL_PRESSURE
> >         bool "Enable periodic averaging of thermal pressure"
> >         depends on SMP
> > +     help
> > +       This option allows the scheduler to be aware of CPU thermal throttling
> > +       (i.e. thermal pressure), providing arch_scale_thermal_pressure() is
> > +       implemented.
> >
> >  config BSD_PROCESS_ACCT
> >         bool "BSD Process Accounting"
> > ---
> >
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: v5.7: new core kernel option missing help text

[Russell King - ARM Linux admin]

On Wed, Jun 03, 2020 at 09:24:56PM +0200, Vincent Guittot wrote:
> On Wed, 3 Jun 2020 at 20:45, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > It's a start.  I'm still wondering whether I should answer yes or no
> > for the platforms I'm building for.
> >
> > So far, all I've found is:
> >
> > arch/arm/include/asm/topology.h:#define arch_scale_thermal_pressure topology_get_thermal_pressure
> >
> > which really doesn't tell me anything about this.  So I'm still in
> > the dark.
> >
> > I guess topology_get_thermal_pressure is provided by something in
> > drivers/ which will be conditional on some driver or something.
> 
> You need cpufreq_cooling device to make it useful and only for SMP
> I don't think that this should not be user configurable because even
> with the description above, it is not easy to choose.
> This should be set by the driver that implement the feature which is
> only cpufreq cooling device for now it

As I have CONFIG_CPU_FREQ_THERMAL=y in my config, I'm guessing (and it's
only a guess) that I should say y to SCHED_THERMAL_PRESSURE ?

> > > +     help
> > > +       This option allows the scheduler to be aware of CPU thermal throttling
> > > +       (i.e. thermal pressure), providing arch_scale_thermal_pressure() is
> > > +       implemented.

Is this feature documented in terms of what it does?  Do I assume that
as the thermal trip points start tripping, that has an influence on
the scheduler?  Or is it the case that the scheduler is wanting to
know when the cpu frequency changes?

Grepping for "thermal" in Documentation/scheduler brings up nothing.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: v5.7: new core kernel option missing help text

[Vincent Guittot]

On Wed, 3 Jun 2020 at 21:59, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 03, 2020 at 09:24:56PM +0200, Vincent Guittot wrote:
> > On Wed, 3 Jun 2020 at 20:45, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > > It's a start.  I'm still wondering whether I should answer yes or no
> > > for the platforms I'm building for.
> > >
> > > So far, all I've found is:
> > >
> > > arch/arm/include/asm/topology.h:#define arch_scale_thermal_pressure topology_get_thermal_pressure
> > >
> > > which really doesn't tell me anything about this.  So I'm still in
> > > the dark.
> > >
> > > I guess topology_get_thermal_pressure is provided by something in
> > > drivers/ which will be conditional on some driver or something.
> >
> > You need cpufreq_cooling device to make it useful and only for SMP
> > I don't think that this should not be user configurable because even
> > with the description above, it is not easy to choose.
> > This should be set by the driver that implement the feature which is
> > only cpufreq cooling device for now it
>
> As I have CONFIG_CPU_FREQ_THERMAL=y in my config, I'm guessing (and it's
> only a guess) that I should say y to SCHED_THERMAL_PRESSURE ?

yes, you're right

>
> > > > +     help
> > > > +       This option allows the scheduler to be aware of CPU thermal throttling
> > > > +       (i.e. thermal pressure), providing arch_scale_thermal_pressure() is
> > > > +       implemented.
>
> Is this feature documented in terms of what it does?  Do I assume that
> as the thermal trip points start tripping, that has an influence on
> the scheduler?  Or is it the case that the scheduler is wanting to
> know when the cpu frequency changes?

When the thermal trip points start tripping, we take into account the
decrease of the compute capacity of the CU. This reduced capacity is
used instead of max capacity when we balance the tasks between CPUs.

A similar mechanism is used to account the time "stolen" by IRQ or
RT/DL tasks to CFS tasks

>
> Grepping for "thermal" in Documentation/scheduler brings up nothing.

John Mathew sent a patch to add documentation:
https://lkml.org/lkml/2020/5/14/290

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: v5.7: new core kernel option missing help text

[Valentin Schneider]

On 03/06/20 20:58, Russell King - ARM Linux admin wrote:
> On Wed, Jun 03, 2020 at 09:24:56PM +0200, Vincent Guittot wrote:
>> On Wed, 3 Jun 2020 at 20:45, Russell King - ARM Linux admin
>> <linux@armlinux.org.uk> wrote:
>> > It's a start.  I'm still wondering whether I should answer yes or no
>> > for the platforms I'm building for.
>> >
>> > So far, all I've found is:
>> >
>> > arch/arm/include/asm/topology.h:#define arch_scale_thermal_pressure topology_get_thermal_pressure
>> >
>> > which really doesn't tell me anything about this.  So I'm still in
>> > the dark.
>> >
>> > I guess topology_get_thermal_pressure is provided by something in
>> > drivers/ which will be conditional on some driver or something.
>>
>> You need cpufreq_cooling device to make it useful and only for SMP
>> I don't think that this should not be user configurable because even
>> with the description above, it is not easy to choose.
>> This should be set by the driver that implement the feature which is
>> only cpufreq cooling device for now it
>
> As I have CONFIG_CPU_FREQ_THERMAL=y in my config, I'm guessing (and it's
> only a guess) that I should say y to SCHED_THERMAL_PRESSURE ?
>

arm and arm64 implement arch_scale_thermal_pressure(); the actual
implementation is in the arch_topology "driver" (GENERIC_ARCH_TOPOLOGY).

Then, the caller of arch_set_thermal_pressure() is cpufreq_cooling (see
below); that'll only get called if you have thermal zones using CPU
cooling devices.

AFAICT the current state of things imply we should have something like

        depends on (ARM || ARM64) && GENERIC_ARCH_TOPOLOGY

for that option.

>> > > +     help
>> > > +       This option allows the scheduler to be aware of CPU thermal throttling
>> > > +       (i.e. thermal pressure), providing arch_scale_thermal_pressure() is
>> > > +       implemented.
>
> Is this feature documented in terms of what it does?  Do I assume that
> as the thermal trip points start tripping, that has an influence on
> the scheduler?  Or is it the case that the scheduler is wanting to
> know when the cpu frequency changes?
>
> Grepping for "thermal" in Documentation/scheduler brings up nothing.

The former; changing a CPU cooling device's state (IOW changing its max
allowed frequency for thermal reasons) leads to a call to
arch_set_thermal_pressure() (see
cpufreq_cooling.c::cpufreq_set_cur_state()).

It's somewhat interesting to have, at least in theory. On plain SMP that
would let the scheduler see if some CPUs are more throttled that others,
which would be leveraged when doing load balancing. It's more
interesting for big.LITTLE & co, where in the worst cases we can have
things like capacity inversion, i.e. the bigs are so thermally throttled
that they give less oomf than a LITTLE.

Re: v5.7: new core kernel option missing help text

[Thara Gopinath]

On 6/3/20 4:25 PM, Valentin Schneider wrote:
> 
> On 03/06/20 20:58, Russell King - ARM Linux admin wrote:
>> On Wed, Jun 03, 2020 at 09:24:56PM +0200, Vincent Guittot wrote:
>>> On Wed, 3 Jun 2020 at 20:45, Russell King - ARM Linux admin
>>> <linux@armlinux.org.uk> wrote:
>>>> It's a start.  I'm still wondering whether I should answer yes or no
>>>> for the platforms I'm building for.
>>>>
>>>> So far, all I've found is:
>>>>
>>>> arch/arm/include/asm/topology.h:#define arch_scale_thermal_pressure topology_get_thermal_pressure
>>>>
>>>> which really doesn't tell me anything about this.  So I'm still in
>>>> the dark.
>>>>
>>>> I guess topology_get_thermal_pressure is provided by something in
>>>> drivers/ which will be conditional on some driver or something.
>>>
>>> You need cpufreq_cooling device to make it useful and only for SMP
>>> I don't think that this should not be user configurable because even
>>> with the description above, it is not easy to choose.
>>> This should be set by the driver that implement the feature which is
>>> only cpufreq cooling device for now it
>>
>> As I have CONFIG_CPU_FREQ_THERMAL=y in my config, I'm guessing (and it's
>> only a guess) that I should say y to SCHED_THERMAL_PRESSURE ?
>>
> 
> arm and arm64 implement arch_scale_thermal_pressure(); the actual
> implementation is in the arch_topology "driver" (GENERIC_ARCH_TOPOLOGY).
> 
> Then, the caller of arch_set_thermal_pressure() is cpufreq_cooling (see
> below); that'll only get called if you have thermal zones using CPU
> cooling devices.
> 
> AFAICT the current state of things imply we should have something like
> 
>          depends on (ARM || ARM64) && GENERIC_ARCH_TOPOLOGY
> 
> for that option.

Hi Russel/Valentin

The feature itself like Valentin explained below allows scheduler to be 
aware of cpu capacity reduced due to thermal throttling. 
arch_set_thermal_pressure feeds the capped capacity to the scheduler and 
hence the feature makes sense only if arch_set_thermal_pressure is 
implemented. Having said that arch_set_thermal_pressure is implemented 
in arch_topology driver for arm and arm64 platforms. But the feature 
itself is not bound to arm/arm64 platforms. So it would make it wrong to 
add a "depends on (ARM || ARM64) option."

I agree with Vincent that allowing user to choose this option is 
probably not the best. IMO, this should be enabled by default in arm64 
defconfig considering both GENERIC_ARCH_TOPOLOGY and CPU_FREQ_THERMAL 
are enabled by default.
So if it is acceptable three things to be done are:
1. Add the help text.
2. Don't allow SCHED_THERMAL_PRESSURE configurable by user
3. Enable it by default in arm64 defconfig

> 
>>>>> +     help
>>>>> +       This option allows the scheduler to be aware of CPU thermal throttling
>>>>> +       (i.e. thermal pressure), providing arch_scale_thermal_pressure() is
>>>>> +       implemented.
>>
>> Is this feature documented in terms of what it does?  Do I assume that
>> as the thermal trip points start tripping, that has an influence on
>> the scheduler?  Or is it the case that the scheduler is wanting to
>> know when the cpu frequency changes?
>>
>> Grepping for "thermal" in Documentation/scheduler brings up nothing.
> 
> The former; changing a CPU cooling device's state (IOW changing its max
> allowed frequency for thermal reasons) leads to a call to
> arch_set_thermal_pressure() (see
> cpufreq_cooling.c::cpufreq_set_cur_state()).
> 
> It's somewhat interesting to have, at least in theory. On plain SMP that
> would let the scheduler see if some CPUs are more throttled that others,
> which would be leveraged when doing load balancing. It's more
> interesting for big.LITTLE & co, where in the worst cases we can have
> things like capacity inversion, i.e. the bigs are so thermally throttled
> that they give less oomf than a LITTLE.
> 

-- 
Warm Regards
Thara

Re: v5.7: new core kernel option missing help text

[Valentin Schneider]

On 04/06/20 01:48, Thara Gopinath wrote:
> Hi Russel/Valentin
>
> The feature itself like Valentin explained below allows scheduler to be
> aware of cpu capacity reduced due to thermal throttling.
> arch_set_thermal_pressure feeds the capped capacity to the scheduler and
> hence the feature makes sense only if arch_set_thermal_pressure is
> implemented. Having said that arch_set_thermal_pressure is implemented
> in arch_topology driver for arm and arm64 platforms. But the feature
> itself is not bound to arm/arm64 platforms. So it would make it wrong to
> add a "depends on (ARM || ARM64) option."
>
> I agree with Vincent that allowing user to choose this option is
> probably not the best. IMO, this should be enabled by default in arm64
> defconfig considering both GENERIC_ARCH_TOPOLOGY and CPU_FREQ_THERMAL
> are enabled by default.

Right, I had skimmed over that but it probably does make more sense not
to bother users with it.

> So if it is acceptable three things to be done are:
> 1. Add the help text.
> 2. Don't allow SCHED_THERMAL_PRESSURE configurable by user
> 3. Enable it by default in arm64 defconfig

... and arm as well, I suppose?

Re: v5.7: new core kernel option missing help text

[Russell King - ARM Linux admin]

On Thu, Jun 04, 2020 at 10:26:27AM +0100, Valentin Schneider wrote:
> 
> On 04/06/20 01:48, Thara Gopinath wrote:
> > Hi Russel/Valentin
> >
> > The feature itself like Valentin explained below allows scheduler to be
> > aware of cpu capacity reduced due to thermal throttling.
> > arch_set_thermal_pressure feeds the capped capacity to the scheduler and
> > hence the feature makes sense only if arch_set_thermal_pressure is
> > implemented. Having said that arch_set_thermal_pressure is implemented
> > in arch_topology driver for arm and arm64 platforms. But the feature
> > itself is not bound to arm/arm64 platforms. So it would make it wrong to
> > add a "depends on (ARM || ARM64) option."
> >
> > I agree with Vincent that allowing user to choose this option is
> > probably not the best. IMO, this should be enabled by default in arm64
> > defconfig considering both GENERIC_ARCH_TOPOLOGY and CPU_FREQ_THERMAL
> > are enabled by default.
> 
> Right, I had skimmed over that but it probably does make more sense not
> to bother users with it.
> 
> > So if it is acceptable three things to be done are:
> > 1. Add the help text.
> > 2. Don't allow SCHED_THERMAL_PRESSURE configurable by user
> > 3. Enable it by default in arm64 defconfig
> 
> ... and arm as well, I suppose?

If it's not a user visible option, then there's no point it being in
defconfig.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: v5.7: new core kernel option missing help text

[Valentin Schneider]

On 04/06/20 10:29, Russell King - ARM Linux admin wrote:
> On Thu, Jun 04, 2020 at 10:26:27AM +0100, Valentin Schneider wrote:
>>
>> On 04/06/20 01:48, Thara Gopinath wrote:
>> > Hi Russel/Valentin
>> >
>> > The feature itself like Valentin explained below allows scheduler to be
>> > aware of cpu capacity reduced due to thermal throttling.
>> > arch_set_thermal_pressure feeds the capped capacity to the scheduler and
>> > hence the feature makes sense only if arch_set_thermal_pressure is
>> > implemented. Having said that arch_set_thermal_pressure is implemented
>> > in arch_topology driver for arm and arm64 platforms. But the feature
>> > itself is not bound to arm/arm64 platforms. So it would make it wrong to
>> > add a "depends on (ARM || ARM64) option."
>> >
>> > I agree with Vincent that allowing user to choose this option is
>> > probably not the best. IMO, this should be enabled by default in arm64
>> > defconfig considering both GENERIC_ARCH_TOPOLOGY and CPU_FREQ_THERMAL
>> > are enabled by default.
>>
>> Right, I had skimmed over that but it probably does make more sense not
>> to bother users with it.
>>
>> > So if it is acceptable three things to be done are:
>> > 1. Add the help text.
>> > 2. Don't allow SCHED_THERMAL_PRESSURE configurable by user
>> > 3. Enable it by default in arm64 defconfig
>>
>> ... and arm as well, I suppose?
>
> If it's not a user visible option, then there's no point it being in
> defconfig.

Right, s/defconfig/arch kconfig/ or somesuch.

Re: v5.7: new core kernel option missing help text

[Valentin Schneider]

On 04/06/20 14:05, Thara Gopinath wrote:
> On Thu, 4 Jun 2020 at 06:56, Valentin Schneider <valentin.schneider@arm.com>
>>
>> Right, s/defconfig/arch kconfig/ or somesuch.
>>
>
>  CPU_FREQ_THERMAL also has to be enabled for this to be effective.
> Since arm64 defconfig enables  CPU_FREQ_THERMAL  (by enabling CPU_THERMAL),
> it should be ok to enable it in arm64/Kconfig. (same with arm/Kconfig)
>
> Another option is to select the  SCHED_THERMAL_PRESSURE when
> CPU_FREQ_THERMAL
> is enabled in drivers/thermal/Kconfig.
>

So interestingly while arch_set_thermal_pressure() (which just writes to a
pcpu variable) is defined in sched/core.c, arch_scale_thermal_pressure()
(which just returns aforementionned pcpu variable) is defined in
arch_topology...

I'm thinking at this point we might as well turn the
arch_scale_thermal_pressure() stub into what arch_topology does. This would
effectively let any architecture use thermal pressure, providing they use
cpufreq cooling.

If we want to keep changes contained to Kconfigs, for now I think the
safest would be:

---
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 16fbf74030fe..1e92080dc275 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -46,6 +46,7 @@ config ARM
        select EDAC_ATOMIC_SCRUB
        select GENERIC_ALLOCATOR
        select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
+	select SCHED_THERMAL_PRESSURE if GENERIC_ARCH_TOPOLOGY
        select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
        select GENERIC_CLOCKEVENTS_BROADCAST if SMP
        select GENERIC_CPU_AUTOPROBE
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 552d36cacc05..cc1944fbae51 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -98,6 +98,7 @@ config ARM64
        select FRAME_POINTER
        select GENERIC_ALLOCATOR
        select GENERIC_ARCH_TOPOLOGY
+	select SCHED_THERMAL_PRESSURE
        select GENERIC_CLOCKEVENTS
        select GENERIC_CLOCKEVENTS_BROADCAST
        select GENERIC_CPU_AUTOPROBE
diff --git a/init/Kconfig b/init/Kconfig
index 74a5ac65644f..ba846f6e805b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -439,8 +439,11 @@ config HAVE_SCHED_AVG_IRQ
        depends on SMP

 config SCHED_THERMAL_PRESSURE
-	bool "Enable periodic averaging of thermal pressure"
+	def_bool n
        depends on SMP
+	depends on CPU_FREQ_THERMAL
+	help
+	  <helpful thing here>

 config BSD_PROCESS_ACCT
        bool "BSD Process Accounting"
---



> Warm Regards
> Thara

Re: v5.7: new core kernel option missing help text

[Thara Gopinath]

On 6/4/20 11:38 AM, Valentin Schneider wrote:
> 
> On 04/06/20 14:05, Thara Gopinath wrote:
>> On Thu, 4 Jun 2020 at 06:56, Valentin Schneider <valentin.schneider@arm.com>
>>>
>>> Right, s/defconfig/arch kconfig/ or somesuch.
>>>
>>
>>   CPU_FREQ_THERMAL also has to be enabled for this to be effective.
>> Since arm64 defconfig enables  CPU_FREQ_THERMAL  (by enabling CPU_THERMAL),
>> it should be ok to enable it in arm64/Kconfig. (same with arm/Kconfig)
>>
>> Another option is to select the  SCHED_THERMAL_PRESSURE when
>> CPU_FREQ_THERMAL
>> is enabled in drivers/thermal/Kconfig.
>>
> 
> So interestingly while arch_set_thermal_pressure() (which just writes to a
> pcpu variable) is defined in sched/core.c, arch_scale_thermal_pressure()
> (which just returns aforementionned pcpu variable) is defined in
> arch_topology...
> 
> I'm thinking at this point we might as well turn the
> arch_scale_thermal_pressure() stub into what arch_topology does. This would
> effectively let any architecture use thermal pressure, providing they use
> cpufreq cooling.
> 
> If we want to keep changes contained to Kconfigs, for now I think the
> safest would be:

Thanks Valentin. This looks good to me for now. Although, I want to 
state that the thermal pressure can be set by some other entity other 
than cpufreq cooling as well. But currently only cpufreq cooling handles 
it. So for now the below looks good.

> 
> ---
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 16fbf74030fe..1e92080dc275 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -46,6 +46,7 @@ config ARM
>          select EDAC_ATOMIC_SCRUB
>          select GENERIC_ALLOCATOR
>          select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
> +	select SCHED_THERMAL_PRESSURE if GENERIC_ARCH_TOPOLOGY
>          select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
>          select GENERIC_CLOCKEVENTS_BROADCAST if SMP
>          select GENERIC_CPU_AUTOPROBE
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 552d36cacc05..cc1944fbae51 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -98,6 +98,7 @@ config ARM64
>          select FRAME_POINTER
>          select GENERIC_ALLOCATOR
>          select GENERIC_ARCH_TOPOLOGY
> +	select SCHED_THERMAL_PRESSURE
>          select GENERIC_CLOCKEVENTS
>          select GENERIC_CLOCKEVENTS_BROADCAST
>          select GENERIC_CPU_AUTOPROBE
> diff --git a/init/Kconfig b/init/Kconfig
> index 74a5ac65644f..ba846f6e805b 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -439,8 +439,11 @@ config HAVE_SCHED_AVG_IRQ
>          depends on SMP
> 
>   config SCHED_THERMAL_PRESSURE
> -	bool "Enable periodic averaging of thermal pressure"
> +	def_bool n
>          depends on SMP
> +	depends on CPU_FREQ_THERMAL
> +	help
> +	  <helpful thing here>
> 
>   config BSD_PROCESS_ACCT
>          bool "BSD Process Accounting"
> ---
> 
> 
> 
>> Warm Regards
>> Thara

-- 
Warm Regards
Thara

Re: v5.7: new core kernel option missing help text

[Vincent Guittot]

On Thu, 4 Jun 2020 at 17:38, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
>
> On 04/06/20 14:05, Thara Gopinath wrote:
> > On Thu, 4 Jun 2020 at 06:56, Valentin Schneider <valentin.schneider@arm.com>
> >>
> >> Right, s/defconfig/arch kconfig/ or somesuch.
> >>
> >
> >  CPU_FREQ_THERMAL also has to be enabled for this to be effective.
> > Since arm64 defconfig enables  CPU_FREQ_THERMAL  (by enabling CPU_THERMAL),
> > it should be ok to enable it in arm64/Kconfig. (same with arm/Kconfig)
> >
> > Another option is to select the  SCHED_THERMAL_PRESSURE when
> > CPU_FREQ_THERMAL
> > is enabled in drivers/thermal/Kconfig.
> >
>
> So interestingly while arch_set_thermal_pressure() (which just writes to a
> pcpu variable) is defined in sched/core.c, arch_scale_thermal_pressure()
> (which just returns aforementionned pcpu variable) is defined in
> arch_topology...
>
> I'm thinking at this point we might as well turn the
> arch_scale_thermal_pressure() stub into what arch_topology does. This would
> effectively let any architecture use thermal pressure, providing they use
> cpufreq cooling.
>
> If we want to keep changes contained to Kconfigs, for now I think the
> safest would be:
>
> ---
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 16fbf74030fe..1e92080dc275 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -46,6 +46,7 @@ config ARM
>         select EDAC_ATOMIC_SCRUB
>         select GENERIC_ALLOCATOR
>         select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
> +       select SCHED_THERMAL_PRESSURE if GENERIC_ARCH_TOPOLOGY

I think that SCHED_THERMAL_PRESSURE depends on ARM_CPU_TOPOLOGY but
not on GENERIC_ARCH_TOPOLOGY.
ARM_CPU_TOPOLOGY is used to define arch_scale_thermal_pressure for arm
architecture
and we only use the header file of the generic arch_topology.h.
Function like arch_set_thermal_pressure() is in sched/core.c

>         select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
>         select GENERIC_CLOCKEVENTS_BROADCAST if SMP
>         select GENERIC_CPU_AUTOPROBE
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 552d36cacc05..cc1944fbae51 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -98,6 +98,7 @@ config ARM64
>         select FRAME_POINTER
>         select GENERIC_ALLOCATOR
>         select GENERIC_ARCH_TOPOLOGY
> +       select SCHED_THERMAL_PRESSURE
>         select GENERIC_CLOCKEVENTS
>         select GENERIC_CLOCKEVENTS_BROADCAST
>         select GENERIC_CPU_AUTOPROBE
> diff --git a/init/Kconfig b/init/Kconfig
> index 74a5ac65644f..ba846f6e805b 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -439,8 +439,11 @@ config HAVE_SCHED_AVG_IRQ
>         depends on SMP
>
>  config SCHED_THERMAL_PRESSURE
> -       bool "Enable periodic averaging of thermal pressure"
> +       def_bool n
>         depends on SMP
> +       depends on CPU_FREQ_THERMAL
> +       help
> +         <helpful thing here>
>
>  config BSD_PROCESS_ACCT
>         bool "BSD Process Accounting"
> ---
>
>
>
> > Warm Regards
> > Thara

Re: v5.7: new core kernel option missing help text

[Russell King - ARM Linux admin]

On Thu, Jun 04, 2020 at 04:38:40PM +0100, Valentin Schneider wrote:
> diff --git a/init/Kconfig b/init/Kconfig
> index 74a5ac65644f..ba846f6e805b 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -439,8 +439,11 @@ config HAVE_SCHED_AVG_IRQ
>         depends on SMP
> 
>  config SCHED_THERMAL_PRESSURE
> -	bool "Enable periodic averaging of thermal pressure"
> +	def_bool n

You don't need to specify this default, as the default default is "n".
"bool" will do.

You should never need to use "def_bool n" or "default n" for anything
this simple. There are more complex cases where there may be multiple
conditional "default" lines where it may be necessary, but the majority
of cases where people use these are completely unnecessary.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: v5.7: new core kernel option missing help text

[Valentin Schneider]

On 05/06/20 08:03, Vincent Guittot wrote:
> On Thu, 4 Jun 2020 at 17:38, Valentin Schneider
> <valentin.schneider@arm.com> wrote:
>> ---
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 16fbf74030fe..1e92080dc275 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -46,6 +46,7 @@ config ARM
>>         select EDAC_ATOMIC_SCRUB
>>         select GENERIC_ALLOCATOR
>>         select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
>> +       select SCHED_THERMAL_PRESSURE if GENERIC_ARCH_TOPOLOGY
>
> I think that SCHED_THERMAL_PRESSURE depends on ARM_CPU_TOPOLOGY but
> not on GENERIC_ARCH_TOPOLOGY.
> ARM_CPU_TOPOLOGY is used to define arch_scale_thermal_pressure for arm
> architecture
> and we only use the header file of the generic arch_topology.h.
> Function like arch_set_thermal_pressure() is in sched/core.c
>

You're right, oh well... Let me spend a bit more time on this and I'll
send actual patches.

Re: v5.7: new core kernel option missing help text

[Valentin Schneider]

On 05/06/20 09:15, Russell King - ARM Linux admin wrote:
> On Thu, Jun 04, 2020 at 04:38:40PM +0100, Valentin Schneider wrote:
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 74a5ac65644f..ba846f6e805b 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -439,8 +439,11 @@ config HAVE_SCHED_AVG_IRQ
>>         depends on SMP
>>
>>  config SCHED_THERMAL_PRESSURE
>> -	bool "Enable periodic averaging of thermal pressure"
>> +	def_bool n
>
> You don't need to specify this default, as the default default is "n".
> "bool" will do.
>
> You should never need to use "def_bool n" or "default n" for anything
> this simple. There are more complex cases where there may be multiple
> conditional "default" lines where it may be necessary, but the majority
> of cases where people use these are completely unnecessary.

Right, thanks!