2024-01-17 19:08:17

by Vincent Guittot

[permalink] [raw]
Subject: [Resend PATCH] topology: Set capacity_freq_ref in all cases

If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
normalization step which includes setting per_cpu capacity_freq_ref.
Always register the notifier but skip the capacity normalization if
raw_capacity is null.

Fixes: 9942cb22ea45 ("sched/topology: Add a new arch_scale_freq_ref() method")
Signed-off-by: Vincent Guittot <[email protected]>
---

Resend with correct email address for Sudeep

drivers/base/arch_topology.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 5aaa0865625d..2aa0c6425290 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -417,9 +417,6 @@ init_cpu_capacity_callback(struct notifier_block *nb,
struct cpufreq_policy *policy = data;
int cpu;

- if (!raw_capacity)
- return 0;
-
if (val != CPUFREQ_CREATE_POLICY)
return 0;

@@ -436,9 +433,11 @@ init_cpu_capacity_callback(struct notifier_block *nb,
}

if (cpumask_empty(cpus_to_visit)) {
- topology_normalize_cpu_scale();
- schedule_work(&update_topology_flags_work);
- free_raw_capacity();
+ if (raw_capacity) {
+ topology_normalize_cpu_scale();
+ schedule_work(&update_topology_flags_work);
+ free_raw_capacity();
+ }
pr_debug("cpu_capacity: parsing done\n");
schedule_work(&parsing_done_work);
}
@@ -458,7 +457,7 @@ static int __init register_cpufreq_notifier(void)
* On ACPI-based systems skip registering cpufreq notifier as cpufreq
* information is not needed for cpu capacity initialization.
*/
- if (!acpi_disabled || !raw_capacity)
+ if (!acpi_disabled)
return -EINVAL;

if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL))
--
2.34.1



2024-01-18 09:42:28

by Sudeep Holla

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

On Wed, Jan 17, 2024 at 08:05:45PM +0100, Vincent Guittot wrote:
> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> normalization step which includes setting per_cpu capacity_freq_ref.
> Always register the notifier but skip the capacity normalization if
> raw_capacity is null.
>

LGTM,

Acked-by: Sudeep Holla <[email protected]>

--
Regards,
Sudeep

2024-01-18 12:10:33

by Dietmar Eggemann

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

On 17/01/2024 20:05, Vincent Guittot wrote:
> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> normalization step which includes setting per_cpu capacity_freq_ref.
> Always register the notifier but skip the capacity normalization if
> raw_capacity is null.
>
> Fixes: 9942cb22ea45 ("sched/topology: Add a new arch_scale_freq_ref() method")
> Signed-off-by: Vincent Guittot <[email protected]>
> ---

Reviewed-by: Dietmar Eggemann <[email protected]>
Tested-by: Dietmar Eggemann <[email protected]>

(on h960 w/ and w/o capacity-dmips-mhz in dts file)

[...]

2024-01-18 17:37:28

by Pierre Gondois

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

Tested-by: Pierre Gondois <[email protected]>

On 1/17/24 20:05, Vincent Guittot wrote:
> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> normalization step which includes setting per_cpu capacity_freq_ref.
> Always register the notifier but skip the capacity normalization if
> raw_capacity is null.
>
> Fixes: 9942cb22ea45 ("sched/topology: Add a new arch_scale_freq_ref() method")
> Signed-off-by: Vincent Guittot <[email protected]>
> ---
>
> Resend with correct email address for Sudeep
>
> drivers/base/arch_topology.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 5aaa0865625d..2aa0c6425290 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -417,9 +417,6 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> struct cpufreq_policy *policy = data;
> int cpu;
>
> - if (!raw_capacity)
> - return 0;
> -
> if (val != CPUFREQ_CREATE_POLICY)
> return 0;
>
> @@ -436,9 +433,11 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> }
>
> if (cpumask_empty(cpus_to_visit)) {
> - topology_normalize_cpu_scale();
> - schedule_work(&update_topology_flags_work);
> - free_raw_capacity();
> + if (raw_capacity) {
> + topology_normalize_cpu_scale();
> + schedule_work(&update_topology_flags_work);
> + free_raw_capacity();
> + }
> pr_debug("cpu_capacity: parsing done\n");
> schedule_work(&parsing_done_work);
> }
> @@ -458,7 +457,7 @@ static int __init register_cpufreq_notifier(void)
> * On ACPI-based systems skip registering cpufreq notifier as cpufreq
> * information is not needed for cpu capacity initialization.
> */
> - if (!acpi_disabled || !raw_capacity)
> + if (!acpi_disabled)
> return -EINVAL;
>
> if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL))

