Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Namhyung Kim]

Hi Arnaldo,

On Mon, Feb 19, 2018 at 9:21 PM, Arnaldo Carvalho de Melo
 wrote:
> Em Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim escreveu:
>> On Thu, Feb 15, 2018 at 01:26:32PM +0100, Jiri Olsa wrote:
>> > if (!machine__get_running_kernel_start(machine, , )) {
>> > if (name &&
>> > maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, 
>> > name, addr)) {
>> > machine__destroy_kernel_maps(machine);
>> > return -1;
>> > }
>> > +   machine__set_kernel_mmap(machine, addr, 0);
>> > }
>
>> > +   /*
>> > +* Now that we have all the maps created, just set the ->end of them:
>> > +*/
>> > +   map_groups__fixup_end(>kmaps);
>>
>> With above change, I think it work.  But the whole point of the above
>> is to set the end address of vmlinux and modules.  And now we don't
>> need it for modules thanks to modules__parse() for passing the size.
>>
>> So we only needs to update the end address of vmlinux using the start
>> address of the first module.  What about this then?
>
> Looks ok, Jiri? I noticed that Namhyung has the patch below in his git
> tree, but I couldn't find it in the mailing list, Jiri, can I have your
> acked-by?

This version has a bug that misses the call when there's no module.
I'll send a new version if Jiri agrees with the idea.

Thanks,
Namhyung


>>
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index fe27ef55cbb9..022f0539b750 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1021,13 +1021,6 @@ int machine__load_vmlinux_path(struct machine 
>> *machine, enum map_type type)
>> return ret;
>>  }
>>
>> -static void map_groups__fixup_end(struct map_groups *mg)
>> -{
>> -   int i;
>> -   for (i = 0; i < MAP__NR_TYPES; ++i)
>> -   __map_groups__fixup_end(mg, i);
>> -}
>> -
>>  static char *get_kernel_version(const char *root_dir)
>>  {
>> char version[PATH_MAX];
>> @@ -1235,6 +1228,7 @@ int machine__create_kernel_maps(struct machine 
>> *machine)
>>  {
>> struct dso *kernel = machine__get_kernel(machine);
>> const char *name = NULL;
>> +   struct map *map;
>> u64 addr = 0;
>> int ret;
>>
>> @@ -1261,13 +1255,13 @@ int machine__create_kernel_maps(struct machine 
>> *machine)
>> machine__destroy_kernel_maps(machine);
>> return -1;
>> }
>> -   machine__set_kernel_mmap(machine, addr, 0);
>> }
>>
>> -   /*
>> -* Now that we have all the maps created, just set the ->end of them:
>> -*/
>> -   map_groups__fixup_end(>kmaps);
>> +   /* update end address of the kernel map using adjacent module 
>> address */
>> +   map = map__next(machine__kernel_map(machine));
>> +   if (map)
>> +   machine__set_kernel_mmap(machine, addr, map->start);
>> +
>> return 0;
>>  }

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Namhyung Kim]

Hi Arnaldo,

On Mon, Feb 19, 2018 at 9:21 PM, Arnaldo Carvalho de Melo
 wrote:
> Em Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim escreveu:
>> On Thu, Feb 15, 2018 at 01:26:32PM +0100, Jiri Olsa wrote:
>> > if (!machine__get_running_kernel_start(machine, , )) {
>> > if (name &&
>> > maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, 
>> > name, addr)) {
>> > machine__destroy_kernel_maps(machine);
>> > return -1;
>> > }
>> > +   machine__set_kernel_mmap(machine, addr, 0);
>> > }
>
>> > +   /*
>> > +* Now that we have all the maps created, just set the ->end of them:
>> > +*/
>> > +   map_groups__fixup_end(>kmaps);
>>
>> With above change, I think it work.  But the whole point of the above
>> is to set the end address of vmlinux and modules.  And now we don't
>> need it for modules thanks to modules__parse() for passing the size.
>>
>> So we only needs to update the end address of vmlinux using the start
>> address of the first module.  What about this then?
>
> Looks ok, Jiri? I noticed that Namhyung has the patch below in his git
> tree, but I couldn't find it in the mailing list, Jiri, can I have your
> acked-by?

