[PATCH 0/2] powerpc: kretprobe updates

[PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

I am posting the powerpc bits in the same thread so as to keep these
changes together. I am not sure how this should be taken upstream as
there are atleast three different trees involved: one for the core
kprobes infrastructure, one for powerpc and one for perf.

Thanks,
Naveen

Naveen N. Rao (2):
  powerpc: kretprobes: override default function entry offset
  perf: powerpc: choose LEP with kretprobes

 arch/powerpc/kernel/kprobes.c               | 9 +++++++++
 tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH 1/2] powerpc: kretprobes: override default function entry offset

[Naveen N. Rao]

With ABIv2, we offset 8 bytes into a function to get at the local entry
point.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index fce05a38851c..331751701fed 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -131,6 +131,15 @@ static void __kprobes set_current_kprobe(struct kprobe *p, struct pt_regs *regs,
 	kcb->kprobe_saved_msr = regs->msr;
 }
 
+bool arch_function_offset_within_entry(unsigned long offset)
+{
+#ifdef PPC64_ELF_ABI_v2
+	return offset <= 8;
+#else
+	return !offset;
+#endif
+}
+
 void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
-- 
2.11.0

[PATCH 2/2] perf: powerpc: choose LEP with kretprobes

[Naveen N. Rao]

perf now uses an offset from _text/_stext for kretprobes, rather than
the actual function name. As such, let's choose the LEP for powerpc
ABIv2 so as to ensure the probe gets hit.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 1030a6e504bb..9fe0f20aa56f 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -79,11 +79,8 @@ void arch__fix_tev_from_maps(struct perf_probe_event *pev,
 	 * However, if the user specifies an offset, we fall back to using the
 	 * GEP since all userspace applications (objdump/readelf) show function
 	 * disassembly with offsets from the GEP.
-	 *
-	 * In addition, we shouldn't specify an offset for kretprobes.
 	 */
-	if (pev->point.offset || (!pev->uprobes && pev->point.retprobe) ||
-	    !map || !sym)
+	if (pev->point.offset || !map || !sym)
 		return;
 
 	lep_offset = PPC64_LOCAL_ENTRY_OFFSET(sym->arch_sym);
-- 
2.11.0

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

On Thu, 16 Feb 2017 13:47:37 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> I am posting the powerpc bits in the same thread so as to keep these
> changes together. I am not sure how this should be taken upstream as
> there are atleast three different trees involved: one for the core
> kprobes infrastructure, one for powerpc and one for perf.

Hmm, could you make these (and other related) patches and
other series in one series? Or wait for the other series
are merged correctly.

Thank you,

> 
> Thanks,
> Naveen
> 
> Naveen N. Rao (2):
>   powerpc: kretprobes: override default function entry offset
>   perf: powerpc: choose LEP with kretprobes
> 
>  arch/powerpc/kernel/kprobes.c               | 9 +++++++++
>  tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/2] powerpc: kretprobe updates

[Arnaldo Carvalho de Melo]

Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> On Thu, 16 Feb 2017 13:47:37 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > I am posting the powerpc bits in the same thread so as to keep these
> > changes together. I am not sure how this should be taken upstream as
> > there are atleast three different trees involved: one for the core
> > kprobes infrastructure, one for powerpc and one for perf.

> Hmm, could you make these (and other related) patches and
> other series in one series? Or wait for the other series
> are merged correctly.

Well, patches like these should be done in a way that the tooling parts
can deal with kernels with or without the kernel changes, so that older
tools work with new kernels and new tools work with older kernels.

"work" as in the previous behaviour is kept when a new tool deals with
an older kernel and an older tool would warn the user that what it needs
is not present in that kernel.

Is this the case? I just looked briefly at the patch commit logs.

If it is, then I can pick the tool ones, and the others can be submitted
to the relevant trees, at some point all will be in, kernels eventually
gets updated everywhere, ditto for the tooling, all gets well.

Regards,

- Arnaldo


 
> Thank you,
> 
> > 
> > Thanks,
> > Naveen
> > 
> > Naveen N. Rao (2):
> >   powerpc: kretprobes: override default function entry offset
> >   perf: powerpc: choose LEP with kretprobes
> > 
> >  arch/powerpc/kernel/kprobes.c               | 9 +++++++++
> >  tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 2.11.0
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

PowerMac G5 Quad Strage lspci

[luigi burdo]

