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;
>   }
> +  

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

[Jiri Olsa]

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;
+   }
+}
+
 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;
}
+   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);
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;
-   }
-}
-
 static bool machine__uses_kcore(struct machine *machine)
 {
struct dso *dso;
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index cb0a20f3a96b..50d587d34459 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -238,7 +238,6 @@ size_t machines__fprintf_dsos_buildid(struct machines 
*machines, FILE *fp,
 

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

[Jiri Olsa]

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;
+   }
+}
+
 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;
}
+   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);
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;
-   }
-}
-
 static bool machine__uses_kcore(struct machine *machine)
 {
struct dso *dso;
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index cb0a20f3a96b..50d587d34459 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -238,7 +238,6 @@ size_t machines__fprintf_dsos_buildid(struct machines 
*machines, FILE *fp,
 bool (skip)(struct