2024-01-23 15:27:21

by Mark Brown

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

On Wed, Jan 17, 2024 at 08:05:45PM +0100, Vincent Guittot wrote:
> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> normalization step which includes setting per_cpu capacity_freq_ref.
> Always register the notifier but skip the capacity normalization if
> raw_capacity is null.

This appears to resolve the issues in mainline with kselftest-dt on
potato:

Tested-by: Mark Brown <[email protected]>


Attachments:
(No filename) (436.00 B)
signature.asc (499.00 B)
Download all attachments

2024-01-23 16:24:09

by Paul Barker

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

On 17/01/2024 19:05, Vincent Guittot wrote:
> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> normalization step which includes setting per_cpu capacity_freq_ref.
> Always register the notifier but skip the capacity normalization if
> raw_capacity is null.
>
> Fixes: 9942cb22ea45 ("sched/topology: Add a new arch_scale_freq_ref() method")
> Signed-off-by: Vincent Guittot <[email protected]>

We've had some Ethernet performance issues with linux-next and v6.8-rc1
on the Renesas RZ/G2L and RZ/G2UL arm64 SoCs. I've confirmed that the
CPU frequency is stuck at the minimum (150MHz) in v6.8-rc1, even when
running iperf3. Applying this patch allows the SoC to switch up the the
maximum frequency (1200MHz) when needed and fixes our Ethernet
performance.

iperf3 results in Mbps for RZ/G2L SMARC evaluation board:
TCP TX TCP RX UDP TX UDP RX
v6.8-rc1 255 175 102 (*)
+this patch 874 650 802 948

(*) Testing UDP RX in v6.8-rc1 with a server sending traffic at 1Gbps
locks up our NFS rootfs mount so we don't get any result at all.

Results with v6.8-rc1 + this patch are in line with what we see in v6.7.

Tested-by: Paul Barker <[email protected]>


Attachments:
OpenPGP_0x27F4B3459F002257.asc (3.49 kB)
OpenPGP public key
OpenPGP_signature.asc (243.00 B)
OpenPGP digital signature
Download all attachments

2024-01-31 13:57:06

by Erico Nunes

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

On Wed, Jan 17, 2024 at 8:06 PM Vincent Guittot
<[email protected]> wrote:
>
> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> normalization step which includes setting per_cpu capacity_freq_ref.
> Always register the notifier but skip the capacity normalization if
> raw_capacity is null.

