percpu: Add raw_cpu_ops

PATCH In review Christoph Lameter 3 participants 7 messages Oct 3, 2013 – Oct 4, 2013 1 message not in the archive

Subsystems (2)

Library code Per-CPU memory allocator

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Christoph Lameter

author

2013-10-03 18:28 UTC #1 lore raw

PATCH v2 1/2 percpu: Add raw_cpu_ops

The following patches will add preemption checks to __this_cpu ops so we need to have an alternative way to use this cpu operations without preemption checks.

raw_cpu ops rely on the __this_cpu_xxx_1/2/4/8 operations having no preemption checks. We therefore do not have to duplicate these functions but can affort to only duplicate the higher level macros.

Also add raw_cpu_ptr for symmetries sake.

Signed-off-by Christoph Lameter <[email protected]>

include/linux/percpu.h +74 ~~-11~~

Index: linux/include/linux/percpu.h
===================================================================
@@ -139,6 +139,9 @@ extern int __init pcpu_page_first_chunk(
 				pcpu_fc_populate_pte_fn_t populate_pte_fn);
 #endif
 
+/* For symmetries sake. */
+#define raw_cpu_ptr __this_cpu_ptr
+
 /*
  * Use this to get to a cpu's version of the per-cpu object
  * dynamically allocated. Non-atomic access to the current CPU's

@@ -520,17 +523,7 @@ do {									\
 
 /*
  * Generic percpu operations for context that are safe from preemption/interrupts.
- * Either we do not care about races or the caller has the
- * responsibility of handling preemption/interrupt issues. Arch code can still
- * override these instructions since the arch per cpu code may be more
- * efficient and may actually get race freeness for free (that is the
- * case for x86 for example).
- *
- * If there is no other protection through preempt disable and/or
- * disabling interupts then one of these RMW operations can show unexpected
- * behavior because the execution thread was rescheduled on another processor
- * or an interrupt occurred and the same percpu variable was modified from
- * the interrupt context.
+ * These functions verify that preemption has been disabled.
  */
 #ifndef __this_cpu_read
 # ifndef __this_cpu_read_1

@@ -755,4 +748,74 @@ do {									\
 	__pcpu_double_call_return_bool(__this_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2))
 #endif
 
+/*
+ * Generic percpu operations for context where we do not want to do
+ * any checks for preemptiosn.
+ *
+ * If there is no other protection through preempt disable and/or
+ * disabling interupts then one of these RMW operations can show unexpected
+ * behavior because the execution thread was rescheduled on another processor
+ * or an interrupt occurred and the same percpu variable was modified from
+ * the interrupt context.
+ */
+#ifndef raw_cpu_read
+# define raw_cpu_read(pcp)	__pcpu_size_call_return(__this_cpu_read_, (pcp))
+#endif
+
+#ifndef raw_cpu_write
+# define raw_cpu_write(pcp, val)	__pcpu_size_call(__this_cpu_write_, (pcp), (val))
+#endif
+
+#ifndef raw_cpu_add
+# define raw_cpu_add(pcp, val)	__pcpu_size_call(__this_cpu_add_, (pcp), (val))
+#endif
+
+#ifndef raw_cpu_sub
+# define raw_cpu_sub(pcp, val)	raw_cpu_add((pcp), -(val))
+#endif
+
+#ifndef raw_cpu_inc
+# define raw_cpu_inc(pcp)		raw_cpu_add((pcp), 1)
+#endif
+
+#ifndef raw_cpu_dec
+# define raw_cpu_dec(pcp)		raw_cpu_sub((pcp), 1)
+#endif
+
+#ifndef raw_cpu_and
+# define raw_cpu_and(pcp, val)	__pcpu_size_call(__this_cpu_and_, (pcp), (val))
+#endif
+
+#ifndef raw_cpu_or
+# define raw_cpu_or(pcp, val)	__pcpu_size_call(__this_cpu_or_, (pcp), (val))
+#endif
+
+#ifndef raw_cpu_xor
+# define raw_cpu_xor(pcp, val)	__pcpu_size_call(__this_cpu_xor_, (pcp), (val))
+#endif
+
+#ifndef raw_cpu_add_return
+# define raw_cpu_add_return(pcp, val)	\
+	__pcpu_size_call_return2(__this_cpu_add_return_, pcp, val)
+#endif
+
+#define raw_cpu_sub_return(pcp, val)	raw_cpu_add_return(pcp, -(val))
+#define raw_cpu_inc_return(pcp)	raw_cpu_add_return(pcp, 1)
+#define raw_cpu_dec_return(pcp)	raw_cpu_add_return(pcp, -1)
+
+#ifndef raw_cpu_xchg
+# define raw_cpu_xchg(pcp, nval)	\
+	__pcpu_size_call_return2(__this_cpu_xchg_, (pcp), nval)
+#endif
+
+#ifndef raw_cpu_cmpxchg
+# define raw_cpu_cmpxchg(pcp, oval, nval)	\
+	__pcpu_size_call_return2(__this_cpu_cmpxchg_, pcp, oval, nval)
+#endif
+
+#ifndef raw_cpu_cmpxchg_double
+# define raw_cpu_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)	\
+	__pcpu_double_call_return_bool(__this_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2))
+#endif
+
 #endif /* __LINUX_PERCPU_H */