This version has a bug that misses the call when there's no module.
I'll send a new version if Jiri agrees with the idea.

Thanks,
Namhyung


>>
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index fe27ef55cbb9..022f0539b750 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1021,13 +1021,6 @@ int machine__load_vmlinux_path(struct machine 
>> *machine, enum map_type type)
>> return ret;
>>  }
>>
>> -static void map_groups__fixup_end(struct map_groups *mg)
>> -{
>> -   int i;
>> -   for (i = 0; i < MAP__NR_TYPES; ++i)
>> -   __map_groups__fixup_end(mg, i);
>> -}
>> -
>>  static char *get_kernel_version(const char *root_dir)
>>  {
>> char version[PATH_MAX];
>> @@ -1235,6 +1228,7 @@ int machine__create_kernel_maps(struct machine 
>> *machine)
>>  {
>> struct dso *kernel = machine__get_kernel(machine);
>> const char *name = NULL;
>> +   struct map *map;
>> u64 addr = 0;
>> int ret;
>>
>> @@ -1261,13 +1255,13 @@ int machine__create_kernel_maps(struct machine 
>> *machine)
>> machine__destroy_kernel_maps(machine);
>> return -1;
>> }
>> -   machine__set_kernel_mmap(machine, addr, 0);
>> }
>>
>> -   /*
>> -* Now that we have all the maps created, just set the ->end of them:
>> -*/
>> -   map_groups__fixup_end(>kmaps);
>> +   /* update end address of the kernel map using adjacent module 
>> address */
>> +   map = map__next(machine__kernel_map(machine));
>> +   if (map)
>> +   machine__set_kernel_mmap(machine, addr, map->start);
>> +
>> return 0;
>>  }

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Arnaldo Carvalho de Melo]

Em Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim escreveu:
> On Thu, Feb 15, 2018 at 01:26:32PM +0100, Jiri Olsa wrote:
> > if (!machine__get_running_kernel_start(machine, , )) {
> > if (name &&
> > maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, 
> > name, addr)) {
> > machine__destroy_kernel_maps(machine);
> > return -1;
> > }
> > +   machine__set_kernel_mmap(machine, addr, 0);
> > }

> > +   /*
> > +* Now that we have all the maps created, just set the ->end of them:
> > +*/
> > +   map_groups__fixup_end(>kmaps);
> 
> With above change, I think it work.  But the whole point of the above
> is to set the end address of vmlinux and modules.  And now we don't
> need it for modules thanks to modules__parse() for passing the size.
> 
> So we only needs to update the end address of vmlinux using the start
> address of the first module.  What about this then?

Looks ok, Jiri? I noticed that Namhyung has the patch below in his git
tree, but I couldn't find it in the mailing list, Jiri, can I have your
acked-by?

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index fe27ef55cbb9..022f0539b750 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1021,13 +1021,6 @@ int machine__load_vmlinux_path(struct machine 
> *machine, enum map_type type)
> return ret;
>  }
>  
> -static void map_groups__fixup_end(struct map_groups *mg)
> -{
> -   int i;
> -   for (i = 0; i < MAP__NR_TYPES; ++i)
> -   __map_groups__fixup_end(mg, i);
> -}
> -
>  static char *get_kernel_version(const char *root_dir)
>  {
> char version[PATH_MAX];
> @@ -1235,6 +1228,7 @@ int machine__create_kernel_maps(struct machine *machine)
>  {
> struct dso *kernel = machine__get_kernel(machine);
> const char *name = NULL;
> +   struct map *map;
> u64 addr = 0;
> int ret;
>  
> @@ -1261,13 +1255,13 @@ int machine__create_kernel_maps(struct machine 
> *machine)
> machine__destroy_kernel_maps(machine);
> return -1;
> }
> -   machine__set_kernel_mmap(machine, addr, 0);
> }
>  
> -   /*
> -* Now that we have all the maps created, just set the ->end of them:
> -*/
> -   map_groups__fixup_end(>kmaps);
> +   /* update end address of the kernel map using adjacent module address 
> */
> +   map = map__next(machine__kernel_map(machine));
> +   if (map)
> +   machine__set_kernel_mmap(machine, addr, map->start);
> +
> return 0;
>  }

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Arnaldo Carvalho de Melo]

