Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Alexei Starovoitov]

On Thu, Nov 14, 2019 at 10:29 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Sat, 9 Nov 2019 07:33:10 -0500
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> > On Fri, 8 Nov 2019 18:29:09 -0800
> > Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > > Is there a way to do a replacement of direct call?
> >
> > I'm curious to what the use case would be. The direct call added would
> > still need to be a trampoline, as the parameters need to be saved
> > before calling any other code, and then restored before going back to
> > the traced function. Couldn't you do a text poke on that trampoline to
> > change what gets called?
> >
> > > If I use unregister(old)+register(new) some events will be missed.
> >
> > > If I use register(new)+unregister(old) for short period of time both new and
> >
> > Actually, as we only allow a single direct call to be added at any time,
> > the register(new) would fail if there was already an old at the
> > location.
> >
> > > old will be triggering on all cpus which will likely confuse bpf tracing.
> > > Something like modify_ftrace_direct() should solve it. It's still racy. In a
> > > sense that some cpus will be executing old while other cpus will be executing
> > > new, but per-cpu there will be no double accounting. How difficult would be
> > > to add such feature?
> >
> > All this said, it would actually be pretty trivial to implement this,
> > as when another ftrace_ops is attached to the direct call location, it
> > falls back to the direct helper. To implement a modify_ftrace_direct(),
> > all that would be needed to do is to:
> >
> > 1) Attached a ftrace_ops stub to the same function that has the direct
> > caller, that will cause ftrace to got to the loop routine, and the
> > direct helper would then define what gets called by what what
> > registered in the direct_functions array.
> >
> > 2) Change what gets called in the direct_functions array, and at that
> > moment, the helper function would be using that. May require syncing
> > CPUs to get all CPUs seeing the same thing.
> >
> > 3) Remove the ftrace_ops stub, which would put back the direct caller
> > in the fentry location, but this time with the new function.
> >
>
> Alexei,
>
> Do you still need such a feature?
>
> Note, I just pushed my tree to my for-next tree, and also have a
> ftrace/direct branch that ends with this patch set that is the basis of
> the rest of the work of my code. Feel free to build against that
> branch if you need to have something built on the net tree.

I'm still trying to figure out what you meant
by your suggestion to implement a modify_ftrace_direct().
I was thinking something much simpler like
modify_ftrace_direct(ip, old_call, new_ca);
will just text poke that call addr from old to new if old matches
and will adjust ftrace inner bookkeeping.
I don't understand why you want to malloc/free ftrace_ops for that.