From: Srikar Dronamraju <[email protected]>
No need to lock the page when copying the opcode in read_opcode().
Signed-off-by: Srikar Dronamraju <[email protected]>
---
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);
From: Srikar Dronamraju <[email protected]>
dso__new() can return NULL. Hence verify dso before creating
a new map.
Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: Srikar Dronamraju <[email protected]>
---
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;
}
From: Srikar Dronamraju <[email protected]>
Since strlist__delete() itself checks, the additional check
before calling strlist__delete() is redundant.
No Functional change.
Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by: Srikar Dronamraju <[email protected]>
---
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);
}
On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> From: Srikar Dronamraju <[email protected]>
>
> 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 :-)
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 <[email protected]>
> >
> > 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
* Peter Zijlstra <[email protected]> [2012-05-31 13:58:38]:
> On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> > From: Srikar Dronamraju <[email protected]>
> >
> > 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
On Thu, 2012-05-31 at 20:37 +0530, Srikar Dronamraju wrote:
> * Peter Zijlstra <[email protected]> [2012-05-31 13:58:38]:
>
> > On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> > > From: Srikar Dronamraju <[email protected]>
> > >
> > > 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.
* Peter Zijlstra <[email protected]> [2012-05-31 18:56:12]:
> On Thu, 2012-05-31 at 20:37 +0530, Srikar Dronamraju wrote:
> > * Peter Zijlstra <[email protected]> [2012-05-31 13:58:38]:
> >
> > > On Thu, 2012-05-31 at 17:16 +0530, Srikar Dronamraju wrote:
> > > > From: Srikar Dronamraju <[email protected]>
> > > >
> > > > 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
From: Srikar Dronamraju <[email protected]>
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 <[email protected]>
---
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);
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.
Commit-ID: 378474e4b23183002f1791b294a4440b6b7df36a
Gitweb: http://git.kernel.org/tip/378474e4b23183002f1791b294a4440b6b7df36a
Author: Srikar Dronamraju <[email protected]>
AuthorDate: Thu, 31 May 2012 17:16:56 +0530
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Anton Arapov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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;
}
Commit-ID: a23c4dc422cff7bd30f40f5dc7c9ac4a6339005a
Gitweb: http://git.kernel.org/tip/a23c4dc422cff7bd30f40f5dc7c9ac4a6339005a
Author: Srikar Dronamraju <[email protected]>
AuthorDate: Thu, 31 May 2012 17:16:43 +0530
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Suggested-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Cc: Anton Arapov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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);
}
* Srikar Dronamraju <[email protected]> wrote:
> From: Srikar Dronamraju <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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.
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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>