Em Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim escreveu:
> On Thu, Feb 15, 2018 at 01:26:32PM +0100, Jiri Olsa wrote:
> > if (!machine__get_running_kernel_start(machine, , )) {
> > if (name &&
> > maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, 
> > name, addr)) {
> > machine__destroy_kernel_maps(machine);
> > return -1;
> > }
> > +   machine__set_kernel_mmap(machine, addr, 0);
> > }

> > +   /*
> > +* Now that we have all the maps created, just set the ->end of them:
> > +*/
> > +   map_groups__fixup_end(>kmaps);
> 
> With above change, I think it work.  But the whole point of the above
> is to set the end address of vmlinux and modules.  And now we don't
> need it for modules thanks to modules__parse() for passing the size.
> 
> So we only needs to update the end address of vmlinux using the start
> address of the first module.  What about this then?

Looks ok, Jiri? I noticed that Namhyung has the patch below in his git
tree, but I couldn't find it in the mailing list, Jiri, can I have your
acked-by?

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index fe27ef55cbb9..022f0539b750 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1021,13 +1021,6 @@ int machine__load_vmlinux_path(struct machine 
> *machine, enum map_type type)
> return ret;
>  }
>  
> -static void map_groups__fixup_end(struct map_groups *mg)
> -{
> -   int i;
> -   for (i = 0; i < MAP__NR_TYPES; ++i)
> -   __map_groups__fixup_end(mg, i);
> -}
> -
>  static char *get_kernel_version(const char *root_dir)
>  {
> char version[PATH_MAX];
> @@ -1235,6 +1228,7 @@ int machine__create_kernel_maps(struct machine *machine)
>  {
> struct dso *kernel = machine__get_kernel(machine);
> const char *name = NULL;
> +   struct map *map;
> u64 addr = 0;
> int ret;
>  
> @@ -1261,13 +1255,13 @@ int machine__create_kernel_maps(struct machine 
> *machine)
> machine__destroy_kernel_maps(machine);
> return -1;
> }
> -   machine__set_kernel_mmap(machine, addr, 0);
> }
>  
> -   /*
> -* Now that we have all the maps created, just set the ->end of them:
> -*/
> -   map_groups__fixup_end(>kmaps);
> +   /* update end address of the kernel map using adjacent module address 
> */
> +   map = map__next(machine__kernel_map(machine));
> +   if (map)
> +   machine__set_kernel_mmap(machine, addr, map->start);
> +
> return 0;
>  }

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Arnaldo Carvalho de Melo]

Em Mon, Feb 19, 2018 at 11:49:44AM +0100, Jiri Olsa escreveu:
> On Mon, Feb 19, 2018 at 07:19:36PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > From b736729e83b62f97d716a011ccf4e430b614fecd Mon Sep 17 00:00:00 2001
> > From: Namhyung Kim 
> > Date: Mon, 19 Feb 2018 19:00:46 +0900
> > Subject: [PATCH] perf tools: Fix paranoid check in 
> > machine__set_kernel_mmap()
> > 
> > The machine__set_kernel_mmap() is to setup addresses of the kernel map
> > using external info.  But it has a check when the address is given
> > from an incorrect input which should have the start and end address of
> > 0 (i.e. machine__process_kernel_mmap_event).
> > 
> > But we also use the end address of 0 for a valid input so change it to
> > check both start and end addresses.
> > 
> > Signed-off-by: Namhyung Kim 
> 
> Acked-by: Jiri Olsa 

Thanks, applied.

- Arnaldo

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Arnaldo Carvalho de Melo]

Em Mon, Feb 19, 2018 at 11:49:44AM +0100, Jiri Olsa escreveu:
> On Mon, Feb 19, 2018 at 07:19:36PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > From b736729e83b62f97d716a011ccf4e430b614fecd Mon Sep 17 00:00:00 2001
> > From: Namhyung Kim 
> > Date: Mon, 19 Feb 2018 19:00:46 +0900
> > Subject: [PATCH] perf tools: Fix paranoid check in 
> > machine__set_kernel_mmap()
> > 
> > The machine__set_kernel_mmap() is to setup addresses of the kernel map
> > using external info.  But it has a check when the address is given
> > from an incorrect input which should have the start and end address of
> > 0 (i.e. machine__process_kernel_mmap_event).
> > 
> > But we also use the end address of 0 for a valid input so change it to
> > check both start and end addresses.
> > 
> > Signed-off-by: Namhyung Kim 
> 
> Acked-by: Jiri Olsa 

