On Sat, Aug 19, 2023 at 10:11:05AM +0900, Masami Hiramatsu wrote:

Hi Francis,
(Cc: Song Liu and BPF ML)

On Fri, 18 Aug 2023 20:12:11 +0200
Francis Laniel [off-list ref] wrote:

quoted

Hi.

Le vendredi 18 août 2023, 15:05:37 CEST Masami Hiramatsu a écrit :

quoted

On Thu, 17 Aug 2023 13:06:20 +0200

Francis Laniel [off-list ref] wrote:

quoted

Hi.

Le jeudi 17 août 2023, 09:50:57 CEST Masami Hiramatsu a écrit :

quoted

Hi,

On Wed, 16 Aug 2023 18:35:17 +0200

Francis Laniel [off-list ref] wrote:

quoted

When using sysfs, it is possible to create kprobe for several kernel
functions sharing the same name, but of course with different
addresses,
by writing their addresses in kprobe_events file.

When using PMU, if only the symbol name is given, the event will be
created for the first address which matches the symbol, as returned by
kallsyms_lookup_name().

Do you mean probing the same name symbols? Yes, it is intended behavior,
since it is not always true that the same name function has the same
prototype (it is mostly true but is not ensured), it is better to leave
user to decide which one is what you want to probe.

This is what I meant.
I also share your mind regarding leaving the users deciding which one they
want to probe but in my case (which I agree is a bit a corner one) it
leaded me to misunderstanding as the PMU kprobe was only added to the
first ntfs_file_write_iter() which is not the one for ntfs3.

Hmm, OK. I think in that case (multiple same-name symbols exist) the default
behavior is rejecting with error message. And optionally, it will probe all
or them like your patch.

I am not sure to understand.
Can you please precise the default behavior of which software component?

I meant that the behavior of the kprobe-events via /sys/kernel/tracing.
But your patch is for the other interface for perf as kprobe-event PMU.
In that case, I think we should CC to other users like BPF because
this may change the expected behavior.

it does not break bpf tests, but of course we don't have such use case, but I
think should make this optional not to potentionaly break existing users,
because you get more probes than you currently ask for

would be great to have some kind of tests for this as well

SNIP

quoted

quoted

quoted

quoted

quoted

+		/*
+		 * alloc_trace_kprobe() first considers symbol name, so we set
+		 * this to NULL to allocate this kprobe on the given address.
+		 */
+		tk_same_name = alloc_trace_kprobe(KPROBE_EVENT_SYSTEM, event,
+						  (void *)address, NULL, offs,
+						  0 /* maxactive */,
+						  0 /* nargs */, is_return);
+
+		if (IS_ERR(tk_same_name)) {
+			ret = -ENOMEM;
+			goto error_free;
+		}
+
+		init_trace_event_call(tk_same_name);
+
+		if (traceprobe_set_print_fmt(&tk_same_name->tp, ptype) < 0) {
+			ret = -ENOMEM;
+			goto error_free;
+		}
+
+		ret = append_trace_kprobe(tk_same_name, tk);
+		if (ret)
+			goto error_free;

this seems tricky if offs is specified, because IIUC that will most
likely fail in the __register_trace_kprobe/register_kprobe call inside
the append_trace_kprobe ... should we allow this just for offs == 0 ?

jirka