[-- Attachment #1: Type: text/plain, Size: 2055 bytes --]

Hi all devs,

from 4.10 i have on G5 Quad this strange lspci

with this 0001:01:01.0 Non-VGA unclassified device: Device 0800:0002 (rev 08)

some one know if it a bug or a new feature of the kernel.


Thanks!


My complete

lspci
0000:00:0b.0 PCI bridge: Apple Inc. CPC945 PCIe Bridge
0000:0a:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Whistler LE [Radeon HD 6610M/7610M]
0000:0a:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Turks HDMI Audio [Radeon HD 6500/6600 / 6700M Series]
0001:00:00.0 Host bridge: Apple Inc. U4 HT Bridge
0001:00:01.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-X bridge (rev a3)
0001:00:02.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-X bridge (rev a3)
0001:00:03.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:04.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:05.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:06.0 PCI bridge: Broadcom BCM5780 [HT2000] PCI-Express Bridge (rev a3)
0001:00:07.0 PCI bridge: Apple Inc. Shasta PCI Bridge
0001:00:08.0 PCI bridge: Apple Inc. Shasta PCI Bridge
0001:00:09.0 PCI bridge: Apple Inc. Shasta PCI Bridge
0001:01:01.0 Non-VGA unclassified device: Device 0800:0002 (rev 08)
0001:01:07.0 Unassigned class [ff00]: Apple Inc. Shasta Mac I/O
0001:01:0b.0 USB controller: NEC Corporation OHCI USB Controller (rev 43)
0001:01:0b.1 USB controller: NEC Corporation OHCI USB Controller (rev 43)
0001:01:0b.2 USB controller: NEC Corporation uPD72010x USB 2.0 Controller (rev 04)
0001:03:0c.0 IDE interface: Broadcom K2 SATA
0001:03:0d.0 Unassigned class [ff00]: Apple Inc. Shasta IDE
0001:03:0e.0 FireWire (IEEE 1394): Apple Inc. Shasta Firewire
0001:05:04.0 Ethernet controller: Broadcom Limited NetXtreme BCM5780 Gigabit Ethernet (rev 03)
0001:05:04.1 Ethernet controller: Broadcom Limited NetXtreme BCM5780 Gigabit Ethernet (rev 03)
0001:06:00.0 VGA compatible controller: NVIDIA Corporation G70GL [Quadro FX 4500] (rev a1)

Luigi


[-- Attachment #2: Type: text/html, Size: 2656 bytes --]

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

On Fri, 17 Feb 2017 17:42:54 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > On Thu, 16 Feb 2017 13:47:37 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > I am posting the powerpc bits in the same thread so as to keep these
> > > changes together. I am not sure how this should be taken upstream as
> > > there are atleast three different trees involved: one for the core
> > > kprobes infrastructure, one for powerpc and one for perf.
> 
> > Hmm, could you make these (and other related) patches and
> > other series in one series? Or wait for the other series
> > are merged correctly.
> 
> Well, patches like these should be done in a way that the tooling parts
> can deal with kernels with or without the kernel changes, so that older
> tools work with new kernels and new tools work with older kernels.
> 
> "work" as in the previous behaviour is kept when a new tool deals with
> an older kernel and an older tool would warn the user that what it needs
> is not present in that kernel.
> 
> Is this the case? I just looked briefly at the patch commit logs.

Thanks Arnaldo,

Naveen, I think this one and your previous series are incompatible
with older kernel. So those should be merged in one series and
at least (1) update ftrace's README special file to show explicitly
which can accept text+offset style for kretprobes, and (2) update
perf probe side to ensure that (and fallback to previous logic if not).

Thank you,

> 
> If it is, then I can pick the tool ones, and the others can be submitted
> to the relevant trees, at some point all will be in, kernels eventually
> gets updated everywhere, ditto for the tooling, all gets well.
> 
> Regards,
> 
> - Arnaldo
> 
> 
>  
> > Thank you,
> > 
> > > 
> > > Thanks,
> > > Naveen
> > > 
> > > Naveen N. Rao (2):
> > >   powerpc: kretprobes: override default function entry offset
> > >   perf: powerpc: choose LEP with kretprobes
> > > 
> > >  arch/powerpc/kernel/kprobes.c               | 9 +++++++++
> > >  tools/perf/arch/powerpc/util/sym-handling.c | 5 +----
> > >  2 files changed, 10 insertions(+), 4 deletions(-)
> > > 
> > > -- 
> > > 2.11.0
> > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

On 2017/02/17 05:42PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > On Thu, 16 Feb 2017 13:47:37 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > I am posting the powerpc bits in the same thread so as to keep these
> > > changes together. I am not sure how this should be taken upstream as
> > > there are atleast three different trees involved: one for the core
> > > kprobes infrastructure, one for powerpc and one for perf.
> 
> > Hmm, could you make these (and other related) patches and
> > other series in one series? Or wait for the other series
> > are merged correctly.
> 
> Well, patches like these should be done in a way that the tooling parts
> can deal with kernels with or without the kernel changes, so that older
> tools work with new kernels and new tools work with older kernels.
> 
> "work" as in the previous behaviour is kept when a new tool deals with
> an older kernel and an older tool would warn the user that what it needs
> is not present in that kernel.
> 
> Is this the case? I just looked briefly at the patch commit logs.

Thanks, that makes sense.

All the kernel patches here can go in as they are about removing the 
restrictions around use of kretprobes with kprobe_event, except for the 
first patch which hardens use of kretprobes in general by validating 
addresses. None of that should cause issues with the existing tools.

> 
> If it is, then I can pick the tool ones, and the others can be submitted
> to the relevant trees, at some point all will be in, kernels eventually
> gets updated everywhere, ditto for the tooling, all gets well.

Older perf tools with newer kernels will be fine. However, with the 
current perf patches, newer perf will fail with older kernels. I will 
redo the perf patches to address this. So, please don't merge the perf 
bits in this series.


Regards,
Naveen

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

On 2017/02/19 01:42PM, Masami Hiramatsu wrote:
> On Fri, 17 Feb 2017 17:42:54 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > > On Thu, 16 Feb 2017 13:47:37 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > 
> > > > I am posting the powerpc bits in the same thread so as to keep these
> > > > changes together. I am not sure how this should be taken upstream as
> > > > there are atleast three different trees involved: one for the core
> > > > kprobes infrastructure, one for powerpc and one for perf.
> > 
> > > Hmm, could you make these (and other related) patches and
> > > other series in one series? Or wait for the other series
> > > are merged correctly.
> > 
> > Well, patches like these should be done in a way that the tooling parts
> > can deal with kernels with or without the kernel changes, so that older
> > tools work with new kernels and new tools work with older kernels.
> > 
> > "work" as in the previous behaviour is kept when a new tool deals with
> > an older kernel and an older tool would warn the user that what it needs
> > is not present in that kernel.
> > 
> > Is this the case? I just looked briefly at the patch commit logs.
> 
> Thanks Arnaldo,
> 
> Naveen, I think this one and your previous series are incompatible
> with older kernel. So those should be merged in one series and
> at least (1) update ftrace's README special file to show explicitly
> which can accept text+offset style for kretprobes, and 

Sure - do you mean Documentation/trace/kprobetrace.txt? And, do you want 
me to include kernel version where this changed?

> (2) update
> perf probe side to ensure that (and fallback to previous logic if not).

Sure. I am trying out an approach and will post it as soon as it's 
ready.

Thanks!
- Naveen

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

On 2017/02/17 05:42PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > On Thu, 16 Feb 2017 13:47:37 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > 
> > > I am posting the powerpc bits in the same thread so as to keep these
> > > changes together. I am not sure how this should be taken upstream as
> > > there are atleast three different trees involved: one for the core
> > > kprobes infrastructure, one for powerpc and one for perf.
> 
> > Hmm, could you make these (and other related) patches and
> > other series in one series? Or wait for the other series
> > are merged correctly.
> 
> Well, patches like these should be done in a way that the tooling parts
> can deal with kernels with or without the kernel changes, so that older
> tools work with new kernels and new tools work with older kernels.

Does the below work?

The idea is to just prefer the real function names when probing 
functions that do not have duplicate names. Offset from _text only if we 
detect that a function name has multiple entries. In this manner,
existing perf will continue to work with newer kernels and vice-versa.

Before this patch:

naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
p:probe/_do_fork _text+857288
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
r:probe/_do_fork _text+857288
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
p:probe/read_mem _text+10019704
p:probe/read_mem_1 _text+6228424
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
r:probe/read_mem _text+10019704
r:probe/read_mem_1 _text+6228424


With the below patch (lightly tested):

naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
p:probe/_do_fork _do_fork+8
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
r:probe/_do_fork _do_fork+8
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
p:probe/read_mem _text+10019704
p:probe/read_mem_1 _text+6228424
naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
r:probe/read_mem _text+10019704
r:probe/read_mem_1 _text+6228424

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/util/machine.h     |  7 +++++
 tools/perf/util/map.c         | 41 +++++++++++++++++++++++++++++
 tools/perf/util/map.h         | 13 ++++++++++
 tools/perf/util/probe-event.c | 18 +++++++++++++
 tools/perf/util/symbol.c      | 60 +++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/symbol.h      |  2 ++
 6 files changed, 141 insertions(+)

diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index 354de6e56109..277ffb1e0896 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -203,6 +203,13 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine,
 	return map_groups__find_function_by_name(&machine->kmaps, name, mapp);
 }
 
+static inline
+unsigned int machine__find_kernel_function_count_by_name(struct machine *machine,
+						     const char *name)
+{
+	return map_groups__find_function_count_by_name(&machine->kmaps, name);
+}
+
 struct map *machine__findnew_module_map(struct machine *machine, u64 start,
 					const char *filename);
 int arch__fix_module_text_start(u64 *start, const char *name);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 4f9a71c63026..b75b35dc54ad 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -349,6 +349,17 @@ struct symbol *map__find_symbol_by_name(struct map *map, const char *name)
 	return dso__find_symbol_by_name(map->dso, map->type, name);
 }
 
