Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753965AbbDJCtR (ORCPT ); Thu, 9 Apr 2015 22:49:17 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:29354 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752260AbbDJCtP (ORCPT ); Thu, 9 Apr 2015 22:49:15 -0400 Message-ID: <55273A1C.90704@huawei.com> Date: Fri, 10 Apr 2015 10:49:00 +0800 From: Wang Nan User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.0.1 MIME-Version: 1.0 To: Jiri Olsa CC: , , , , , , Subject: Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem. References: <20150407151322.GK11983@kernel.org> <1428465138-124112-1-git-send-email-wangnan0@huawei.com> <20150409115231.GC1321@krava.brq.redhat.com> In-Reply-To: <20150409115231.GC1321@krava.brq.redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.69.129] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5223 Lines: 147 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; >> >> 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 > misc = bev->header.misc & PERF_RECORD_MISC_CPUMODE_MASK; So misc is cpumode. I'll rename it to cpumode in v5 patch. > 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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/