From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754011AbbDJB5v (ORCPT ); Thu, 9 Apr 2015 21:57:51 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:22173 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751468AbbDJB5s (ORCPT ); Thu, 9 Apr 2015 21:57:48 -0400 Message-ID: <55272E07.4010200@huawei.com> Date: Fri, 10 Apr 2015 09:57:27 +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 X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020206.55272E1A.007C,ss=1,re=0.001,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: c6f85b0f2c6be1ec5a671e719eecee85 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>