mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Guenter Roeck]

Hi,

the upstream kernel fails to build mips:nlm_xlp_defconfig,
mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
other targets, with errors such as

arch/mips/kernel/smp.c:211:2: error:
	passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
	from pointer target type
arch/mips/kernel/process.c:52:2: error:
	passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
	from pointer target type
arch/mips/cavium-octeon/smp.c:242:2: error:
	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
	from pointer target type

The problem was introduced with commit 8dd928915a73 (" mips: fix up obsolete cpu
function usage"). I would send a patch to fix it, but I am not sure if removing
'volatile' from the variable declaration(s) would be a good idea.

I don't recall seeing the problem in -next, but unless I am missing something,
the patch never made it into -next to start with.

Guenter

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Ralf Baechle]

On Mon, Apr 20, 2015 at 12:40:28PM -0700, Guenter Roeck wrote:

> the upstream kernel fails to build mips:nlm_xlp_defconfig,
> mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
> other targets, with errors such as
> 
> arch/mips/kernel/smp.c:211:2: error:
> 	passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
> 	from pointer target type
> arch/mips/kernel/process.c:52:2: error:
> 	passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
> 	from pointer target type
> arch/mips/cavium-octeon/smp.c:242:2: error:
> 	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
> 	from pointer target type
> 
> The problem was introduced with commit 8dd928915a73 (" mips: fix up obsolete cpu
> function usage"). I would send a patch to fix it, but I am not sure if removing
> 'volatile' from the variable declaration(s) would be a good idea.
> 
> I don't recall seeing the problem in -next, but unless I am missing something,
> the patch never made it into -next to start with.

It was posted on March 2nd as part of a larger series.  Only this patch
9/16 was send to linux-mips and when I tested the patch it failed.  Back
then I blamed it on not having patches 1 to 8 in my test tree so I didn't
diver deeper into the issue and dropped the patch ...

  Ralf

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Guenter Roeck]

On Mon, Apr 20, 2015 at 10:06:42PM +0200, Ralf Baechle wrote:
> On Mon, Apr 20, 2015 at 12:40:28PM -0700, Guenter Roeck wrote:
> 
> > the upstream kernel fails to build mips:nlm_xlp_defconfig,
> > mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
> > other targets, with errors such as
> > 
> > arch/mips/kernel/smp.c:211:2: error:
> > 	passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
> > 	from pointer target type
> > arch/mips/kernel/process.c:52:2: error:
> > 	passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
> > 	from pointer target type
> > arch/mips/cavium-octeon/smp.c:242:2: error:
> > 	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
> > 	from pointer target type
> > 
> > The problem was introduced with commit 8dd928915a73 (" mips: fix up obsolete cpu
> > function usage"). I would send a patch to fix it, but I am not sure if removing
> > 'volatile' from the variable declaration(s) would be a good idea.
> > 
> > I don't recall seeing the problem in -next, but unless I am missing something,
> > the patch never made it into -next to start with.
> 
> It was posted on March 2nd as part of a larger series.  Only this patch
> 9/16 was send to linux-mips and when I tested the patch it failed.  Back
> then I blamed it on not having patches 1 to 8 in my test tree so I didn't
> diver deeper into the issue and dropped the patch ...
> 

Hi Ralf,

The patch failed all by itself, no context needed ;-). Anyway, since it was part
of a larger series, I would assume that it should have been added to -next as
part of that series, not through the mips tree. For whatever reason that did not
happen ... and as far as I can see, many if not all mips smp builds now fail
in the upstream kernel.

Any idea what the fix should be ?

Thanks,
Guenter

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Aaro Koskinen]

Hi,