+unsigned int map__find_symbol_count_by_name(struct map *map, const char *name)
+{
+	if (map__load(map) < 0)
+		return 0;
+
+	if (!dso__sorted_by_name(map->dso, map->type))
+		dso__sort_by_name(map->dso, map->type);
+
+	return dso__find_symbol_count_by_name(map->dso, map->type, name);
+}
+
 struct map *map__clone(struct map *from)
 {
 	struct map *map = memdup(from, sizeof(*map));
@@ -593,6 +604,29 @@ struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
 	return sym;
 }
 
+unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name)
+{
+	struct symbol *sym;
+	struct rb_node *nd;
+	unsigned int count = 0;
+
+	pthread_rwlock_rdlock(&maps->lock);
+
+	for (nd = rb_first(&maps->entries); nd; nd = rb_next(nd)) {
+		struct map *pos = rb_entry(nd, struct map, rb_node);
+
+		sym = map__find_symbol_by_name(pos, name);
+
+		if (sym != NULL) {
+			count = map__find_symbol_count_by_name(pos, name);
+			break;
+		}
+	}
+
+	pthread_rwlock_unlock(&maps->lock);
+	return count;
+}
+
 struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
 					       enum map_type type,
 					       const char *name,
@@ -603,6 +637,13 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
 	return sym;
 }
 
+unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
+					       enum map_type type,
+					       const char *name)
+{
+	return maps__find_symbol_count_by_name(&mg->maps[type], name);
+}
+
 int map_groups__find_ams(struct addr_map_symbol *ams)
 {
 	if (ams->addr < ams->map->start || ams->addr >= ams->map->end) {
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index abdacf800c98..ecf995f033ed 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -173,6 +173,7 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
 int map__load(struct map *map);
 struct symbol *map__find_symbol(struct map *map, u64 addr);
 struct symbol *map__find_symbol_by_name(struct map *map, const char *name);
+unsigned int map__find_symbol_count_by_name(struct map *map, const char *name);
 void map__fixup_start(struct map *map);
 void map__fixup_end(struct map *map);
 
@@ -187,6 +188,7 @@ struct map *maps__first(struct maps *maps);
 struct map *map__next(struct map *map);
 struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
                                          struct map **mapp);
+unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name);
 void map_groups__init(struct map_groups *mg, struct machine *machine);
 void map_groups__exit(struct map_groups *mg);
 int map_groups__clone(struct thread *thread,
@@ -233,6 +235,10 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
 					       const char *name,
 					       struct map **mapp);
 
+unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
+					       enum map_type type,
+					       const char *name);
+
 struct addr_map_symbol;
 
 int map_groups__find_ams(struct addr_map_symbol *ams);
@@ -244,6 +250,13 @@ struct symbol *map_groups__find_function_by_name(struct map_groups *mg,
 	return map_groups__find_symbol_by_name(mg, MAP__FUNCTION, name, mapp);
 }
 
+static inline
+unsigned int map_groups__find_function_count_by_name(struct map_groups *mg,
+						 const char *name)
+{
+	return map_groups__find_symbol_count_by_name(mg, MAP__FUNCTION, name);
+}
+
 int map_groups__fixup_overlappings(struct map_groups *mg, struct map *map,
 				   FILE *fp);
 
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index fa7f81af11e8..011e938a0a84 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -113,6 +113,11 @@ static struct symbol *__find_kernel_function_by_name(const char *name,
 	return machine__find_kernel_function_by_name(host_machine, name, mapp);
 }
 
