[PATCH hw_breakpoint] percpu: add __percpu sparse annotations to hw_breakpoint

[PATCH hw_breakpoint] percpu: add __percpu sparse annotations to hw_breakpoint

[Tejun Heo]

Add __percpu sparse annotations to hw_breakpoint.

These annotations are to make sparse consider percpu variables to be
in a different address space and warn if accessed without going
through percpu accessors.  This patch doesn't affect normal builds.

In kernel/hw_breakpoint.c, per_cpu(nr_task_bp_pinned, cpu)'s will
trigger sprious noderef related warnings from sparse.  Changing it to
&per_cpu(nr_task_bp_pinned[0], cpu) will work around the problem but
deemed to ugly by the maintainer.  Leave it alone until better
solution can be found.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
---
Frederic, can you please put this into the tree for hw_breakpoint?

Thanks.

 include/linux/hw_breakpoint.h           |    8 ++++----
 kernel/hw_breakpoint.c                  |   10 +++++-----
 samples/hw_breakpoint/data_breakpoint.c |    6 +++---
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index 070ba06..3cf78a7 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -66,14 +66,14 @@ register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr,
 				perf_overflow_handler_t	triggered,
 				int cpu);
 
-extern struct perf_event **
+extern struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered);
 
 extern int register_perf_hw_breakpoint(struct perf_event *bp);
 extern int __register_perf_hw_breakpoint(struct perf_event *bp);
 extern void unregister_hw_breakpoint(struct perf_event *bp);
-extern void unregister_wide_hw_breakpoint(struct perf_event **cpu_events);
+extern void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events);
 
 extern int dbg_reserve_bp_slot(struct perf_event *bp);
 extern int dbg_release_bp_slot(struct perf_event *bp);
@@ -100,7 +100,7 @@ static inline struct perf_event *
 register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr,
 				perf_overflow_handler_t	 triggered,
 				int cpu)		{ return NULL; }
-static inline struct perf_event **
+static inline struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered)	{ return NULL; }
 static inline int
@@ -109,7 +109,7 @@ static inline int
 __register_perf_hw_breakpoint(struct perf_event *bp) 	{ return -ENOSYS; }
 static inline void unregister_hw_breakpoint(struct perf_event *bp)	{ }
 static inline void
-unregister_wide_hw_breakpoint(struct perf_event **cpu_events)		{ }
+unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)	{ }
 static inline int
 reserve_bp_slot(struct perf_event *bp)			{return -ENOSYS; }
 static inline void release_bp_slot(struct perf_event *bp) 		{ }
diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index 8a5c7d5..4781c68 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -413,17 +413,17 @@ EXPORT_SYMBOL_GPL(unregister_hw_breakpoint);
  *
  * @return a set of per_cpu pointers to perf events
  */
-struct perf_event **
+struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered)
 {
-	struct perf_event **cpu_events, **pevent, *bp;
+	struct perf_event * __percpu *cpu_events, **pevent, *bp;
 	long err;
 	int cpu;
 
 	cpu_events = alloc_percpu(typeof(*cpu_events));
 	if (!cpu_events)
-		return ERR_PTR(-ENOMEM);
+		return (void __percpu __force *)ERR_PTR(-ENOMEM);
 
 	get_online_cpus();
 	for_each_online_cpu(cpu) {
@@ -451,7 +451,7 @@ fail:
 	put_online_cpus();
 
 	free_percpu(cpu_events);
-	return ERR_PTR(err);
+	return (void __percpu __force *)ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);
 
@@ -459,7 +459,7 @@ EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);
  * unregister_wide_hw_breakpoint - unregister a wide breakpoint in the kernel
  * @cpu_events: the per cpu set of events to unregister
  */
