[PATCH 0/5] perf cleanups

[PATCH 0/5] perf cleanups

[Namhyung Kim]

Hello,

This patchset tries to remove some of warnings from sparse.
Please take a look and consider applying it. Any comments
would be welcomed.

Thanks.

---

Namhyung Kim (5):
  perf: Wrap perf_lock_task_context using __cond_lock()
  perf: Annotate lock context on perf_output_begin/end()
  perf: Remove 'extern' on hw_perf_event_init() definition
  perf: Make perf_event_wakeup() static
  perf: Declare hw_perf_{dis,en}able() in perf_event.h

 include/linux/perf_event.h |    8 +++++++-
 kernel/perf_event.c        |   17 +++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

--
1.7.2.2

[PATCH 1/5] perf: Wrap perf_lock_task_context using __cond_lock()

[Namhyung Kim]

perf_lock_task_context() conditionally grabs ctx->lock in case of
returning non-NULL. Rename and wrap it using __cond_lock to make
sparse happy.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 kernel/perf_event.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index db5b560..c058a42 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -151,7 +151,7 @@ static u64 primary_event_id(struct perf_event *event)
  * the context could get moved to another task.
  */
 static struct perf_event_context *
-perf_lock_task_context(struct task_struct *task, unsigned long *flags)
+__perf_lock_task_context(struct task_struct *task, unsigned long *flags)
 {
 	struct perf_event_context *ctx;
 
@@ -184,6 +184,14 @@ perf_lock_task_context(struct task_struct *task, unsigned long *flags)
 	return ctx;
 }
 
+static inline struct perf_event_context *
+perf_lock_task_context(struct task_struct *task, unsigned long *flags)
+{
+	struct perf_event_context *ctx;
+	__cond_lock(&ctx->lock, ctx = __perf_lock_task_context(task, flags));
+	return ctx;
+}
+
 /*
  * Get the context for a task and increment its pin_count so it
  * can't get swapped to another task.  This also increments its
-- 
1.7.2.2

[PATCH 2/5] perf: Annotate lock context on perf_output_begin/end()

[Namhyung Kim]

perf_output_begin() conditionally grabs RCU read lock in case of
returning 0. Rename and wrap it using __cond_lock() to annotate
this. Although perf_output_begin() returns 0 or error code, IMHO
converting it to boolean will not be a problem since there is no
user takes account of the actual code. Annotate perf_output_end()
also.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 include/linux/perf_event.h |    6 +++++-
 kernel/perf_event.c        |    3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 716f99b..167dd3d 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1009,9 +1009,13 @@ extern void perf_bp_event(struct perf_event *event, void *data);
 #define perf_instruction_pointer(regs)	instruction_pointer(regs)
 #endif
 
-extern int perf_output_begin(struct perf_output_handle *handle,
+extern int __perf_output_begin(struct perf_output_handle *handle,
 			     struct perf_event *event, unsigned int size,
 			     int nmi, int sample);
+
+#define perf_output_begin(h, e, s, n, sp)				\
+	(!__cond_lock(RCU, !__perf_output_begin(h, e, s, n, sp)))
+
 extern void perf_output_end(struct perf_output_handle *handle);
 extern void perf_output_copy(struct perf_output_handle *handle,
 			     const void *buf, unsigned int len);
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index c058a42..bd5d750 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -3106,7 +3106,7 @@ __always_inline void perf_output_copy(struct perf_output_handle *handle,
 	} while (len);
 }
 
-int perf_output_begin(struct perf_output_handle *handle,
+int __perf_output_begin(struct perf_output_handle *handle,
 		      struct perf_event *event, unsigned int size,
 		      int nmi, int sample)
 {
@@ -3190,6 +3190,7 @@ out:
 }
 
 void perf_output_end(struct perf_output_handle *handle)
+	__releases(RCU)
 {
 	struct perf_event *event = handle->event;
 	struct perf_buffer *buffer = handle->buffer;
-- 
1.7.2.2

[PATCH 3/5] perf: Remove 'extern' on hw_perf_event_init() definition

[Namhyung Kim]

Using 'extern' keyword on function definition has no meaning and
sparse complains about it:

 kernel/perf_event.c:78:32: warning: function 'hw_perf_event_init' with external linkage has definition

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index bd5d750..fd26451 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -75,7 +75,7 @@ static DEFINE_SPINLOCK(perf_resource_lock);
 /*
  * Architecture provided APIs - weak aliases:
  */
-extern __weak const struct pmu *hw_perf_event_init(struct perf_event *event)
+__weak const struct pmu *hw_perf_event_init(struct perf_event *event)
 {
 	return NULL;
 }
-- 
1.7.2.2

[PATCH 4/5] perf: Make perf_event_wakeup() static

[Namhyung Kim]

