[Resend PATCH] topology: Set capacity_freq_ref in all cases

[Resend PATCH] topology: Set capacity_freq_ref in all cases

[Vincent Guittot]

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 <vincent.guittot@linaro.org>
---

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

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

[Sudeep Holla]

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 <sudeep.holla@arm.com>

--
Regards,
Sudeep

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

[Dietmar Eggemann]

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 <vincent.guittot@linaro.org>
> ---

Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

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

[...]

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

[Pierre Gondois]

Tested-by: Pierre Gondois <pierre.gondois@arm.com>

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 <vincent.guittot@linaro.org>
> ---
> 
> 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))

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

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 <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 1294 bytes --]

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 <vincent.guittot@linaro.org>

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 <paul.barker.ct@bp.renesas.com>

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 1830 bytes --]

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 <vincent.guittot@linaro.org>
> 
> 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 <paul.barker.ct@bp.renesas.com>

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Greg KH]

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 <vincent.guittot@linaro.org>
> > 
> > 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 <paul.barker.ct@bp.renesas.com>
> 
> 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

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

[Erico Nunes]

On Wed, Jan 17, 2024 at 8:06 PM Vincent Guittot
<vincent.guittot@linaro.org> 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 <nunes.erico@gmail.com>

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

[Vincent Guittot]

On Wed, 31 Jan 2024 at 14:56, Erico Nunes <nunes.erico@gmail.com> wrote:
>
> On Wed, Jan 17, 2024 at 8:06 PM Vincent Guittot
> <vincent.guittot@linaro.org> 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 <nunes.erico@gmail.com>