Thanks, applied.

- Arnaldo

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Jiri Olsa]

On Mon, Feb 19, 2018 at 07:19:36PM +0900, Namhyung Kim wrote:

SNIP

> From b736729e83b62f97d716a011ccf4e430b614fecd Mon Sep 17 00:00:00 2001
> From: Namhyung Kim 
> Date: Mon, 19 Feb 2018 19:00:46 +0900
> Subject: [PATCH] perf tools: Fix paranoid check in machine__set_kernel_mmap()
> 
> The machine__set_kernel_mmap() is to setup addresses of the kernel map
> using external info.  But it has a check when the address is given
> from an incorrect input which should have the start and end address of
> 0 (i.e. machine__process_kernel_mmap_event).
> 
> But we also use the end address of 0 for a valid input so change it to
> check both start and end addresses.
> 
> Signed-off-by: Namhyung Kim 

Acked-by: Jiri Olsa 

thanks,
jirka

> ---
>  tools/perf/util/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index fe27ef55cbb9..12b7427444a3 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
> *machine,
>* Be a bit paranoid here, some perf.data file came with
>* a zero sized synthesized MMAP event for the kernel.
>*/
> - if (machine->vmlinux_maps[i]->end == 0)
> + if (start == 0 && end == 0)
>   machine->vmlinux_maps[i]->end = ~0ULL;
>   }
>  }
> -- 
> 2.16.1
> 

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Jiri Olsa]

On Mon, Feb 19, 2018 at 07:19:36PM +0900, Namhyung Kim wrote:

SNIP

> From b736729e83b62f97d716a011ccf4e430b614fecd Mon Sep 17 00:00:00 2001
> From: Namhyung Kim 
> Date: Mon, 19 Feb 2018 19:00:46 +0900
> Subject: [PATCH] perf tools: Fix paranoid check in machine__set_kernel_mmap()
> 
> The machine__set_kernel_mmap() is to setup addresses of the kernel map
> using external info.  But it has a check when the address is given
> from an incorrect input which should have the start and end address of
> 0 (i.e. machine__process_kernel_mmap_event).
> 
> But we also use the end address of 0 for a valid input so change it to
> check both start and end addresses.
> 
> Signed-off-by: Namhyung Kim 

Acked-by: Jiri Olsa 

thanks,
jirka

> ---
>  tools/perf/util/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index fe27ef55cbb9..12b7427444a3 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
> *machine,
>* Be a bit paranoid here, some perf.data file came with
>* a zero sized synthesized MMAP event for the kernel.
>*/
> - if (machine->vmlinux_maps[i]->end == 0)
> + if (start == 0 && end == 0)
>   machine->vmlinux_maps[i]->end = ~0ULL;
>   }
>  }
> -- 
> 2.16.1
> 

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Namhyung Kim]

On Mon, Feb 19, 2018 at 11:01:40AM +0100, Jiri Olsa wrote:
> On Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > > +static void machine__set_kernel_mmap(struct machine *machine,
> > > +  u64 start, u64 end)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < MAP__NR_TYPES; i++) {
> > > + machine->vmlinux_maps[i]->start = start;
> > > + machine->vmlinux_maps[i]->end   = end;
> > > +
> > > + /*
> > > +  * Be a bit paranoid here, some perf.data file came with
> > > +  * a zero sized synthesized MMAP event for the kernel.
> > > +  */
> > > + if (machine->vmlinux_maps[i]->end == 0)
> > > + machine->vmlinux_maps[i]->end = ~0ULL;
> > 
> > Hmm.. this will make map_groups__fixup_end() below not working since
> > it only updates if the end address of a map is zero.
> > 
> > And about the paranoid check, AFAIK the only case it cares is the
> > machine__process_kernel_mmap_event() which calls it with
> > event->mmap.start and event->mmap.start + event->mmap.len.  Thus, in
> > order for the end to be zero, both start and len should be zero.  Then
> > I think this condition can be changed like below:
> > 
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index fe27ef55cbb9..c8acb603c359 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
> > *machine,
> >  * Be a bit paranoid here, some perf.data file came with
> >  * a zero sized synthesized MMAP event for the kernel.
> >  */
> > -   if (machine->vmlinux_maps[i]->end == 0)
> > +   if (start == 0 && end == 0)
> > machine->vmlinux_maps[i]->end = ~0ULL;
> > }
> 
> right, the call from machine__create_kernel_maps will have always
> the start != 0 and there will be subsequent map_groups__fixup_end
> call..
> 
> seems ok to me.. will you send the patch?

