From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755205AbbDILws (ORCPT ); Thu, 9 Apr 2015 07:52:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38860 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755012AbbDILwp (ORCPT ); Thu, 9 Apr 2015 07:52:45 -0400 Date: Thu, 9 Apr 2015 13:52:31 +0200 From: Jiri Olsa To: Wang Nan 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. Message-ID: <20150409115231.GC1321@krava.brq.redhat.com> References: <20150407151322.GK11983@kernel.org> <1428465138-124112-1-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428465138-124112-1-git-send-email-wangnan0@huawei.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 >