I bisected an issue of cpufreq no longer working on v6.8-rc on a TI
SK-AM62 board and it pointed me to b3edde44e5d4 ("cpufreq/schedutil:
Use a fixed reference frequency").
This commit does fix the issue but doesn't appear to be merged
anywhere yet. It would be good to have this fixed before 6.8 release.

Tested-by: Erico Nunes <[email protected]>

2024-01-31 15:09:20

by Vincent Guittot

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

On Wed, 31 Jan 2024 at 14:56, Erico Nunes <[email protected]> wrote:
>
> On Wed, Jan 17, 2024 at 8:06 PM Vincent Guittot
> <[email protected]> wrote:
> >
> > If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> > normalization step which includes setting per_cpu capacity_freq_ref.
> > Always register the notifier but skip the capacity normalization if
> > raw_capacity is null.
>
> I bisected an issue of cpufreq no longer working on v6.8-rc on a TI
> SK-AM62 board and it pointed me to b3edde44e5d4 ("cpufreq/schedutil:
> Use a fixed reference frequency").
> This commit does fix the issue but doesn't appear to be merged
> anywhere yet. It would be good to have this fixed before 6.8 release.

Greg has queued the patch which should show up in the next release of
the linux-next tree

>
> Tested-by: Erico Nunes <[email protected]>

2024-02-13 11:07:07

by Paul Barker

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

Hi Greg,

On 23/01/2024 16:23, Paul Barker wrote:
> On 17/01/2024 19:05, Vincent Guittot wrote:
>> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
>> normalization step which includes setting per_cpu capacity_freq_ref.
>> Always register the notifier but skip the capacity normalization if
>> raw_capacity is null.
>>
>> Fixes: 9942cb22ea45 ("sched/topology: Add a new arch_scale_freq_ref() method")
>> Signed-off-by: Vincent Guittot <[email protected]>
>
> We've had some Ethernet performance issues with linux-next and v6.8-rc1
> on the Renesas RZ/G2L and RZ/G2UL arm64 SoCs. I've confirmed that the
> CPU frequency is stuck at the minimum (150MHz) in v6.8-rc1, even when
> running iperf3. Applying this patch allows the SoC to switch up the the
> maximum frequency (1200MHz) when needed and fixes our Ethernet
> performance.
>
> iperf3 results in Mbps for RZ/G2L SMARC evaluation board:
> TCP TX TCP RX UDP TX UDP RX
> v6.8-rc1 255 175 102 (*)
> +this patch 874 650 802 948
>
> (*) Testing UDP RX in v6.8-rc1 with a server sending traffic at 1Gbps
> locks up our NFS rootfs mount so we don't get any result at all.
>
> Results with v6.8-rc1 + this patch are in line with what we see in v6.7.
>
> Tested-by: Paul Barker <[email protected]>

I see this patch landed in the 'driver-core-linus' branch of
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
on 2024-01-30 as commit 98323e9d7017, however it looks like that hasn't
been merged to Linus' tree yet. In v6.8-rc4 I see the same issue on the
RZ/G2L with the CPU frequency stuck at 150MHz.

Is there anything I need to do to help get this patch merged before v6.8
is released?

Thanks,
Paul


Attachments:
OpenPGP_0x27F4B3459F002257.asc (3.49 kB)
OpenPGP public key
OpenPGP_signature.asc (243.00 B)
OpenPGP digital signature
Download all attachments

2024-02-13 11:45:31

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [Resend PATCH] topology: Set capacity_freq_ref in all cases

On Tue, Feb 13, 2024 at 11:06:48AM +0000, Paul Barker wrote:
> Hi Greg,
>
> On 23/01/2024 16:23, Paul Barker wrote:
> > On 17/01/2024 19:05, Vincent Guittot wrote:
> >> If "capacity-dmips-mhz" is not set, raw_capacity is null and we skip the
> >> normalization step which includes setting per_cpu capacity_freq_ref.
> >> Always register the notifier but skip the capacity normalization if
> >> raw_capacity is null.
> >>
> >> Fixes: 9942cb22ea45 ("sched/topology: Add a new arch_scale_freq_ref() method")
> >> Signed-off-by: Vincent Guittot <[email protected]>
> >
> > We've had some Ethernet performance issues with linux-next and v6.8-rc1
> > on the Renesas RZ/G2L and RZ/G2UL arm64 SoCs. I've confirmed that the
> > CPU frequency is stuck at the minimum (150MHz) in v6.8-rc1, even when
> > running iperf3. Applying this patch allows the SoC to switch up the the
> > maximum frequency (1200MHz) when needed and fixes our Ethernet
> > performance.
> >
> > iperf3 results in Mbps for RZ/G2L SMARC evaluation board:
> > TCP TX TCP RX UDP TX UDP RX
> > v6.8-rc1 255 175 102 (*)
> > +this patch 874 650 802 948
> >
> > (*) Testing UDP RX in v6.8-rc1 with a server sending traffic at 1Gbps
> > locks up our NFS rootfs mount so we don't get any result at all.
> >
> > Results with v6.8-rc1 + this patch are in line with what we see in v6.7.
> >
> > Tested-by: Paul Barker <[email protected]>
>
> I see this patch landed in the 'driver-core-linus' branch of
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> on 2024-01-30 as commit 98323e9d7017, however it looks like that hasn't
> been merged to Linus' tree yet. In v6.8-rc4 I see the same issue on the
> RZ/G2L with the CPU frequency stuck at 150MHz.
>
> Is there anything I need to do to help get this patch merged before v6.8
> is released?

It will get there, sorry, been traveling for weeks and will catch up and
send my trees to Linus later this week.

greg k-h