Make perf_event_wakeup() static since it is called only in
static functions.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index fd26451..6611349 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -2845,7 +2845,7 @@ static const struct file_operations perf_fops = {
  * to user-space before waking everybody up.
  */
 
-void perf_event_wakeup(struct perf_event *event)
+static void perf_event_wakeup(struct perf_event *event)
 {
 	wake_up_all(&event->waitq);
 
-- 
1.7.2.2

[PATCH 5/5] perf: Declare hw_perf_{dis,en}able() in perf_event.h

[Namhyung Kim]

Since they can be defined in arch-specific code declare them in
perf_event.h and remove following warnings from sparse:

 kernel/perf_event.c:83:13: warning: symbol 'hw_perf_disable' was not declared. Should it be static?
 kernel/perf_event.c:84:13: warning: symbol 'hw_perf_enable' was not declared. Should it be static?

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 include/linux/perf_event.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 167dd3d..26859c7 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -848,6 +848,8 @@ struct perf_output_handle {
 extern int perf_max_events;
 
 extern const struct pmu *hw_perf_event_init(struct perf_event *event);
+extern void hw_perf_disable(void);
+extern void hw_perf_enable(void);
 
 extern void perf_event_task_sched_in(struct task_struct *task);
 extern void perf_event_task_sched_out(struct task_struct *task, struct task_struct *next);
-- 
1.7.2.2

Re: [PATCH 1/5] perf: Wrap perf_lock_task_context using __cond_lock()

[Frederic Weisbecker]

On Mon, Sep 27, 2010 at 07:39:53PM +0900, Namhyung Kim wrote:
> perf_lock_task_context() conditionally grabs ctx->lock in case of
> returning non-NULL. Rename and wrap it using __cond_lock to make
> sparse happy.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  kernel/perf_event.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index db5b560..c058a42 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -151,7 +151,7 @@ static u64 primary_event_id(struct perf_event *event)
>   * the context could get moved to another task.
>   */
>  static struct perf_event_context *
> -perf_lock_task_context(struct task_struct *task, unsigned long *flags)
> +__perf_lock_task_context(struct task_struct *task, unsigned long *flags)
>  {
>  	struct perf_event_context *ctx;
>  
> @@ -184,6 +184,14 @@ perf_lock_task_context(struct task_struct *task, unsigned long *flags)
>  	return ctx;
>  }
>  
> +static inline struct perf_event_context *
> +perf_lock_task_context(struct task_struct *task, unsigned long *flags)
> +{
> +	struct perf_event_context *ctx;
> +	__cond_lock(&ctx->lock, ctx = __perf_lock_task_context(task, flags));
> +	return ctx;
> +}



That's a pity we need to make the code less readable just to make a tool
happy.

How do functions like mutex_lock_interruptible() that may
or may not lock? I don't see __cond_lock adds there.

Thanks.

Re: [PATCH 1/5] perf: Wrap perf_lock_task_context using __cond_lock()

[Namhyung Kim]

On Mon, Sep 27, 2010 at 22:46, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> How do functions like mutex_lock_interruptible() that may
> or may not lock? I don't see __cond_lock adds there.
>

Hi,
AFAIK mutex code does not use such notations at all.


-- 
Regards,
Namhyung Kim

Re: [PATCH 5/5] perf: Declare hw_perf_{dis,en}able() in perf_event.h

[Peter Zijlstra]

On Mon, 2010-09-27 at 19:39 +0900, Namhyung Kim wrote:
> Since they can be defined in arch-specific code declare them in
> perf_event.h and remove following warnings from sparse:
> 
>  kernel/perf_event.c:83:13: warning: symbol 'hw_perf_disable' was not declared. Should it be static?
>  kernel/perf_event.c:84:13: warning: symbol 'hw_perf_enable' was not declared. Should it be static?
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>

What tree is this against, those function no longer exist!?

Re: [PATCH 1/5] perf: Wrap perf_lock_task_context using __cond_lock()

[Peter Zijlstra]

On Mon, 2010-09-27 at 19:39 +0900, Namhyung Kim wrote:
> perf_lock_task_context() conditionally grabs ctx->lock in case of
> returning non-NULL. Rename and wrap it using __cond_lock to make
> sparse happy.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  kernel/perf_event.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index db5b560..c058a42 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -151,7 +151,7 @@ static u64 primary_event_id(struct perf_event *event)
>   * the context could get moved to another task.
>   */
>  static struct perf_event_context *
> -perf_lock_task_context(struct task_struct *task, unsigned long *flags)
> +__perf_lock_task_context(struct task_struct *task, unsigned long *flags)
>  {
>  	struct perf_event_context *ctx;
>  
> @@ -184,6 +184,14 @@ perf_lock_task_context(struct task_struct *task, unsigned long *flags)
>  	return ctx;
>  }
>  
> +static inline struct perf_event_context *
> +perf_lock_task_context(struct task_struct *task, unsigned long *flags)
> +{
> +	struct perf_event_context *ctx;
> +	__cond_lock(&ctx->lock, ctx = __perf_lock_task_context(task, flags));
> +	return ctx;
> +}
> +
>  /*
>   * Get the context for a task and increment its pin_count so it
>   * can't get swapped to another task.  This also increments its

This is a rather ugly annotation... why can't sparse use something like
the regular __acquire() function annotation but instead all it
__cond_acquire() ?

Re: [PATCH 5/5] perf: Declare hw_perf_{dis,en}able() in perf_event.h

[Namhyung Kim]

2010-09-30 (목), 13:22 +0200, Peter Zijlstra:
> On Mon, 2010-09-27 at 19:39 +0900, Namhyung Kim wrote:
> > Since they can be defined in arch-specific code declare them in
> > perf_event.h and remove following warnings from sparse:
> > 
> >  kernel/perf_event.c:83:13: warning: symbol 'hw_perf_disable' was not declared. Should it be static?
> >  kernel/perf_event.c:84:13: warning: symbol 'hw_perf_enable' was not declared. Should it be static?
> > 
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> 
> What tree is this against, those function no longer exist!?

That was Linus' tree. Sorry, I didn't check -tip tree.
Please ignore this :-(


-- 
Regards,
Namhyung Kim