-void unregister_wide_hw_breakpoint(struct perf_event **cpu_events)
+void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)
 {
 	int cpu;
 	struct perf_event **pevent;
diff --git a/samples/hw_breakpoint/data_breakpoint.c b/samples/hw_breakpoint/data_breakpoint.c
index c69cbe9..bd0f337 100644
--- a/samples/hw_breakpoint/data_breakpoint.c
+++ b/samples/hw_breakpoint/data_breakpoint.c
@@ -34,7 +34,7 @@
 #include <linux/perf_event.h>
 #include <linux/hw_breakpoint.h>
 
-struct perf_event **sample_hbp;
+struct perf_event * __percpu *sample_hbp;
 
 static char ksym_name[KSYM_NAME_LEN] = "pid_max";
 module_param_string(ksym, ksym_name, KSYM_NAME_LEN, S_IRUGO);
@@ -61,8 +61,8 @@ static int __init hw_break_module_init(void)
 	attr.bp_type = HW_BREAKPOINT_W | HW_BREAKPOINT_R;
 
 	sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler);
-	if (IS_ERR(sample_hbp)) {
-		ret = PTR_ERR(sample_hbp);
+	if (IS_ERR((void __force *)sample_hbp)) {
+		ret = PTR_ERR((void __force *)sample_hbp);
 		goto fail;
 	}
 
-- 
1.6.4.2

Re: [PATCH hw_breakpoint] percpu: add __percpu sparse annotations to hw_breakpoint

[Frederic Weisbecker]

On Wed, Feb 17, 2010 at 10:50:50AM +0900, Tejun Heo wrote:
> Add __percpu sparse annotations to hw_breakpoint.
> 
> These annotations are to make sparse consider percpu variables to be
> in a different address space and warn if accessed without going
> through percpu accessors.  This patch doesn't affect normal builds.
> 
> In kernel/hw_breakpoint.c, per_cpu(nr_task_bp_pinned, cpu)'s will
> trigger sprious noderef related warnings from sparse.  Changing it to
> &per_cpu(nr_task_bp_pinned[0], cpu) will work around the problem but
> deemed to ugly by the maintainer.  Leave it alone until better
> solution can be found.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> Frederic, can you please put this into the tree for hw_breakpoint?
> 
> Thanks.


Yeah, looks good, I'm queuing it.
Just few comments below, for nano-considerations.



>  	cpu_events = alloc_percpu(typeof(*cpu_events));
>  	if (!cpu_events)
> -		return ERR_PTR(-ENOMEM);
> +		return (void __percpu __force *)ERR_PTR(-ENOMEM);



Is this pattern common enough that we can think about a ERR_CPU_PTR ?




>  	sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler);
> -	if (IS_ERR(sample_hbp)) {
> -		ret = PTR_ERR(sample_hbp);
> +	if (IS_ERR((void __force *)sample_hbp)) {
> +		ret = PTR_ERR((void __force *)sample_hbp);


Same comments here, although I wouldn't like much a CPU_PTR_ERR or
IS_ERR_CPU.... CPP is just so poor in magic for that.

I must confess I miss a bit the old per_cpu prefix that guarded the implicit
separate namespace.

Anyway, I'm queuing it, thanks.

Re: [PATCH hw_breakpoint] percpu: add __percpu sparse annotations to hw_breakpoint

[Tejun Heo]

Hello,

On 02/18/2010 01:39 AM, Frederic Weisbecker wrote:
> On Wed, Feb 17, 2010 at 10:50:50AM +0900, Tejun Heo wrote:
> Yeah, looks good, I'm queuing it.
> Just few comments below, for nano-considerations.
>>  	cpu_events = alloc_percpu(typeof(*cpu_events));
>>  	if (!cpu_events)
>> -		return ERR_PTR(-ENOMEM);
>> +		return (void __percpu __force *)ERR_PTR(-ENOMEM);
> 
> Is this pattern common enough that we can think about a ERR_CPU_PTR ?

I thought about that but there aren't too many yet, so I just added
the ugly castings.  It would be cool if sparse can be taught that
ERR_PTR() returns universal pseudo pointer.

>>  	sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler);
>> -	if (IS_ERR(sample_hbp)) {
>> -		ret = PTR_ERR(sample_hbp);
>> +	if (IS_ERR((void __force *)sample_hbp)) {
>> +		ret = PTR_ERR((void __force *)sample_hbp);
> 
> Same comments here, although I wouldn't like much a CPU_PTR_ERR or
> IS_ERR_CPU.... CPP is just so poor in magic for that.
> 
> I must confess I miss a bit the old per_cpu prefix that guarded the implicit
> separate namespace.

Yeap, I agree that the prefix had its advantages.  It's just that it
can't scale to the new situation where static and dynamic percpu
variables behave uniformly.

Thank you.

-- 
tejun

Re: [PATCH hw_breakpoint] percpu: add __percpu sparse annotations to hw_breakpoint

[Frederic Weisbecker]

On Thu, Feb 18, 2010 at 09:49:08AM +0900, Tejun Heo wrote:
> Hello,
> 
> On 02/18/2010 01:39 AM, Frederic Weisbecker wrote:
> > On Wed, Feb 17, 2010 at 10:50:50AM +0900, Tejun Heo wrote:
> > Yeah, looks good, I'm queuing it.
> > Just few comments below, for nano-considerations.
> >>  	cpu_events = alloc_percpu(typeof(*cpu_events));
> >>  	if (!cpu_events)
> >> -		return ERR_PTR(-ENOMEM);
> >> +		return (void __percpu __force *)ERR_PTR(-ENOMEM);
> > 
> > Is this pattern common enough that we can think about a ERR_CPU_PTR ?
> 
> I thought about that but there aren't too many yet, so I just added
> the ugly castings.  It would be cool if sparse can be taught that
> ERR_PTR() returns universal pseudo pointer.



Yeah, it would be nice to just have a universal address space
that is compatible with all others. It's sad to see such
uglification to make a secondary tool happy.

 
> >>  	sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler);
> >> -	if (IS_ERR(sample_hbp)) {
> >> -		ret = PTR_ERR(sample_hbp);
> >> +	if (IS_ERR((void __force *)sample_hbp)) {
> >> +		ret = PTR_ERR((void __force *)sample_hbp);
> > 
> > Same comments here, although I wouldn't like much a CPU_PTR_ERR or
> > IS_ERR_CPU.... CPP is just so poor in magic for that.
> > 
> > I must confess I miss a bit the old per_cpu prefix that guarded the implicit
> > separate namespace.
> 
> Yeap, I agree that the prefix had its advantages.  It's just that it
> can't scale to the new situation where static and dynamic percpu
> variables behave uniformly.


Well, I miss a bit of per cpu internals so I won't argue further :)

Thanks.

[tip:perf/core] percpu: Add __percpu sparse annotations to hw_breakpoint

[tip-bot for Tejun Heo]

Commit-ID:  44ee63587dce85593c22497140db16f4e5027860
Gitweb:     http://git.kernel.org/tip/44ee63587dce85593c22497140db16f4e5027860
Author:     Tejun Heo <tj@kernel.org>
AuthorDate: Wed, 17 Feb 2010 10:50:50 +0900
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Sat, 27 Feb 2010 16:23:39 +0100

percpu: Add __percpu sparse annotations to hw_breakpoint

Add __percpu sparse annotations to hw_breakpoint.

These annotations are to make sparse consider percpu variables to be
in a different address space and warn if accessed without going
through percpu accessors.  This patch doesn't affect normal builds.

In kernel/hw_breakpoint.c, per_cpu(nr_task_bp_pinned, cpu)'s will
trigger spurious noderef related warnings from sparse.  Changing it to
&per_cpu(nr_task_bp_pinned[0], cpu) will work around the problem but
deemed to ugly by the maintainer.  Leave it alone until better
solution can be found.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: K.Prasad <prasad@linux.vnet.ibm.com>
LKML-Reference: <4B7B4B7A.9050902@kernel.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/hw_breakpoint.h           |    8 ++++----
 kernel/hw_breakpoint.c                  |   10 +++++-----
 samples/hw_breakpoint/data_breakpoint.c |    6 +++---
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index 5977b72..c70d27a 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -66,14 +66,14 @@ register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr,
 				perf_overflow_handler_t	triggered,
 				int cpu);
 
-extern struct perf_event **
+extern struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered);
 
 extern int register_perf_hw_breakpoint(struct perf_event *bp);
 extern int __register_perf_hw_breakpoint(struct perf_event *bp);
 extern void unregister_hw_breakpoint(struct perf_event *bp);
-extern void unregister_wide_hw_breakpoint(struct perf_event **cpu_events);
+extern void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events);
 
 extern int dbg_reserve_bp_slot(struct perf_event *bp);
 extern int dbg_release_bp_slot(struct perf_event *bp);
@@ -100,7 +100,7 @@ static inline struct perf_event *
 register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr,
 				perf_overflow_handler_t	 triggered,
 				int cpu)		{ return NULL; }
