Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Masami Hiramatsu <mhiramat@kernel.org>]

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>