Sure. :)


>From b736729e83b62f97d716a011ccf4e430b614fecd Mon Sep 17 00:00:00 2001
From: Namhyung Kim 
Date: Mon, 19 Feb 2018 19:00:46 +0900
Subject: [PATCH] perf tools: Fix paranoid check in machine__set_kernel_mmap()

The machine__set_kernel_mmap() is to setup addresses of the kernel map
using external info.  But it has a check when the address is given
from an incorrect input which should have the start and end address of
0 (i.e. machine__process_kernel_mmap_event).

But we also use the end address of 0 for a valid input so change it to
check both start and end addresses.

Signed-off-by: Namhyung Kim 
---
 tools/perf/util/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index fe27ef55cbb9..12b7427444a3 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
*machine,
 * Be a bit paranoid here, some perf.data file came with
 * a zero sized synthesized MMAP event for the kernel.
 */
-   if (machine->vmlinux_maps[i]->end == 0)
+   if (start == 0 && end == 0)
machine->vmlinux_maps[i]->end = ~0ULL;
}
 }
-- 
2.16.1

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Namhyung Kim]

On Mon, Feb 19, 2018 at 11:01:40AM +0100, Jiri Olsa wrote:
> On Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > > +static void machine__set_kernel_mmap(struct machine *machine,
> > > +  u64 start, u64 end)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < MAP__NR_TYPES; i++) {
> > > + machine->vmlinux_maps[i]->start = start;
> > > + machine->vmlinux_maps[i]->end   = end;
> > > +
> > > + /*
> > > +  * Be a bit paranoid here, some perf.data file came with
> > > +  * a zero sized synthesized MMAP event for the kernel.
> > > +  */
> > > + if (machine->vmlinux_maps[i]->end == 0)
> > > + machine->vmlinux_maps[i]->end = ~0ULL;
> > 
> > Hmm.. this will make map_groups__fixup_end() below not working since
> > it only updates if the end address of a map is zero.
> > 
> > And about the paranoid check, AFAIK the only case it cares is the
> > machine__process_kernel_mmap_event() which calls it with
> > event->mmap.start and event->mmap.start + event->mmap.len.  Thus, in
> > order for the end to be zero, both start and len should be zero.  Then
> > I think this condition can be changed like below:
> > 
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index fe27ef55cbb9..c8acb603c359 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
> > *machine,
> >  * Be a bit paranoid here, some perf.data file came with
> >  * a zero sized synthesized MMAP event for the kernel.
> >  */
> > -   if (machine->vmlinux_maps[i]->end == 0)
> > +   if (start == 0 && end == 0)
> > machine->vmlinux_maps[i]->end = ~0ULL;
> > }
> 
> right, the call from machine__create_kernel_maps will have always
> the start != 0 and there will be subsequent map_groups__fixup_end
> call..
> 
> seems ok to me.. will you send the patch?

Sure. :)


>From b736729e83b62f97d716a011ccf4e430b614fecd Mon Sep 17 00:00:00 2001
From: Namhyung Kim 
Date: Mon, 19 Feb 2018 19:00:46 +0900
Subject: [PATCH] perf tools: Fix paranoid check in machine__set_kernel_mmap()

The machine__set_kernel_mmap() is to setup addresses of the kernel map
using external info.  But it has a check when the address is given
from an incorrect input which should have the start and end address of
0 (i.e. machine__process_kernel_mmap_event).

But we also use the end address of 0 for a valid input so change it to
check both start and end addresses.

Signed-off-by: Namhyung Kim 
---
 tools/perf/util/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index fe27ef55cbb9..12b7427444a3 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
*machine,
 * Be a bit paranoid here, some perf.data file came with
 * a zero sized synthesized MMAP event for the kernel.
 */