On Mon, Apr 20, 2015 at 12:40:28PM -0700, Guenter Roeck wrote:
> the upstream kernel fails to build mips:nlm_xlp_defconfig,
> mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
> other targets, with errors such as
> 
> arch/mips/kernel/smp.c:211:2: error:
> 	passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
> 	from pointer target type
> arch/mips/kernel/process.c:52:2: error:
> 	passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
> 	from pointer target type
> arch/mips/cavium-octeon/smp.c:242:2: error:
> 	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
> 	from pointer target type
> 
> The problem was introduced with commit 8dd928915a73 (" mips: fix up
> obsolete cpu function usage"). I would send a patch to fix it, but I
> am not sure if removing 'volatile' from the variable declaration(s)
> would be a good idea.

I think removing volatile from cpu_callin_map declaration should be OK,
since test_cpu (only reader) uses test_bit which takes care of it:

	static inline int test_bit(int nr, const volatile unsigned long *addr)

A.

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Guenter Roeck]

On 04/20/2015 02:09 PM, Aaro Koskinen wrote:
> Hi,
>
> On Mon, Apr 20, 2015 at 12:40:28PM -0700, Guenter Roeck wrote:
>> the upstream kernel fails to build mips:nlm_xlp_defconfig,
>> mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
>> other targets, with errors such as
>>
>> arch/mips/kernel/smp.c:211:2: error:
>> 	passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
>> 	from pointer target type
>> arch/mips/kernel/process.c:52:2: error:
>> 	passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
>> 	from pointer target type
>> arch/mips/cavium-octeon/smp.c:242:2: error:
>> 	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
>> 	from pointer target type
>>
>> The problem was introduced with commit 8dd928915a73 (" mips: fix up
>> obsolete cpu function usage"). I would send a patch to fix it, but I
>> am not sure if removing 'volatile' from the variable declaration(s)
>> would be a good idea.
>
> I think removing volatile from cpu_callin_map declaration should be OK,
> since test_cpu (only reader) uses test_bit which takes care of it:
>
> 	static inline int test_bit(int nr, const volatile unsigned long *addr)
>

I ran two tests with nlm_xlp_defconfig:

- add volatile to the second argument of cpumask_set_cpu() and cpumask_test_cpu():
   vmlinux image size 194664946
- remove volatile from cpu_callin_map:
   vmlinux image size 194664066

Given that, I am not sure I understand the impact of removing volatile
from cpu_callin_map. Maybe it is just better optimization without
volatile. Maybe there is no impact. Maybe the use of volatile is wrong
to start with ('Volatile Considered Harmful' comes into mind).
Maybe the use of volatile for cpu_callin_map is wrong for other architectures
as well (powerpc, ia64). Either case, I don't know to code well enough to
make this call.

Guenter

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Rusty Russell]

Aaro Koskinen <aaro.koskinen@iki.fi> writes:
> Hi,
>
> On Mon, Apr 20, 2015 at 12:40:28PM -0700, Guenter Roeck wrote:
>> the upstream kernel fails to build mips:nlm_xlp_defconfig,
>> mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
>> other targets, with errors such as
>> 
>> arch/mips/kernel/smp.c:211:2: error:
>> 	passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
>> 	from pointer target type
>> arch/mips/kernel/process.c:52:2: error:
>> 	passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
>> 	from pointer target type
>> arch/mips/cavium-octeon/smp.c:242:2: error:
>> 	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
>> 	from pointer target type
>> 
>> The problem was introduced with commit 8dd928915a73 (" mips: fix up
>> obsolete cpu function usage"). I would send a patch to fix it, but I
>> am not sure if removing 'volatile' from the variable declaration(s)
>> would be a good idea.
>
> I think removing volatile from cpu_callin_map declaration should be OK,
> since test_cpu (only reader) uses test_bit which takes care of it:
>
> 	static inline int test_bit(int nr, const volatile unsigned long *addr)

No, that got replaced too, with cpumask_test_cpu AFAICT.

You can open-code it, like so:

        test_bit(0, cpumask_bits(cpu_callin_map));

But you probably want to put a barrier in that loop instead of relying
on volatile.

Thanks,
Rusty.

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Guenter Roeck]

