Messages in this thread | | | Date | Wed, 13 Oct 2021 23:42:06 +0900 | From | Masami Hiramatsu <> | Subject | Re: [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>
| |