lkml.org 
[lkml]   [2023]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC PATCH v1 1/1] tracing/kprobe: Add multi-probe support for 'perf_kprobe' PMU
    Date
    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




    \
     
     \ /
      Last update: 2023-08-21 14:23    [W:4.073 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site