Hi.

Le dimanche 20 août 2023, 22:23:55 CEST Jiri Olsa a écrit :

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

quoted

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

If we decide to go further with this contribution, I will add some kind of 
test (even though I do not really see how to test it at the moment).

SNIP

quoted

quoted

quoted

quoted

quoted

quoted

+		/*
+		 * alloc_trace_kprobe() first considers symbol name, 

so we

quoted

quoted

quoted

quoted

quoted

quoted

set
+		 * this to NULL to allocate this kprobe on the given 

address.

quoted

quoted

quoted

quoted

quoted

quoted

+		 */
+		tk_same_name = 

alloc_trace_kprobe(KPROBE_EVENT_SYSTEM, event,

quoted

quoted

quoted

quoted

quoted

quoted

+						  (void *)address, NULL, 

offs,

quoted

quoted

quoted

quoted

quoted

quoted

+						  0 /* maxactive */,
+						  0 /* nargs */, 

is_return);

quoted

quoted

quoted

quoted

quoted

quoted

+
+		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) {

quoted

quoted

quoted

quoted

quoted

quoted

+			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 ?

Excellent catch!
I will correct it for v2 if I send one!

jirka