percpu: Define this_cpu_cpumask_var_t_ptr

percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

I think I finally found the source of the nasty failures of the
__get_cpu_var patchset:



Subject: __get_cpu_var/cpumask_var_t: Resolve ambiguities

__get_cpu_var can paper over differences in the definitions
of cpumask_var_t and either use the address of the cpumask
variable directly or perform a fetch of the address of the
struct cpumask allocated elsewhere. This is important
particularly when using per cpu cpumask_var_t declarations
because in one case we have an offset into a per cpu area
to handle and in the other case we need to fetch a pointer
from the offset.

This patch introduces a new macro

this_cpu_cpumask_var_t_ptr()

that is defined where cpumask_var_t is defined and performs
the proper actions. All use cases where __get_cpu_var
is used with cpumask_var_t are converted to the use
of this_cpu_cpumask_var_t_ptr().

Signed-off-by: Christoph Lameter <cl@linux.com>
---
 include/linux/kernel_stat.h   |  4 ++--
 kernel/events/callchain.c     |  4 ++--
 kernel/events/core.c          | 24 ++++++++++++------------
 kernel/sched/deadline.c       |  2 +-
 kernel/sched/fair.c           |  2 +-
 kernel/sched/rt.c             |  2 +-
 kernel/sched/sched.h          |  4 ++--
 kernel/taskstats.c            |  2 +-
 kernel/time/tick-sched.c      |  4 ++--
 kernel/user-return-notifier.c |  4 ++--
 10 files changed, 26 insertions(+), 26 deletions(-)

Index: linux/include/linux/cpumask.h
===================================================================
--- linux.orig/include/linux/cpumask.h	2014-08-07 09:36:05.954916998 -0500
+++ linux/include/linux/cpumask.h	2014-08-07 09:55:38.732748680 -0500
@@ -666,10 +666,19 @@
  *
  * This code makes NR_CPUS length memcopy and brings to a memory corruption.
  * cpumask_copy() provide safe copy functionality.
+ *
+ * Note that there is another evil here: If you define a cpumask_var_t
+ * as a percpu variable then the way to obtain the address of the cpumask
+ * structure differently influences what this_cpu_* operation needs to be
+ * used. Please use this_cpu_cpumask_var_t in those cases. The direct use
+ * of this_cpu_ptr() or this_cpu_read() will lead to failures when the
+ * other type of cpumask_var_t implementation is configured.
  */
 #ifdef CONFIG_CPUMASK_OFFSTACK
 typedef struct cpumask *cpumask_var_t;

+#define this_cpu_cpumask_var_t_ptr(x) this_cpu_read(x)
+
 bool alloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
 bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags);
 bool zalloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
@@ -681,6 +690,8 @@
 #else
 typedef struct cpumask cpumask_var_t[1];

