fix vmstat per cpu usage

PATCH In review Jan Blunck 4 participants 8 messages Aug 1, 2006 – Aug 7, 2006

Subsystems (3)

MM core Generic include/asm header files Per-CPU memory allocator

lore

Share & tools

lore

Jan Blunck

author

2006-08-01 17:36 UTC #0 lore raw

PATCH 1/1 fix vmstat per cpu usage

From: Jan Blunck <[email protected]>
Subject: fix vmstat per cpu usage

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(-)

include/linux/vmstat.h +4 -4

Index: 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]++;
 }
 
 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]++;
 	put_cpu();
 }
 
 static inline void __count_vm_events(enum vm_event_item item, long delta)
 {
-	__get_cpu_var(vm_event_states.event[item]) += delta;
+	__get_cpu_var(vm_event_states).event[item] += delta;
 }
 
 static inline void count_vm_events(enum vm_event_item item, long delta)
 {
-	get_cpu_var(vm_event_states.event[item]) += delta;
+	get_cpu_var(vm_event_states).event[item] += delta;
 	put_cpu();
 }

Andrew Morton

2006-08-01 21:07 UTC #1 lore raw

Quoted 12 lines · 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
===================================================================

include/linux/vmstat.h DECLARE_PER_CPU(struct vm_event_state, v · line 45

Quoted 6 lines

include/linux/vmstat.h +1 -1

@@ -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?

Paul Mackerras

2006-08-01 22:44 UTC #2 lore raw

Quoted 4 lines · Andrew Morton

Quoted 1 line · 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]

Quoted 5 lines

include/linux/vmstat.h DECLARE_PER_CPU(struct vm_event_state, v · line 45
Quoted 3 lines
include/linux/vmstat.h +1 -1
-	__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.

Jan Blunck

author

2006-08-02 09:01 UTC #3 lore raw

In reply to Andrew Morton

Quoted 8 lines · Andrew Morton

include/linux/vmstat.h DECLARE_PER_CPU(struct vm_event_state, v · line 45
Quoted 6 lines
static inline void __count_vm_event(enum vm_event_item item) {
include/linux/vmstat.h +1 -1
-	__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.

Jan Blunck

author

2006-08-02 13:30 UTC #4 lore raw

In reply to Andrew Morton

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

---
 include/asm-generic/percpu.h |    4 +++-
 include/asm-s390/percpu.h    |    6 ++++--
 include/linux/percpu.h       |    5 ++++-
 3 files changed, 11 insertions(+), 4 deletions(-)

include/asm-generic/percpu.h +3 -1

Index: 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]); }))
 #define __get_cpu_var(var) per_cpu(var, smp_processor_id())
 #define __raw_get_cpu_var(var) per_cpu(var, raw_smp_processor_id())

include/asm-s390/percpu.h +4 -2

Index: linux-2.6/include/asm-s390/percpu.h
===================================================================
@@ -16,7 +16,8 @@
 #if defined(__s390x__) && defined(MODULE)
 
 #define __reloc_hide(var,offset) \