-   if (machine->vmlinux_maps[i]->end == 0)
+   if (start == 0 && end == 0)
machine->vmlinux_maps[i]->end = ~0ULL;
}
 }
-- 
2.16.1

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Jiri Olsa]

On Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim wrote:

SNIP

> > +static void machine__set_kernel_mmap(struct machine *machine,
> > +u64 start, u64 end)
> > +{
> > +   int i;
> > +
> > +   for (i = 0; i < MAP__NR_TYPES; i++) {
> > +   machine->vmlinux_maps[i]->start = start;
> > +   machine->vmlinux_maps[i]->end   = end;
> > +
> > +   /*
> > +* Be a bit paranoid here, some perf.data file came with
> > +* a zero sized synthesized MMAP event for the kernel.
> > +*/
> > +   if (machine->vmlinux_maps[i]->end == 0)
> > +   machine->vmlinux_maps[i]->end = ~0ULL;
> 
> Hmm.. this will make map_groups__fixup_end() below not working since
> it only updates if the end address of a map is zero.
> 
> And about the paranoid check, AFAIK the only case it cares is the
> machine__process_kernel_mmap_event() which calls it with
> event->mmap.start and event->mmap.start + event->mmap.len.  Thus, in
> order for the end to be zero, both start and len should be zero.  Then
> I think this condition can be changed like below:
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index fe27ef55cbb9..c8acb603c359 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
> *machine,
>  * Be a bit paranoid here, some perf.data file came with
>  * a zero sized synthesized MMAP event for the kernel.
>  */
> -   if (machine->vmlinux_maps[i]->end == 0)
> +   if (start == 0 && end == 0)
> machine->vmlinux_maps[i]->end = ~0ULL;
> }

right, the call from machine__create_kernel_maps will have always
the start != 0 and there will be subsequent map_groups__fixup_end
call..

seems ok to me.. will you send the patch?

thanks,
jirka

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Jiri Olsa]

On Mon, Feb 19, 2018 at 11:20:36AM +0900, Namhyung Kim wrote:

SNIP

> > +static void machine__set_kernel_mmap(struct machine *machine,
> > +u64 start, u64 end)
> > +{
> > +   int i;
> > +
> > +   for (i = 0; i < MAP__NR_TYPES; i++) {
> > +   machine->vmlinux_maps[i]->start = start;
> > +   machine->vmlinux_maps[i]->end   = end;
> > +
> > +   /*
> > +* Be a bit paranoid here, some perf.data file came with
> > +* a zero sized synthesized MMAP event for the kernel.
> > +*/
> > +   if (machine->vmlinux_maps[i]->end == 0)
> > +   machine->vmlinux_maps[i]->end = ~0ULL;
> 
> Hmm.. this will make map_groups__fixup_end() below not working since
> it only updates if the end address of a map is zero.
> 
> And about the paranoid check, AFAIK the only case it cares is the
> machine__process_kernel_mmap_event() which calls it with
> event->mmap.start and event->mmap.start + event->mmap.len.  Thus, in
> order for the end to be zero, both start and len should be zero.  Then
> I think this condition can be changed like below:
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index fe27ef55cbb9..c8acb603c359 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
> *machine,
>  * Be a bit paranoid here, some perf.data file came with
>  * a zero sized synthesized MMAP event for the kernel.
>  */
> -   if (machine->vmlinux_maps[i]->end == 0)
> +   if (start == 0 && end == 0)
> machine->vmlinux_maps[i]->end = ~0ULL;
> }

right, the call from machine__create_kernel_maps will have always
the start != 0 and there will be subsequent map_groups__fixup_end
call..

seems ok to me.. will you send the patch?

thanks,
jirka

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Namhyung Kim]

Hi Jiri and Arnaldo,

