[PATCH -next] ia64/sn: fix percpu warnings

[PATCH -next] ia64/sn: fix percpu warnings

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Fix percpu types warning in ia64/sn:

arch/ia64/sn/kernel/setup.c:74: error: conflicting types for '__pcpu_scope___sn_cnodeid_to_nasid'
arch/ia64/include/asm/sn/arch.h:74: error: previous declaration of '__pcpu_scope___sn_cnodeid_to_nasid' was here

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Cc: Jes Sorensen <jes@sgi.com>
---
 arch/ia64/include/asm/sn/arch.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next-20091014.orig/arch/ia64/include/asm/sn/arch.h
+++ linux-next-20091014/arch/ia64/include/asm/sn/arch.h
@@ -71,7 +71,7 @@ DECLARE_PER_CPU(struct sn_hub_info_s, __
  * Compact node ID to nasid mappings kept in the per-cpu data areas of each
  * cpu.
  */
-DECLARE_PER_CPU(short, __sn_cnodeid_to_nasid[MAX_COMPACT_NODES]);
+DECLARE_PER_CPU(short [MAX_COMPACT_NODES], __sn_cnodeid_to_nasid);
 #define sn_cnodeid_to_nasid	(&__get_cpu_var(__sn_cnodeid_to_nasid[0]))
 
 

Re: [PATCH -next] ia64/sn: fix percpu warnings

[Tejun Heo]

Randy Dunlap wrote:
> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Fix percpu types warning in ia64/sn:
> 
> arch/ia64/sn/kernel/setup.c:74: error: conflicting types for '__pcpu_scope___sn_cnodeid_to_nasid'
> arch/ia64/include/asm/sn/arch.h:74: error: previous declaration of '__pcpu_scope___sn_cnodeid_to_nasid' was here
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> Cc: Jes Sorensen <jes@sgi.com>

Acked-by: Tejun Heo <tj@kernel.org>

Can this go through ia64 tree?  If not, I can route it through
percpu#for-fixes.

Thanks.

-- 
tejun

Re: [PATCH -next] ia64/sn: fix percpu warnings

[Tony Luck]

> Acked-by: Tejun Heo <tj@kernel.org>
>
> Can this go through ia64 tree?  If not, I can route it through
> percpu#for-fixes.

It can ... but I'm getting a strange warning since I applied it:

  Building modules, stage 2.
  GZIP    arch/ia64/hp/sim/boot/vmlinux.gz
  MODPOST 198 modules
WARNING: "per_cpu____sn_cnodeid_to_nasid" [drivers/misc/sgi-xp/xpc.ko]
has no CRC!

Any idea what this means?

-Tony

RE: [PATCH -next] ia64/sn: fix percpu warnings

[Luck, Tony]

> WARNING: "per_cpu____sn_cnodeid_to_nasid" [drivers/misc/sgi-xp/xpc.ko] has no CRC!


Note that this warning goes away if I fix the mismatch declaration so
that we have:

DECLARE_PER_CPU(short, __sn_cnodeide_to_nasid[MAX_COMPACT_NODES]);
in arch.h

and

DEFINE_PER_CPU(short, __sn_cnodeide_to_nasid[MAX_COMPACT_NODES]);
in setup.c


-Tony

Re: [PATCH -next] ia64/sn: fix percpu warnings

[Tejun Heo]

Hello,

Luck, Tony wrote:
>> WARNING: "per_cpu____sn_cnodeid_to_nasid" [drivers/misc/sgi-xp/xpc.ko] has no CRC!
> 
> Note that this warning goes away if I fix the mismatch declaration so
> that we have:
> 
> DECLARE_PER_CPU(short, __sn_cnodeide_to_nasid[MAX_COMPACT_NODES]);
> in arch.h
> 
> and
> 
> DEFINE_PER_CPU(short, __sn_cnodeide_to_nasid[MAX_COMPACT_NODES]);
> in setup.c

Umm... the correct correct declaration and definition would be

DECLARE_PER_CPU(short [MAX_COMPACT_NODES], __sn_cnodeide_to_nasid);

and

DEFINE_PER_CPU(short [MAX_COMPACT_NODES], __sn_cnodeide_to_nasid);

So that the first part contains full type.  Doing it the other way
might cause problems if the __weak trick is turned on.

Thanks.

-- 
tejun

RE: [PATCH -next] ia64/sn: fix percpu warnings

[Luck, Tony]

> Umm... the correct correct declaration and definition would be
> 
> DECLARE_PER_CPU(short [MAX_COMPACT_NODES], __sn_cnodeide_to_nasid);
>
> and
>
> DEFINE_PER_CPU(short [MAX_COMPACT_NODES], __sn_cnodeide_to_nasid);
>
> So that the first part contains full type.  Doing it the other way
> might cause problems if the __weak trick is turned on.

That's what Randy's patch uses ... but doing it the "right" way gives
me the "has no CRC!" warning.

This seems to be a feature of exported per cpu arrays.  If I hack
up a driver to make use of softirq_work_list, I see a similar
no CRC warning for it.

Is this problem in the ia64 tool chain[1]?  Or do other architectures
have problems with exported per cpu arrays?

-Tony

[1] My default toolchain is uses gcc 4.1.2.  But 4.4.1 has the same
behavior.

Re: [PATCH -next] ia64/sn: fix percpu warnings

[Tejun Heo]

Hello,

Luck, Tony wrote:
> That's what Randy's patch uses ... but doing it the "right" way gives
> me the "has no CRC!" warning.

Ah, right.  I got confused.

> This seems to be a feature of exported per cpu arrays.  If I hack
> up a driver to make use of softirq_work_list, I see a similar
> no CRC warning for it.
> 
> Is this problem in the ia64 tool chain[1]?  Or do other architectures
> have problems with exported per cpu arrays?

kern/softirq.c has the followings.

 DEFINE_PER_CPU(struct list_head [NR_SOFTIRQS], softirq_work_list);
 EXPORT_PER_CPU_SYMBOL(softirq_work_list);

and it doesn't cause any warning on x86 neither does it on ia64 with
defconfig.  softirq_work_list doesn't trigger any warning there,
right?

Thanks.

-- 
tejun

RE: [PATCH -next] ia64/sn: fix percpu warnings

[Luck, Tony]

> DEFINE_PER_CPU(struct list_head [NR_SOFTIRQS], softirq_work_list);
> EXPORT_PER_CPU_SYMBOL(softirq_work_list);
>
> and it doesn't cause any warning on x86 neither does it on ia64 with
> defconfig.  softirq_work_list doesn't trigger any warning there,
> right?

Of the 10 EXPORT_PER_CPU_SYMBOL and 4 EXPORT_PER_CPU_SYMBOL_GPL
items on ia64, only __sn_cnodeid_to_nasid and softirq_work_list are
arrays.  The latter appears not to be actually used by any in-tree
modules (janitors: does it need to be exported?).

The CRC warning only shows up on a module that uses an exported
per-cpu array.  There seems to be nothing special about the
__sn_cnodeid_to_nasid.  When I hacked a module to make a random
use of softirq_work_list, it too gave a CRC error.

I just tried an x86_64 build with a module also modified to access
softirq_work_list.  It did NOT get a CRC error.  So the x86_64
toolchain doesn't seem to have the same issue as ia64.

-Tony

RE: [PATCH -next] ia64/sn: fix percpu warnings

[Luck, Tony]

> I just tried an x86_64 build with a module also modified to access
> softirq_work_list.  It did NOT get a CRC error.  So the x86_64
> toolchain doesn't seem to have the same issue as ia64.

Ok.  x86 doesn't see this because the defconfig has
  CONFIG_MODVERSIONS is not set

If I turn MODVERSION off, then the problem disappears for me.

If I turn MODVERSIONS on for x86 (and fudge a driver to make use of
softirq_work_list), then x86 gets the CRC warning too.


So this isn't a tool chain problem. Neither x86 nor ia64
can handle exported per-cpu array objects when CONFIG_MODVERSIONS
is set.

Looking at the __crc symbols in the vmlinux for x86 with
CONFIG_MODVERSIONS=y I see:

000000006dcaeb88 A __crc_per_cpu__kernel_stack
00000000b3994c7a A __crc_per_cpu__kstat
00000000d917c158 A __crc_per_cpu__node_number
                 w __crc_per_cpu__softirq_work_list
0000000036a1f502 A __crc_per_cpu__softnet_data
0000000057adf756 A __crc_per_cpu__this_cpu_off

which explains why "modpost" is unable to find a CRC.

Maybe the comments in <linux/module.h> are supposed to be a
clue? :

#ifdef CONFIG_MODVERSIONS
/* Mark the CRC weak since genksyms apparently decides not to
 * generate a checksums for some symbols */
#define __CRC_SYMBOL(sym, sec)                                  \
        extern void *__crc_##sym __attribute__((weak));         \
        static const unsigned long __kcrctab_##sym              \
        __used                                                  \
        __attribute__((section("__kcrctab" sec), unused))       \
        = (unsigned long) &__crc_##sym;

But not enough of a clue for me :-(


-Tony

Re: [PATCH -next] ia64/sn: fix percpu warnings

[Tejun Heo]

Hello,

Luck, Tony wrote:
> Ok.  x86 doesn't see this because the defconfig has
>   CONFIG_MODVERSIONS is not set
> 
> If I turn MODVERSION off, then the problem disappears for me.
> 
> If I turn MODVERSIONS on for x86 (and fudge a driver to make use of
> softirq_work_list), then x86 gets the CRC warning too.

Right, I can reproduce it here too.

> So this isn't a tool chain problem. Neither x86 nor ia64
> can handle exported per-cpu array objects when CONFIG_MODVERSIONS
> is set.
> 
> Looking at the __crc symbols in the vmlinux for x86 with
> CONFIG_MODVERSIONS=y I see:
> 
> 000000006dcaeb88 A __crc_per_cpu__kernel_stack
> 00000000b3994c7a A __crc_per_cpu__kstat
> 00000000d917c158 A __crc_per_cpu__node_number
>                  w __crc_per_cpu__softirq_work_list
> 0000000036a1f502 A __crc_per_cpu__softnet_data
> 0000000057adf756 A __crc_per_cpu__this_cpu_off
> 
> which explains why "modpost" is unable to find a CRC.
> 
> Maybe the comments in <linux/module.h> are supposed to be a
> clue? :
> 
> #ifdef CONFIG_MODVERSIONS
> /* Mark the CRC weak since genksyms apparently decides not to
>  * generate a checksums for some symbols */
> #define __CRC_SYMBOL(sym, sec)                                  \
>         extern void *__crc_##sym __attribute__((weak));         \
>         static const unsigned long __kcrctab_##sym              \
>         __used                                                  \
>         __attribute__((section("__kcrctab" sec), unused))       \
>         = (unsigned long) &__crc_##sym;
> 
> But not enough of a clue for me :-(

I have no idea either.  I'll dig a bit and try to find out what's
going on.

Thanks.

-- 
tejun

Re: [PATCH -next] ia64/sn: fix percpu warnings

[Tejun Heo]

Yiee... lexer/parser problem in genksyms.  cc'ing Andreas, Sam and Jan
for help and quoting whole body.

Hello,

Tejun Heo wrote:
> Hello,
> 
> Luck, Tony wrote:
>> Ok.  x86 doesn't see this because the defconfig has
>>   CONFIG_MODVERSIONS is not set
>>
>> If I turn MODVERSION off, then the problem disappears for me.
>>
>> If I turn MODVERSIONS on for x86 (and fudge a driver to make use of
>> softirq_work_list), then x86 gets the CRC warning too.
> 
> Right, I can reproduce it here too.
> 
>> So this isn't a tool chain problem. Neither x86 nor ia64
>> can handle exported per-cpu array objects when CONFIG_MODVERSIONS
>> is set.
>>
>> Looking at the __crc symbols in the vmlinux for x86 with
>> CONFIG_MODVERSIONS=y I see:
>>
>> 000000006dcaeb88 A __crc_per_cpu__kernel_stack
>> 00000000b3994c7a A __crc_per_cpu__kstat
>> 00000000d917c158 A __crc_per_cpu__node_number
>>                  w __crc_per_cpu__softirq_work_list
>> 0000000036a1f502 A __crc_per_cpu__softnet_data
>> 0000000057adf756 A __crc_per_cpu__this_cpu_off
>>
>> which explains why "modpost" is unable to find a CRC.
>>
>> Maybe the comments in <linux/module.h> are supposed to be a
>> clue? :
>>
>> #ifdef CONFIG_MODVERSIONS
>> /* Mark the CRC weak since genksyms apparently decides not to
>>  * generate a checksums for some symbols */
>> #define __CRC_SYMBOL(sym, sec)                                  \
>>         extern void *__crc_##sym __attribute__((weak));         \
>>         static const unsigned long __kcrctab_##sym              \
>>         __used                                                  \
>>         __attribute__((section("__kcrctab" sec), unused))       \
>>         = (unsigned long) &__crc_##sym;
>>
>> But not enough of a clue for me :-(
> 
> I have no idea either.  I'll dig a bit and try to find out what's
> going on.

So, the problem is that genksyms can't understand the following.

  __typeof__(int [10]) my_ar;
  EXPORT_SYMBOL(my_ar);

Would it be difficult to teach it how to parse it?

Thanks.

-- 
tejun

Re: [PATCH -next] ia64/sn: fix percpu warnings

[Jan Beulich]

>>> Tejun Heo <tj@kernel.org> 29.10.09 16:43 >>>
>So, the problem is that genksyms can't understand the following.
>
>  __typeof__(int [10]) my_ar;
>  EXPORT_SYMBOL(my_ar);

In fact almost no use of typeof() works with genksyms: Out of

#include <linux/module.h>

extern int x;

#define export(t, n) typeof(t) export_##n; EXPORT_SYMBOL(export_##n)

export(x, a);
export(&x, b);
export(int, c);
export(int*, d);
export(int**, e);
export(int[4], f);
export(typeof(x), g);
export(typeof(&x), h);
export(typeof(x)*, i);

only c and d get a non-zero CRC. This is clearly due to the naive parsing
scripts/genksyms/parse.y does:

type_specifier:
	simple_type_specifier
	| cvar_qualifier
	| TYPEOF_KEYW '(' decl_specifier_seq '*' ')'
	| TYPEOF_KEYW '(' decl_specifier_seq ')'
	...

>Would it be difficult to teach it how to parse it?

For someone familiar with bison/yacc this would seem not very difficult
a job, but as far as I'm concerned this is not something I would feel
comfortable trying to fix. Of course one could go the simplistic route
and just add the array case here, but imo this wouldn't be the right
way to deal with it.

Jan