+#define this_cpu_cpumask_var_t_ptr(x) this_cpu_ptr(&x)
+
 static inline bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
 {
 	return true;
Index: linux/kernel/sched/deadline.c
===================================================================
--- linux.orig/kernel/sched/deadline.c	2014-08-07 09:36:05.954916998 -0500
+++ linux/kernel/sched/deadline.c	2014-08-07 09:36:05.902917978 -0500
@@ -1158,7 +1158,7 @@
 static int find_later_rq(struct task_struct *task)
 {
 	struct sched_domain *sd;
-	struct cpumask *later_mask = __get_cpu_var(local_cpu_mask_dl);
+	struct cpumask *later_mask = this_cpu_cpumask_var_t_ptr(local_cpu_mask_dl);
 	int this_cpu = smp_processor_id();
 	int best_cpu, cpu = task_cpu(task);

Index: linux/kernel/sched/fair.c
===================================================================
--- linux.orig/kernel/sched/fair.c	2014-08-07 09:36:05.954916998 -0500
+++ linux/kernel/sched/fair.c	2014-08-07 09:36:05.906917903 -0500
@@ -6490,7 +6490,7 @@
 	struct sched_group *group;
 	struct rq *busiest;
 	unsigned long flags;
-	struct cpumask *cpus = __get_cpu_var(load_balance_mask);
+	struct cpumask *cpus = this_cpu_cpumask_var_t_ptr(load_balance_mask);

 	struct lb_env env = {
 		.sd		= sd,
Index: linux/kernel/sched/rt.c
===================================================================
--- linux.orig/kernel/sched/rt.c	2014-08-07 09:36:05.954916998 -0500
+++ linux/kernel/sched/rt.c	2014-08-07 09:36:05.906917903 -0500
@@ -1522,7 +1522,7 @@
 static int find_lowest_rq(struct task_struct *task)
 {
 	struct sched_domain *sd;
-	struct cpumask *lowest_mask = __get_cpu_var(local_cpu_mask);
+	struct cpumask *lowest_mask = this_cpu_cpumask_var_t_ptr(local_cpu_mask);
 	int this_cpu = smp_processor_id();
 	int cpu      = task_cpu(task);

Index: linux/arch/x86/include/asm/perf_event_p4.h
===================================================================
--- linux.orig/arch/x86/include/asm/perf_event_p4.h	2014-08-07 09:36:05.954916998 -0500
+++ linux/arch/x86/include/asm/perf_event_p4.h	2014-08-07 09:36:05.910917828 -0500
@@ -189,7 +189,7 @@
 {
 #ifdef CONFIG_SMP
 	if (smp_num_siblings == 2)
-		return cpu != cpumask_first(__get_cpu_var(cpu_sibling_map));
+		return cpu != cpumask_first(this_cpu_cpumask_var_t_ptr(cpu_sibling_map));
 #endif
 	return 0;
 }
Index: linux/arch/x86/oprofile/op_model_p4.c
===================================================================
--- linux.orig/arch/x86/oprofile/op_model_p4.c	2014-08-07 09:36:05.954916998 -0500
+++ linux/arch/x86/oprofile/op_model_p4.c	2014-08-07 09:36:05.910917828 -0500
@@ -372,7 +372,7 @@
 {
 #ifdef CONFIG_SMP
 	int cpu = smp_processor_id();
-	return cpu != cpumask_first(__get_cpu_var(cpu_sibling_map));
+	return cpu != cpumask_first(this_cpu_cpumask_var_t_ptr(cpu_sibling_map));
 #endif
 	return 0;
 }

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

Needs a small fixup

From: Christoph Lameter <cl@linux.com>
Subject: One & too much

Remove it.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/include/linux/cpumask.h
===================================================================
--- linux.orig/include/linux/cpumask.h
+++ linux/include/linux/cpumask.h
@@ -690,7 +690,7 @@ void free_bootmem_cpumask_var(cpumask_va
 #else
 typedef struct cpumask cpumask_var_t[1];

-#define this_cpu_cpumask_var_t_ptr(x) this_cpu_ptr(&x)
+#define this_cpu_cpumask_var_t_ptr(x) this_cpu_ptr(x)

 static inline bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
 {

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Tejun Heo]

Hello, Christoph.

On Thu, Aug 07, 2014 at 10:05:45AM -0500, Christoph Lameter wrote:
> @@ -666,10 +666,19 @@
>   *
>   * This code makes NR_CPUS length memcopy and brings to a memory corruption.
>   * cpumask_copy() provide safe copy functionality.
> + *
> + * Note that there is another evil here: If you define a cpumask_var_t
> + * as a percpu variable then the way to obtain the address of the cpumask
> + * structure differently influences what this_cpu_* operation needs to be
> + * used. Please use this_cpu_cpumask_var_t in those cases. The direct use
> + * of this_cpu_ptr() or this_cpu_read() will lead to failures when the
> + * other type of cpumask_var_t implementation is configured.
>   */
>  #ifdef CONFIG_CPUMASK_OFFSTACK
>  typedef struct cpumask *cpumask_var_t;
> 
> +#define this_cpu_cpumask_var_t_ptr(x) this_cpu_read(x)
> +
>  bool alloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
>  bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags);
>  bool zalloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
> @@ -681,6 +690,8 @@
>  #else
>  typedef struct cpumask cpumask_var_t[1];
> 
> +#define this_cpu_cpumask_var_t_ptr(x) this_cpu_ptr(&x)

Urgh, this is nasty but yeah I can't think of any other way around it
either. :(

Do we need the "_t" in the name tho?  Maybe we can shorten the name to
this_cpumask_var_ptr(x)?  Also, wouldn't it be better to define it as
a static inline function so that the input type is explicit?

Thanks.

-- 
tejun

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

On Thu, 21 Aug 2014, Tejun Heo wrote:

> >
> > +#define this_cpu_cpumask_var_t_ptr(x) this_cpu_ptr(&x)
>
> Urgh, this is nasty but yeah I can't think of any other way around it
> either. :(
>
> Do we need the "_t" in the name tho?  Maybe we can shorten the name to
> this_cpumask_var_ptr(x)?  Also, wouldn't it be better to define it as
> a static inline function so that the input type is explicit?

Its a pretty simple function (actually more a name substituion) so I
did not think it worth creating an inline function.

_t is there because I wanted to include the full "ugly" name of the
variable to make it similarly ugly. It is needed to make the clear
distinction to "struct cpumask *" which does not have these issues.

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Tejun Heo]

Hello, Christoph.

On Thu, Aug 21, 2014 at 08:03:25PM -0500, Christoph Lameter wrote:
> Its a pretty simple function (actually more a name substituion) so I
> did not think it worth creating an inline function.

Unless there are specific reasons like multi-type arg or breaking
hellish definition order dependency, I think we're better off with
inline functions, especially here, as the implementation will happily
accept arguments of the wrong type.

> _t is there because I wanted to include the full "ugly" name of the
> variable to make it similarly ugly. It is needed to make the clear
> distinction to "struct cpumask *" which does not have these issues.

The compiler can enforce that rule easily if the interface functions
are properly typed.  I think it'd be far better to go with properly
typed accessors with less unwieldy names.

Thanks.

-- 
tejun

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

On Fri, 22 Aug 2014, Tejun Heo wrote:

> On Thu, Aug 21, 2014 at 08:03:25PM -0500, Christoph Lameter wrote:
> > Its a pretty simple function (actually more a name substituion) so I
> > did not think it worth creating an inline function.
>
> Unless there are specific reasons like multi-type arg or breaking
> hellish definition order dependency, I think we're better off with
> inline functions, especially here, as the implementation will happily
> accept arguments of the wrong type.
>

It wont accept the wrong type since the this_cpu_* functions will do type
checking.

> > _t is there because I wanted to include the full "ugly" name of the
> > variable to make it similarly ugly. It is needed to make the clear
> > distinction to "struct cpumask *" which does not have these issues.
>
> The compiler can enforce that rule easily if the interface functions
> are properly typed.  I think it'd be far better to go with properly
> typed accessors with less unwieldy names.

What rule are we talking about? Accessors for what?

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Tejun Heo]

Hello, Christoph.

On Fri, Aug 22, 2014 at 12:43:25PM -0500, Christoph Lameter wrote:
> It wont accept the wrong type since the this_cpu_* functions will do type
> checking.

It should only accept cpumask_var_t but the macro version accepts
anything that this_cpu_*() can handle.

> > > _t is there because I wanted to include the full "ugly" name of the
> > > variable to make it similarly ugly. It is needed to make the clear
> > > distinction to "struct cpumask *" which does not have these issues.
> >
> > The compiler can enforce that rule easily if the interface functions
> > are properly typed.  I think it'd be far better to go with properly
> > typed accessors with less unwieldy names.
> 
> What rule are we talking about? Accessors for what?

I meant that if the new accessors you're adding are proper inline
functions, the compiler would be able to verify the specific type they
should take.  IOW, let's go for shorter name w/ stricter type
checking.

Thanks.

-- 
tejun

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

On Sat, 23 Aug 2014, Tejun Heo wrote:

> Hello, Christoph.
>
> On Fri, Aug 22, 2014 at 12:43:25PM -0500, Christoph Lameter wrote:
> > It wont accept the wrong type since the this_cpu_* functions will do type
> > checking.
>
> It should only accept cpumask_var_t but the macro version accepts
> anything that this_cpu_*() can handle.

Ah, Ok.

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

On Sat, 23 Aug 2014, Christoph Lameter wrote:

> On Sat, 23 Aug 2014, Tejun Heo wrote:
>
> > Hello, Christoph.
> >
> > On Fri, Aug 22, 2014 at 12:43:25PM -0500, Christoph Lameter wrote:
> > > It wont accept the wrong type since the this_cpu_* functions will do type
> > > checking.
> >
> > It should only accept cpumask_var_t but the macro version accepts
> > anything that this_cpu_*() can handle.
>
> Ah, Ok.

Ok I tried to change it to an inline function. The problem is the
cpumask.h is included very early. this_cpu ops require functionality
that is not available at that point. I think it cannot be more than a
macro unless we define it elsewhere.

Regarding naming:

this_cpu_ptr_cpumask_var()

is ok?

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Tejun Heo]

Hello,

On Tue, Aug 26, 2014 at 04:33:28PM -0500, Christoph Lameter wrote:
> Ok I tried to change it to an inline function. The problem is the
> cpumask.h is included very early. this_cpu ops require functionality
> that is not available at that point. I think it cannot be more than a
> macro unless we define it elsewhere.

Ugh.... include hell. :( Does putting the accessors in percpu.h make
any difference?  Given the tricky nature of cpumask_var_t, I think
type checking can be pretty useful.

> Regarding naming:
> 
> this_cpu_ptr_cpumask_var()
> 
> is ok?

Wouldn't this_cpu_cpumask_var_ptr() be a bit more natural?

Thanks.

-- 
tejun

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

On Tue, 26 Aug 2014, Tejun Heo wrote:

> Ugh.... include hell. :( Does putting the accessors in percpu.h make
> any difference?  Given the tricky nature of cpumask_var_t, I think
> type checking can be pretty useful.

Then its going to be difficult to find. This is related to the
cpumark_var_t handling and should be defined close to where it is
introduced and discussed.

>
> > Regarding naming:
> >
> > this_cpu_ptr_cpumask_var()
> >
> > is ok?
>
> Wouldn't this_cpu_cpumask_var_ptr() be a bit more natural?

Ok.

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Christoph Lameter]

New patch:

From: Christoph Lameter <cl@linux.com>
Subject: __get_cpu_var/cpumask_var_t: Resolve ambiguities v2

__get_cpu_var can paper over differences in the definitions
of cpumask_var_t and either use the address of the cpumask
variable directly or perform a fetch of the address of the
struct cpumask allocated elsewhere. This is important
particularly when using per cpu cpumask_var_t declarations
because in one case we have an offset into a per cpu area
to handle and in the other case we need to fetch a pointer
from the offset.

This patch introduces a new macro

this_cpu_cpumask_var_t_ptr()

that is defined where cpumask_var_t is defined and performs
the proper actions. All use cases where __get_cpu_var
is used with cpumask_var_t are converted to the use
of this_cpu_cpumask_var_t_ptr().

Signed-off-by: Christoph Lameter <cl@linux.com>
---
 arch/x86/include/asm/perf_event_p4.h |  2 +-
 arch/x86/oprofile/op_model_p4.c      |  2 +-
 include/linux/cpumask.h              | 11 +++++++++++
 kernel/sched/deadline.c              |  2 +-
 kernel/sched/fair.c                  |  2 +-
 kernel/sched/rt.c                    |  2 +-
 6 files changed, 16 insertions(+), 5 deletions(-)

Index: linux/arch/x86/include/asm/perf_event_p4.h
===================================================================
--- linux.orig/arch/x86/include/asm/perf_event_p4.h
+++ linux/arch/x86/include/asm/perf_event_p4.h
@@ -189,7 +189,7 @@ static inline int p4_ht_thread(int cpu)
 {
 #ifdef CONFIG_SMP
 	if (smp_num_siblings == 2)
-		return cpu != cpumask_first(__get_cpu_var(cpu_sibling_map));
+		return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));
 #endif
 	return 0;
 }
Index: linux/arch/x86/oprofile/op_model_p4.c
===================================================================
--- linux.orig/arch/x86/oprofile/op_model_p4.c
+++ linux/arch/x86/oprofile/op_model_p4.c
@@ -372,7 +372,7 @@ static unsigned int get_stagger(void)
 {
 #ifdef CONFIG_SMP
 	int cpu = smp_processor_id();
-	return cpu != cpumask_first(__get_cpu_var(cpu_sibling_map));
+	return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));
 #endif
 	return 0;
 }
