[PATCH 0/4] perf tools: Fix kmap handling

[PATCH 0/4] perf tools: Fix kmap handling

[Jiri Olsa]

hi,
Ravi Bangoria reported crash in perf top due to wrong kmap
objects management, this patchset should fix that.

thanks,
jirka


---
Jiri Olsa (4):
      perf tools: Mark modules dsos with kernel type
      perf tools: Mark ksymbol dsos with kernel type
      perf tools: Fix map__clone for struct kmap
      perf tools: Move kmap::kmaps setup to maps__insert

 tools/perf/util/machine.c | 24 ++++++++++--------------
 tools/perf/util/map.c     | 17 ++++++++++++++++-
 2 files changed, 26 insertions(+), 15 deletions(-)

[PATCH 1/4] perf tools: Mark modules dsos with kernel type

[Jiri Olsa]

We add kernel module map into machine->kmaps, so it needs
to be created as 'struct kmap', which is dependent on its
dso having kernel type.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index c8c5410315e8..e3e5490f6de5 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -686,6 +686,7 @@ static struct dso *machine__findnew_module_dso(struct machine *machine,
 
 		dso__set_module_info(dso, m, machine);
 		dso__set_long_name(dso, strdup(filename), true);
+		dso->kernel = DSO_TYPE_KERNEL;
 	}
 
 	dso__get(dso);
-- 
2.24.1

[PATCH 2/4] perf tools: Mark ksymbol dsos with kernel type

[Jiri Olsa]

