[PATCH 1/3] uprobes/core: Remove redundant lock_page/unlock_page

[PATCH 1/3] uprobes/core: Remove redundant lock_page/unlock_page

[Srikar Dronamraju]

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

No need to lock the page when copying the opcode in read_opcode().

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 kernel/events/uprobes.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 42b21eb..b3f3095 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -315,12 +315,10 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_
 	if (ret <= 0)
 		return ret;
 
-	lock_page(page);
 	vaddr_new = kmap_atomic(page);
 	vaddr &= ~PAGE_MASK;
 	memcpy(opcode, vaddr_new + vaddr, UPROBE_SWBP_INSN_SIZE);
 	kunmap_atomic(vaddr_new);
-	unlock_page(page);
 
 	put_page(page);
 

[PATCH 2/3] perf/uprobes: Remove unnecessary check before strlist__delete

[Srikar Dronamraju]

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Since strlist__delete() itself checks, the additional check
before calling strlist__delete() is redundant.

No Functional change.

Suggested-by: Arnaldo Carvalho de Melo <acme@infradead.org>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 59dccc9..0dda25d 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2164,16 +2164,12 @@ int del_perf_probe_events(struct strlist *dellist)
 
 error:
 	if (kfd >= 0) {
-		if (namelist)
-			strlist__delete(namelist);
-
+		strlist__delete(namelist);
 		close(kfd);
 	}
 
 	if (ufd >= 0) {
-		if (unamelist)
-			strlist__delete(unamelist);
-
+		strlist__delete(unamelist);
 		close(ufd);
 	}
 

[tip:perf/urgent] perf uprobes: Remove unnecessary check before strlist__delete

[tip-bot for Srikar Dronamraju]

Commit-ID:  a23c4dc422cff7bd30f40f5dc7c9ac4a6339005a
Gitweb:     http://git.kernel.org/tip/a23c4dc422cff7bd30f40f5dc7c9ac4a6339005a
Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
AuthorDate: Thu, 31 May 2012 17:16:43 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 31 May 2012 12:08:49 -0300

perf uprobes: Remove unnecessary check before strlist__delete

Since strlist__delete() itself checks, the additional check before
calling strlist__delete() is redundant.

No Functional change.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Suggested-by: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Anton Arapov <anton@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/20120531114643.23691.38666.sendpatchset@srdronam.in.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-event.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 59dccc9..0dda25d 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2164,16 +2164,12 @@ int del_perf_probe_events(struct strlist *dellist)
 
 error:
 	if (kfd >= 0) {
-		if (namelist)
-			strlist__delete(namelist);
-
+		strlist__delete(namelist);
 		close(kfd);
 	}
 
 	if (ufd >= 0) {
-		if (unamelist)
-			strlist__delete(unamelist);
-
+		strlist__delete(unamelist);
 		close(ufd);
 	}
 

[PATCH 3/3] perf: Check for valid dso before creating map

[Srikar Dronamraju]

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

dso__new() can return NULL. Hence verify dso before creating
a new map.

Suggested-by: Arnaldo Carvalho de Melo <acme@infradead.org>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 tools/perf/util/symbol.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index e2ba885..0ef529e 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2786,8 +2786,11 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type,
 
 struct map *dso__new_map(const char *name)
 {
+	struct map *map = NULL;
 	struct dso *dso = dso__new(name);
-	struct map *map = map__new2(0, dso, MAP__FUNCTION);
+
+	if (dso)
+		map = map__new2(0, dso, MAP__FUNCTION);
 
 	return map;
 }

[tip:perf/urgent] perf symbols: Check for valid dso before creating map

[tip-bot for Srikar Dronamraju]

Commit-ID:  378474e4b23183002f1791b294a4440b6b7df36a
Gitweb:     http://git.kernel.org/tip/378474e4b23183002f1791b294a4440b6b7df36a
Author:     Srikar Dronamraju <srikar@linux.vnet.ibm.com>
AuthorDate: Thu, 31 May 2012 17:16:56 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 31 May 2012 12:08:22 -0300

