[1/8] ftrace: add ftrace_init_nop()

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

-- 
2.11.0

Comments

Steven Rostedt Oct. 21, 2019, 6:07 p.m. | #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. | #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. | #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. | #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. | #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. | #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 --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++;