On Tue, Apr 21, 2015 at 01:45:35PM +0930, Rusty Russell wrote:
> Aaro Koskinen <aaro.koskinen@iki.fi> writes:
> > Hi,
> >
> > On Mon, Apr 20, 2015 at 12:40:28PM -0700, Guenter Roeck wrote:
> >> the upstream kernel fails to build mips:nlm_xlp_defconfig,
> >> mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
> >> other targets, with errors such as
> >> 
> >> arch/mips/kernel/smp.c:211:2: error:
> >> 	passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
> >> 	from pointer target type
> >> arch/mips/kernel/process.c:52:2: error:
> >> 	passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
> >> 	from pointer target type
> >> arch/mips/cavium-octeon/smp.c:242:2: error:
> >> 	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
> >> 	from pointer target type
> >> 
> >> The problem was introduced with commit 8dd928915a73 (" mips: fix up
> >> obsolete cpu function usage"). I would send a patch to fix it, but I
> >> am not sure if removing 'volatile' from the variable declaration(s)
> >> would be a good idea.
> >
> > I think removing volatile from cpu_callin_map declaration should be OK,
> > since test_cpu (only reader) uses test_bit which takes care of it:
> >
> > 	static inline int test_bit(int nr, const volatile unsigned long *addr)
> 
> No, that got replaced too, with cpumask_test_cpu AFAICT.
> 
> You can open-code it, like so:
> 
>         test_bit(0, cpumask_bits(cpu_callin_map));
> 
> But you probably want to put a barrier in that loop instead of relying
> on volatile.
> 
The following might do it. Note that I can not really test it since I don't have
a real mips system, and qemu gets rcu hangs if I enable more than one CPU (I see
that with older kernels as well, so it is not a new problem). Someone will have
to test the patch on a real multi-core system.

Guenter

---
>From 94026cc98a6b7b3567780a5443674c71202e2497 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Tue, 21 Apr 2015 08:31:01 -0700
Subject: [PATCH] mips: Fix SMP builds

Mips SMP builds fail with error messages similar to the following.

arch/mips/kernel/smp.c:211:2: error:
	passing argument 2 of 'cpumask_set_cpu' discards 'volatile'
	qualifier from pointer target type
arch/mips/kernel/process.c:52:2: error:
	passing argument 2 of 'cpumask_test_cpu' discards 'volatile'
	qualifier from pointer target type
arch/mips/cavium-octeon/smp.c:242:2: error:
	passing argument 2 of 'cpumask_clear_cpu' discards 'volatile'
	qualifier from pointer target type

cpu_callin_map is declared as volatile variable, but passed to various
functions with non-volatile arguments. Make it non-volatile and add a
memory barrier at the one location where volatile might be needed.

Fixes: 8dd928915a73 ("mips: fix up obsolete cpu function usage")
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 arch/mips/include/asm/smp.h | 2 +-
 arch/mips/kernel/smp.c      | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/smp.h b/arch/mips/include/asm/smp.h
index bb02fac9b4fa..2b25d1ba1ea0 100644
--- a/arch/mips/include/asm/smp.h
+++ b/arch/mips/include/asm/smp.h
@@ -45,7 +45,7 @@ extern int __cpu_logical_map[NR_CPUS];
 #define SMP_DUMP		0x8
 #define SMP_ASK_C0COUNT		0x10
 
-extern volatile cpumask_t cpu_callin_map;
+extern cpumask_t cpu_callin_map;
 
 /* Mask of CPUs which are currently definitely operating coherently */
 extern cpumask_t cpu_coherent_mask;
diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index 193ace7955fb..158191394770 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -43,7 +43,7 @@
 #include <asm/time.h>
 #include <asm/setup.h>
 
-volatile cpumask_t cpu_callin_map;	/* Bitmask of started secondaries */
+cpumask_t cpu_callin_map;		/* Bitmask of started secondaries */
 
 int __cpu_number_map[NR_CPUS];		/* Map physical to logical */
 EXPORT_SYMBOL(__cpu_number_map);
@@ -218,8 +218,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
 	/*
 	 * Trust is futile.  We should really have timeouts ...
 	 */
-	while (!cpumask_test_cpu(cpu, &cpu_callin_map))
+	while (!cpumask_test_cpu(cpu, &cpu_callin_map)) {
 		udelay(100);
+		mb();
+	}
 
 	synchronise_count_master(cpu);
 	return 0;
-- 
2.1.0

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Florian Fainelli]