perf symbols: Check for valid dso before creating map

dso__new() can return NULL. Hence verify dso before creating a new map.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Suggested-by: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Anton Arapov <anton@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/20120531114656.23691.54223.sendpatchset@srdronam.in.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 9d04dcd..3e2e5ea 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2817,8 +2817,11 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type,
 
 struct map *dso__new_map(const char *name)
 {
+	struct map *map = NULL;
 	struct dso *dso = dso__new(name);
-	struct map *map = map__new2(0, dso, MAP__FUNCTION);
+
+	if (dso)
+		map = map__new2(0, dso, MAP__FUNCTION);
 
 	return map;
 }

Re: [PATCH 1/3] uprobes/core: Remove redundant lock_page/unlock_page

[Peter Zijlstra]

On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> No need to lock the page when copying the opcode in read_opcode().

It would be good if the changelog said _why_ this is so :-)

Re: [PATCH 1/3] uprobes/core: Remove redundant lock_page/unlock_page

[Steven Rostedt]

On Thu, 2012-05-31 at 13:58 +0200, Peter Zijlstra wrote:
> On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> > From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > 
> > No need to lock the page when copying the opcode in read_opcode().
> 
> It would be good if the changelog said _why_ this is so :-)

I thought the same thing when I read it.

-- Steve

Re: [PATCH 1/3] uprobes/core: Remove redundant lock_page/unlock_page

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2012-05-31 13:58:38]:

> On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> > From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > 
> > No need to lock the page when copying the opcode in read_opcode().
> 
> It would be good if the changelog said _why_ this is so :-)

In read_opcode(), we have the reference for the page and we only are reading
from the the page. i.e we are neither modifying the page contents, not
the page attributes. 

Existing kernel code has enough examples where we read the contents
of the page without taking the page lock.

Further this was discussed here too https://lkml.org/lkml/2012/4/17/361.

-- 
Thanks and Regards
Srikar

Re: [PATCH 1/3] uprobes/core: Remove redundant lock_page/unlock_page

[Peter Zijlstra]

On Thu, 2012-05-31 at 20:37 +0530, Srikar Dronamraju wrote:
> * Peter Zijlstra <peterz@infradead.org> [2012-05-31 13:58:38]:
> 
> > On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> > > From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > > 
> > > No need to lock the page when copying the opcode in read_opcode().
> > 
> > It would be good if the changelog said _why_ this is so :-)
> 
> In read_opcode(), we have the reference for the page and we only are reading
> from the the page. i.e we are neither modifying the page contents, not
> the page attributes. 

Fair enough, so put that in the changelog. The changelog should explain
things, not raise questions.

> Existing kernel code has enough examples where we read the contents
> of the page without taking the page lock.

Yes, but that doesn't tell us this site is ok, doing it because others
do isn't an argument.

> Further this was discussed here too https://lkml.org/lkml/2012/4/17/361.

That wasn't a discussion, that was two people saying they don't know.

Re: [PATCH 1/3] uprobes/core: Remove redundant lock_page/unlock_page

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2012-05-31 18:56:12]:

> On Thu, 2012-05-31 at 20:37 +0530, Srikar Dronamraju wrote:
> > * Peter Zijlstra <peterz@infradead.org> [2012-05-31 13:58:38]:
> > 
> > > On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> > > > From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > > > 
> > > > No need to lock the page when copying the opcode in read_opcode().
> > > 
> > > It would be good if the changelog said _why_ this is so :-)
> > 
> > In read_opcode(), we have the reference for the page and we only are reading
> > from the the page. i.e we are neither modifying the page contents, not
> > the page attributes. 
> 
> Fair enough, so put that in the changelog. The changelog should explain
> things, not raise questions.
> 

Okay, will add this info to the Changelog and resend the patch.