-static inline struct perf_event **
+static inline struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered)	{ return NULL; }
 static inline int
@@ -109,7 +109,7 @@ static inline int
 __register_perf_hw_breakpoint(struct perf_event *bp) 	{ return -ENOSYS; }
 static inline void unregister_hw_breakpoint(struct perf_event *bp)	{ }
 static inline void
-unregister_wide_hw_breakpoint(struct perf_event **cpu_events)		{ }
+unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)	{ }
 static inline int
 reserve_bp_slot(struct perf_event *bp)			{return -ENOSYS; }
 static inline void release_bp_slot(struct perf_event *bp) 		{ }
diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index 967e661..6542eac 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -413,17 +413,17 @@ EXPORT_SYMBOL_GPL(unregister_hw_breakpoint);
  *
  * @return a set of per_cpu pointers to perf events
  */
-struct perf_event **
+struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered)
 {
-	struct perf_event **cpu_events, **pevent, *bp;
+	struct perf_event * __percpu *cpu_events, **pevent, *bp;
 	long err;
 	int cpu;
 
 	cpu_events = alloc_percpu(typeof(*cpu_events));
 	if (!cpu_events)
-		return ERR_PTR(-ENOMEM);
+		return (void __percpu __force *)ERR_PTR(-ENOMEM);
 
 	get_online_cpus();
 	for_each_online_cpu(cpu) {
@@ -451,7 +451,7 @@ fail:
 	put_online_cpus();
 
 	free_percpu(cpu_events);
-	return ERR_PTR(err);
+	return (void __percpu __force *)ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);
 
@@ -459,7 +459,7 @@ EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);
  * unregister_wide_hw_breakpoint - unregister a wide breakpoint in the kernel
  * @cpu_events: the per cpu set of events to unregister
  */