2015-04-21 8:41 GMT-07:00 Guenter Roeck <linux@roeck-us.net>:
> On Tue, Apr 21, 2015 at 01:45:35PM +0930, Rusty Russell wrote:
>> Aaro Koskinen <aaro.koskinen@iki.fi> writes:
>> > Hi,
>> >
>> > On Mon, Apr 20, 2015 at 12:40:28PM -0700, Guenter Roeck wrote:
>> >> the upstream kernel fails to build mips:nlm_xlp_defconfig,
>> >> mips:nlm_xlp_defconfig, mips:cavium_octeon_defconfig, and possibly
>> >> other targets, with errors such as
>> >>
>> >> arch/mips/kernel/smp.c:211:2: error:
>> >>    passing argument 2 of 'cpumask_set_cpu' discards 'volatile' qualifier
>> >>    from pointer target type
>> >> arch/mips/kernel/process.c:52:2: error:
>> >>    passing argument 2 of 'cpumask_test_cpu' discards 'volatile' qualifier
>> >>    from pointer target type
>> >> arch/mips/cavium-octeon/smp.c:242:2: error:
>> >>    passing argument 2 of 'cpumask_clear_cpu' discards 'volatile' qualifier
>> >>    from pointer target type
>> >>
>> >> The problem was introduced with commit 8dd928915a73 (" mips: fix up
>> >> obsolete cpu function usage"). I would send a patch to fix it, but I
>> >> am not sure if removing 'volatile' from the variable declaration(s)
>> >> would be a good idea.
>> >
>> > I think removing volatile from cpu_callin_map declaration should be OK,
>> > since test_cpu (only reader) uses test_bit which takes care of it:
>> >
>> >     static inline int test_bit(int nr, const volatile unsigned long *addr)
>>
>> No, that got replaced too, with cpumask_test_cpu AFAICT.
>>
>> You can open-code it, like so:
>>
>>         test_bit(0, cpumask_bits(cpu_callin_map));
>>
>> But you probably want to put a barrier in that loop instead of relying
>> on volatile.
>>
> The following might do it. Note that I can not really test it since I don't have
> a real mips system, and qemu gets rcu hangs if I enable more than one CPU (I see
> that with older kernels as well, so it is not a new problem). Someone will have
> to test the patch on a real multi-core system.

Would be nice to get this merged ASAP as a build failure is not great.
See below for the tested tag, thanks for the fix!

>
> Guenter
>
> ---
> From 94026cc98a6b7b3567780a5443674c71202e2497 Mon Sep 17 00:00:00 2001
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Tue, 21 Apr 2015 08:31:01 -0700
> Subject: [PATCH] mips: Fix SMP builds
>
> Mips SMP builds fail with error messages similar to the following.
>
> arch/mips/kernel/smp.c:211:2: error:
>         passing argument 2 of 'cpumask_set_cpu' discards 'volatile'
>         qualifier from pointer target type
> arch/mips/kernel/process.c:52:2: error:
>         passing argument 2 of 'cpumask_test_cpu' discards 'volatile'
>         qualifier from pointer target type
> arch/mips/cavium-octeon/smp.c:242:2: error:
>         passing argument 2 of 'cpumask_clear_cpu' discards 'volatile'
>         qualifier from pointer target type
>
> cpu_callin_map is declared as volatile variable, but passed to various
> functions with non-volatile arguments. Make it non-volatile and add a
> memory barrier at the one location where volatile might be needed.
>
> Fixes: 8dd928915a73 ("mips: fix up obsolete cpu function usage")
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

On Netlogic XLP-FVP (8 cores):

Tested-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  arch/mips/include/asm/smp.h | 2 +-
>  arch/mips/kernel/smp.c      | 6 ++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/mips/include/asm/smp.h b/arch/mips/include/asm/smp.h
> index bb02fac9b4fa..2b25d1ba1ea0 100644
> --- a/arch/mips/include/asm/smp.h
> +++ b/arch/mips/include/asm/smp.h
> @@ -45,7 +45,7 @@ extern int __cpu_logical_map[NR_CPUS];
>  #define SMP_DUMP               0x8
>  #define SMP_ASK_C0COUNT                0x10
>
> -extern volatile cpumask_t cpu_callin_map;
> +extern cpumask_t cpu_callin_map;
>
>  /* Mask of CPUs which are currently definitely operating coherently */
>  extern cpumask_t cpu_coherent_mask;
> diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> index 193ace7955fb..158191394770 100644
> --- a/arch/mips/kernel/smp.c
> +++ b/arch/mips/kernel/smp.c
> @@ -43,7 +43,7 @@
>  #include <asm/time.h>
>  #include <asm/setup.h>
>
> -volatile cpumask_t cpu_callin_map;     /* Bitmask of started secondaries */
> +cpumask_t cpu_callin_map;              /* Bitmask of started secondaries */
>
>  int __cpu_number_map[NR_CPUS];         /* Map physical to logical */
>  EXPORT_SYMBOL(__cpu_number_map);
> @@ -218,8 +218,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>         /*
>          * Trust is futile.  We should really have timeouts ...
>          */
> -       while (!cpumask_test_cpu(cpu, &cpu_callin_map))
> +       while (!cpumask_test_cpu(cpu, &cpu_callin_map)) {
>                 udelay(100);
> +               mb();
> +       }
>
>         synchronise_count_master(cpu);
>         return 0;
> --
> 2.1.0
>



-- 
Florian

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Aaro Koskinen]