+static unsigned int __find_kernel_function_count_by_name(const char *name)
+{
+	return machine__find_kernel_function_count_by_name(host_machine, name);
+}
+
 static struct symbol *__find_kernel_function(u64 addr, struct map **mapp)
 {
 	return machine__find_kernel_function(host_machine, addr, mapp);
@@ -155,6 +160,11 @@ static int kernel_get_symbol_address_by_name(const char *name, u64 *addr,
 	return 0;
 }
 
+static unsigned int kernel_get_symbol_count_by_name(const char *name)
+{
+	return __find_kernel_function_count_by_name(name);
+}
+
 static struct map *kernel_get_module_map(const char *module)
 {
 	struct map_groups *grp = &host_machine->kmaps;
@@ -259,6 +269,11 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
 	return true;
 }
 
+static bool kprobe_is_duplicate_symbol(const char *symbol)
+{
+	return kernel_get_symbol_count_by_name(symbol) > 1;
+}
+
 /*
  * @module can be module name of module file path. In case of path,
  * inspect elf and find out what is actual module name.
@@ -759,6 +774,9 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
 	for (i = 0; i < ntevs; i++) {
 		if (!tevs[i].point.address)
 			continue;
+		if (!kprobe_is_duplicate_symbol(tevs[i].point.symbol))
+			continue;
+
 		/* If we found a wrong one, mark it by NULL symbol */
 		if (kprobe_warn_out_range(tevs[i].point.symbol,
 					  tevs[i].point.address)) {
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index dc93940de351..ca4a11a92c6f 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -438,6 +438,60 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 	return &s->sym;
 }
 
+static unsigned int symbols__find_count_by_name(struct rb_root *symbols,
+					    const char *name)
+{
+	struct rb_node *n, *m;
+	struct symbol_name_rb_node *s = NULL;
+	unsigned int count = 0;
+
+	if (symbols == NULL)
+		return 0;
+
+	n = symbols->rb_node;
+
+	while (n) {
+		int cmp;
+
+		s = rb_entry(n, struct symbol_name_rb_node, rb_node);
+		cmp = arch__compare_symbol_names(name, s->sym.name);
+
+		if (cmp < 0)
+			n = n->rb_left;
+		else if (cmp > 0)
+			n = n->rb_right;
+		else
+			break;
+	}
+
+	if (n == NULL)
+		return 0;
+
+	count++;
+
+	for (m = rb_prev(n); m; m = rb_prev(m)) {
+		struct symbol_name_rb_node *tmp;
+
+		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
+		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
+			break;
+
+		count++;
+	}
+
+	for (m = rb_next(n); m; m = rb_next(m)) {
+		struct symbol_name_rb_node *tmp;
+
+		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
+		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
+			break;
+
+		count++;
+	}
+
+	return count;
+}
+
 void dso__reset_find_symbol_cache(struct dso *dso)
 {
 	enum map_type type;
@@ -503,6 +557,12 @@ struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
 	return symbols__find_by_name(&dso->symbol_names[type], name);
 }
 
+unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
+					const char *name)
+{
+	return symbols__find_count_by_name(&dso->symbol_names[type], name);
+}
+
 void dso__sort_by_name(struct dso *dso, enum map_type type)
 {
 	dso__set_sorted_by_name(dso, type);
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 6c358b7ed336..c4bd1b035a9b 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -260,6 +260,8 @@ struct symbol *dso__find_symbol(struct dso *dso, enum map_type type,
 				u64 addr);
 struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
 					const char *name);
+unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
+					const char *name);
 struct symbol *symbol__next_by_name(struct symbol *sym);
 
 struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
-- 
2.11.0

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

On Mon, 20 Feb 2017 17:13:05 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/02/17 05:42PM, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > > On Thu, 16 Feb 2017 13:47:37 +0530
> > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > 
> > > > I am posting the powerpc bits in the same thread so as to keep these
> > > > changes together. I am not sure how this should be taken upstream as
> > > > there are atleast three different trees involved: one for the core
> > > > kprobes infrastructure, one for powerpc and one for perf.
> > 
> > > Hmm, could you make these (and other related) patches and
> > > other series in one series? Or wait for the other series
> > > are merged correctly.
> > 
> > Well, patches like these should be done in a way that the tooling parts
> > can deal with kernels with or without the kernel changes, so that older
> > tools work with new kernels and new tools work with older kernels.
> 
> Does the below work?

Sorry, no that is not what we meant. Please see my previous reply.

> 
> The idea is to just prefer the real function names when probing 
> functions that do not have duplicate names. Offset from _text only if we 
> detect that a function name has multiple entries. In this manner,
> existing perf will continue to work with newer kernels and vice-versa.

Even with this, the latter will fail (instead of putting rprobe on the
first symbol) on older kernel. Instead, we'll check the ftrace's README
on current kernel and if it supports sym+offs style on retprobe,
we'll use _text+offs event definition. If not, we continue to use
older style.

Thank you,

