Hi
I found some bugs when integrating the 3.18 EAS backport [1] into the LSK 3.18 based kernel I look after for ARM's Juno and Versatile Express boards. These patches are my fixes for those bugs, I don't know whether they are useful or relevent to other versions of EAS. If they are OK, I guess I should at least add them to [1] ?
---------------------------------------------------------------- Jon Medhurst (3): arm: Fix build error "conflicting types for 'scale_cpu_capacity'" arm: Fix #if/#ifdef mixup in topology.c sched/tune: Avoid null pointer dereference in schedtune_add_cluster_nrg
arch/arm/include/asm/topology.h | 1 + arch/arm/kernel/topology.c | 2 +- kernel/sched/tune.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)
[1] http://git.linaro.org/arm/eas/kernel.git/shortlog/refs/heads/linux-3.18-eas-...
If CONFIG_CPU_FREQ is not defined then we don't include a definition for struct sched_domain, which leads to a stream of build warnings and an error...
./arch/arm/include/asm/topology.h:33:48: warning: 'struct sched_domain' declared inside parameter list extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu); ^ ./arch/arm/include/asm/topology.h:33:48: warning: its scope is only this definition or declaration, which is probably not what you want arch/arm/kernel/topology.c:45:15: error: conflicting types for 'scale_cpu_capacity' unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu)
Signed-off-by: Jon Medhurst tixy@linaro.org --- arch/arm/include/asm/topology.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h index 94d3265..a6aa17b 100644 --- a/arch/arm/include/asm/topology.h +++ b/arch/arm/include/asm/topology.h @@ -28,6 +28,7 @@ const struct cpumask *cpu_coregroup_mask(int cpu); #include <linux/cpufreq.h> #define arch_scale_freq_capacity cpufreq_scale_freq_capacity #endif +struct sched_domain; #define arch_scale_cpu_capacity scale_cpu_capacity extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu);
-- 2.1.4
This fixes the warning...
arch/arm/kernel/topology.c: In function 'scale_cpu_capacity': arch/arm/kernel/topology.c:47:5: warning: "CONFIG_CPU_FREQ" is not defined [-Wundef]
Signed-off-by: Jon Medhurst tixy@linaro.org --- arch/arm/kernel/topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c index 18ea2fd..7ee74a0 100644 --- a/arch/arm/kernel/topology.c +++ b/arch/arm/kernel/topology.c @@ -44,7 +44,7 @@ static DEFINE_PER_CPU(unsigned long, cpu_scale);
unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu) { -#if CONFIG_CPU_FREQ +#ifdef CONFIG_CPU_FREQ unsigned long max_freq_scale = cpufreq_scale_max_freq_capacity(cpu);
return per_cpu(cpu_scale, cpu) * max_freq_scale >> SCHED_CAPACITY_SHIFT; -- 2.1.4
A CPU's scheduler domain doesn't always have a parent e.g. if we only have one cluster present or enabled in the system. --- kernel/sched/tune.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/tune.c b/kernel/sched/tune.c index 6edfd44..53c6de56 100644 --- a/kernel/sched/tune.c +++ b/kernel/sched/tune.c @@ -719,7 +719,7 @@ schedtune_add_cluster_nrg( * Assume we have EM data only at the CPU and * the upper CLUSTER level */ - BUG_ON(!cpumask_equal( + BUG_ON(sd2->parent && !cpumask_equal( sched_group_cpus(sg), sched_group_cpus(sd2->parent->groups) )); -- 2.1.4
On Thu, Jun 02, 2016 at 01:18:07PM +0100, Jon Medhurst (Tixy) wrote:
If CONFIG_CPU_FREQ is not defined then we don't include a definition for struct sched_domain, which leads to a stream of build warnings and an error...
./arch/arm/include/asm/topology.h:33:48: warning: 'struct sched_domain' declared inside parameter list extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu); ^ ./arch/arm/include/asm/topology.h:33:48: warning: its scope is only this definition or declaration, which is probably not what you want arch/arm/kernel/topology.c:45:15: error: conflicting types for 'scale_cpu_capacity' unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu)
Signed-off-by: Jon Medhurst tixy@linaro.org
arch/arm/include/asm/topology.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h index 94d3265..a6aa17b 100644 --- a/arch/arm/include/asm/topology.h +++ b/arch/arm/include/asm/topology.h @@ -28,6 +28,7 @@ const struct cpumask *cpu_coregroup_mask(int cpu); #include <linux/cpufreq.h> #define arch_scale_freq_capacity cpufreq_scale_freq_capacity #endif +struct sched_domain; #define arch_scale_cpu_capacity scale_cpu_capacity extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu);
Yes, arm64 has added it already. But arm has missed the definition.
-- 2.1.4
eas-dev mailing list eas-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/eas-dev
On Thu, Jun 02, 2016 at 01:18:09PM +0100, Jon Medhurst (Tixy) wrote:
A CPU's scheduler domain doesn't always have a parent e.g. if we only have one cluster present or enabled in the system.
kernel/sched/tune.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/tune.c b/kernel/sched/tune.c index 6edfd44..53c6de56 100644 --- a/kernel/sched/tune.c +++ b/kernel/sched/tune.c @@ -719,7 +719,7 @@ schedtune_add_cluster_nrg( * Assume we have EM data only at the CPU and * the upper CLUSTER level */
BUG_ON(!cpumask_equal(
BUG_ON(sd2->parent && !cpumask_equal( sched_group_cpus(sg), sched_group_cpus(sd2->parent->groups)
Just curious, this issue can be fixed by creating root schedule domain before Deitmar has meantioned. right?
));-- 2.1.4
eas-dev mailing list eas-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/eas-dev
On Thu, 2016-06-02 at 22:30 +0800, Leo Yan wrote:
On Thu, Jun 02, 2016 at 01:18:09PM +0100, Jon Medhurst (Tixy) wrote:
A CPU's scheduler domain doesn't always have a parent e.g. if we only have one cluster present or enabled in the system.
kernel/sched/tune.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/tune.c b/kernel/sched/tune.c index 6edfd44..53c6de56 100644 --- a/kernel/sched/tune.c +++ b/kernel/sched/tune.c @@ -719,7 +719,7 @@ schedtune_add_cluster_nrg( * Assume we have EM data only at the CPU and * the upper CLUSTER level */
BUG_ON(!cpumask_equal(
BUG_ON(sd2->parent && !cpumask_equal( sched_group_cpus(sg), sched_group_cpus(sd2->parent->groups)Just curious, this issue can be fixed by creating root schedule domain before Deitmar has meantioned. right?
I haven't followed any of the EAS development nor looked at the code in any detail, just been trying to fix up builds errors and OOPes I hit.
But it sounds like you're saying that if all domains were in a single tree with a single root then this patch wouldn't be needed. If so, I'd agree, as all nodes (except the root) would have a valid parent. Though you would need to make sure there was still a separate root note added even for the case where the system has a single CPU.
There would also be the consideration that any code walking the tree would have an extra node to traverse (don't know if that happens on any hot paths).
-- Tixy
On Thu, Jun 02, 2016 at 01:18:07PM +0100, Jon Medhurst wrote:
If CONFIG_CPU_FREQ is not defined then we don't include a definition for struct sched_domain, which leads to a stream of build warnings and an error...
./arch/arm/include/asm/topology.h:33:48: warning: 'struct sched_domain' declared inside parameter list extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu); ^ ./arch/arm/include/asm/topology.h:33:48: warning: its scope is only this definition or declaration, which is probably not what you want arch/arm/kernel/topology.c:45:15: error: conflicting types for 'scale_cpu_capacity' unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu)
Signed-off-by: Jon Medhurst tixy@linaro.org
arch/arm/include/asm/topology.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h index 94d3265..a6aa17b 100644 --- a/arch/arm/include/asm/topology.h +++ b/arch/arm/include/asm/topology.h @@ -28,6 +28,7 @@ const struct cpumask *cpu_coregroup_mask(int cpu); #include <linux/cpufreq.h> #define arch_scale_freq_capacity cpufreq_scale_freq_capacity #endif +struct sched_domain; #define arch_scale_cpu_capacity scale_cpu_capacity extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu);
I ran into this a while back and wrote the attached patch - I think it may be cleaner than defining struct sched_domain again.
thanks, Steve
On Thu, 2016-06-02 at 10:48 -0700, Steve Muckle wrote:
I ran into this a while back and wrote the attached patch - I think it may be cleaner than defining struct sched_domain again.
Seems strange to me to include cpufreq.h to get its copy of a forward declaration of a struct defined in sched.h. And the arm64 version of this code does things differently again.
Guess it doesn't matter though as this is experimental code, backported to old Linux versions, and doesn't have a maintainer or canonical 'upstream' source code repository.
-- Tixy
Hi,
Personally I don't have a strong opinion on how it should be fixed - this is likely to be temporary IMO as we will probably soon want to add extracting energy model data in the dtb like we do for arm64, so that we can have more similarity between the two in a post-hmp world. That will necessitate including sched_energy.h sooner or later I think, which will pull in sched.h and get the forward declaration.
Best Regards,
--Chris
________________________________________ From: Jon Medhurst (Tixy) tixy@linaro.org Sent: 03 June 2016 14:37 To: Steve Muckle Cc: Chris Redpath; eas-dev@lists.linaro.org Subject: Re: [Eas-dev] [PATCH 1/3] arm: Fix build error "conflicting types for 'scale_cpu_capacity'"
On Thu, 2016-06-02 at 10:48 -0700, Steve Muckle wrote:
I ran into this a while back and wrote the attached patch - I think it may be cleaner than defining struct sched_domain again.
Seems strange to me to include cpufreq.h to get its copy of a forward declaration of a struct defined in sched.h. And the arm64 version of this code does things differently again.
Guess it doesn't matter though as this is experimental code, backported to old Linux versions, and doesn't have a maintainer or canonical 'upstream' source code repository.
-- Tixy IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ack from me :)
________________________________________ From: Jon Medhurst tixy@linaro.org Sent: 02 June 2016 13:18 To: Chris Redpath Cc: eas-dev@lists.linaro.org Subject: [PATCH 2/3] arm: Fix #if/#ifdef mixup in topology.c
This fixes the warning...
arch/arm/kernel/topology.c: In function 'scale_cpu_capacity': arch/arm/kernel/topology.c:47:5: warning: "CONFIG_CPU_FREQ" is not defined [-Wundef]
Signed-off-by: Jon Medhurst tixy@linaro.org --- arch/arm/kernel/topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c index 18ea2fd..7ee74a0 100644 --- a/arch/arm/kernel/topology.c +++ b/arch/arm/kernel/topology.c @@ -44,7 +44,7 @@ static DEFINE_PER_CPU(unsigned long, cpu_scale);
unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu) { -#if CONFIG_CPU_FREQ +#ifdef CONFIG_CPU_FREQ unsigned long max_freq_scale = cpufreq_scale_max_freq_capacity(cpu);
return per_cpu(cpu_scale, cpu) * max_freq_scale >> SCHED_CAPACITY_SHIFT; -- 2.1.4
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Fri, Jun 03, 2016 at 02:37:58PM +0100, Jon Medhurst (Tixy) wrote:
On Thu, 2016-06-02 at 10:48 -0700, Steve Muckle wrote:
I ran into this a while back and wrote the attached patch - I think it may be cleaner than defining struct sched_domain again.
Seems strange to me to include cpufreq.h to get its copy of a forward declaration of a struct defined in sched.h. And the arm64 version of this code does things differently again.
Guess it doesn't matter though as this is experimental code, backported to old Linux versions, and doesn't have a maintainer or canonical 'upstream' source code repository.
Perhaps it's a matter of preference... The forward declarations of a sched struct outside sched.h seemed ugly enough to me to be worth minimizing, so I figured I'd just move the existing #include <cpufreq.h>. But I agree it's not very consequential and I'm not maintaining this code :) .
-
Chris Redpath -
Jon Medhurst -
Jon Medhurst (Tixy)
-
Leo Yan -
Steve Muckle