-- 
Thanks and Regards
Srikar

[RESEND PATCH 1/3] uprobes: Remove redundant lock_page/unlock_page

[Srikar Dronamraju]

From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Since read_opcode() reads from the referenced page and doesnt modify
the page contents nor the page attributes, there is no need to lock
the page.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 Modified changelog based on comments from Peter Zijlstra

 kernel/events/uprobes.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 42b21eb..b3f3095 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -315,12 +315,10 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_
 	if (ret <= 0)
 		return ret;
 
-	lock_page(page);
 	vaddr_new = kmap_atomic(page);
 	vaddr &= ~PAGE_MASK;
 	memcpy(opcode, vaddr_new + vaddr, UPROBE_SWBP_INSN_SIZE);
 	kunmap_atomic(vaddr_new);
-	unlock_page(page);
 
 	put_page(page);
 

Re: [RESEND PATCH 1/3] uprobes: Remove redundant lock_page/unlock_page

[Oleg Nesterov]

On 06/01, Srikar Dronamraju wrote:
>
> @@ -315,12 +315,10 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_
>  	if (ret <= 0)
>  		return ret;
>
> -	lock_page(page);
>  	vaddr_new = kmap_atomic(page);
>  	vaddr &= ~PAGE_MASK;
>  	memcpy(opcode, vaddr_new + vaddr, UPROBE_SWBP_INSN_SIZE);
>  	kunmap_atomic(vaddr_new);
> -	unlock_page(page);

Great, this lock_page() was really confusing.

Oleg.

Re: [RESEND PATCH 1/3] uprobes: Remove redundant lock_page/unlock_page

[Ingo Molnar]

* Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> Since read_opcode() reads from the referenced page and doesnt modify
> the page contents nor the page attributes, there is no need to lock
> the page.
> 
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  Modified changelog based on comments from Peter Zijlstra
> 
>  kernel/events/uprobes.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 42b21eb..b3f3095 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -315,12 +315,10 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_
>  	if (ret <= 0)
>  		return ret;
>  
> -	lock_page(page);
>  	vaddr_new = kmap_atomic(page);
>  	vaddr &= ~PAGE_MASK;
>  	memcpy(opcode, vaddr_new + vaddr, UPROBE_SWBP_INSN_SIZE);
>  	kunmap_atomic(vaddr_new);
> -	unlock_page(page);
>  
>  	put_page(page);

Is this also a bug fix, or can it wait until v3.6?

Thanks,

	Ingo

Re: [RESEND PATCH 1/3] uprobes: Remove redundant lock_page/unlock_page

[Peter Zijlstra]

On Wed, 2012-06-06 at 11:18 +0200, Ingo Molnar wrote:

> Is this also a bug fix, or can it wait until v3.6?

No bug, just superfluous locking.

Re: [RESEND PATCH 1/3] uprobes: Remove redundant lock_page/unlock_page

[Anton Arapov]

Ingo,

  just a "ping" message, so that this change won't be forgotten.
  It was waiting for v3.6: https://lkml.org/lkml/2012/6/6/134

thank you!
Anton.

On Fri, 2012-06-01 at 14:49 +0530, Srikar Dronamraju wrote:
> From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> 
> Since read_opcode() reads from the referenced page and doesnt modify
> the page contents nor the page attributes, there is no need to lock
> the page.
> 
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  Modified changelog based on comments from Peter Zijlstra
> 
>  kernel/events/uprobes.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 42b21eb..b3f3095 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -315,12 +315,10 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_
>  	if (ret <= 0)
>  		return ret;
>  
> -	lock_page(page);
>  	vaddr_new = kmap_atomic(page);
>  	vaddr &= ~PAGE_MASK;
>  	memcpy(opcode, vaddr_new + vaddr, UPROBE_SWBP_INSN_SIZE);
>  	kunmap_atomic(vaddr_new);
> -	unlock_page(page);
>  
>  	put_page(page);
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
>