On Tue, 1 Aug 2006 19:36:21 +0200
Jan Blunck <[email protected]> wrote:
> The per cpu variables are used incorrectly in vmstat.h.
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> include/linux/vmstat.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-v2.6.18-rc3/include/linux/vmstat.h
> ===================================================================
> --- linux-v2.6.18-rc3.orig/include/linux/vmstat.h
> +++ linux-v2.6.18-rc3/include/linux/vmstat.h
> @@ -41,23 +41,23 @@ DECLARE_PER_CPU(struct vm_event_state, v
>
> static inline void __count_vm_event(enum vm_event_item item)
> {
> - __get_cpu_var(vm_event_states.event[item])++;
> + __get_cpu_var(vm_event_states).event[item]++;
> }
How odd. Are there any negative consequences to the existing code?
Andrew Morton writes:
> On Tue, 1 Aug 2006 19:36:21 +0200
> Jan Blunck <[email protected]> wrote:
>
> > The per cpu variables are used incorrectly in vmstat.h.
[snip]
> > - __get_cpu_var(vm_event_states.event[item])++;
> > + __get_cpu_var(vm_event_states).event[item]++;
> > }
>
> How odd. Are there any negative consequences to the existing code?
That sort of thing - i.e. using __get_cpu_var on something which isn't
just a simple variable name - works with the current per-cpu macro
definitions, because they are pretty simple, but one can imagine other
reasonable implementations of __get_cpu_var which need the argument to
be a simple variable name. I struck this when I was experimenting
with using __thread variables for per-cpu data.
Paul.
On Tue, Aug 01, Andrew Morton wrote:
> >
> > static inline void __count_vm_event(enum vm_event_item item)
> > {
> > - __get_cpu_var(vm_event_states.event[item])++;
> > + __get_cpu_var(vm_event_states).event[item]++;
> > }
>
> How odd. Are there any negative consequences to the existing code?
In asm-s390/percpu.h we use
#define __get_cpu_var(var) __reloc_hide(var,S390_lowcore.percpu_offset)
and for modules on s390x __reloc_hide() is defined as
#define __reloc_hide(var,offset) \
(*({ unsigned long *__ptr; \
asm ( "larl %0,per_cpu__"#var"@GOTENT" \
: "=a" (__ptr) : "X" (per_cpu__##var) ); \
(typeof(&per_cpu__##var))((*__ptr) + (offset)); }))
which leads in this case to
larl %0, per_cpu__vm_event_states.event[item]@GOTENT
which is invalid asm.
Here comes another idea. To find further wrong usage of percpu variables I
wrote the following patch. It still needs some work for the other archs but
I'm interested in your feedback about that.
Jan
On Wed, 2 Aug 2006 15:30:06 +0200
Jan Blunck <[email protected]> wrote:
> Here comes another idea. To find further wrong usage of percpu variables I
> wrote the following patch. It still needs some work for the other archs but
> I'm interested in your feedback about that.
>
> ...
>
> Index: linux-2.6/include/asm-generic/percpu.h
> ===================================================================
> --- linux-2.6.orig/include/asm-generic/percpu.h
> +++ linux-2.6/include/asm-generic/percpu.h
> @@ -14,7 +14,9 @@ extern unsigned long __per_cpu_offset[NR
> __attribute__((__section__(".data.percpu"))) __typeof__(type) per_cpu__##name
>
> /* var is in discarded region: offset to particular copy we want */
> -#define per_cpu(var, cpu) (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
> +#define per_cpu(var, cpu) (*({ \
> + int user_error_##var __attribute__ ((unused)); \
> + RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]); }))
What's it do? Forces a syntax error if `var' isn't a simple identifier?
Seems sane, although I'd check that the compiler doesn't accidentally
waste a stack slot for that local. Perhaps it's be safer to make
it a non-existing function:
extern int user_error#var(void);
On Wed, Aug 02, Andrew Morton wrote:
> > +#define per_cpu(var, cpu) (*({ \
> > + int user_error_##var __attribute__ ((unused)); \
> > + RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]); }))
>
> What's it do? Forces a syntax error if `var' isn't a simple identifier?
>
> Seems sane, although I'd check that the compiler doesn't accidentally
> waste a stack slot for that local. Perhaps it's be safer to make
> it a non-existing function:
>
> extern int user_error#var(void);
>
Yes, that is even better. See attached patch with the modifications for my
major archs (s390, x86-64, generic). I did a run of 'make allmodconfig' with
the attached patch and it produced some errors. I'll send the patches in a
seperate mail.