-void unregister_wide_hw_breakpoint(struct perf_event **cpu_events)
+void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)
 {
 	int cpu;
 	struct perf_event **pevent;
diff --git a/samples/hw_breakpoint/data_breakpoint.c b/samples/hw_breakpoint/data_breakpoint.c
index c69cbe9..bd0f337 100644
--- a/samples/hw_breakpoint/data_breakpoint.c
+++ b/samples/hw_breakpoint/data_breakpoint.c
@@ -34,7 +34,7 @@
 #include <linux/perf_event.h>
 #include <linux/hw_breakpoint.h>
 
-struct perf_event **sample_hbp;
+struct perf_event * __percpu *sample_hbp;
 
 static char ksym_name[KSYM_NAME_LEN] = "pid_max";
 module_param_string(ksym, ksym_name, KSYM_NAME_LEN, S_IRUGO);
@@ -61,8 +61,8 @@ static int __init hw_break_module_init(void)
 	attr.bp_type = HW_BREAKPOINT_W | HW_BREAKPOINT_R;
 
 	sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler);
-	if (IS_ERR(sample_hbp)) {
-		ret = PTR_ERR(sample_hbp);
+	if (IS_ERR((void __force *)sample_hbp)) {
+		ret = PTR_ERR((void __force *)sample_hbp);
 		goto fail;
 	}