From: Wang Nan <wangnan0@huawei.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: <acme@kernel.org>, <jolsa@kernel.org>, <namhyung@kernel.org>,
<mingo@redhat.com>, <lizefan@huawei.com>, <pi3orama@163.com>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.
Date: Fri, 10 Apr 2015 09:57:27 +0800 [thread overview]
Message-ID: <55272E07.4010200@huawei.com> (raw)
In-Reply-To: <20150409115231.GC1321@krava.brq.redhat.com>
On 2015/4/9 19:52, Jiri Olsa wrote:
> On Wed, Apr 08, 2015 at 03:52:18AM +0000, Wang Nan wrote:
> SNIP
>
>>
>> -bool is_kernel_module(const char *pathname)
>> +bool is_kernel_module(const char *pathname, int cpumode)
>> {
>> struct kmod_path m;
>>
>> - if (kmod_path__parse(&m, pathname))
>> + if (kmod_path__parse(&m, pathname, cpumode))
>> return NULL;
Find another problem here: return NULL for bool function. Will fix in v5.
>>
>> return m.kmod;
>> @@ -210,21 +210,56 @@ bool dso__needs_decompress(struct dso *dso)
>> * Returns 0 if there's no strdup error, -ENOMEM otherwise.
>> */
>> int __kmod_path__parse(struct kmod_path *m, const char *path,
>> - bool alloc_name, bool alloc_ext)
>> + int cpumode, bool alloc_name, bool alloc_ext)
>> {
>> const char *name = strrchr(path, '/');
>> const char *ext = strrchr(path, '.');
>> + bool is_simple_name = false;
>> + bool cpu_mode_kernel, cpu_mode_unknown = false, is_kernel = false;
>> +
>> + /* treat PERF_RECORD_MISC_CPUMODE_UNKNOWN as kernel. */
>> + switch (cpumode & PERF_RECORD_MISC_CPUMODE_MASK) {
>> + case PERF_RECORD_MISC_USER:
>> + case PERF_RECORD_MISC_HYPERVISOR:
>> + case PERF_RECORD_MISC_GUEST_USER:
>> + cpu_mode_kernel = false;
>> + break;
>> + case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
>> + cpu_mode_unknown = true;
>> + /* fall through */
>> + default:
>> + cpu_mode_kernel = true;
>> + }
>
> so the cpumode is valid only for the is_kernel_module caller,
> the rest of the users use PERF_RECORD_MISC_CPUMODE_UNKNOWN
>
> could you move this logic into is_kernel_module function
> and let __kmod_path__parse do only parsing work..?
>
> also new test for is_kernel_module functionality would be nice ;-)
>
>>
>> memset(m, 0x0, sizeof(*m));
>> name = name ? name + 1 : path;
>>
>> + /*
>> + * '.' is also a valid character. For example: [aaa.bbb] is a
>> + * valid module name. '[' should have higher priority than
>> + * '.ko' suffix.
>> + *
>> + * The kernel names are from machine__mmap_name. Such
>> + * name should belong to kernel itself, not kernel module.
>> + */
>> + if (name[0] == '[') {
>> + is_simple_name = true;
>> + if ((strncmp(name, "[kernel.kallsyms]", 17) == 0) ||
>> + (strncmp(name, "[guest.kernel.kallsyms", 22) == 0)) {
>
> these checks would stay in here together with checks
> for [vdso] and [vsyscall] right?
>
> SNIP
>
>> bool is_supported_compression(const char *ext);
>> -bool is_kernel_module(const char *pathname);
>> +bool is_kernel_module(const char *pathname, int cpumode);
>> bool decompress_to_file(const char *ext, const char *filename, int output_fd);
>> bool dso__needs_decompress(struct dso *dso);
>>
>> @@ -228,11 +228,13 @@ struct kmod_path {
>> };
>>
>> int __kmod_path__parse(struct kmod_path *m, const char *path,
>> - bool alloc_name, bool alloc_ext);
>> + int cpumode, bool alloc_name, bool alloc_ext);
>>
>> -#define kmod_path__parse(__m, __p) __kmod_path__parse(__m, __p, false, false)
>> -#define kmod_path__parse_name(__m, __p) __kmod_path__parse(__m, __p, true , false)
>> -#define kmod_path__parse_ext(__m, __p) __kmod_path__parse(__m, __p, false, true)
>> +#define kmod_path__parse(__m, __p, __c) __kmod_path__parse(__m, __p, __c, false, false)
>> +#define kmod_path__parse_name(__m, __p) __kmod_path__parse(__m, __p, \
>> + PERF_RECORD_MISC_CPUMODE_UNKNOWN, true , false)
>> +#define kmod_path__parse_ext(__m, __p) __kmod_path__parse(__m, __p, \
>> + PERF_RECORD_MISC_CPUMODE_UNKNOWN, false, true)
>>
>> /*
>> * The dso__data_* external interface provides following functions:
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index fb43215..d106e12 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -1266,7 +1266,7 @@ static int __event_process_build_id(struct build_id_event *bev,
>>
>> dso__set_build_id(dso, &bev->build_id);
>>
>> - if (!is_kernel_module(filename))
>> + if (!is_kernel_module(filename, misc))
>
> please pass either whole misc or just cpumode like you do below
> in machine__process_kernel_mmap_event
>
> however the is_kernel_module declaration says cpumode so
> I'd expect cpumode, not complete misc
>
>> dso->kernel = dso_type;
>>
>> build_id__sprintf(dso->build_id, sizeof(dso->build_id),
>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
>> index e45c8f3..e084943 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1109,7 +1109,9 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
>> struct dso *dso;
>>
>> list_for_each_entry(dso, &machine->kernel_dsos.head, node) {
>> - if (is_kernel_module(dso->long_name))
>> + if (is_kernel_module(dso->long_name,
>> + event->header.misc &
>> + PERF_RECORD_MISC_CPUMODE_MASK))
>> continue;
>>
>> kernel = dso;
>> --
>> 1.8.3.4
>>
next prev parent reply other threads:[~2015-04-10 1:57 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-03 5:56 [PATCH v2] perf: report/annotate: fix segfault problem Wang Nan
2015-04-03 6:48 ` Ingo Molnar
2015-04-03 8:47 ` [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs Wang Nan
2015-04-03 8:49 ` Ingo Molnar
2015-04-03 11:11 ` Jiri Olsa
2015-04-07 10:42 ` Adrian Hunter
2015-04-08 10:50 ` [PATCH v4] " Wang Nan
2015-04-03 9:07 ` [PATCH v2] perf: report/annotate: fix segfault problem Jiri Olsa
2015-04-03 10:57 ` Jiri Olsa
2015-04-06 12:52 ` Arnaldo Carvalho de Melo
2015-04-07 8:22 ` [PATCH v3 0/2] " Wang Nan
2015-04-07 8:22 ` [PATCH v3 1/2] perf: kmaps: enforce usage of kmaps to protect futher bugs Wang Nan
2015-04-08 15:10 ` [tip:perf/core] perf kmaps: Check kmaps to make code more robust tip-bot for Wang Nan
2015-04-07 8:22 ` [PATCH v3 2/2] perf: report/annotate: fix segfault problem Wang Nan
2015-04-07 15:13 ` Arnaldo Carvalho de Melo
2015-04-08 3:49 ` Wang Nan
2015-04-08 3:52 ` [PATCH v4 " Wang Nan
2015-04-08 13:59 ` Jiri Olsa
2015-04-09 7:05 ` Wang Nan
2015-04-09 11:40 ` Jiri Olsa
2015-04-09 11:52 ` Jiri Olsa
2015-04-10 1:57 ` Wang Nan [this message]
2015-04-10 2:49 ` Wang Nan
2015-04-10 3:53 ` [PATCH v5 " Wang Nan
2015-04-15 1:27 ` Wang Nan
2015-04-20 1:18 ` Wang Nan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55272E07.4010200@huawei.com \
--to=wangnan0@huawei.com \
--cc=acme@kernel.org \
--cc=jolsa@kernel.org \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lizefan@huawei.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=pi3orama@163.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).