> 
> Before this patch:
> 
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
> p:probe/_do_fork _text+857288
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
> r:probe/_do_fork _text+857288
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
> p:probe/read_mem _text+10019704
> p:probe/read_mem_1 _text+6228424
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
> r:probe/read_mem _text+10019704
> r:probe/read_mem_1 _text+6228424
> 
> 
> With the below patch (lightly tested):
> 
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork
> p:probe/_do_fork _do_fork+8
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D _do_fork%return
> r:probe/_do_fork _do_fork+8
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem
> p:probe/read_mem _text+10019704
> p:probe/read_mem_1 _text+6228424
> naveen@ubuntu:~/linux/tools/perf$ sudo ./perf probe -D read_mem%return
> r:probe/read_mem _text+10019704
> r:probe/read_mem_1 _text+6228424
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  tools/perf/util/machine.h     |  7 +++++
>  tools/perf/util/map.c         | 41 +++++++++++++++++++++++++++++
>  tools/perf/util/map.h         | 13 ++++++++++
>  tools/perf/util/probe-event.c | 18 +++++++++++++
>  tools/perf/util/symbol.c      | 60 +++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/symbol.h      |  2 ++
>  6 files changed, 141 insertions(+)
> 
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index 354de6e56109..277ffb1e0896 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -203,6 +203,13 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine,
>  	return map_groups__find_function_by_name(&machine->kmaps, name, mapp);
>  }
>  
> +static inline
> +unsigned int machine__find_kernel_function_count_by_name(struct machine *machine,
> +						     const char *name)
> +{
> +	return map_groups__find_function_count_by_name(&machine->kmaps, name);
> +}
> +
>  struct map *machine__findnew_module_map(struct machine *machine, u64 start,
>  					const char *filename);
>  int arch__fix_module_text_start(u64 *start, const char *name);
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 4f9a71c63026..b75b35dc54ad 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -349,6 +349,17 @@ struct symbol *map__find_symbol_by_name(struct map *map, const char *name)
>  	return dso__find_symbol_by_name(map->dso, map->type, name);
>  }
>  
> +unsigned int map__find_symbol_count_by_name(struct map *map, const char *name)
> +{
> +	if (map__load(map) < 0)
> +		return 0;
> +
> +	if (!dso__sorted_by_name(map->dso, map->type))
> +		dso__sort_by_name(map->dso, map->type);
> +
> +	return dso__find_symbol_count_by_name(map->dso, map->type, name);
> +}
> +
>  struct map *map__clone(struct map *from)
>  {
>  	struct map *map = memdup(from, sizeof(*map));
> @@ -593,6 +604,29 @@ struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
>  	return sym;
>  }
>  
> +unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name)
> +{
> +	struct symbol *sym;
> +	struct rb_node *nd;
> +	unsigned int count = 0;
> +
> +	pthread_rwlock_rdlock(&maps->lock);
> +
> +	for (nd = rb_first(&maps->entries); nd; nd = rb_next(nd)) {
> +		struct map *pos = rb_entry(nd, struct map, rb_node);
> +
> +		sym = map__find_symbol_by_name(pos, name);
> +
> +		if (sym != NULL) {
> +			count = map__find_symbol_count_by_name(pos, name);
> +			break;
> +		}
> +	}
> +
> +	pthread_rwlock_unlock(&maps->lock);
> +	return count;
> +}
> +
>  struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
>  					       enum map_type type,
>  					       const char *name,
> @@ -603,6 +637,13 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
>  	return sym;
>  }
>  
> +unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
> +					       enum map_type type,
> +					       const char *name)
> +{
> +	return maps__find_symbol_count_by_name(&mg->maps[type], name);
> +}
> +
>  int map_groups__find_ams(struct addr_map_symbol *ams)
>  {
>  	if (ams->addr < ams->map->start || ams->addr >= ams->map->end) {
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index abdacf800c98..ecf995f033ed 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -173,6 +173,7 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
>  int map__load(struct map *map);
>  struct symbol *map__find_symbol(struct map *map, u64 addr);
>  struct symbol *map__find_symbol_by_name(struct map *map, const char *name);
> +unsigned int map__find_symbol_count_by_name(struct map *map, const char *name);
>  void map__fixup_start(struct map *map);
>  void map__fixup_end(struct map *map);
>  
> @@ -187,6 +188,7 @@ struct map *maps__first(struct maps *maps);
>  struct map *map__next(struct map *map);
>  struct symbol *maps__find_symbol_by_name(struct maps *maps, const char *name,
>                                           struct map **mapp);
> +unsigned int maps__find_symbol_count_by_name(struct maps *maps, const char *name);
>  void map_groups__init(struct map_groups *mg, struct machine *machine);
>  void map_groups__exit(struct map_groups *mg);
>  int map_groups__clone(struct thread *thread,
> @@ -233,6 +235,10 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
>  					       const char *name,
>  					       struct map **mapp);
>  
> +unsigned int map_groups__find_symbol_count_by_name(struct map_groups *mg,
> +					       enum map_type type,
> +					       const char *name);
> +
>  struct addr_map_symbol;
>  
>  int map_groups__find_ams(struct addr_map_symbol *ams);
> @@ -244,6 +250,13 @@ struct symbol *map_groups__find_function_by_name(struct map_groups *mg,
>  	return map_groups__find_symbol_by_name(mg, MAP__FUNCTION, name, mapp);
>  }
>  
> +static inline
> +unsigned int map_groups__find_function_count_by_name(struct map_groups *mg,
> +						 const char *name)
> +{
> +	return map_groups__find_symbol_count_by_name(mg, MAP__FUNCTION, name);
> +}
> +
>  int map_groups__fixup_overlappings(struct map_groups *mg, struct map *map,
>  				   FILE *fp);
>  
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index fa7f81af11e8..011e938a0a84 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -113,6 +113,11 @@ static struct symbol *__find_kernel_function_by_name(const char *name,
>  	return machine__find_kernel_function_by_name(host_machine, name, mapp);
>  }
>  
> +static unsigned int __find_kernel_function_count_by_name(const char *name)
> +{
> +	return machine__find_kernel_function_count_by_name(host_machine, name);
> +}
> +
>  static struct symbol *__find_kernel_function(u64 addr, struct map **mapp)
>  {
>  	return machine__find_kernel_function(host_machine, addr, mapp);
> @@ -155,6 +160,11 @@ static int kernel_get_symbol_address_by_name(const char *name, u64 *addr,
>  	return 0;
>  }
>  
> +static unsigned int kernel_get_symbol_count_by_name(const char *name)
> +{
> +	return __find_kernel_function_count_by_name(name);
> +}
> +
>  static struct map *kernel_get_module_map(const char *module)
>  {
>  	struct map_groups *grp = &host_machine->kmaps;
> @@ -259,6 +269,11 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>  	return true;
>  }
>  
> +static bool kprobe_is_duplicate_symbol(const char *symbol)
> +{
> +	return kernel_get_symbol_count_by_name(symbol) > 1;
> +}
> +
>  /*
>   * @module can be module name of module file path. In case of path,
>   * inspect elf and find out what is actual module name.
> @@ -759,6 +774,9 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  	for (i = 0; i < ntevs; i++) {
>  		if (!tevs[i].point.address)
>  			continue;
> +		if (!kprobe_is_duplicate_symbol(tevs[i].point.symbol))
> +			continue;
> +
>  		/* If we found a wrong one, mark it by NULL symbol */
>  		if (kprobe_warn_out_range(tevs[i].point.symbol,
>  					  tevs[i].point.address)) {
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index dc93940de351..ca4a11a92c6f 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -438,6 +438,60 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>  	return &s->sym;
>  }
>  
> +static unsigned int symbols__find_count_by_name(struct rb_root *symbols,
> +					    const char *name)
> +{
> +	struct rb_node *n, *m;
> +	struct symbol_name_rb_node *s = NULL;
> +	unsigned int count = 0;
> +
> +	if (symbols == NULL)
> +		return 0;
> +
> +	n = symbols->rb_node;
> +
> +	while (n) {
> +		int cmp;
> +
> +		s = rb_entry(n, struct symbol_name_rb_node, rb_node);
> +		cmp = arch__compare_symbol_names(name, s->sym.name);
> +
> +		if (cmp < 0)
> +			n = n->rb_left;
> +		else if (cmp > 0)
> +			n = n->rb_right;
> +		else
> +			break;
> +	}
> +
> +	if (n == NULL)
> +		return 0;
> +
> +	count++;
> +
> +	for (m = rb_prev(n); m; m = rb_prev(m)) {
> +		struct symbol_name_rb_node *tmp;
> +
> +		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
> +		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> +			break;
> +
> +		count++;
> +	}
> +
> +	for (m = rb_next(n); m; m = rb_next(m)) {
> +		struct symbol_name_rb_node *tmp;
> +
> +		tmp = rb_entry(m, struct symbol_name_rb_node, rb_node);
> +		if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> +			break;
> +
> +		count++;
> +	}
> +
> +	return count;
> +}
> +
>  void dso__reset_find_symbol_cache(struct dso *dso)
>  {
>  	enum map_type type;
> @@ -503,6 +557,12 @@ struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
>  	return symbols__find_by_name(&dso->symbol_names[type], name);
>  }
>  
> +unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
> +					const char *name)
> +{
> +	return symbols__find_count_by_name(&dso->symbol_names[type], name);
> +}
> +
>  void dso__sort_by_name(struct dso *dso, enum map_type type)
>  {
>  	dso__set_sorted_by_name(dso, type);
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 6c358b7ed336..c4bd1b035a9b 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -260,6 +260,8 @@ struct symbol *dso__find_symbol(struct dso *dso, enum map_type type,
>  				u64 addr);
>  struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
>  					const char *name);
> +unsigned int dso__find_symbol_count_by_name(struct dso *dso, enum map_type type,
> +					const char *name);
>  struct symbol *symbol__next_by_name(struct symbol *sym);
>  
>  struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
> -- 
> 2.11.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/2] powerpc: kretprobe updates

[Masami Hiramatsu]

On Mon, 20 Feb 2017 15:20:24 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On 2017/02/19 01:42PM, Masami Hiramatsu wrote:
> > On Fri, 17 Feb 2017 17:42:54 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > 
> > > Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > > > On Thu, 16 Feb 2017 13:47:37 +0530
> > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > I am posting the powerpc bits in the same thread so as to keep these
> > > > > changes together. I am not sure how this should be taken upstream as
> > > > > there are atleast three different trees involved: one for the core
> > > > > kprobes infrastructure, one for powerpc and one for perf.
> > > 
> > > > Hmm, could you make these (and other related) patches and
> > > > other series in one series? Or wait for the other series
> > > > are merged correctly.
> > > 
> > > Well, patches like these should be done in a way that the tooling parts
> > > can deal with kernels with or without the kernel changes, so that older
> > > tools work with new kernels and new tools work with older kernels.
> > > 
> > > "work" as in the previous behaviour is kept when a new tool deals with
> > > an older kernel and an older tool would warn the user that what it needs
> > > is not present in that kernel.
> > > 
> > > Is this the case? I just looked briefly at the patch commit logs.
> > 
> > Thanks Arnaldo,
> > 
> > Naveen, I think this one and your previous series are incompatible
> > with older kernel. So those should be merged in one series and
> > at least (1) update ftrace's README special file to show explicitly
> > which can accept text+offset style for kretprobes, and 
> 
> Sure - do you mean Documentation/trace/kprobetrace.txt? And, do you want 
> me to include kernel version where this changed?

No, I meant /sys/kernel/debug/tracing/README. For some reasons, perf
probe already parse it in util/probe-file.c. 
Please see commit 180b20616ce57e93eb692170c793be94c456b1e2 and 
864256255597aad86abcecbe6c53da8852ded15b

Thank you,

> 
> > (2) update
> > perf probe side to ensure that (and fallback to previous logic if not).
> 
> Sure. I am trying out an approach and will post it as soon as it's 
> ready.
> 
> Thanks!
> - Naveen
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/2] powerpc: kretprobe updates

