| Message ID | 1403658329-13196-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, 2014-06-24 at 18:05 -0700, Guenter Roeck wrote: .../... > Drop 'const' from the function declarations to fix the problem. > > The fix for all three patches has to be applied together to avoid > compilation failures for the affected architectures. > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@kernel.org> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Vincent Guittot <vincent.guittot@linaro.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v2: Fix problem in all affected architectures with a single patch > to avoid compilation errors.
On 25 June 2014 03:05, Guenter Roeck <linux@roeck-us.net> wrote: > Commit 143e1e28cb (sched: Rework sched_domain topology definition) > introduced a number of functions with a return value of 'const int'. > gcc doesn't know what to do with that and, if the kernel is compiled > with W=1, complains with the following warnings whenever sched.h > is included. > > include/linux/sched.h:875:25: warning: > type qualifiers ignored on function return type > include/linux/sched.h:882:25: warning: > type qualifiers ignored on function return type > include/linux/sched.h:889:25: warning: > type qualifiers ignored on function return type > include/linux/sched.h:1002:21: warning: > type qualifiers ignored on function return type > > Commits fb2aa855 (sched, ARM: Create a dedicated scheduler topology table) > and 607b45e9a (sched, powerpc: Create a dedicated topology table) introduce > the same warning in the arm and powerpc code. > > Drop 'const' from the function declarations to fix the problem. > > The fix for all three patches has to be applied together to avoid > compilation failures for the affected architectures. > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Vincent Guittot <vincent.guittot@linaro.org> Acked-by Vincent Guittot <vincent.guittot@linaro.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v2: Fix problem in all affected architectures with a single patch > to avoid compilation errors. > > arch/arm/kernel/topology.c | 2 +- > arch/powerpc/kernel/smp.c | 2 +- > include/linux/sched.h | 8 ++++---- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c > index 9d85318..e35d880 100644 > --- a/arch/arm/kernel/topology.c > +++ b/arch/arm/kernel/topology.c > @@ -275,7 +275,7 @@ void store_cpu_topology(unsigned int cpuid) > cpu_topology[cpuid].socket_id, mpidr); > } > > -static inline const int cpu_corepower_flags(void) > +static inline int cpu_corepower_flags(void) > { > return SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN; > } > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > index 51a3ff7..1007fb8 100644 > --- a/arch/powerpc/kernel/smp.c > +++ b/arch/powerpc/kernel/smp.c > @@ -747,7 +747,7 @@ int setup_profiling_timer(unsigned int multiplier) > > #ifdef CONFIG_SCHED_SMT > /* cpumask of CPUs with asymetric SMT dependancy */ > -static const int powerpc_smt_flags(void) > +static int powerpc_smt_flags(void) > { > int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES; > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 306f4f0..0376b05 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -872,21 +872,21 @@ enum cpu_idle_type { > #define SD_NUMA 0x4000 /* cross-node balancing */ > > #ifdef CONFIG_SCHED_SMT > -static inline const int cpu_smt_flags(void) > +static inline int cpu_smt_flags(void) > { > return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES; > } > #endif > > #ifdef CONFIG_SCHED_MC > -static inline const int cpu_core_flags(void) > +static inline int cpu_core_flags(void) > { > return SD_SHARE_PKG_RESOURCES; > } > #endif > > #ifdef CONFIG_NUMA > -static inline const int cpu_numa_flags(void) > +static inline int cpu_numa_flags(void) > { > return SD_NUMA; > } > @@ -999,7 +999,7 @@ void free_sched_domains(cpumask_var_t doms[], unsigned int ndoms); > bool cpus_share_cache(int this_cpu, int that_cpu); > > typedef const struct cpumask *(*sched_domain_mask_f)(int cpu); > -typedef const int (*sched_domain_flags_f)(void); > +typedef int (*sched_domain_flags_f)(void); > > #define SDTL_OVERLAP 0x01 > > -- > 1.9.1 >
On Tue, Jun 24, 2014 at 06:05:29PM -0700, Guenter Roeck wrote: > Commit 143e1e28cb (sched: Rework sched_domain topology definition) > introduced a number of functions with a return value of 'const int'. > gcc doesn't know what to do with that and, if the kernel is compiled > with W=1, complains with the following warnings whenever sched.h > is included. > > include/linux/sched.h:875:25: warning: > type qualifiers ignored on function return type > include/linux/sched.h:882:25: warning: > type qualifiers ignored on function return type > include/linux/sched.h:889:25: warning: > type qualifiers ignored on function return type > include/linux/sched.h:1002:21: warning: > type qualifiers ignored on function return type > > Commits fb2aa855 (sched, ARM: Create a dedicated scheduler topology table) > and 607b45e9a (sched, powerpc: Create a dedicated topology table) introduce > the same warning in the arm and powerpc code. > > Drop 'const' from the function declarations to fix the problem. > > The fix for all three patches has to be applied together to avoid > compilation failures for the affected architectures. > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Vincent Guittot <vincent.guittot@linaro.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v2: Fix problem in all affected architectures with a single patch > to avoid compilation errors. > > arch/arm/kernel/topology.c | 2 +- > arch/powerpc/kernel/smp.c | 2 +- > include/linux/sched.h | 8 ++++---- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c > index 9d85318..e35d880 100644 > --- a/arch/arm/kernel/topology.c > +++ b/arch/arm/kernel/topology.c > @@ -275,7 +275,7 @@ void store_cpu_topology(unsigned int cpuid) > cpu_topology[cpuid].socket_id, mpidr); > } > > -static inline const int cpu_corepower_flags(void) > +static inline int cpu_corepower_flags(void) > { > return SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN; > } Maybe the author's intention was: static inline int cpu_corepower_flags(void) __attribute__((const)); ? This specifies that the function has no side effects and the return value only depends on the (here non-existing) function arguments. Best regards Uwe
On 06/25/2014 12:14 AM, Uwe Kleine-König wrote: > On Tue, Jun 24, 2014 at 06:05:29PM -0700, Guenter Roeck wrote: >> Commit 143e1e28cb (sched: Rework sched_domain topology definition) >> introduced a number of functions with a return value of 'const int'. >> gcc doesn't know what to do with that and, if the kernel is compiled >> with W=1, complains with the following warnings whenever sched.h >> is included. >> >> include/linux/sched.h:875:25: warning: >> type qualifiers ignored on function return type >> include/linux/sched.h:882:25: warning: >> type qualifiers ignored on function return type >> include/linux/sched.h:889:25: warning: >> type qualifiers ignored on function return type >> include/linux/sched.h:1002:21: warning: >> type qualifiers ignored on function return type >> >> Commits fb2aa855 (sched, ARM: Create a dedicated scheduler topology table) >> and 607b45e9a (sched, powerpc: Create a dedicated topology table) introduce >> the same warning in the arm and powerpc code. >> >> Drop 'const' from the function declarations to fix the problem. >> >> The fix for all three patches has to be applied together to avoid >> compilation failures for the affected architectures. >> >> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@kernel.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Vincent Guittot <vincent.guittot@linaro.org> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> v2: Fix problem in all affected architectures with a single patch >> to avoid compilation errors. >> >> arch/arm/kernel/topology.c | 2 +- >> arch/powerpc/kernel/smp.c | 2 +- >> include/linux/sched.h | 8 ++++---- >> 3 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c >> index 9d85318..e35d880 100644 >> --- a/arch/arm/kernel/topology.c >> +++ b/arch/arm/kernel/topology.c >> @@ -275,7 +275,7 @@ void store_cpu_topology(unsigned int cpuid) >> cpu_topology[cpuid].socket_id, mpidr); >> } >> >> -static inline const int cpu_corepower_flags(void) >> +static inline int cpu_corepower_flags(void) >> { >> return SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN; >> } > Maybe the author's intention was: > > static inline int cpu_corepower_flags(void) __attribute__((const)); > > ? > This specifies that the function has no side effects and the return value > only depends on the (here non-existing) function arguments. > Possibly, but either I am missing something or this doesn't compile. Guenter
Hello Guenter, On Wed, Jun 25, 2014 at 07:27:47AM -0700, Guenter Roeck wrote: > >Maybe the author's intention was: > > > > static inline int cpu_corepower_flags(void) __attribute__((const)); > > > >? > >This specifies that the function has no side effects and the return value > >only depends on the (here non-existing) function arguments. > > > > Possibly, but either I am missing something or this doesn't compile. You need to do a separate declaration: static inline int cpu_corepower_flags(void) __attribute__((const)); static inline int cpu_corepower_flags(void) { ... Does this help? Best regards Uwe
On 06/25/2014 07:49 AM, Uwe Kleine-König wrote: > Hello Guenter, > > On Wed, Jun 25, 2014 at 07:27:47AM -0700, Guenter Roeck wrote: >>> Maybe the author's intention was: >>> >>> static inline int cpu_corepower_flags(void) __attribute__((const)); >>> >>> ? >>> This specifies that the function has no side effects and the return value >>> only depends on the (here non-existing) function arguments. >>> >> >> Possibly, but either I am missing something or this doesn't compile. > You need to do a separate declaration: > > static inline int cpu_corepower_flags(void) __attribute__((const)); > static inline int cpu_corepower_flags(void) > { > ... Actually turns out one can use __attribute_const__, and it is static inline int __attribute_const__ cpu_corepower_flags(void) which turns out to be widely used. I'll change that and resubmit after testing. Guenter
From: Guenter Roeck > On 06/25/2014 07:49 AM, Uwe Kleine-Knig wrote: > > Hello Guenter, > > > > On Wed, Jun 25, 2014 at 07:27:47AM -0700, Guenter Roeck wrote: > >>> Maybe the author's intention was: > >>> > >>> static inline int cpu_corepower_flags(void) __attribute__((const)); > >>> > >>> ? > >>> This specifies that the function has no side effects and the return value > >>> only depends on the (here non-existing) function arguments. > >>> > >> > >> Possibly, but either I am missing something or this doesn't compile. > > You need to do a separate declaration: > > > > static inline int cpu_corepower_flags(void) __attribute__((const)); > > static inline int cpu_corepower_flags(void) > > { > > ... > > Actually turns out one can use __attribute_const__, and it is > > static inline int __attribute_const__ cpu_corepower_flags(void) > > which turns out to be widely used. > > I'll change that and resubmit after testing. You don't need to tell the compiler that for an inline function. David
Hello, On Wed, Jun 25, 2014 at 03:40:28PM +0000, David Laight wrote: > From: Guenter Roeck > > Actually turns out one can use __attribute_const__, and it is > > > > static inline int __attribute_const__ cpu_corepower_flags(void) > > > > which turns out to be widely used. > > > > I'll change that and resubmit after testing. > > You don't need to tell the compiler that for an inline function. I didn't check for the functions in question here, but in general your statement is wrong. For example: static inline unsigned int __attribute_const__ read_cpuid_id(void) { return readl(BASEADDR_V7M_SCB + V7M_SCB_CPUID); } from arch/arm/include/asm/cputype.h. The V7M_SCB_CPUID register never changes, but there is no way gcc can deduce that. Best regards Uwe
From: Uwe Kleine-König > Hello, > > On Wed, Jun 25, 2014 at 03:40:28PM +0000, David Laight wrote: > > From: Guenter Roeck > > > Actually turns out one can use __attribute_const__, and it is > > > > > > static inline int __attribute_const__ cpu_corepower_flags(void) > > > > > > which turns out to be widely used. > > > > > > I'll change that and resubmit after testing. > > > > You don't need to tell the compiler that for an inline function. > I didn't check for the functions in question here, but in general your > statement is wrong. > > For example: > > static inline unsigned int __attribute_const__ read_cpuid_id(void) > { > return readl(BASEADDR_V7M_SCB + V7M_SCB_CPUID); > } > > from arch/arm/include/asm/cputype.h. The V7M_SCB_CPUID register never > changes, but there is no way gcc can deduce that. Hmm... it all rather depends on the order of the optimisations and inlining. I've tried to use 'restrict' on the parameters to an inline function in an attempt to get 'noalias' - but the reverse inference never seems to be applied. David
On 06/25/2014 08:52 AM, Uwe Kleine-König wrote: > Hello, > > On Wed, Jun 25, 2014 at 03:40:28PM +0000, David Laight wrote: >> From: Guenter Roeck >>> Actually turns out one can use __attribute_const__, and it is >>> >>> static inline int __attribute_const__ cpu_corepower_flags(void) >>> >>> which turns out to be widely used. >>> >>> I'll change that and resubmit after testing. >> >> You don't need to tell the compiler that for an inline function. > I didn't check for the functions in question here, but in general your > statement is wrong. > > For example: > > static inline unsigned int __attribute_const__ read_cpuid_id(void) > { > return readl(BASEADDR_V7M_SCB + V7M_SCB_CPUID); > } > > from arch/arm/include/asm/cputype.h. The V7M_SCB_CPUID register never > changes, but there is no way gcc can deduce that. > Sigh. As I mentioned earlier, it is much easier to introduce a problem than to fix it. Ok, I'll leave this alone. I already spent much more time on this than I should or have, so it is really time to move on. Guenter
Hi Guenter, [I know I'm a bit late to this, but ...] On Tue, 24 Jun 2014 18:05:29 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > > diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c > index 9d85318..e35d880 100644 > --- a/arch/arm/kernel/topology.c > +++ b/arch/arm/kernel/topology.c > @@ -275,7 +275,7 @@ void store_cpu_topology(unsigned int cpuid) > cpu_topology[cpuid].socket_id, mpidr); > } > > -static inline const int cpu_corepower_flags(void) > +static inline int cpu_corepower_flags(void) > { > return SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN; > } The only reference to this function is to take its address, so "inline" is useless, right? > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 306f4f0..0376b05 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -872,21 +872,21 @@ enum cpu_idle_type { > #define SD_NUMA 0x4000 /* cross-node balancing */ > > #ifdef CONFIG_SCHED_SMT > -static inline const int cpu_smt_flags(void) > +static inline int cpu_smt_flags(void) > { > return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES; > } > #endif > > #ifdef CONFIG_SCHED_MC > -static inline const int cpu_core_flags(void) > +static inline int cpu_core_flags(void) > { > return SD_SHARE_PKG_RESOURCES; > } > #endif > > #ifdef CONFIG_NUMA > -static inline const int cpu_numa_flags(void) > +static inline int cpu_numa_flags(void) > { > return SD_NUMA; > } The same is true of those three, but then they would have to be moved into a .c file and replaced with prototypes ...
On 06/25/2014 05:59 PM, Stephen Rothwell wrote: > Hi Guenter, > > [I know I'm a bit late to this, but ...] > > On Tue, 24 Jun 2014 18:05:29 -0700 Guenter Roeck <linux@roeck-us.net> wrote: >> >> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c >> index 9d85318..e35d880 100644 >> --- a/arch/arm/kernel/topology.c >> +++ b/arch/arm/kernel/topology.c >> @@ -275,7 +275,7 @@ void store_cpu_topology(unsigned int cpuid) >> cpu_topology[cpuid].socket_id, mpidr); >> } >> >> -static inline const int cpu_corepower_flags(void) >> +static inline int cpu_corepower_flags(void) >> { >> return SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN; >> } > > The only reference to this function is to take its address, so "inline" > is useless, right? > >> diff --git a/include/linux/sched.h b/include/linux/sched.h >> index 306f4f0..0376b05 100644 >> --- a/include/linux/sched.h >> +++ b/include/linux/sched.h >> @@ -872,21 +872,21 @@ enum cpu_idle_type { >> #define SD_NUMA 0x4000 /* cross-node balancing */ >> >> #ifdef CONFIG_SCHED_SMT >> -static inline const int cpu_smt_flags(void) >> +static inline int cpu_smt_flags(void) >> { >> return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES; >> } >> #endif >> >> #ifdef CONFIG_SCHED_MC >> -static inline const int cpu_core_flags(void) >> +static inline int cpu_core_flags(void) >> { >> return SD_SHARE_PKG_RESOURCES; >> } >> #endif >> >> #ifdef CONFIG_NUMA >> -static inline const int cpu_numa_flags(void) >> +static inline int cpu_numa_flags(void) >> { >> return SD_NUMA; >> } > > The same is true of those three, but then they would have to be moved > into a .c file and replaced with prototypes ... > Personally I wasn't sure why it had to be functions instead of defines, but who knows. Anyway, seems others are not happy with my proposed fix either, and everyone seems to suggest a different solution, so I guess it won't go anywhere. I "solved" my immediate problem of getting a polluted build log by filtering the warnings out, so I don't really care too much anymore ;-). Guenter
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c index 9d85318..e35d880 100644 --- a/arch/arm/kernel/topology.c +++ b/arch/arm/kernel/topology.c @@ -275,7 +275,7 @@ void store_cpu_topology(unsigned int cpuid) cpu_topology[cpuid].socket_id, mpidr); } -static inline const int cpu_corepower_flags(void) +static inline int cpu_corepower_flags(void) { return SD_SHARE_PKG_RESOURCES | SD_SHARE_POWERDOMAIN; } diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index 51a3ff7..1007fb8 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -747,7 +747,7 @@ int setup_profiling_timer(unsigned int multiplier) #ifdef CONFIG_SCHED_SMT /* cpumask of CPUs with asymetric SMT dependancy */ -static const int powerpc_smt_flags(void) +static int powerpc_smt_flags(void) { int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES; diff --git a/include/linux/sched.h b/include/linux/sched.h index 306f4f0..0376b05 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -872,21 +872,21 @@ enum cpu_idle_type { #define SD_NUMA 0x4000 /* cross-node balancing */ #ifdef CONFIG_SCHED_SMT -static inline const int cpu_smt_flags(void) +static inline int cpu_smt_flags(void) { return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES; } #endif #ifdef CONFIG_SCHED_MC -static inline const int cpu_core_flags(void) +static inline int cpu_core_flags(void) { return SD_SHARE_PKG_RESOURCES; } #endif #ifdef CONFIG_NUMA -static inline const int cpu_numa_flags(void) +static inline int cpu_numa_flags(void) { return SD_NUMA; } @@ -999,7 +999,7 @@ void free_sched_domains(cpumask_var_t doms[], unsigned int ndoms); bool cpus_share_cache(int this_cpu, int that_cpu); typedef const struct cpumask *(*sched_domain_mask_f)(int cpu); -typedef const int (*sched_domain_flags_f)(void); +typedef int (*sched_domain_flags_f)(void); #define SDTL_OVERLAP 0x01
Commit 143e1e28cb (sched: Rework sched_domain topology definition) introduced a number of functions with a return value of 'const int'. gcc doesn't know what to do with that and, if the kernel is compiled with W=1, complains with the following warnings whenever sched.h is included. include/linux/sched.h:875:25: warning: type qualifiers ignored on function return type include/linux/sched.h:882:25: warning: type qualifiers ignored on function return type include/linux/sched.h:889:25: warning: type qualifiers ignored on function return type include/linux/sched.h:1002:21: warning: type qualifiers ignored on function return type Commits fb2aa855 (sched, ARM: Create a dedicated scheduler topology table) and 607b45e9a (sched, powerpc: Create a dedicated topology table) introduce the same warning in the arm and powerpc code. Drop 'const' from the function declarations to fix the problem. The fix for all three patches has to be applied together to avoid compilation failures for the affected architectures. Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Vincent Guittot <vincent.guittot@linaro.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v2: Fix problem in all affected architectures with a single patch to avoid compilation errors. arch/arm/kernel/topology.c | 2 +- arch/powerpc/kernel/smp.c | 2 +- include/linux/sched.h | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-)