Re: [RFC PATCH v1 1/1] tracing/kprobe: Add multi-probe support for 'perf_kprobe' PMU

[Francis Laniel <flaniel@linux.microsoft.com>]

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:
> > Hi Francis,
> > (Cc: Song Liu and BPF ML)
> > 
> > On Fri, 18 Aug 2023 20:12:11 +0200
> > 
> > Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > > Hi.
> > > 
> > > Le vendredi 18 août 2023, 15:05:37 CEST Masami Hiramatsu a écrit :
> > > > On Thu, 17 Aug 2023 13:06:20 +0200
> > > > 
> > > > Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > > > > Hi.
> > > > > 
> > > > > Le jeudi 17 août 2023, 09:50:57 CEST Masami Hiramatsu a écrit :
> > > > > > Hi,
> > > > > > 
> > > > > > On Wed, 16 Aug 2023 18:35:17 +0200
> > > > > > 
> > > > > > Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > > > > > > 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
> 
> > > > > > > +		/*
> > > > > > > +		 * 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 ?

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

> jirka