We add ksymbol map into machine->kmaps, so it needs to be
created as 'struct kmap', which is dependent on its dso
having kernel type.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e3e5490f6de5..0a43dc83d7b2 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -727,8 +727,14 @@ static int machine__process_ksymbol_register(struct machine *machine,
 	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
 
 	if (!map) {
-		map = dso__new_map(event->ksymbol.name);
-		if (!map)
+		struct dso *dso = dso__new(event->ksymbol.name);
+
+		if (dso) {
+			dso->kernel = DSO_TYPE_KERNEL;
+			map = map__new2(0, dso);
+		}
+
+		if (!dso || !map)
 			return -ENOMEM;
 
 		map->start = event->ksymbol.addr;
-- 
2.24.1

[PATCH 3/4] perf tools: Fix map__clone for struct kmap

[Jiri Olsa]

The map__clone function can be called on kernel maps
as well, so it needs to duplicate the whole kmap data.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/map.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index f67960bedebb..cea05fc9595c 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -375,8 +375,13 @@ struct symbol *map__find_symbol_by_name(struct map *map, const char *name)
 
 struct map *map__clone(struct map *from)
 {
-	struct map *map = memdup(from, sizeof(*map));
+	size_t size = sizeof(struct map);
+	struct map *map;
+
+	if (from->dso && from->dso->kernel)
+		size += sizeof(struct kmap);
 
+	map = memdup(from, size);
 	if (map != NULL) {
 		refcount_set(&map->refcnt, 1);
 		RB_CLEAR_NODE(&map->rb_node);
-- 
2.24.1

[PATCH 4/4] perf tools: Move kmap::kmaps setup to maps__insert

[Jiri Olsa]

So the kmaps pointer setup is centralized and we do not need
to update it in all those places (2 current places and few
more missing) after calling maps__insert.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 13 +------------
 tools/perf/util/map.c     | 10 ++++++++++
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 0a43dc83d7b2..460315476314 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -979,7 +979,6 @@ int machine__create_extra_kernel_map(struct machine *machine,
 
 	kmap = map__kmap(map);
 
-	kmap->kmaps = &machine->kmaps;
 	strlcpy(kmap->name, xm->name, KMAP_NAME_LEN);
 
 	maps__insert(&machine->kmaps, map);
@@ -1089,9 +1088,6 @@ int __weak machine__create_extra_kernel_maps(struct machine *machine __maybe_unu
 static int
 __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 {
-	struct kmap *kmap;
-	struct map *map;
-
 	/* In case of renewal the kernel map, destroy previous one */
 	machine__destroy_kernel_maps(machine);
 
@@ -1100,14 +1096,7 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 		return -1;
 
 	machine->vmlinux_map->map_ip = machine->vmlinux_map->unmap_ip = identity__map_ip;
-	map = machine__kernel_map(machine);
-	kmap = map__kmap(map);
-	if (!kmap)
-		return -1;
-
-	kmap->kmaps = &machine->kmaps;
-	maps__insert(&machine->kmaps, map);
-
+	maps__insert(&machine->kmaps, machine->vmlinux_map);
 	return 0;
 }
 
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index cea05fc9595c..a08ca276098e 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -543,6 +543,16 @@ void maps__insert(struct maps *maps, struct map *map)
 	__maps__insert(maps, map);
 	++maps->nr_maps;
 
+	if (map->dso && map->dso->kernel) {
+		struct kmap *kmap = map__kmap(map);
+
+		if (kmap)
+			kmap->kmaps = maps;
+		else
+			pr_err("Internal error: kernel dso with non kernel map\n");
+	}
+
+
 	/*
 	 * If we already performed some search by name, then we need to add the just
 	 * inserted map and resort.
-- 
2.24.1

Re: [PATCH 2/4] perf tools: Mark ksymbol dsos with kernel type

[Arnaldo Carvalho de Melo]

Em Mon, Feb 10, 2020 at 03:32:16PM +0100, Jiri Olsa escreveu:
> We add ksymbol map into machine->kmaps, so it needs to be
> created as 'struct kmap', which is dependent on its dso
> having kernel type.
> 
> Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/machine.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index e3e5490f6de5..0a43dc83d7b2 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -727,8 +727,14 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
>  
>  	if (!map) {
> -		map = dso__new_map(event->ksymbol.name);
> -		if (!map)
> +		struct dso *dso = dso__new(event->ksymbol.name);
> +
> +		if (dso) {
> +			dso->kernel = DSO_TYPE_KERNEL;
> +			map = map__new2(0, dso);
> +		}
> +
> +		if (!dso || !map)

We leak dso if map creation fails?


- Arnaldo

>  			return -ENOMEM;
>  
>  		map->start = event->ksymbol.addr;
> -- 
> 2.24.1
> 

-- 

- Arnaldo

Re: [PATCH 2/4] perf tools: Mark ksymbol dsos with kernel type

[Jiri Olsa]

On Mon, Feb 10, 2020 at 12:17:59PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 10, 2020 at 03:32:16PM +0100, Jiri Olsa escreveu:
> > We add ksymbol map into machine->kmaps, so it needs to be
> > created as 'struct kmap', which is dependent on its dso
> > having kernel type.
> > 
> > Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/machine.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index e3e5490f6de5..0a43dc83d7b2 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -727,8 +727,14 @@ static int machine__process_ksymbol_register(struct machine *machine,
> >  	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
> >  
> >  	if (!map) {
> > -		map = dso__new_map(event->ksymbol.name);
> > -		if (!map)
> > +		struct dso *dso = dso__new(event->ksymbol.name);
> > +
> > +		if (dso) {
> > +			dso->kernel = DSO_TYPE_KERNEL;
> > +			map = map__new2(0, dso);
> > +		}
> > +
> > +		if (!dso || !map)
> 
> We leak dso if map creation fails?

yep :-\ will post v2

thanks,
jirka

> 
> 
> - Arnaldo
> 
> >  			return -ENOMEM;
> >  
> >  		map->start = event->ksymbol.addr;
> > -- 
> > 2.24.1
> > 
> 
> -- 
> 
> - Arnaldo
> 

Re: [PATCH 0/4] perf tools: Fix kmap handling

[Kim Phillips]

On 2/10/20 8:32 AM, Jiri Olsa wrote:
> hi,
> Ravi Bangoria reported crash in perf top due to wrong kmap
> objects management, this patchset should fix that.
> 
> thanks,
> jirka
> 
> 
> ---
> Jiri Olsa (4):
>       perf tools: Mark modules dsos with kernel type
>       perf tools: Mark ksymbol dsos with kernel type
>       perf tools: Fix map__clone for struct kmap
>       perf tools: Move kmap::kmaps setup to maps__insert
> 
>  tools/perf/util/machine.c | 24 ++++++++++--------------
>  tools/perf/util/map.c     | 17 ++++++++++++++++-
>  2 files changed, 26 insertions(+), 15 deletions(-)
> 

This series fixes a segmentation fault I was seeing on a
couple of AMD systems, so:

Tested-by: Kim Phillips <kim.phillips@amd.com>

Thanks,

Kim

Thread 259 "perf" received signal SIGSEGV, Segmentation fault.
                                                              [Switching to Thread 0x7fff3b7b6700 (LWP 13241)]
__map__is_kernel (map=0x7fffd80098d0) at util/map.c:244
244		return machine__kernel_map(map__kmaps((struct map *)map)->machine) == map;
(gdb) bt
#0  __map__is_kernel (map=0x7fffd80098d0) at util/map.c:244
#1  0x000055555575d756 in perf_event__process_sample (machine=<optimized out>, sample=0x7fff3b7b5710, 
    evsel=0x555555ef9dd0, event=<optimized out>, tool=0x7fffffff8660) at builtin-top.c:799
#2  deliver_event (qe=<optimized out>, qevent=<optimized out>) at builtin-top.c:1192
#3  0x0000555555831f81 in do_flush (show_progress=false, oe=0x7fffffff8960) at util/ordered-events.c:244
#4  __ordered_events__flush (oe=oe@entry=0x7fffffff8960, how=how@entry=OE_FLUSH__TOP, timestamp=timestamp@entry=0)
    at util/ordered-events.c:323
#5  0x00005555558328df in __ordered_events__flush (timestamp=<optimized out>, how=<optimized out>, oe=<optimized out>)
    at util/ordered-events.c:342
#6  ordered_events__flush (oe=oe@entry=0x7fffffff8960, how=how@entry=OE_FLUSH__TOP) at util/ordered-events.c:341
#7  0x000055555575d0f1 in process_thread (arg=0x7fffffff8660) at builtin-top.c:1104
#8  0x00007ffff7bbd6db in start_thread (arg=0x7fff3b7b6700) at pthread_create.c:463
#9  0x00007ffff5d9788f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb) p map
$1 = (const struct map *) 0x7fffd80098d0

Re: [PATCH 2/4] perf tools: Mark ksymbol dsos with kernel type

[Arnaldo Carvalho de Melo]

Em Mon, Feb 10, 2020 at 04:25:37PM +0100, Jiri Olsa escreveu:
> On Mon, Feb 10, 2020 at 12:17:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Feb 10, 2020 at 03:32:16PM +0100, Jiri Olsa escreveu:
> > > We add ksymbol map into machine->kmaps, so it needs to be
> > > created as 'struct kmap', which is dependent on its dso
> > > having kernel type.
> > > 
> > > Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/util/machine.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > index e3e5490f6de5..0a43dc83d7b2 100644
> > > --- a/tools/perf/util/machine.c
> > > +++ b/tools/perf/util/machine.c
> > > @@ -727,8 +727,14 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > >  	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
> > >  
> > >  	if (!map) {
> > > -		map = dso__new_map(event->ksymbol.name);
> > > -		if (!map)
> > > +		struct dso *dso = dso__new(event->ksymbol.name);
> > > +
> > > +		if (dso) {
> > > +			dso->kernel = DSO_TYPE_KERNEL;
> > > +			map = map__new2(0, dso);
> > > +		}
> > > +
> > > +		if (!dso || !map)
> > 
> > We leak dso if map creation fails?
> 
> yep :-\ will post v2

Please collect Kim's Tested-by then,

- Arnaldo
 
> thanks,
> jirka
> 
> > 
> > 
> > - Arnaldo
> > 
> > >  			return -ENOMEM;
> > >  
> > >  		map->start = event->ksymbol.addr;
> > > -- 
> > > 2.24.1
> > > 
> > 
> > -- 
> > 
> > - Arnaldo
> > 
> 

-- 

- Arnaldo

[PATCHv2 2/4] perf tools: Mark ksymbol dsos with kernel type

[Jiri Olsa]

On Mon, Feb 10, 2020 at 04:25:37PM +0100, Jiri Olsa wrote:
> On Mon, Feb 10, 2020 at 12:17:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Feb 10, 2020 at 03:32:16PM +0100, Jiri Olsa escreveu:
> > > We add ksymbol map into machine->kmaps, so it needs to be
> > > created as 'struct kmap', which is dependent on its dso
> > > having kernel type.
> > > 
> > > Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/perf/util/machine.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > index e3e5490f6de5..0a43dc83d7b2 100644
> > > --- a/tools/perf/util/machine.c
> > > +++ b/tools/perf/util/machine.c
> > > @@ -727,8 +727,14 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > >  	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
> > >  
> > >  	if (!map) {
> > > -		map = dso__new_map(event->ksymbol.name);
> > > -		if (!map)
> > > +		struct dso *dso = dso__new(event->ksymbol.name);
> > > +
> > > +		if (dso) {
> > > +			dso->kernel = DSO_TYPE_KERNEL;
> > > +			map = map__new2(0, dso);
> > > +		}
> > > +
> > > +		if (!dso || !map)
> > 
> > We leak dso if map creation fails?
> 
> yep :-\ will post v2

v2 attached, it's also all in my perf/top branch

thanks,
jirka


---
We add ksymbol map into machine->kmaps, so it needs to be
created as 'struct kmap', which is dependent on its dso
having kernel type.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/machine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e3e5490f6de5..0ad026561c7f 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -727,9 +727,17 @@ static int machine__process_ksymbol_register(struct machine *machine,
 	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
 
 	if (!map) {
-		map = dso__new_map(event->ksymbol.name);
-		if (!map)
+		struct dso *dso = dso__new(event->ksymbol.name);
+
+		if (dso) {
+			dso->kernel = DSO_TYPE_KERNEL;
+			map = map__new2(0, dso);
+		}
+
+		if (!dso || !map) {
+			dso__put(dso);
 			return -ENOMEM;
+		}
 
 		map->start = event->ksymbol.addr;
 		map->end = map->start + event->ksymbol.len;
-- 
2.24.1

Re: [PATCH 2/4] perf tools: Mark ksymbol dsos with kernel type

[Jiri Olsa]

On Mon, Feb 10, 2020 at 04:58:01PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 10, 2020 at 04:25:37PM +0100, Jiri Olsa escreveu:
> > On Mon, Feb 10, 2020 at 12:17:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Feb 10, 2020 at 03:32:16PM +0100, Jiri Olsa escreveu:
> > > > We add ksymbol map into machine->kmaps, so it needs to be
> > > > created as 'struct kmap', which is dependent on its dso
> > > > having kernel type.
> > > > 
> > > > Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > > Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  tools/perf/util/machine.c | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > > index e3e5490f6de5..0a43dc83d7b2 100644
> > > > --- a/tools/perf/util/machine.c
> > > > +++ b/tools/perf/util/machine.c
> > > > @@ -727,8 +727,14 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > > >  	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
> > > >  
> > > >  	if (!map) {
> > > > -		map = dso__new_map(event->ksymbol.name);
> > > > -		if (!map)
> > > > +		struct dso *dso = dso__new(event->ksymbol.name);
> > > > +
> > > > +		if (dso) {
> > > > +			dso->kernel = DSO_TYPE_KERNEL;
> > > > +			map = map__new2(0, dso);
> > > > +		}
> > > > +
> > > > +		if (!dso || !map)
> > > 
> > > We leak dso if map creation fails?
> > 
> > yep :-\ will post v2
> 
> Please collect Kim's Tested-by then,

oops, did not notice, I updated commits with them in the perf/top branch

jirka

> 
> - Arnaldo
>  
> > thanks,
> > jirka
> > 
> > > 
> > > 
> > > - Arnaldo
> > > 
> > > >  			return -ENOMEM;
> > > >  
> > > >  		map->start = event->ksymbol.addr;
> > > > -- 
> > > > 2.24.1
> > > > 
> > > 
> > > -- 
> > > 
> > > - Arnaldo
> > > 
> > 
> 
> -- 
> 
> - Arnaldo
> 

Re: [PATCH 0/4] perf tools: Fix kmap handling

[Jiri Olsa]

On Mon, Feb 10, 2020 at 10:47:11AM -0600, Kim Phillips wrote:
> On 2/10/20 8:32 AM, Jiri Olsa wrote:
> > hi,
> > Ravi Bangoria reported crash in perf top due to wrong kmap
> > objects management, this patchset should fix that.
> > 
> > thanks,
> > jirka
> > 
> > 
> > ---
> > Jiri Olsa (4):
> >       perf tools: Mark modules dsos with kernel type
> >       perf tools: Mark ksymbol dsos with kernel type
> >       perf tools: Fix map__clone for struct kmap
> >       perf tools: Move kmap::kmaps setup to maps__insert
> > 
> >  tools/perf/util/machine.c | 24 ++++++++++--------------
> >  tools/perf/util/map.c     | 17 ++++++++++++++++-
> >  2 files changed, 26 insertions(+), 15 deletions(-)
> > 
> 
> This series fixes a segmentation fault I was seeing on a
> couple of AMD systems, so:
> 
> Tested-by: Kim Phillips <kim.phillips@amd.com>

great, thanks a lot for testing

jirka

Re: [PATCHv2 2/4] perf tools: Mark ksymbol dsos with kernel type

[Arnaldo Carvalho de Melo]

Em Mon, Feb 10, 2020 at 09:08:47PM +0100, Jiri Olsa escreveu:
> On Mon, Feb 10, 2020 at 04:25:37PM +0100, Jiri Olsa wrote:
> > On Mon, Feb 10, 2020 at 12:17:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Feb 10, 2020 at 03:32:16PM +0100, Jiri Olsa escreveu:
> > > > We add ksymbol map into machine->kmaps, so it needs to be
> > > > created as 'struct kmap', which is dependent on its dso
> > > > having kernel type.
> > > > 
> > > > Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > > Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  tools/perf/util/machine.c | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > > index e3e5490f6de5..0a43dc83d7b2 100644
> > > > --- a/tools/perf/util/machine.c
> > > > +++ b/tools/perf/util/machine.c
> > > > @@ -727,8 +727,14 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > > >  	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
> > > >  
> > > >  	if (!map) {
> > > > -		map = dso__new_map(event->ksymbol.name);
> > > > -		if (!map)
> > > > +		struct dso *dso = dso__new(event->ksymbol.name);
> > > > +
> > > > +		if (dso) {
> > > > +			dso->kernel = DSO_TYPE_KERNEL;
> > > > +			map = map__new2(0, dso);
> > > > +		}
> > > > +
> > > > +		if (!dso || !map)
> > > 
> > > We leak dso if map creation fails?
> > 
> > yep :-\ will post v2
> 
> v2 attached, it's also all in my perf/top branch

Thanks for fixing it, will process it into my perf/urgent branch.

- Arnaldo
 
> thanks,
> jirka
> 
> 
> ---
> We add ksymbol map into machine->kmaps, so it needs to be
> created as 'struct kmap', which is dependent on its dso
> having kernel type.
> 
> Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/machine.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index e3e5490f6de5..0ad026561c7f 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -727,9 +727,17 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
>  
>  	if (!map) {
> -		map = dso__new_map(event->ksymbol.name);
> -		if (!map)
> +		struct dso *dso = dso__new(event->ksymbol.name);
> +
> +		if (dso) {
> +			dso->kernel = DSO_TYPE_KERNEL;
> +			map = map__new2(0, dso);
> +		}
> +
> +		if (!dso || !map) {
> +			dso__put(dso);
>  			return -ENOMEM;
> +		}
>  
>  		map->start = event->ksymbol.addr;
>  		map->end = map->start + event->ksymbol.len;
> -- 
> 2.24.1
> 

-- 

- Arnaldo

[tip: perf/urgent] perf maps: Move kmap::kmaps setup to maps__insert()

[tip-bot2 for Jiri Olsa]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     484214f49bd0948d716832a94e4737ca4dd02c16
Gitweb:        https://git.kernel.org/tip/484214f49bd0948d716832a94e4737ca4dd02c16
Author:        Jiri Olsa <jolsa@kernel.org>
AuthorDate:    Mon, 10 Feb 2020 15:32:18 +01:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 11 Feb 2020 16:41:49 -03:00

perf maps: Move kmap::kmaps setup to maps__insert()

So the kmaps pointer setup is centralized and we do not need to update
it in all those places (2 current places and few more missing) after
calling maps__insert().

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200210143218.24948-5-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 13 +------------
 tools/perf/util/map.c     | 10 ++++++++++
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 0ad0265..fb5c2cd 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -981,7 +981,6 @@ int machine__create_extra_kernel_map(struct machine *machine,
 
 	kmap = map__kmap(map);
 
-	kmap->kmaps = &machine->kmaps;
 	strlcpy(kmap->name, xm->name, KMAP_NAME_LEN);
 
 	maps__insert(&machine->kmaps, map);
@@ -1091,9 +1090,6 @@ int __weak machine__create_extra_kernel_maps(struct machine *machine __maybe_unu
 static int
 __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 {
-	struct kmap *kmap;
-	struct map *map;
-
 	/* In case of renewal the kernel map, destroy previous one */
 	machine__destroy_kernel_maps(machine);
 
@@ -1102,14 +1098,7 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 		return -1;
 
 	machine->vmlinux_map->map_ip = machine->vmlinux_map->unmap_ip = identity__map_ip;
-	map = machine__kernel_map(machine);
-	kmap = map__kmap(map);
-	if (!kmap)
-		return -1;
-
-	kmap->kmaps = &machine->kmaps;
-	maps__insert(&machine->kmaps, map);
-
+	maps__insert(&machine->kmaps, machine->vmlinux_map);
 	return 0;
 }
 
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index cea05fc..a08ca27 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -543,6 +543,16 @@ void maps__insert(struct maps *maps, struct map *map)
 	__maps__insert(maps, map);
 	++maps->nr_maps;
 
+	if (map->dso && map->dso->kernel) {
+		struct kmap *kmap = map__kmap(map);
+
+		if (kmap)
+			kmap->kmaps = maps;
+		else
+			pr_err("Internal error: kernel dso with non kernel map\n");
+	}
+
+
 	/*
 	 * If we already performed some search by name, then we need to add the just
 	 * inserted map and resort.

[tip: perf/urgent] perf maps: Fix map__clone() for struct kmap

[tip-bot2 for Jiri Olsa]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     7ce66139a99ce57caaf47b64afed5cb6ed02c5ed
Gitweb:        https://git.kernel.org/tip/7ce66139a99ce57caaf47b64afed5cb6ed02c5ed
Author:        Jiri Olsa <jolsa@kernel.org>
AuthorDate:    Mon, 10 Feb 2020 15:32:17 +01:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 11 Feb 2020 16:41:49 -03:00

perf maps: Fix map__clone() for struct kmap

The map__clone() function can be called on kernel maps as well, so it
needs to duplicate the whole kmap data.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200210143218.24948-4-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index f67960b..cea05fc 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -375,8 +375,13 @@ struct symbol *map__find_symbol_by_name(struct map *map, const char *name)
 
 struct map *map__clone(struct map *from)
 {
-	struct map *map = memdup(from, sizeof(*map));
+	size_t size = sizeof(struct map);
+	struct map *map;
+
+	if (from->dso && from->dso->kernel)
+		size += sizeof(struct kmap);
 
+	map = memdup(from, size);
 	if (map != NULL) {
 		refcount_set(&map->refcnt, 1);
 		RB_CLEAR_NODE(&map->rb_node);

[tip: perf/urgent] perf maps: Mark ksymbol DSOs with kernel type

[tip-bot2 for Jiri Olsa]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     4a4eb6154d67f7766cc7eb74e9f1db424073e832
Gitweb:        https://git.kernel.org/tip/4a4eb6154d67f7766cc7eb74e9f1db424073e832
Author:        Jiri Olsa <jolsa@redhat.com>
AuthorDate:    Mon, 10 Feb 2020 21:08:47 +01:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 11 Feb 2020 16:41:49 -03:00

perf maps: Mark ksymbol DSOs with kernel type

We add ksymbol map into machine->kmaps, so it needs to be created as
'struct kmap', which is dependent on its dso having kernel type.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200210200847.GA36715@krava
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e3e5490..0ad0265 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -727,9 +727,17 @@ static int machine__process_ksymbol_register(struct machine *machine,
 	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
 
 	if (!map) {
-		map = dso__new_map(event->ksymbol.name);
-		if (!map)
+		struct dso *dso = dso__new(event->ksymbol.name);
+
+		if (dso) {
+			dso->kernel = DSO_TYPE_KERNEL;
+			map = map__new2(0, dso);
+		}
+
+		if (!dso || !map) {
+			dso__put(dso);
 			return -ENOMEM;
+		}
 
 		map->start = event->ksymbol.addr;
 		map->end = map->start + event->ksymbol.len;

[tip: perf/urgent] perf maps: Mark module DSOs with kernel type

[tip-bot2 for Jiri Olsa]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     02213cec64bbef66d7ad9ddc3b7c47236da64343
Gitweb:        https://git.kernel.org/tip/02213cec64bbef66d7ad9ddc3b7c47236da64343
Author:        Jiri Olsa <jolsa@kernel.org>
AuthorDate:    Mon, 10 Feb 2020 15:32:15 +01:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 11 Feb 2020 16:41:49 -03:00

perf maps: Mark module DSOs with kernel type

We add kernel module map into machine->kmaps, so it needs to be created
as 'struct kmap', which is dependent on its dso having kernel type.

Reported-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Kim Phillips <kim.phillips@amd.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200210143218.24948-2-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index c8c5410..e3e5490 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -686,6 +686,7 @@ static struct dso *machine__findnew_module_dso(struct machine *machine,
 
 		dso__set_module_info(dso, m, machine);
 		dso__set_long_name(dso, strdup(filename), true);
+		dso->kernel = DSO_TYPE_KERNEL;
 	}
 
 	dso__get(dso);