On Thu, Feb 15, 2018 at 01:26:32PM +0100, Jiri Olsa wrote:
> We should not search for kernel start address in
> __machine__create_kernel_maps function, because it's being
> used in 'report' code path, where we are interested in kernel
> MMAP data address instead of in current kernel address
> 
> The __machine__create_kernel_maps serves purely for creating
> the machines kernel maps and setting up the kmap group. The
> report code path then sets the address based on the data from
> kernel MMAP event in machine__set_kernel_mmap function.
> 
> The kallsyms search address logic is used for test code, that
> calls machine__create_kernel_maps to get current maps and calls
> machine__get_running_kernel_start to get kernel starting address.
> 
> Using machine__set_kernel_mmap to se the kernel maps start
> address and moving map_groups__fixup_end to be call when
> all maps are in place.
> 
> Also making __machine__create_kernel_maps static, because there's
> no external user.
> 
> Link: http://lkml.kernel.org/n/tip-37lmdjzh8dwq03golnk7h...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  tools/perf/util/machine.c | 55 
> ++-
>  tools/perf/util/machine.h |  1 -
>  2 files changed, 26 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 292e70c774bd..2db8d7dd0f80 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -856,13 +856,10 @@ static int machine__get_running_kernel_start(struct 
> machine *machine,
>   return 0;
>  }
>  
> -int __machine__create_kernel_maps(struct machine *machine, struct dso 
> *kernel)
> +static int
> +__machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
>  {
>   int type;
> - u64 start = 0;
> -
> - if (machine__get_running_kernel_start(machine, NULL, ))
> - return -1;
>  
>   /* In case of renewal the kernel map, destroy previous one */
>   machine__destroy_kernel_maps(machine);
> @@ -871,7 +868,7 @@ int __machine__create_kernel_maps(struct machine 
> *machine, struct dso *kernel)
>   struct kmap *kmap;
>   struct map *map;
>  
> - machine->vmlinux_maps[type] = map__new2(start, kernel, type);
> + machine->vmlinux_maps[type] = map__new2(0, kernel, type);
>   if (machine->vmlinux_maps[type] == NULL)
>   return -1;
>  
> @@ -1222,6 +1219,24 @@ static int machine__create_modules(struct machine 
> *machine)
>   return 0;
>  }
>  
> +static void machine__set_kernel_mmap(struct machine *machine,
> +  u64 start, u64 end)
> +{
> + int i;
> +
> + for (i = 0; i < MAP__NR_TYPES; i++) {
> + machine->vmlinux_maps[i]->start = start;
> + machine->vmlinux_maps[i]->end   = end;
> +
> + /*
> +  * Be a bit paranoid here, some perf.data file came with
> +  * a zero sized synthesized MMAP event for the kernel.
> +  */
> + if (machine->vmlinux_maps[i]->end == 0)
> + machine->vmlinux_maps[i]->end = ~0ULL;

Hmm.. this will make map_groups__fixup_end() below not working since
it only updates if the end address of a map is zero.

And about the paranoid check, AFAIK the only case it cares is the
machine__process_kernel_mmap_event() which calls it with
event->mmap.start and event->mmap.start + event->mmap.len.  Thus, in
order for the end to be zero, both start and len should be zero.  Then
I think this condition can be changed like below:

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index fe27ef55cbb9..c8acb603c359 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
*machine,
 * Be a bit paranoid here, some perf.data file came with
 * a zero sized synthesized MMAP event for the kernel.
 */
-   if (machine->vmlinux_maps[i]->end == 0)
+   if (start == 0 && end == 0)
machine->vmlinux_maps[i]->end = ~0ULL;
}
 }



