Re: [PATCH] perf probe: Fix module probing with shortname

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
> > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> > >>After commit 3d39ac538629e4f00a6e1c38d46346f1b8e69505 ("perf machine:
> > >>No need to have two DSOs lists"), perf probe with module short name doesn't
> > >>work again. For example:
> > >>
> > >>  # lsmod | grep e1000e
> > >>  e1000e                233472  0
> > >>
> > >>  # cat /proc/modules | grep e1000e
> > >>  e1000e 233472 0 - Live 0xffffffffa0073000
> > >>
> > >>  # cat /proc/kallsyms | grep '\<e1000e_up\>'
> > >>  ffffffffa0093860 t e1000e_up[e1000e]
> > >>
> > >>  # perf probe -v -m e1000e --add e1000e_up
> > >>  probe-definition(0): e1000e_up
> > >>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
> > >>  0 arguments
> > >>  Failed to find module e1000e.
> > >>  Could not open debuginfo. Try to use symbols.
> > >>  Looking at the vmlinux_path (7 entries long)
> > >>  Using /lib/modules/4.2.0-rc7+/build/vmlinux for symbols
> > >>  e1000e_up is out of .text, skip it.
> > >>    Error: Failed to add events. Reason: No such file or directory (Code: -2)
> > >>
> > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> > >>iff dso is vmlinux.
> > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> > >check that...
> > 
> > I also noticed this problem when I working on commit
> > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
> > kernel module names in '[]' correctly") ;)
> 
> Thanks for working on this, it is an area that needs cleaning up, too
> many ways to say what a dso is, will study your findings and try to come
> up with a patch proposal tomorrow.
> 
> - Arnaldo
>  
> > It should be bug, but I think fixing it is costy. Here's an
> > assumption that, if dso->kernel
> > is not zero, the dso should be vmlinux (not kernel module):
> > 
> > $ grep 'dso.>kernel)' ./tools/perf/ -r
> > ./tools/perf/builtin-inject.c:    if (dso->kernel)
> > ./tools/perf/util/symbol.c:    if (dso->kernel) {
> > ./tools/perf/util/symbol-elf.c:        if (dso->kernel)
> > ./tools/perf/util/symbol-elf.c:                if (remap_kernel &&
> > dso->kernel) {
> > ./tools/perf/util/event.c:        if (pos->dso->kernel)
> > ./tools/perf/util/probe-event.c:            if (dso->kernel)
> > ./tools/perf/util/map.c: * map->dso->kernel) before calling
> > __map__is_{kernel,kmodule}())
> > ./tools/perf/util/map.c:    if (!map->dso || !map->dso->kernel) {
> > ./tools/perf/builtin-top.c:    if (!map->dso->kernel)
> > 
> > So care must be taken.

So, yes, there are multiple uses for this dso->kernel thing, we need to
look at each one and go on clarifying it so that this gets corrected and
sane, but I think we need some helpers to clarify all this, namely:

	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
dso->kernel with it when loading host and guest kernel modules and
adding:

	bool dso__is_host_kernel(struct dso *dso)
	bool dso__is_host_kmodule(struct dso *dso)

	bool dso__is_guest_kernel(struct dso *dso)
	bool dso__is_guest_kmodule(struct dso *dso)

	And then these:

	static inline bool dso__is_host_kernel_level(struct dso *dso)
	{
		return dso__is_host_kernel(dso) || dso__is_host_kmodule(module);
	}

	static inline bool dso__is_guest_kernel_level(struct dso *dso)
	{
		return dso__is_guest_kernel(dso) || dso__is_guest_kmodule(module);
	}

	static inline bool dso__is_kernel_level(struct dso *dso)
	{
		return dso__is_host_kernel_level(dso) || dso__is_guest_kernel_level(module);
	}

	static inline bool dso__is_kmodule(struct dso *dso)
	{
		return  dso__is_host_kmodule(dso) || dso__is_guest_kmodule(dso);
	}

	static inline bool dso__is_kernel(struct dso *dso)
	{
		return dso__is_host_kernel(dso) || dso__is_guest_kernel(module);
	}

Then, looking at where dso->kernel is used now, we use:

tools/perf/builtin-inject.c

  static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
                                  struct machine *machine)
  {
          if (dso->kernel)
                misc = PERF_RECORD_MISC_KERNEL;

   Should use dso__is_host_kernel_level()

   And then there is a bug here, where we need to check for
dso__is_guest_kernel_level() as well and if that is true, set misc to
PERF_RECORD_MISC_GUEST_KERNEL, ditto for PERF_RECORD_MISC_GUEST_USER.

---------------------------------------------------------

  tools/perf/builtin-top.c:    if (!map->dso->kernel)

  static int symbol_filter(struct map *map, struct symbol *sym)
  {
          const char *name = sym->name;
                                               
          if (!map->dso->kernel)
                  return 0;

  Should use dso__is_kernel(), this is an old filter function dealing
only with vmlinux symbol names.

---------------------------------------------------------

  tools/perf/ui/browsers/hists.c

        if (asprintf(optstr, "Zoom %s %s DSO",
                     browser->hists->dso_filter ? "out of" : "into",
                     dso->kernel ? "the Kernel" : dso->short_name) < 0)

  Should use dso__is_host_kernel() and probably should support the guest
cases, with "Guest kernel" and a "Guest module " prefix for guest kernel
modules.

---------------------------------------------------------

int perf_event__synthesize_modules(struct perf_tool *tool,
                                   perf_event__handler_t process,
                                   struct machine *machine)

        for (pos = maps__first(maps); pos; pos = map__next(pos)) {
                size_t size;

                if (pos->dso->kernel)
                        continue;

This one is ok for non guest stuff, but should be clarified, and guests
should be supported, do that by using:

	if (!dso__is_kmodule(dso))
		continue;

---------------------------------------------------------

tools/perf/util/map.c

struct map *map__new2(u64 start, struct dso *dso, enum map_type type)
{
        struct map *map = calloc(1, (sizeof(*map) +
                                     (dso->kernel ? sizeof(struct kmap) : 0)));

Should use dso__is_kernel(dso)

---------------------------------------------------------
tools/perf/util/map.c

struct kmap *map__kmap(struct map *map)
{
        if (!map->dso || !map->dso->kernel) {
                pr_err("Internal error: map__kmap with a non-kernel map\n");

Same thing as above with map__new2()

---------------------------------------------------------

tools/perf/util/machine.c

machine__process_kernel_mmap_event()

                        if (!dso->kernel ||
                            is_kernel_module(dso->long_name,
                                             PERF_RECORD_MISC_CPUMODE_UNKNOWN))

	Should use:

	dso__is_kmodule()

	And remove that long comment :-)

---------------------------------------------------------
struct dso *machine__findnew_kernel(struct machine *machine, const char *name,
                                    const char *short_name, int dso_type)
{
        if (dso != NULL) {
                dso__set_short_name(dso, short_name, false);
                dso->kernel = dso_type;


This one is ok as it is, checked its callers.

---------------------------------------------------------

tools/perf/util/probe-event.c

static int kernel_get_module_dso(const char *module, struct dso **pdso)
{
        if (module) {
                list_for_each_entry(dso, &host_machine->dsos.head, node) {
                        if (!dso->kernel)
                                continue;


        Should use dso__is_kmodule(dso)

---------------------------------------------------------

Looking at the others after lunch, will try to add these step by step so
that we can bisect any thing we miss. But this has to be cleaned
out/clarified, got out of hand.

- Arnaldo
   
> > Another solution seems simpler: we can redefine the meaning of enum
> > dso_kernel_type like this:
> > 
> > # find  ./tools/perf/ -type f | xargs -n1 sed -i
> > 's/DSO_TYPE_USER/DSO_TYPE_NOT_VMLINUX/g'
> > # find  ./tools/perf/ -type f | xargs -n1 sed -i
> > 's/DSO_TYPE_KERNEL/DSO_TYPE_VMLINUX/g'
> > # find  ./tools/perf/ -type f | xargs -n1 sed -i
> > 's/DSO_TYPE_GUEST_KERNEL/DSO_TYPE_GUEST_VMLINUX/g'
> > 
> > By fixing the name of DSO_TYPE_USER, kernel module with
> > DSO_TYPE_NOT_VMLINUX seems
> > not so buggy. (Please choose a better name...)
> > 
> > What's your opinion?
> > 
> > Thank you.
> > 
> > >- Arnaldo
> > >
> > >>This patch fix 'perf probe -m' with an ad-hoc way.
> > >>
> > >>After this patch:
> > >>
> > >>  # perf probe -v -m e1000e --add e1000e_up
> > >>  probe-definition(0): e1000e_up
> > >>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
> > >>  0 arguments
> > >>  Open Debuginfo file: /lib/modules/4.2.0-rc7+/kernel/drivers/net/ethernet/intel/e1000e/e1000e.ko
> > >>  Try to find probe point from debuginfo.
> > >>  Matched function: e1000e_up
> > >>  Probe point found: e1000e_up+0
> > >>  Found 1 probe_trace_events.
> > >>  Opening /sys/kernel/debug/tracing//kprobe_events write=1
> > >>  Writing event: p:probe/e1000e_up e1000e:e1000e_up+0
> > >>  Added new event:
> > >>    probe:e1000e_up      (on e1000e_up in e1000e)
> > >>
> > >>  You can now use it in all perf tools, such as:
> > >>
> > >>  	perf record -e probe:e1000e_up -aR sleep 1
> > >>
> > >>  # perf probe -l
> > >>  Failed to find debug information for address ffffffffa0093860
> > >>    probe:e1000e_up      (on e1000e_up in e1000e)
> > >>
> > >>Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > >>Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > >>Cc: Namhyung Kim <namhyung@kernel.org>
> > >>Cc: Jiri Olsa <jolsa@redhat.com>
> > >>Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > >>---
> > >>
> > >>I think there may be other places where dso->kernel is misused.
> > >>machine__process_kernel_mmap_event() may be one of them. If I understand
> > >>correctly, 'dso->kernel && is_kernel_module(dso->long_name)' should always
> > >>false theoretically. However, I don't have enough time to check whether that
> > >>code really cause problem.
> > >>
> > >>---
> > >>  tools/perf/util/probe-event.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >>diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > >>index 2b78e8f..c7d6d3d 100644
> > >>--- a/tools/perf/util/probe-event.c
> > >>+++ b/tools/perf/util/probe-event.c
> > >>@@ -270,7 +270,7 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> > >>  	if (module) {
> > >>  		list_for_each_entry(dso, &host_machine->dsos.head, node) {
> > >>-			if (!dso->kernel)
> > >>+			if (dso->kernel)
> > >>  				continue;
> > >>  			if (strncmp(dso->short_name + 1, module,
> > >>  				    dso->short_name_len - 2) == 0)
> > >>-- 
> > >>1.8.3.4
> >