lkml.org 
[lkml]   [2021]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] tracing: Fix event probe removal from dynamic events
On Tue, 12 Oct 2021 20:08:56 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 13 Oct 2021 07:42:26 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > On Tue, 12 Oct 2021 12:03:10 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > On Tue, 12 Oct 2021 23:31:07 +0900
> > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > > Hmm, this seems something wrong. Via dynamic_events interface, all the
> > > > events must be parsed equaly. If you have to pass the attached "system/event"
> > > > that's something wrong. The dynamic_events interface will accept
> > > >
> > > > -:[GROUP/]EVENT [optional arguments]
> > > >
> > > > Or
> > > >
> > > > !e:[GROUP/]EVENT [optional arguments]
> > > >
> > > > What did you expect other that these syntax?
> > >
> > > But there are non "optional arguments".
> > >
> > > To create the event probe, we need to send:
> > >
> > > e:[GROUP/]EVENT system/event [optional arguments]
> > >
> > > Where the "system/event" is what we attach to. Similar to adding a function
> > > or address to kprobes. Do you not need to add that for deleting a kprobe?
> >
> > No, since if the GROUP name is given, we can identify the event.
> >
> > And sorry. I misunderstood your patch, simply I mixed the group/event is
> > the name of group/event or the attached group/event.
>
> The GROUP/EVENT is the name of the created eprobe, my "system/event" is the
> name of the probe being attached.

OK, I understand that.

>
> >
> > Actually, the dynamic_events delete command is something like wildcard
> > unless you specify the options.
>
> OK, so we need to update this a little bit more. But currently, it fails if
> you do:
>
> # echo 'e:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
> # cat dynamic_events
> eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8

BTW, Isn't there 'e:' prefix like below?
e:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8

> # echo '-:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
>
> It will error out with "-EBUSY".

That may be another reason. (Did you enable the eprobes/hrstate?)
If the event doesn't match, it will return -ENOENT.

>
> It would make sense if we echo in exactly what is in dynamic_events with
> "-:" in front of it it, that it will remove it. But currently, it expects:
>
> # echo '-:eprobes/hrstate state=+0x38($hrtimer):u8' >> dynamic_events
>
> Which is completely wrong.

Ah, I see what was wrong... you used trace_probe_match_command_args() but
didn't skip the attached system/event. Thus the patch check and skipped it.

> > > That is, if I create a kprobe with:
> > >
> > > p:myprobe schedule > dynamic_events
> > >
> > > To remove it, don't we need to have:
> > >
> > > -:myprobe schedule >> dynamic_events
> >
> > Yes, it is possible. But you can also do
> >
> > -:kprobes/myprobe >> dynamic_events
> >
> > So, the "schedule" trace point is optional.
> >
> > Anyway, let me comment on your patch again.
>
> OK, we can make it optional, but we need to fix it so that it also works.

Agreed.

Thank you,


--
Masami Hiramatsu <mhiramat@kernel.org>

\
 
 \ /
  Last update: 2021-10-13 16:42    [W:0.052 / U:0.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site