Christoph Lameter

author

2013-10-03 18:28 UTC #2 lore raw

PATCH v2 2/2 percpu: Add preemption checks to __this_cpu ops

In reply to a message not in the archive

We define a check function in order to avoid trouble with the include files. Then the higher level __this_cpu macros are modified to invoke the check before __this_cpu operation

Signed-off-by Christoph Lameter <[email protected]>

include/linux/percpu.h +42 ~~-10~~

Index: linux/include/linux/percpu.h
===================================================================
@@ -175,6 +175,12 @@ extern phys_addr_t per_cpu_ptr_to_phys(v
 
 extern void __bad_size_call_parameter(void);
 
+#ifdef CONFIG_DEBUG_PREEMPT
+extern void __this_cpu_preempt_check(void);
+#else
+static inline void __this_cpu_preempt_check(void) { }
+#endif
+
 #define __pcpu_size_call_return(stem, variable)				\
 ({	typeof(variable) pscr_ret__;					\
 	__verify_pcpu_ptr(&(variable));					\

@@ -538,7 +544,8 @@ do {									\
 # ifndef __this_cpu_read_8
 #  define __this_cpu_read_8(pcp)	(*__this_cpu_ptr(&(pcp)))
 # endif
-# define __this_cpu_read(pcp)	__pcpu_size_call_return(__this_cpu_read_, (pcp))
+# define __this_cpu_read(pcp) \
+	(__this_cpu_preempt_check(),__pcpu_size_call_return(__this_cpu_read_, (pcp)))
 #endif
 
 #define __this_cpu_generic_to_op(pcp, val, op)				\

@@ -559,7 +566,12 @@ do {									\
 # ifndef __this_cpu_write_8
 #  define __this_cpu_write_8(pcp, val)	__this_cpu_generic_to_op((pcp), (val), =)
 # endif
-# define __this_cpu_write(pcp, val)	__pcpu_size_call(__this_cpu_write_, (pcp), (val))
+
+# define __this_cpu_write(pcp, val) \
+do { __this_cpu_preempt_check();					\
+     __pcpu_size_call(__this_cpu_write_, (pcp), (val));			\
+} while (0)
+
 #endif
 
 #ifndef __this_cpu_add

@@ -575,7 +587,12 @@ do {									\
 # ifndef __this_cpu_add_8
 #  define __this_cpu_add_8(pcp, val)	__this_cpu_generic_to_op((pcp), (val), +=)
 # endif
-# define __this_cpu_add(pcp, val)	__pcpu_size_call(__this_cpu_add_, (pcp), (val))
+
+# define __this_cpu_add(pcp, val) \
+do { __this_cpu_preempt_check();					\
+	__pcpu_size_call(__this_cpu_add_, (pcp), (val));		\
+} while (0)
+
 #endif
 
 #ifndef __this_cpu_sub

@@ -603,7 +620,12 @@ do {									\
 # ifndef __this_cpu_and_8
 #  define __this_cpu_and_8(pcp, val)	__this_cpu_generic_to_op((pcp), (val), &=)
 # endif
-# define __this_cpu_and(pcp, val)	__pcpu_size_call(__this_cpu_and_, (pcp), (val))
+
+# define __this_cpu_and(pcp, val) \
+do { __this_cpu_preempt_check();					\
+	__pcpu_size_call(__this_cpu_and_, (pcp), (val));		\
+} while (0)
+
 #endif
 
 #ifndef __this_cpu_or

@@ -619,7 +641,12 @@ do {									\
 # ifndef __this_cpu_or_8
 #  define __this_cpu_or_8(pcp, val)	__this_cpu_generic_to_op((pcp), (val), |=)
 # endif
-# define __this_cpu_or(pcp, val)	__pcpu_size_call(__this_cpu_or_, (pcp), (val))
+
+# define __this_cpu_or(pcp, val)	\
+do { __this_cpu_preempt_check();					\
+	__pcpu_size_call(__this_cpu_or_, (pcp), (val));			\
+} while (0)
+
 #endif
 
 #ifndef __this_cpu_xor

@@ -635,7 +662,12 @@ do {									\
 # ifndef __this_cpu_xor_8
 #  define __this_cpu_xor_8(pcp, val)	__this_cpu_generic_to_op((pcp), (val), ^=)
 # endif
-# define __this_cpu_xor(pcp, val)	__pcpu_size_call(__this_cpu_xor_, (pcp), (val))
+
+# define __this_cpu_xor(pcp, val) \
+do { __this_cpu_preempt_check();					\
+	__pcpu_size_call(__this_cpu_xor_, (pcp), (val));		\
+} while (0)
+
 #endif
 
 #define __this_cpu_generic_add_return(pcp, val)				\

@@ -658,7 +690,7 @@ do {									\
 #  define __this_cpu_add_return_8(pcp, val)	__this_cpu_generic_add_return(pcp, val)
 # endif
 # define __this_cpu_add_return(pcp, val)	\
-	__pcpu_size_call_return2(__this_cpu_add_return_, pcp, val)
+	(__this_cpu_preempt_check(),__pcpu_size_call_return2(__this_cpu_add_return_, pcp, val))
 #endif
 
 #define __this_cpu_sub_return(pcp, val)	__this_cpu_add_return(pcp, -(val))

@@ -686,7 +718,7 @@ do {									\
 #  define __this_cpu_xchg_8(pcp, nval)	__this_cpu_generic_xchg(pcp, nval)
 # endif
 # define __this_cpu_xchg(pcp, nval)	\
-	__pcpu_size_call_return2(__this_cpu_xchg_, (pcp), nval)
+	(__this_cpu_preempt_check(),__pcpu_size_call_return2(__this_cpu_xchg_, (pcp), nval))
 #endif
 
 #define __this_cpu_generic_cmpxchg(pcp, oval, nval)			\

@@ -712,7 +744,7 @@ do {									\
 #  define __this_cpu_cmpxchg_8(pcp, oval, nval)	__this_cpu_generic_cmpxchg(pcp, oval, nval)
 # endif
 # define __this_cpu_cmpxchg(pcp, oval, nval)	\
-	__pcpu_size_call_return2(__this_cpu_cmpxchg_, pcp, oval, nval)
+	(__this_cpu_preempt_check(),__pcpu_size_call_return2(__this_cpu_cmpxchg_, pcp, oval, nval))
 #endif
 
 #define __this_cpu_generic_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)	\

@@ -745,7 +777,7 @@ do {									\
 	__this_cpu_generic_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)
 # endif
 # define __this_cpu_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)	\
-	__pcpu_double_call_return_bool(__this_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2))
+	(__this_cpu_preempt_check(),__pcpu_double_call_return_bool(__this_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2)))
 #endif
 
 /*

lib/smp_processor_id.c +15 -3

Index: linux/lib/smp_processor_id.c
===================================================================
@@ -7,7 +7,7 @@
 #include <linux/kallsyms.h>
 #include <linux/sched.h>
 
-notrace unsigned int debug_smp_processor_id(void)
+notrace static unsigned int check_preemption_disabled(char *what)
 {
 	unsigned long preempt_count = preempt_count();
 	int this_cpu = raw_smp_processor_id();

@@ -39,8 +39,8 @@ notrace unsigned int debug_smp_processor
 	if (!printk_ratelimit())
 		goto out_enable;
 
-	printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
-			"code: %s/%d\n",
+	printk(KERN_ERR "%s in preemptible [%08x] "
+			"code: %s/%d\n", what,
 			preempt_count() - 1, current->comm, current->pid);
 	print_symbol("caller is %s\n", (long)__builtin_return_address(0));
 	dump_stack();

@@ -51,5 +51,17 @@ out:
 	return this_cpu;
 }
 
+notrace unsigned int debug_smp_processor_id(void)
+{
+	return check_preemption_disabled("BUG: using smp_processor_id()");
+}
 EXPORT_SYMBOL(debug_smp_processor_id);
 
+notrace void __this_cpu_preempt_check(void)
+{
+#ifdef CONFIG_DEBUG_THIS_CPU_OPERATIONS
+	check_preemption_disabled("__this_cpu operation");
+#endif
+}
+EXPORT_SYMBOL(__this_cpu_preempt_check);
+

lib/Kconfig.debug +13 -0

Index: linux/lib/Kconfig.debug
===================================================================
@@ -797,6 +797,19 @@ config DEBUG_PREEMPT
 	  if kernel code uses it in a preemption-unsafe way. Also, the kernel
 	  will detect preemption count underflows.
 
+config DEBUG_THIS_CPU_OPERATIONS
+	bool "Preemption checks for __this_cpu operations"
+	depends on DEBUG_PREEMPT
+	default n
+	help
+	  Enables preemption checks for __this_cpu macros. These
+	  are macros to generate single instruction operations on
+	  per cpu data. The option only affects the __this_cpu variant
+	  which is used when fallback to multiple instructions without
+	  other synchronization is possible should the arch not support
+	  these types of instruction. A verification is then
+	  done to make sure that the thread cannot be preempted.
+
 menu "Lock Debugging (spinlocks, mutexes, etc...)"
 
 config DEBUG_RT_MUTEXES

Peter Zijlstra

2013-10-04 08:27 UTC #3 lore raw

Re: [pchecks v2 2/2] percpu: Add preemption checks to __this_cpu ops

include/linux/percpu.h do { \ · line 549

Quoted 8 lines · Christoph Lameter

include/linux/percpu.h +2 -1

@@ -538,7 +544,8 @@ do {									\
 # ifndef __this_cpu_read_8
 #  define __this_cpu_read_8(pcp)	(*__this_cpu_ptr(&(pcp)))
 # endif
-# define __this_cpu_read(pcp)	__pcpu_size_call_return(__this_cpu_read_, (pcp))
+# define __this_cpu_read(pcp) \
+	(__this_cpu_preempt_check(),__pcpu_size_call_return(__this_cpu_read_, (pcp)))
 #endif

Would it not be move convenient to implement it in terms of the raw_this_cpu*() thingies? That way you're sure they actually do the same thing and there's only 1 site to change when changing the implementation.

Something like:

#define __this_cpu_read(pcp) \

({									\
	__this_cpu_preempt_check();					\
	raw_this_cpu_read(pcp);						\
})

lib/smp_processor_id.c notrace unsigned int debug_smp_processor · line 44

Quoted 9 lines

lib/smp_processor_id.c +2 -2

@@ -39,8 +39,8 @@ notrace unsigned int debug_smp_processor
 	if (!printk_ratelimit())
 		goto out_enable;
 
-	printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
-			"code: %s/%d\n",
+	printk(KERN_ERR "%s in preemptible [%08x] "
+			"code: %s/%d\n", what,
 			preempt_count() - 1, current->comm, current->pid);

I would argue for keeping the "BUG" string intact and in front of the %s.

Quoted 7 lines

lib/smp_processor_id.c +1 -0

 	print_symbol("caller is %s\n", (long)__builtin_return_address(0));
 	dump_stack();

@@ -51,5 +51,17 @@ out:
 	return this_cpu;
 }
 
+notrace unsigned int debug_smp_processor_id(void)

lib/smp_processor_id.c out: · line 64

Quoted 10 lines

lib/smp_processor_id.c +8 -0

+{
+	return check_preemption_disabled("BUG: using smp_processor_id()");
+}
 EXPORT_SYMBOL(debug_smp_processor_id);
 
+notrace void __this_cpu_preempt_check(void)
+{
+#ifdef CONFIG_DEBUG_THIS_CPU_OPERATIONS
+	check_preemption_disabled("__this_cpu operation");
+#endif

Because here you've forgotten it..

Quoted 2 lines

(quoted diff) +2 -0

+}
+EXPORT_SYMBOL(__this_cpu_preempt_check);

Ingo Molnar

2013-10-04 08:37 UTC #4 lore raw

Re: [pchecks v2 2/2] percpu: Add preemption checks to __this_cpu ops

Quoted 4 lines · Peter Zijlstra

Quoted 4 lines

lib/smp_processor_id.c +0 -0

@@ -39,8 +39,8 @@ notrace unsigned int debug_smp_processor
 	if (!printk_ratelimit())
 		goto out_enable;

Quoted 8 lines

lib/smp_processor_id.c notrace unsigned int debug_smp_processor · line 44
Quoted 5 lines
lib/smp_processor_id.c +2 -2
-	printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
-			"code: %s/%d\n",
+	printk(KERN_ERR "%s in preemptible [%08x] "
+			"code: %s/%d\n", what,
 			preempt_count() - 1, current->comm, current->pid);
I would argue for keeping the "BUG" string intact and in front of the %s.

Definitely.

Thanks,

Ingo

Christoph Lameter

author

2013-10-04 15:27 UTC #5 lore raw

Re: [pchecks v2 2/2] percpu: Add preemption checks to __this_cpu ops

In reply to Peter Zijlstra

Quoted 9 lines · Peter Zijlstra

include/linux/percpu.h do { \ · line 549
Quoted 4 lines
include/linux/percpu.h +2 -1
-# define __this_cpu_read(pcp)	__pcpu_size_call_return(__this_cpu_read_, (pcp))
+# define __this_cpu_read(pcp) \
+	(__this_cpu_preempt_check(),__pcpu_size_call_return(__this_cpu_read_, (pcp)))
 #endif
Would it not be move convenient to implement it in terms of the raw_this_cpu*() thingies? That way you're sure they actually do the same thing and there's only 1 site to change when changing the implementation.

The __this_cpu_read_xxx() are asm primitives provided by various arches. __this_cpu_read() is currently not overriden by any arches. That is why the approach here of replicating only the higher level for raw_cpu_ops works. Renaming the __this_cpu_xxx primitives would be a significant change.

Quoted 11 lines

lib/smp_processor_id.c notrace unsigned int debug_smp_processor · line 44
Quoted 8 lines
lib/smp_processor_id.c +2 -2
 	if (!printk_ratelimit())
 		goto out_enable;
 
-	printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
-			"code: %s/%d\n",
+	printk(KERN_ERR "%s in preemptible [%08x] "
+			"code: %s/%d\n", what,
 			preempt_count() - 1, current->comm, current->pid);
I would argue for keeping the "BUG" string intact and in front of the %s.

Most of the place that I have seen are not bugs but there was a reason for the code to run a __this_cpu op without preemption disabled.

Peter Zijlstra

2013-10-04 16:52 UTC #6 lore raw

Re: [pchecks v2 2/2] percpu: Add preemption checks to __this_cpu ops

Quoted 12 lines · Christoph Lameter

Quoted 9 lines · Peter Zijlstra
include/linux/percpu.h do { \ · line 549
Quoted 4 lines
include/linux/percpu.h +2 -1
-# define __this_cpu_read(pcp)	__pcpu_size_call_return(__this_cpu_read_, (pcp))
+# define __this_cpu_read(pcp) \
+	(__this_cpu_preempt_check(),__pcpu_size_call_return(__this_cpu_read_, (pcp)))
 #endif
Would it not be move convenient to implement it in terms of the raw_this_cpu*() thingies? That way you're sure they actually do the same thing and there's only 1 site to change when changing the implementation.

Quoted 5 lines

The __this_cpu_read_xxx() are asm primitives provided by various arches. __this_cpu_read() is currently not overriden by any arches. That is why the approach here of replicating only the higher level for raw_cpu_ops works. Renaming the __this_cpu_xxx primitives would be a significant change.

This isn't about read; this is about all of them. Make sure the raw_* implementation is the actual real implementation; then implement the checking variant in terms of those.

Quoted 12 lines

Quoted 11 lines
lib/smp_processor_id.c notrace unsigned int debug_smp_processor · line 44
Quoted 8 lines
lib/smp_processor_id.c +2 -2
 	if (!printk_ratelimit())
 		goto out_enable;
 
-	printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
-			"code: %s/%d\n",
+	printk(KERN_ERR "%s in preemptible [%08x] "
+			"code: %s/%d\n", what,
 			preempt_count() - 1, current->comm, current->pid);
I would argue for keeping the "BUG" string intact and in front of the %s.

Quoted 2 lines

Most of the place that I have seen are not bugs but there was a reason for the code to run a __this_cpu op without preemption disabled.

No; it is an actual BUG; it means that whoemever wrote the code didn't think straight and forgot to use the right primitive and comments.

Christoph Lameter

author

2013-10-04 17:26 UTC #7 lore raw

Re: [pchecks v2 2/2] percpu: Add preemption checks to __this_cpu ops

Quoted 9 lines · Peter Zijlstra

Quoted 5 lines
The __this_cpu_read_xxx() are asm primitives provided by various arches. __this_cpu_read() is currently not overriden by any arches. That is why the approach here of replicating only the higher level for raw_cpu_ops works. Renaming the __this_cpu_xxx primitives would be a significant change.
This isn't about read; this is about all of them. Make sure the raw_* implementation is the actual real implementation; then implement the checking variant in terms of those.

Yes this is valid for all of them. The __this_cpu op is the low level implementation.

Quoted 12 lines

Quoted 12 lines
Quoted 11 lines
lib/smp_processor_id.c notrace unsigned int debug_smp_processor · line 44
Quoted 8 lines
lib/smp_processor_id.c +2 -2
 	if (!printk_ratelimit())
 		goto out_enable;
 
-	printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
-			"code: %s/%d\n",
+	printk(KERN_ERR "%s in preemptible [%08x] "
+			"code: %s/%d\n", what,
 			preempt_count() - 1, current->comm, current->pid);
I would argue for keeping the "BUG" string intact and in front of the %s.

Quoted 5 lines

Quoted 2 lines
Most of the place that I have seen are not bugs but there was a reason for the code to run a __this_cpu op without preemption disabled.
No; it is an actual BUG; it means that whoemever wrote the code didn't think straight and forgot to use the right primitive and comments.

??? The assumption needs to be that they used the function as it was documented. No checks because they knew they were not needed in a particular context.