[Naveen N. Rao]

On 2017/02/21 10:07PM, Masami Hiramatsu wrote:
> On Mon, 20 Feb 2017 15:20:24 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
> > On 2017/02/19 01:42PM, Masami Hiramatsu wrote:
> > > On Fri, 17 Feb 2017 17:42:54 -0300
> > > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > 
> > > > Em Fri, Feb 17, 2017 at 07:44:33PM +0900, Masami Hiramatsu escreveu:
> > > > > On Thu, 16 Feb 2017 13:47:37 +0530
> > > > > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > I am posting the powerpc bits in the same thread so as to keep these
> > > > > > changes together. I am not sure how this should be taken upstream as
> > > > > > there are atleast three different trees involved: one for the core
> > > > > > kprobes infrastructure, one for powerpc and one for perf.
> > > > 
> > > > > Hmm, could you make these (and other related) patches and
> > > > > other series in one series? Or wait for the other series
> > > > > are merged correctly.
> > > > 
> > > > Well, patches like these should be done in a way that the tooling parts
> > > > can deal with kernels with or without the kernel changes, so that older
> > > > tools work with new kernels and new tools work with older kernels.
> > > > 
> > > > "work" as in the previous behaviour is kept when a new tool deals with
> > > > an older kernel and an older tool would warn the user that what it needs
> > > > is not present in that kernel.
> > > > 
> > > > Is this the case? I just looked briefly at the patch commit logs.
> > > 
> > > Thanks Arnaldo,
> > > 
> > > Naveen, I think this one and your previous series are incompatible
> > > with older kernel. So those should be merged in one series and
> > > at least (1) update ftrace's README special file to show explicitly
> > > which can accept text+offset style for kretprobes, and 
> > 
> > Sure - do you mean Documentation/trace/kprobetrace.txt? And, do you want 
> > me to include kernel version where this changed?
> 
> No, I meant /sys/kernel/debug/tracing/README. For some reasons, perf
> probe already parse it in util/probe-file.c. 
> Please see commit 180b20616ce57e93eb692170c793be94c456b1e2 and 
> 864256255597aad86abcecbe6c53da8852ded15b

Got it.

Thanks,
Naveen