Index: linux/include/linux/cpumask.h
===================================================================
--- linux.orig/include/linux/cpumask.h
+++ linux/include/linux/cpumask.h
@@ -666,10 +666,19 @@ static inline size_t cpumask_size(void)
  *
  * This code makes NR_CPUS length memcopy and brings to a memory corruption.
  * cpumask_copy() provide safe copy functionality.
+ *
+ * Note that there is another evil here: If you define a cpumask_var_t
+ * as a percpu variable then the way to obtain the address of the cpumask
+ * structure differently influences what this_cpu_* operation needs to be
+ * used. Please use this_cpu_cpumask_var_t in those cases. The direct use
+ * of this_cpu_ptr() or this_cpu_read() will lead to failures when the
+ * other type of cpumask_var_t implementation is configured.
  */
 #ifdef CONFIG_CPUMASK_OFFSTACK
 typedef struct cpumask *cpumask_var_t;

+#define this_cpu_cpumask_var_ptr(x) this_cpu_read(x)
+
 bool alloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
 bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags);
 bool zalloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
@@ -681,6 +690,8 @@ void free_bootmem_cpumask_var(cpumask_va
 #else
 typedef struct cpumask cpumask_var_t[1];

+#define this_cpu_cpumask_var_ptr(x) this_cpu_ptr(x)
+
 static inline bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
 {
 	return true;
Index: linux/kernel/sched/deadline.c
===================================================================
--- linux.orig/kernel/sched/deadline.c
+++ linux/kernel/sched/deadline.c
@@ -1158,7 +1158,7 @@ static DEFINE_PER_CPU(cpumask_var_t, loc
 static int find_later_rq(struct task_struct *task)
 {
 	struct sched_domain *sd;
-	struct cpumask *later_mask = __get_cpu_var(local_cpu_mask_dl);
+	struct cpumask *later_mask = this_cpu_cpumask_var_ptr(local_cpu_mask_dl);
 	int this_cpu = smp_processor_id();
 	int best_cpu, cpu = task_cpu(task);

Index: linux/kernel/sched/fair.c
===================================================================
--- linux.orig/kernel/sched/fair.c
+++ linux/kernel/sched/fair.c
@@ -6539,7 +6539,7 @@ static int load_balance(int this_cpu, st
 	struct sched_group *group;
 	struct rq *busiest;
 	unsigned long flags;
-	struct cpumask *cpus = __get_cpu_var(load_balance_mask);
+	struct cpumask *cpus = this_cpu_cpumask_var_ptr(load_balance_mask);

 	struct lb_env env = {
 		.sd		= sd,
Index: linux/kernel/sched/rt.c
===================================================================
--- linux.orig/kernel/sched/rt.c
+++ linux/kernel/sched/rt.c
@@ -1526,7 +1526,7 @@ static DEFINE_PER_CPU(cpumask_var_t, loc
 static int find_lowest_rq(struct task_struct *task)
 {
 	struct sched_domain *sd;
-	struct cpumask *lowest_mask = __get_cpu_var(local_cpu_mask);
+	struct cpumask *lowest_mask = this_cpu_cpumask_var_ptr(local_cpu_mask);
 	int this_cpu = smp_processor_id();
 	int cpu      = task_cpu(task);

Index: linux/arch/x86/kernel/apic/x2apic_cluster.c
===================================================================
--- linux.orig/arch/x86/kernel/apic/x2apic_cluster.c
+++ linux/arch/x86/kernel/apic/x2apic_cluster.c
@@ -42,8 +42,7 @@ __x2apic_send_IPI_mask(const struct cpum
 	 * We are to modify mask, so we need an own copy
 	 * and be sure it's manipulated with irq off.
 	 */
-	ipi_mask_ptr = __raw_get_cpu_var(ipi_mask);
-	cpumask_copy(ipi_mask_ptr, mask);
+	ipi_mask_ptr = this_cpu_cpumask_var_ptr(ipi_mask);

 	/*
 	 * The idea is to send one IPI per cluster.

Re: percpu: Define this_cpu_cpumask_var_t_ptr

[Tejun Heo]

Hello,

On Tue, Aug 26, 2014 at 07:12:21PM -0500, Christoph Lameter wrote:
> 
> New patch:
> 
> From: Christoph Lameter <cl@linux.com>
> Subject: __get_cpu_var/cpumask_var_t: Resolve ambiguities v2
> 
> __get_cpu_var can paper over differences in the definitions
> of cpumask_var_t and either use the address of the cpumask
> variable directly or perform a fetch of the address of the
> struct cpumask allocated elsewhere. This is important
> particularly when using per cpu cpumask_var_t declarations
> because in one case we have an offset into a per cpu area
> to handle and in the other case we need to fetch a pointer
> from the offset.
> 
> This patch introduces a new macro
> 
> this_cpu_cpumask_var_t_ptr()
> 
> that is defined where cpumask_var_t is defined and performs
> the proper actions. All use cases where __get_cpu_var
> is used with cpumask_var_t are converted to the use
> of this_cpu_cpumask_var_t_ptr().
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>

Rusty, if this looks okay to you, I'll route this through
percpu/for-3.18-consistent-ops branch.

Thanks.

-- 
tejun