On Tue, Apr 21, 2015 at 08:41:08AM -0700, Guenter Roeck wrote:
> The following might do it. Note that I can not really test it since I
> don't have a real mips system, and qemu gets rcu hangs if I enable more
> than one CPU (I see that with older kernels as well, so it is not a new
> problem). Someone will have to test the patch on a real multi-core system.

That works on Octeon+ EBH5600 board (8 cores) with 4.1-rc1.

Tested-by: Aaro Koskinen <aaro.koskinen@nokia.com>

Thanks,

A.

Re: mips build failures due to commit 8dd928915a73 (mips: fix up obsolete cpu function usage)

[Paul Martin]

On Mon, Apr 27, 2015 at 04:03:48PM +0300, Aaro Koskinen wrote:
> On Tue, Apr 21, 2015 at 08:41:08AM -0700, Guenter Roeck wrote:
> > The following might do it. Note that I can not really test it since I
> > don't have a real mips system, and qemu gets rcu hangs if I enable more
> > than one CPU (I see that with older kernels as well, so it is not a new
> > problem). Someone will have to test the patch on a real multi-core system.
> 
> That works on Octeon+ EBH5600 board (8 cores) with 4.1-rc1.
> 
> Tested-by: Aaro Koskinen <aaro.koskinen@nokia.com>

http://patchwork.linux-mips.org/patch/9828/

I've been using this patch since 21 Apr on the 4.1 merge HEAD on
EdgeRouter Pro (Octeon II, dual core).  It's survived several
builds of gcc (using make -j3).

Tested-by: Paul Martin <paul.martin@codethink.co.uk>

-- 
Paul Martin                                  http://www.codethink.co.uk/
Senior Software Developer, Codethink Ltd.