> + }
> +}
> +
>  int machine__create_kernel_maps(struct machine *machine)
>  {
>   struct dso *kernel = machine__get_kernel(machine);
> @@ -1246,40 +1261,22 @@ int machine__create_kernel_maps(struct machine 
> *machine)
>"continuing anyway...\n", machine->pid);
>   }
>  
> - /*
> -  * Now that we have all the maps created, just set the ->end of them:
> -  */
> - map_groups__fixup_end(>kmaps);
> -
>   if (!machine__get_running_kernel_start(machine, , )) {
>   if (name &&
>   maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, 
> name, addr)) {
>   machine__destroy_kernel_maps(machine);
>   return -1;
>

Re: [PATCH 6/9] perf tools: Don't search for active kernel start in __machine__create_kernel_maps

[Namhyung Kim]

Hi Jiri and Arnaldo,

On Thu, Feb 15, 2018 at 01:26:32PM +0100, Jiri Olsa wrote:
> We should not search for kernel start address in
> __machine__create_kernel_maps function, because it's being
> used in 'report' code path, where we are interested in kernel
> MMAP data address instead of in current kernel address
> 
> The __machine__create_kernel_maps serves purely for creating
> the machines kernel maps and setting up the kmap group. The
> report code path then sets the address based on the data from
> kernel MMAP event in machine__set_kernel_mmap function.
> 
> The kallsyms search address logic is used for test code, that
> calls machine__create_kernel_maps to get current maps and calls
> machine__get_running_kernel_start to get kernel starting address.
> 
> Using machine__set_kernel_mmap to se the kernel maps start
> address and moving map_groups__fixup_end to be call when
> all maps are in place.
> 
> Also making __machine__create_kernel_maps static, because there's
> no external user.
> 
> Link: http://lkml.kernel.org/n/tip-37lmdjzh8dwq03golnk7h...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  tools/perf/util/machine.c | 55 
> ++-
>  tools/perf/util/machine.h |  1 -
>  2 files changed, 26 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 292e70c774bd..2db8d7dd0f80 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -856,13 +856,10 @@ static int machine__get_running_kernel_start(struct 
> machine *machine,
>   return 0;
>  }
>  
> -int __machine__create_kernel_maps(struct machine *machine, struct dso 
> *kernel)
> +static int
> +__machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
>  {
>   int type;
> - u64 start = 0;
> -
> - if (machine__get_running_kernel_start(machine, NULL, ))
> - return -1;
>  
>   /* In case of renewal the kernel map, destroy previous one */
>   machine__destroy_kernel_maps(machine);
> @@ -871,7 +868,7 @@ int __machine__create_kernel_maps(struct machine 
> *machine, struct dso *kernel)
>   struct kmap *kmap;
>   struct map *map;
>  
> - machine->vmlinux_maps[type] = map__new2(start, kernel, type);
> + machine->vmlinux_maps[type] = map__new2(0, kernel, type);
>   if (machine->vmlinux_maps[type] == NULL)
>   return -1;
>  
> @@ -1222,6 +1219,24 @@ static int machine__create_modules(struct machine 
> *machine)
>   return 0;
>  }
>  
> +static void machine__set_kernel_mmap(struct machine *machine,
> +  u64 start, u64 end)
> +{
> + int i;
> +
> + for (i = 0; i < MAP__NR_TYPES; i++) {
> + machine->vmlinux_maps[i]->start = start;
> + machine->vmlinux_maps[i]->end   = end;
> +
> + /*
> +  * Be a bit paranoid here, some perf.data file came with
> +  * a zero sized synthesized MMAP event for the kernel.
> +  */
> + if (machine->vmlinux_maps[i]->end == 0)
> + machine->vmlinux_maps[i]->end = ~0ULL;

Hmm.. this will make map_groups__fixup_end() below not working since
it only updates if the end address of a map is zero.

And about the paranoid check, AFAIK the only case it cares is the
machine__process_kernel_mmap_event() which calls it with
event->mmap.start and event->mmap.start + event->mmap.len.  Thus, in
order for the end to be zero, both start and len should be zero.  Then
I think this condition can be changed like below:

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index fe27ef55cbb9..c8acb603c359 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1226,7 +1226,7 @@ static void machine__set_kernel_mmap(struct machine 
*machine,
 * Be a bit paranoid here, some perf.data file came with
 * a zero sized synthesized MMAP event for the kernel.
 */
-   if (machine->vmlinux_maps[i]->end == 0)
+   if (start == 0 && end == 0)
machine->vmlinux_maps[i]->end = ~0ULL;
}
 }



> + }
> +}
> +
>  int machine__create_kernel_maps(struct machine *machine)
>  {
>   struct dso *kernel = machine__get_kernel(machine);
> @@ -1246,40 +1261,22 @@ int machine__create_kernel_maps(struct machine 
> *machine)
>"continuing anyway...\n", machine->pid);
>   }
>  
> - /*
> -  * Now that we have all the maps created, just set the ->end of them:
> -  */
> - map_groups__fixup_end(>kmaps);
> -
>   if (!machine__get_running_kernel_start(machine, , )) {
>   if (name &&
>   maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, 
> name, addr)) {
>   machine__destroy_kernel_maps(machine);
>   return -1;
>   }
> +