-  (*({ unsigned long *__ptr; \
+  (*({ int user_error_##var __attribute__ ((unused)); \
+       unsigned long *__ptr; \
        asm ( "larl %0,per_cpu__"#var"@GOTENT" \
              : "=a" (__ptr) : "X" (per_cpu__##var) ); \
        (typeof(&per_cpu__##var))((*__ptr) + (offset)); }))

@@ -24,7 +25,8 @@
 #else
 
 #define __reloc_hide(var, offset) \
-  (*({ unsigned long __ptr; \
+  (*({ int user_error_##var __attribute__ ((unused)); \
+       unsigned long __ptr; \
        asm ( "" : "=a" (__ptr) : "0" (&per_cpu__##var) ); \
        (typeof(&per_cpu__##var)) (__ptr + (offset)); }))

include/linux/percpu.h +4 -1

Index: linux-2.6/include/linux/percpu.h
===================================================================
@@ -12,7 +12,10 @@
 #endif
 
 /* Must be an lvalue. */
-#define get_cpu_var(var) (*({ preempt_disable(); &__get_cpu_var(var); }))
+#define get_cpu_var(var) (*({				\
+	int user_error_##var __attribute__ ((unused));	\
+	preempt_disable();				\
+	&__get_cpu_var(var); }))
 #define put_cpu_var(var) preempt_enable()
 
 #ifdef CONFIG_SMP

Andrew Morton

2006-08-02 14:43 UTC #5 lore raw

Quoted 11 lines · 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
===================================================================

include/asm-generic/percpu.h extern unsigned long __per_cpu_offset[NR · line 19

Quoted 7 lines

include/asm-generic/percpu.h +3 -1

@@ -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);

Jan Blunck

author

2006-08-07 12:42 UTC #6 lore raw

Quoted 6 lines · Andrew Morton

Review
Quoted 3 lines
(quoted diff) +3 -0
+#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?

Quoted 6 lines

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.

From: Jan Blunck <[email protected]>
Subject: trigger a syntax error if percpu macros are incorrectly used

get_cpu_var()/per_cpu()/__get_cpu_var() arguments must be simple identifiers. Otherwise the arch dependant implementations might break. This patch enforces the correct usage of the macros by producing a syntax error if the variable is not a simple identifier.

Signed-off-by Jan Blunck <[email protected]>

---
 include/asm-generic/percpu.h |    4 +++-
 include/asm-s390/percpu.h    |   20 +++++++++++---------
 include/asm-x86_64/percpu.h  |   12 +++++++++---
 include/linux/percpu.h       |   10 ++++++++--
 4 files changed, 31 insertions(+), 15 deletions(-)

include/asm-generic/percpu.h +3 -1

Index: 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) (*({				\
+	extern int simple_indentifier_##var(void);	\
+	RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]); }))
 #define __get_cpu_var(var) per_cpu(var, smp_processor_id())
 #define __raw_get_cpu_var(var) per_cpu(var, raw_smp_processor_id())

include/asm-s390/percpu.h +11 -9

Index: linux-2.6/include/asm-s390/percpu.h
===================================================================
@@ -15,18 +15,20 @@
  */
 #if defined(__s390x__) && defined(MODULE)
 
-#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)); }))
+#define __reloc_hide(var,offset) (*({			\
+	extern int simple_indentifier_##var(void);	\
+	unsigned long *__ptr;				\
+	asm ( "larl %0,per_cpu__"#var"@GOTENT"		\
+	    : "=a" (__ptr) : "X" (per_cpu__##var) );	\
+	(typeof(&per_cpu__##var))((*__ptr) + (offset));	}))
 
 #else
 
-#define __reloc_hide(var, offset) \
-  (*({ unsigned long __ptr; \
-       asm ( "" : "=a" (__ptr) : "0" (&per_cpu__##var) ); \
-       (typeof(&per_cpu__##var)) (__ptr + (offset)); }))
+#define __reloc_hide(var, offset) (*({				\
+	extern int simple_indentifier_##var(void);		\
+	unsigned long __ptr;					\
+	asm ( "" : "=a" (__ptr) : "0" (&per_cpu__##var) );	\
+	(typeof(&per_cpu__##var)) (__ptr + (offset)); }))
 
 #endif

include/asm-x86_64/percpu.h +9 -3

Index: linux-2.6/include/asm-x86_64/percpu.h
===================================================================
@@ -21,9 +21,15 @@
     __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 __get_cpu_var(var) (*RELOC_HIDE(&per_cpu__##var, __my_cpu_offset()))
-#define __raw_get_cpu_var(var) (*RELOC_HIDE(&per_cpu__##var, __my_cpu_offset()))
+#define per_cpu(var, cpu) (*({				\
+	extern int simple_indentifier_##var(void);	\
+	RELOC_HIDE(&per_cpu__##var, __per_cpu_offset(cpu)); }))
+#define __get_cpu_var(var) (*({				\
+	extern int simple_indentifier_##var(void);	\
+	RELOC_HIDE(&per_cpu__##var, __my_cpu_offset()); }))
+#define __raw_get_cpu_var(var) (*({			\
+	extern int simple_indentifier_##var(void);	\
+	RELOC_HIDE(&per_cpu__##var, __my_cpu_offset()); }))
 
 /* A macro to avoid #include hell... */
 #define percpu_modcopy(pcpudst, src, size)			\

include/linux/percpu.h +8 -2

Index: linux-2.6/include/linux/percpu.h
===================================================================
@@ -11,8 +11,14 @@
 #define PERCPU_ENOUGH_ROOM 32768
 #endif
 
-/* Must be an lvalue. */
-#define get_cpu_var(var) (*({ preempt_disable(); &__get_cpu_var(var); }))
+/*
+ * Must be an lvalue. Since @var must be a simple identifier,
+ * we force a syntax error here if it isn't.
+ */
+#define get_cpu_var(var) (*({				\
+	extern int simple_indentifier_##var(void);	\
+	preempt_disable();				\
+	&__get_cpu_var(var); }))
 #define put_cpu_var(var) preempt_enable()
 
 #ifdef CONFIG_SMP

Steve Fox

Acked-by on v1 1/1

2006-08-02 16:54 UTC #7 lore raw

In reply to Jan Blunck

Quoted 6 lines · Jan Blunck

From: Jan Blunck <[email protected]>
Subject: fix vmstat per cpu usage
The per cpu variables are used incorrectly in vmstat.h.
Signed-off-by Jan Blunck <[email protected]>

This patch has allowed the first 2.6.18-rc* successful compilation on s390.

Andrew, please pull this into your tree too. Thanks.

Acked-by Steve Fox <[email protected]>

Signature · 2 lines

Steve Fox
IBM Linux Technology Center