Re: [PATCH 1/8] ftrace: add ftrace_init_nop()

[Mark Rutland]

On Mon, Oct 21, 2019 at 02:07:56PM -0400, Steven Rostedt wrote:
> On Mon, 21 Oct 2019 17:34:19 +0100
> Mark Rutland <mark.rutland@xxxxxxx> wrote:
> 
> > Architectures may need to perform special initialization of ftrace
> > callsites, and today they do so by special-casing ftrace_make_nop() when
> > the expected branch address is MCOUNT_ADDR. In some cases (e.g. for
> > patchable-function-entry), we don't have an mcount-like symbol and don't
> > want a synthetic MCOUNT_ADDR, but we may need to perform some
> > initialization of callsites.
> > 
> > To make it possible to separate initialization from runtime
> > modification, and to handle cases without an mcount-like symbol, this
> > patch adds an optional ftrace_init_nop() function that architectures can
> > implement, which does not pass a branch address.
> > 
> > Where an architecture does not provide ftrace_init_nop(), we will fall
> > back to the existing behaviour of calling ftrace_make_nop() with
> > MCOUNT_ADDR.
> > 
> > At the same time, ftrace_code_disable() is renamed to
> > ftrace_code_init_disabled() to make it clearer that it is intended to
> > intialize a callsite into a disabled state, and is not for disabling a
> > callsite that has been runtime enabled.
> 
> To make the name even better, let's just rename it to:
> 
>  ftrace_nop_initialization()
> 
> I think that may be the best description for it.

Perhaps ftrace_nop_initialize(), so that it's not a noun?

I've made it ftrace_nop_initialization() in my branch for now.

> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index f296d89be757..afd7e210e595 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -2493,15 +2493,22 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
> >  	return &iter->pg->records[iter->index];
> >  }
> >  
> > +#ifndef ftrace_init_nop
> > +static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > +{
> > +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > +}
> > +#endif
> 
> Can you place the above in the ftrace.h header. That's where that would
> belong.
> 
> #ifndef ftrace_init_nop
> struct module;
> static inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> {
> 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> }
> #endif

True.

I've put this immediately after ftrace_make_nop() in the header, and
given it a kerneldoc comment. There's a declaration for struct module at
the top of the header, so I've just relied on that

That looks like:

| /**
|  * ftrace_init_nop - initialize a nop call site
|  * @mod: module structure if called by module load initialization
|  * @rec: the mcount call site record
|  *
|  * This is a very sensitive operation and great care needs
|  * to be taken by the arch.  The operation should carefully
|  * read the location, check to see if what is read is indeed
|  * what we expect it to be, and then on success of the compare,
|  * it should write to the location.
|  *
|  * The code segment at @rec->ip should be as initialized by the
|  * compiler
|  *
|  * Return must be:
|  *  0 on success
|  *  -EFAULT on error reading the location
|  *  -EINVAL on a failed compare of the contents
|  *  -EPERM  on error writing to the location
|  * Any other value will be considered a failure.
|  */
| #ifndef ftrace_init_nop
| static int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
| {
| 	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
| }
| #endif

Thanks,
Mark.