[1/8] ftrace: add ftrace_init_nop()
diff mbox series

Message ID 20191021163426.9408-2-mark.rutland@arm.com
State New
Headers show
Series
  • arm64: ftrace cleanup + FTRACE_WITH_REGS
Related show

Commit Message

Mark Rutland Oct. 21, 2019, 4:34 p.m. UTC
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.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Torsten Duwe <duwe@suse.de>
---
 kernel/trace/ftrace.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Steven Rostedt Oct. 21, 2019, 6:07 p.m. UTC | #1
On Mon, 21 Oct 2019 17:34:19 +0100
Mark Rutland <mark.rutland@arm.com> 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.

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Torsten Duwe <duwe@suse.de>
> ---
>  kernel/trace/ftrace.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> 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

-- Steve

> +
>  static int
> -ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
> +ftrace_code_init_disabled(struct module *mod, struct dyn_ftrace *rec)
>  {
>  	int ret;
>  
>  	if (unlikely(ftrace_disabled))
>  		return 0;
>  
> -	ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +	ret = ftrace_init_nop(mod, rec);
>  	if (ret) {
>  		ftrace_bug_type = FTRACE_BUG_INIT;
>  		ftrace_bug(ret, rec);
> @@ -2943,7 +2950,7 @@ static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
>  			 * to the NOP instructions.
>  			 */
>  			if (!__is_defined(CC_USING_NOP_MCOUNT) &&
> -			    !ftrace_code_disable(mod, p))
> +			    !ftrace_code_init_disabled(mod, p))
>  				break;
>  
>  			update_cnt++;
Mark Rutland Oct. 22, 2019, 11:28 a.m. UTC | #2
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@arm.com> 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.
Steven Rostedt Oct. 22, 2019, 12:54 p.m. UTC | #3
On Tue, 22 Oct 2019 12:28:11 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> > 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.

I'm fine with ftrace_nop_initialize().

> 
> > > 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

Perhaps say "mcount/fentry"

> |  *
> |  * 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

"should be as" is a bit confusing. What about?

 "The code segment at @rec->ip should contain the contents created by
  the compiler".

-- Steve


> |  * 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.
Mark Rutland Oct. 22, 2019, 3:30 p.m. UTC | #4
On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:
> On Tue, 22 Oct 2019 12:28:11 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> > > 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.
> 
> I'm fine with ftrace_nop_initialize().

It's settled, then. :)

[...]

> > | /**
> > |  * ftrace_init_nop - initialize a nop call site
> > |  * @mod: module structure if called by module load initialization
> > |  * @rec: the mcount call site record
> 
> Perhaps say "mcount/fentry"

This is the exact wording that ftrace_make_nop and ftrace_modify_call
have. For consistency, I think those should all match.

I can add " (e.g. mcount/fentry)" to all of those if you'd like?

... or leave them all as-is?

> > |  *
> > |  * 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
> 
> "should be as" is a bit confusing. What about?
> 
>  "The code segment at @rec->ip should contain the contents created by
>   the compiler".

Works for me.

Mark.
Mark Rutland Oct. 22, 2019, 3:33 p.m. UTC | #5
On Tue, Oct 22, 2019 at 04:30:35PM +0100, Mark Rutland wrote:
> On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:
> > On Tue, 22 Oct 2019 12:28:11 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > | /**
> > > |  * ftrace_init_nop - initialize a nop call site
> > > |  * @mod: module structure if called by module load initialization
> > > |  * @rec: the mcount call site record
> > 
> > Perhaps say "mcount/fentry"
> 
> This is the exact wording that ftrace_make_nop and ftrace_modify_call
> have. For consistency, I think those should all match.

Now that I read this again, I see what you meant.

If it's ok, I'll change those to:

| @rec: the call site record (e.g. mcount/fentry)

Thanks,
Mark.
Steven Rostedt Oct. 22, 2019, 4:01 p.m. UTC | #6
On Tue, 22 Oct 2019 16:33:35 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, Oct 22, 2019 at 04:30:35PM +0100, Mark Rutland wrote:
> > On Tue, Oct 22, 2019 at 08:54:28AM -0400, Steven Rostedt wrote:  
> > > On Tue, 22 Oct 2019 12:28:11 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:  
> > > > | /**
> > > > |  * ftrace_init_nop - initialize a nop call site
> > > > |  * @mod: module structure if called by module load initialization
> > > > |  * @rec: the mcount call site record  
> > > 
> > > Perhaps say "mcount/fentry"  
> > 
> > This is the exact wording that ftrace_make_nop and ftrace_modify_call
> > have. For consistency, I think those should all match.  
> 
> Now that I read this again, I see what you meant.
> 
> If it's ok, I'll change those to:
> 
> | @rec: the call site record (e.g. mcount/fentry)
> 

Ack

-- Steve

Patch
diff mbox series

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
+
 static int
-ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
+ftrace_code_init_disabled(struct module *mod, struct dyn_ftrace *rec)
 {
 	int ret;
 
 	if (unlikely(ftrace_disabled))
 		return 0;
 
-	ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+	ret = ftrace_init_nop(mod, rec);
 	if (ret) {
 		ftrace_bug_type = FTRACE_BUG_INIT;
 		ftrace_bug(ret, rec);
@@ -2943,7 +2950,7 @@  static int ftrace_update_code(struct module *mod, struct ftrace_page *new_pgs)
 			 * to the NOP instructions.
 			 */
 			if (!__is_defined(CC_USING_NOP_MCOUNT) &&
-			    !ftrace_code_disable(mod, p))
+			    !ftrace_code_init_disabled(mod, p))
 				break;
 
 			update_cnt++;