Re: [PATCH v2] perf machine: Fix load kernel symbol with '-k' option
On Sat, Mar 10, 2018 at 05:50:56PM +0800, Leo Yan wrote:
SNIP
> > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > index 12b7427..3125871 100644
> > > --- a/tools/perf/util/machine.c
> > > +++ b/tools/perf/util/machine.c
> > > @@ -1299,9 +1299,18 @@ static int
> > > machine__process_kernel_mmap_event(struct machine *machine,
> > > else
> > > kernel_type = DSO_TYPE_GUEST_KERNEL;
> > >
> > > - is_kernel_mmap = memcmp(event->mmap.filename,
> > > - machine->mmap_name,
> > > - strlen(machine->mmap_name) - 1) == 0;
> > > + /*
> > > + * When symbol_conf.vmlinux_name is not NULL, it includes the specified
> > > + * kernel vmlinux path with option '-k'. So set 'is_kernel_mmap' to
> > > + * true for creating machine symbol map.
> > > + */
> > > + if (symbol_conf.vmlinux_name)
> > > + is_kernel_mmap = true;
> > > + else
> > > + is_kernel_mmap = memcmp(event->mmap.filename,
> > > + machine->mmap_name,
> > > + strlen(machine->mmap_name) - 1) == 0;
> > > +
> > > if (event->mmap.filename[0] == '/' ||
> > > (!is_kernel_mmap && event->mmap.filename[0] == '[')) {
> > > map = machine__findnew_module_map(machine, event->mmap.start,
> >
> > right, the mmap gets confused with the vmlinux path, but I wonder
> > the fix should be not to include symbol_conf.vmlinux_name in the
> > mmap_name like below.. untested
>
> I tested below fixing at my side, and confirm your fixing also can
> resolve this issue. After reviewing the change, it's more neat than my
> own change . So you could add my test tag :)
>
> Tested-by: Leo Yan
thanks
SNIP
> static int machine__set_mmap_name(struct machine *machine)
> {
> - if (machine__is_host(machine)) {
> - if (symbol_conf.vmlinux_name)
> - machine->mmap_name = strdup(symbol_conf.vmlinux_name);
> - else
> - machine->mmap_name = strdup("[kernel.kallsyms]");
> - } else if (machine__is_default_guest(machine)) {
> - if (symbol_conf.default_guest_vmlinux_name)
> - machine->mmap_name =
> strdup(symbol_conf.default_guest_vmlinux_name);
> - else
> - machine->mmap_name =
> strdup("[guest.kernel.kallsyms]");
> - } else {
> - if (asprintf(&machine->mmap_name,
> "[guest.kernel.kallsyms.%d]",
> -machine->pid) < 0)
> - machine->mmap_name = NULL;
> - }
> + if (machine__is_host(machine))
> + machine->mmap_name = strdup("[kernel.kallsyms]");
> + else if (machine__is_default_guest(machine))
> + machine->mmap_name = strdup("[guest.kernel.kallsyms]");
> + else if (asprintf(&machine->mmap_name, "[guest.kernel.kallsyms.%d]",
> +machine->pid) < 0)
> + machine->mmap_name = NULL;
yea, that looks better.. will change
jirka
Re: [PATCH v2] perf machine: Fix load kernel symbol with '-k' option
Hi Jiri,
On Fri, Mar 09, 2018 at 09:56:00PM +0100, Jiri Olsa wrote:
> On Fri, Mar 09, 2018 at 02:05:23PM +0800, Leo Yan wrote:
> > On Hikey arm64 octa A53 platform, when use command './perf report -v
> > -k vmlinux --stdio' it outputs below error info, and it skips to load
> > kernel symbol and doesn't print symbol for event:
> > Failed to open [kernel.kallsyms]_text, continuing without symbols.
> >
> > The regression is introduced by commit ("8c7f1bb37b29 perf machine: Move
> > kernel mmap name into struct machine"), which changes the logic for
> > machine mmap_name by removing function machine__mmap_name() and always
> > use 'machine->mmap_name'. Comparing difference between
> > machine__mmap_name() and 'machine->mmap_name', the later one includes
> > the string for specified kernel vmlinux string with option '-k' in
> > command, but the old function machine__mmap_name() ignores vmlinux
> > path string. As result, event's mmap file name doesn't match with
> > machine mmap file name anymore and it skips to load kernel symbol from
> > vmlinux file.
> >
> > To resolve this issue, this patch adds extra checking for
> > 'symbol_conf.vmlinux_name', when it has been set string so we can know
> > it includes vmlinux path string specified for option '-k'. For this
> > case it sets 'is_kernel_mmap' to true and run into flow to load kernel
> > symbol from vmlinux.
> >
> > This patch has been verified with two commands: './perf report -v
> > -k vmlinux --stdio' and './perf script -v -F cpu,event,ip,sym,symoff
> > -k vmlinux'.
> >
> > Suggested-by: Mathieu Poirier
> > Signed-off-by: Leo Yan
> > ---
> > tools/perf/util/machine.c | 15 ---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 12b7427..3125871 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1299,9 +1299,18 @@ static int machine__process_kernel_mmap_event(struct
> > machine *machine,
> > else
> > kernel_type = DSO_TYPE_GUEST_KERNEL;
> >
> > - is_kernel_mmap = memcmp(event->mmap.filename,
> > - machine->mmap_name,
> > - strlen(machine->mmap_name) - 1) == 0;
> > + /*
> > +* When symbol_conf.vmlinux_name is not NULL, it includes the specified
> > +* kernel vmlinux path with option '-k'. So set 'is_kernel_mmap' to
> > +* true for creating machine symbol map.
> > +*/
> > + if (symbol_conf.vmlinux_name)
> > + is_kernel_mmap = true;
> > + else
> > + is_kernel_mmap = memcmp(event->mmap.filename,
> > + machine->mmap_name,
> > + strlen(machine->mmap_name) - 1) == 0;
> > +
> > if (event->mmap.filename[0] == '/' ||
> > (!is_kernel_mmap && event->mmap.filename[0] == '[')) {
> > map = machine__findnew_module_map(machine, event->mmap.start,
>
> right, the mmap gets confused with the vmlinux path, but I wonder
> the fix should be not to include symbol_conf.vmlinux_name in the
> mmap_name like below.. untested
I tested below fixing at my side, and confirm your fixing also can
resolve this issue. After reviewing the change, it's more neat than my
own change . So you could add my test tag :)
Tested-by: Leo Yan
BTW, I have a minor comment for code refactoring, it's up to you if
need or not and if need use a new patch for refactoring, please see below
inline comment.
Thanks,
Leo Yan
> ---
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 43fbbee409ec..f0cb72022177 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -51,15 +51,9 @@ static void machine__threads_init(struct machine *machine)
> static int machine__set_mmap_name(struct machine *machine)
> {
> if (machine__is_host(machine)) {
> - if (symbol_conf.vmlinux_name)
> - machine->mmap_name = strdup(symbol_conf.vmlinux_name);
> - else
> - machine->mmap_name = strdup("[kernel.kallsyms]");
> + machine->mmap_name = strdup("[kernel.kallsyms]");
> } else if (machine__is_default_guest(machine)) {
> - if (symbol_conf.default_guest_vmlinux_name)
> - machine->mmap_name =
> strdup(symbol_conf.default_guest_vmlinux_name);
> - else
> - machine->mmap_name = strdup("[guest.kernel.kallsyms]");
> + machine->mmap_name = strdup("[guest.kernel.kallsyms]");
> } else {
> if (asprintf(&machine->mmap_name, "[guest.kernel.kallsyms.%d]",
>machine->pid) < 0)
Do you think below format is better or not?
static int machine__set_mmap_name(struct machine *machine)
{
- if (machine__is_host(machine)) {
- if (symbol_conf.vmlinux_name)
- machine->mmap_name = strdup(symbol_conf.vmlinux_name);
-
Re: [PATCH v2] perf machine: Fix load kernel symbol with '-k' option
On Fri, Mar 09, 2018 at 02:05:23PM +0800, Leo Yan wrote:
> On Hikey arm64 octa A53 platform, when use command './perf report -v
> -k vmlinux --stdio' it outputs below error info, and it skips to load
> kernel symbol and doesn't print symbol for event:
> Failed to open [kernel.kallsyms]_text, continuing without symbols.
>
> The regression is introduced by commit ("8c7f1bb37b29 perf machine: Move
> kernel mmap name into struct machine"), which changes the logic for
> machine mmap_name by removing function machine__mmap_name() and always
> use 'machine->mmap_name'. Comparing difference between
> machine__mmap_name() and 'machine->mmap_name', the later one includes
> the string for specified kernel vmlinux string with option '-k' in
> command, but the old function machine__mmap_name() ignores vmlinux
> path string. As result, event's mmap file name doesn't match with
> machine mmap file name anymore and it skips to load kernel symbol from
> vmlinux file.
>
> To resolve this issue, this patch adds extra checking for
> 'symbol_conf.vmlinux_name', when it has been set string so we can know
> it includes vmlinux path string specified for option '-k'. For this
> case it sets 'is_kernel_mmap' to true and run into flow to load kernel
> symbol from vmlinux.
>
> This patch has been verified with two commands: './perf report -v
> -k vmlinux --stdio' and './perf script -v -F cpu,event,ip,sym,symoff
> -k vmlinux'.
>
> Suggested-by: Mathieu Poirier
> Signed-off-by: Leo Yan
> ---
> tools/perf/util/machine.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 12b7427..3125871 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1299,9 +1299,18 @@ static int machine__process_kernel_mmap_event(struct
> machine *machine,
> else
> kernel_type = DSO_TYPE_GUEST_KERNEL;
>
> - is_kernel_mmap = memcmp(event->mmap.filename,
> - machine->mmap_name,
> - strlen(machine->mmap_name) - 1) == 0;
> + /*
> + * When symbol_conf.vmlinux_name is not NULL, it includes the specified
> + * kernel vmlinux path with option '-k'. So set 'is_kernel_mmap' to
> + * true for creating machine symbol map.
> + */
> + if (symbol_conf.vmlinux_name)
> + is_kernel_mmap = true;
> + else
> + is_kernel_mmap = memcmp(event->mmap.filename,
> + machine->mmap_name,
> + strlen(machine->mmap_name) - 1) == 0;
> +
> if (event->mmap.filename[0] == '/' ||
> (!is_kernel_mmap && event->mmap.filename[0] == '[')) {
> map = machine__findnew_module_map(machine, event->mmap.start,
right, the mmap gets confused with the vmlinux path, but I wonder
the fix should be not to include symbol_conf.vmlinux_name in the
mmap_name like below.. untested
thanks,
jirka
---
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 43fbbee409ec..f0cb72022177 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -51,15 +51,9 @@ static void machine__threads_init(struct machine *machine)
static int machine__set_mmap_name(struct machine *machine)
{
if (machine__is_host(machine)) {
- if (symbol_conf.vmlinux_name)
- machine->mmap_name = strdup(symbol_conf.vmlinux_name);
- else
- machine->mmap_name = strdup("[kernel.kallsyms]");
+ machine->mmap_name = strdup("[kernel.kallsyms]");
} else if (machine__is_default_guest(machine)) {
- if (symbol_conf.default_guest_vmlinux_name)
- machine->mmap_name =
strdup(symbol_conf.default_guest_vmlinux_name);
- else
- machine->mmap_name = strdup("[guest.kernel.kallsyms]");
+ machine->mmap_name = strdup("[guest.kernel.kallsyms]");
} else {
if (asprintf(&machine->mmap_name, "[guest.kernel.kallsyms.%d]",
machine->pid) < 0)
@@ -794,9 +788,15 @@ static struct dso *machine__get_kernel(struct machine
*machine)
struct dso *kernel;
if (machine__is_host(machine)) {
+ if (symbol_conf.vmlinux_name)
+ vmlinux_name = symbol_conf.vmlinux_name;
+
kernel = machine__findnew_kernel(machine, vmlinux_name,
"[kernel]", DSO_TYPE_KERNEL);
} else {
+ if (symbol_conf.default_guest_vmlinux_name)
+ vmlinux_name = symbol_conf.default_guest_vmlinux_name;
+
kernel = machine__findnew_kernel(machine, vmlinux_name,
"[guest.kernel]",
DSO_TYPE_GUEST_KERNEL);
Re: [PATCH v2] perf machine: Fix load kernel symbol with '-k' option
Em Fri, Mar 09, 2018 at 02:05:23PM +0800, Leo Yan escreveu:
> On Hikey arm64 octa A53 platform, when use command './perf report -v
> -k vmlinux --stdio' it outputs below error info, and it skips to load
> kernel symbol and doesn't print symbol for event:
> Failed to open [kernel.kallsyms]_text, continuing without symbols.
>
> The regression is introduced by commit ("8c7f1bb37b29 perf machine: Move
> kernel mmap name into struct machine"), which changes the logic for
commit 8c7f1bb37b29f140e08175132f3abb4d5ad229fc
Author: Jiri Olsa
Date: Thu Feb 15 13:26:30 2018 +0100
perf machine: Move kernel mmap name into struct machine
Jiri, can you please check this and Ack?
- Arnaldo
> machine mmap_name by removing function machine__mmap_name() and always
> use 'machine->mmap_name'. Comparing difference between
> machine__mmap_name() and 'machine->mmap_name', the later one includes
> the string for specified kernel vmlinux string with option '-k' in
> command, but the old function machine__mmap_name() ignores vmlinux
> path string. As result, event's mmap file name doesn't match with
> machine mmap file name anymore and it skips to load kernel symbol from
> vmlinux file.
>
> To resolve this issue, this patch adds extra checking for
> 'symbol_conf.vmlinux_name', when it has been set string so we can know
> it includes vmlinux path string specified for option '-k'. For this
> case it sets 'is_kernel_mmap' to true and run into flow to load kernel
> symbol from vmlinux.
>
> This patch has been verified with two commands: './perf report -v
> -k vmlinux --stdio' and './perf script -v -F cpu,event,ip,sym,symoff
> -k vmlinux'.
>
> Suggested-by: Mathieu Poirier
> Signed-off-by: Leo Yan
> ---
> tools/perf/util/machine.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 12b7427..3125871 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1299,9 +1299,18 @@ static int machine__process_kernel_mmap_event(struct
> machine *machine,
> else
> kernel_type = DSO_TYPE_GUEST_KERNEL;
>
> - is_kernel_mmap = memcmp(event->mmap.filename,
> - machine->mmap_name,
> - strlen(machine->mmap_name) - 1) == 0;
> + /*
> + * When symbol_conf.vmlinux_name is not NULL, it includes the specified
> + * kernel vmlinux path with option '-k'. So set 'is_kernel_mmap' to
> + * true for creating machine symbol map.
> + */
> + if (symbol_conf.vmlinux_name)
> + is_kernel_mmap = true;
> + else
> + is_kernel_mmap = memcmp(event->mmap.filename,
> + machine->mmap_name,
> + strlen(machine->mmap_name) - 1) == 0;
> +
> if (event->mmap.filename[0] == '/' ||
> (!is_kernel_mmap && event->mmap.filename[0] == '[')) {
> map = machine__findnew_module_map(machine, event->mmap.start,
> --
> 2.7.4
Re: [PATCH v2] perf machine: Fix load kernel symbol with '-k' option
On 8 March 2018 at 23:05, Leo Yan wrote:
> On Hikey arm64 octa A53 platform, when use command './perf report -v
> -k vmlinux --stdio' it outputs below error info, and it skips to load
> kernel symbol and doesn't print symbol for event:
> Failed to open [kernel.kallsyms]_text, continuing without symbols.
>
> The regression is introduced by commit ("8c7f1bb37b29 perf machine: Move
> kernel mmap name into struct machine"), which changes the logic for
> machine mmap_name by removing function machine__mmap_name() and always
> use 'machine->mmap_name'. Comparing difference between
> machine__mmap_name() and 'machine->mmap_name', the later one includes
> the string for specified kernel vmlinux string with option '-k' in
> command, but the old function machine__mmap_name() ignores vmlinux
> path string. As result, event's mmap file name doesn't match with
> machine mmap file name anymore and it skips to load kernel symbol from
> vmlinux file.
>
> To resolve this issue, this patch adds extra checking for
> 'symbol_conf.vmlinux_name', when it has been set string so we can know
> it includes vmlinux path string specified for option '-k'. For this
> case it sets 'is_kernel_mmap' to true and run into flow to load kernel
> symbol from vmlinux.
>
> This patch has been verified with two commands: './perf report -v
> -k vmlinux --stdio' and './perf script -v -F cpu,event,ip,sym,symoff
> -k vmlinux'.
>
> Suggested-by: Mathieu Poirier
> Signed-off-by: Leo Yan
> ---
> tools/perf/util/machine.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 12b7427..3125871 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1299,9 +1299,18 @@ static int machine__process_kernel_mmap_event(struct
> machine *machine,
> else
> kernel_type = DSO_TYPE_GUEST_KERNEL;
>
> - is_kernel_mmap = memcmp(event->mmap.filename,
> - machine->mmap_name,
> - strlen(machine->mmap_name) - 1) == 0;
> + /*
> +* When symbol_conf.vmlinux_name is not NULL, it includes the
> specified
> +* kernel vmlinux path with option '-k'. So set 'is_kernel_mmap' to
> +* true for creating machine symbol map.
> +*/
> + if (symbol_conf.vmlinux_name)
> + is_kernel_mmap = true;
> + else
> + is_kernel_mmap = memcmp(event->mmap.filename,
> + machine->mmap_name,
> + strlen(machine->mmap_name) - 1) == 0;
> +
I have tested this on my side and things work properly.
Reviewed-by: Mathieu Poirier
> if (event->mmap.filename[0] == '/' ||
> (!is_kernel_mmap && event->mmap.filename[0] == '[')) {
> map = machine__findnew_module_map(machine, event->mmap.start,
> --
> 2.7.4
>
