[PATCHv2,1/8] ftrace: add ftrace_init_nop()

Commit Message

Mark Rutland Oct. 29, 2019, 4:58 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_nop_initialize() 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. The kerneldoc description of rec
arguments is updated to cover non-mcount callsites.

Signed-off-by: Mark Rutland <[e-mail]>
Reviewed-by: Ard Biesheuvel <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Steven Rostedt <[e-mail]>
Cc: Torsten Duwe <[e-mail]>
---
 include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
 kernel/trace/ftrace.c  |  6 +++---
 2 files changed, 35 insertions(+), 6 deletions(-)

Comments

Miroslav Benes Oct. 30, 2019, 3 p.m. UTC | #1

On Tue, 29 Oct 2019, Mark Rutland 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_nop_initialize() 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. The kerneldoc description of rec
> arguments is updated to cover non-mcount callsites.
> 
> Signed-off-by: Mark Rutland <[e-mail]>
> Reviewed-by: Ard Biesheuvel <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: Steven Rostedt <[e-mail]>
> Cc: Torsten Duwe <[e-mail]>

Reviewed-by: Miroslav Benes <[e-mail]>

M

Amit Daniel Kachhap Nov. 2, 2019, 12:19 p.m. UTC | #2

Hi Mark,

On 10/29/19 10:28 PM, Mark Rutland 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
s/an mcount/a mcount.
> 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
Same as above.
> 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_nop_initialize() 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. The kerneldoc description of rec
> arguments is updated to cover non-mcount callsites.
> 
> Signed-off-by: Mark Rutland <[e-mail]>
> Reviewed-by: Ard Biesheuvel <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: Steven Rostedt <[e-mail]>
> Cc: Torsten Duwe <[e-mail]>
> ---
>   include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
>   kernel/trace/ftrace.c  |  6 +++---
>   2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8a8cb3c401b2..9867d90d635e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>   /**
>    * ftrace_make_nop - convert code into nop
>    * @mod: module structure if called by module load initialization
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
>    * @addr: the address that the call site should be calling
>    *
>    * This is a very sensitive operation and great care needs
> @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>   extern int ftrace_make_nop(struct module *mod,
>   			   struct dyn_ftrace *rec, unsigned long addr);
>   
> +
> +/**
> + * ftrace_init_nop - initialize a nop call site
> + * @mod: module structure if called by module load initialization
> + * @rec: the call site record (e.g. 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 contain the contents created by
> + * the compiler
Nit: Will it be better to write it as "@rec->ip should store the 
adjusted ftrace entry address of the call site" or something like that.
> + *
> + * 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 inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +}
> +#endif
> +
Now that ftrace_init_nop is also an arch implemented function so it may 
be added in Documentation/trace/ftrace-design.rst along with 
ftrace_make_nop.
In general also, adding some description about patchable-function-entry
in kernel Documentation will be useful.

Thanks,
Amit Daniel
>   /**
>    * ftrace_make_call - convert a nop call site into a call to addr
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
>    * @addr: the address that the call site should call
>    *
>    * This is a very sensitive operation and great care needs
> @@ -545,7 +574,7 @@ extern int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr);
>   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>   /**
>    * ftrace_modify_call - convert from one addr to another (no nop)
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
>    * @old_addr: the address expected to be currently called to
>    * @addr: the address to change to
>    *
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f296d89be757..5259d4dea675 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2494,14 +2494,14 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
>   }
>   
>   static int
> -ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
> +ftrace_nop_initialize(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 +2943,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_nop_initialize(mod, p))
>   				break;
>   
>   			update_cnt++;
>

Steven Rostedt Nov. 4, 2019, 1:11 p.m. UTC | #3

On Sat, 2 Nov 2019 17:49:00 +0530
Amit Daniel Kachhap <[e-mail]> wrote:

> Now that ftrace_init_nop is also an arch implemented function so it may 
> be added in Documentation/trace/ftrace-design.rst along with 
> ftrace_make_nop.
> In general also, adding some description about patchable-function-entry
> in kernel Documentation will be useful.

I think this part is outside the scope of this patch set. Honestly, I
need to chisel out some time to rewrite the ftrace-design document, as
that's been long needed. But that can come at a later time. I'm
currently rewriting some of it now, so it will be best to not waste
effort to update a document that will soon become stale. ;-)

-- Steve

Steven Rostedt Nov. 4, 2019, 1:16 p.m. UTC | #4

On Tue, 29 Oct 2019 16:58:25 +0000
Mark Rutland <[e-mail]> 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_nop_initialize() 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. The kerneldoc description of rec
> arguments is updated to cover non-mcount callsites.
> 
> Signed-off-by: Mark Rutland <[e-mail]>
> Reviewed-by: Ard Biesheuvel <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: Steven Rostedt <[e-mail]>

Reviewed-by: Steven Rostedt (VMware) <[e-mail]>

-- Steve

> Cc: Torsten Duwe <[e-mail]>
> ---
>  include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
>  kernel/trace/ftrace.c  |  6 +++---
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8a8cb3c401b2..9867d90d635e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>  /**
>   * ftrace_make_nop - convert code into nop
>   * @mod: module structure if called by module load initialization
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
>   * @addr: the address that the call site should be calling
>   *
>   * This is a very sensitive operation and great care needs
> @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>  extern int ftrace_make_nop(struct module *mod,
>  			   struct dyn_ftrace *rec, unsigned long addr);
>  
> +
> +/**
> + * ftrace_init_nop - initialize a nop call site
> + * @mod: module structure if called by module load initialization
> + * @rec: the call site record (e.g. 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 contain the contents created 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 inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +}
> +#endif
> +
>  /**
>   * ftrace_make_call - convert a nop call site into a call to addr
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
>   * @addr: the address that the call site should call
>   *
>   * This is a very sensitive operation and great care needs
> @@ -545,7 +574,7 @@ extern int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr);
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>  /**
>   * ftrace_modify_call - convert from one addr to another (no nop)
> - * @rec: the mcount call site record
> + * @rec: the call site record (e.g. mcount/fentry)
>   * @old_addr: the address expected to be currently called to
>   * @addr: the address to change to
>   *
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f296d89be757..5259d4dea675 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2494,14 +2494,14 @@ struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
>  }
>  
>  static int
> -ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
> +ftrace_nop_initialize(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 +2943,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_nop_initialize(mod, p))
>  				break;
>  
>  			update_cnt++;

Mark Rutland Nov. 4, 2019, 1:36 p.m. UTC | #5

On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
> Hi Mark,

Hi Amit,

Thanks for the review!
 
> On 10/29/19 10:28 PM, Mark Rutland 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
> s/an mcount/a mcount.
> > 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
> Same as above.

Using 'an' in both of these cases is correct, even though 'mcount'
starts with a consonant, since it's pronounced as-if it were 'emcount'.
I will leave this as-is.

> > 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_nop_initialize() 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. The kerneldoc description of rec
> > arguments is updated to cover non-mcount callsites.
> > 
> > Signed-off-by: Mark Rutland <[e-mail]>
> > Reviewed-by: Ard Biesheuvel <[e-mail]>
> > Cc: Ingo Molnar <[e-mail]>
> > Cc: Steven Rostedt <[e-mail]>
> > Cc: Torsten Duwe <[e-mail]>
> > ---
> >   include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
> >   kernel/trace/ftrace.c  |  6 +++---
> >   2 files changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 8a8cb3c401b2..9867d90d635e 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> >   /**
> >    * ftrace_make_nop - convert code into nop
> >    * @mod: module structure if called by module load initialization
> > - * @rec: the mcount call site record
> > + * @rec: the call site record (e.g. mcount/fentry)
> >    * @addr: the address that the call site should be calling
> >    *
> >    * This is a very sensitive operation and great care needs
> > @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> >   extern int ftrace_make_nop(struct module *mod,
> >   			   struct dyn_ftrace *rec, unsigned long addr);
> > +
> > +/**
> > + * ftrace_init_nop - initialize a nop call site
> > + * @mod: module structure if called by module load initialization
> > + * @rec: the call site record (e.g. 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 contain the contents created by
> > + * the compiler
> Nit: Will it be better to write it as "@rec->ip should store the adjusted
> ftrace entry address of the call site" or something like that.

This was the specific wording requested by Steve, and it's trying to
describe the instructions at rec->ip, rather than the value of rec->ip,
so I think it's better to leave this as-is.

> > + *
> > + * 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 inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > +{
> > +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> > +}
> > +#endif
> > +
> Now that ftrace_init_nop is also an arch implemented function so it may be
> added in Documentation/trace/ftrace-design.rst along with ftrace_make_nop.
> In general also, adding some description about patchable-function-entry
> in kernel Documentation will be useful.

I agree that would be a good thing, but as Steve suggests I will leave
this for subsequent rework, as I think that also implies more
rework/renaming in the core code (e.g. to more cleanly separate the
notion of a callsite from mcount specifically).

Steve, if you'd like help with that (or review), I'd be happy to help.

Thanks,
Mark.

Mark Rutland Nov. 4, 2019, 1:38 p.m. UTC | #6

On Mon, Nov 04, 2019 at 08:16:20AM -0500, Steven Rostedt wrote:
> On Tue, 29 Oct 2019 16:58:25 +0000
> Mark Rutland <[e-mail]> 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_nop_initialize() 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. The kerneldoc description of rec
> > arguments is updated to cover non-mcount callsites.
> > 
> > Signed-off-by: Mark Rutland <[e-mail]>
> > Reviewed-by: Ard Biesheuvel <[e-mail]>
> > Cc: Ingo Molnar <[e-mail]>
> > Cc: Steven Rostedt <[e-mail]>
> 
> Reviewed-by: Steven Rostedt (VMware) <[e-mail]>

Thanks!

Just to check, are you happy if this were to go via the arm64 tree with
the rest of the patches?

Mark.

Steven Rostedt Nov. 4, 2019, 1:53 p.m. UTC | #7

On Mon, 4 Nov 2019 13:38:36 +0000
Mark Rutland <[e-mail]> wrote:

> > Reviewed-by: Steven Rostedt (VMware) <[e-mail]>  
> 
> Thanks!
> 
> Just to check, are you happy if this were to go via the arm64 tree with
> the rest of the patches?

Yes, if I wasn't I would have said something. But I guess I should have
been explicit and stated that I'm fine with it going in your tree. My
current updates should not conflict with this.

-- Steve

Amit Daniel Kachhap Nov. 5, 2019, 6:47 a.m. UTC | #8

Hi Mark,

On 11/4/19 7:06 PM, Mark Rutland wrote:
> On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
>> Hi Mark,
> 
> Hi Amit,
> 
> Thanks for the review!
>   
>> On 10/29/19 10:28 PM, Mark Rutland 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
>> s/an mcount/a mcount.
>>> 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
>> Same as above.
> 
> Using 'an' in both of these cases is correct, even though 'mcount'
> starts with a consonant, since it's pronounced as-if it were 'emcount'.
> I will leave this as-is.
Ok sure. It makes sense.
> 
>>> 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_nop_initialize() 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. The kerneldoc description of rec
>>> arguments is updated to cover non-mcount callsites.
>>>
>>> Signed-off-by: Mark Rutland <[e-mail]>
>>> Reviewed-by: Ard Biesheuvel <[e-mail]>
>>> Cc: Ingo Molnar <[e-mail]>
>>> Cc: Steven Rostedt <[e-mail]>
>>> Cc: Torsten Duwe <[e-mail]>
>>> ---
>>>    include/linux/ftrace.h | 35 ++++++++++++++++++++++++++++++++---
>>>    kernel/trace/ftrace.c  |  6 +++---
>>>    2 files changed, 35 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>>> index 8a8cb3c401b2..9867d90d635e 100644
>>> --- a/include/linux/ftrace.h
>>> +++ b/include/linux/ftrace.h
>>> @@ -499,7 +499,7 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>>>    /**
>>>     * ftrace_make_nop - convert code into nop
>>>     * @mod: module structure if called by module load initialization
>>> - * @rec: the mcount call site record
>>> + * @rec: the call site record (e.g. mcount/fentry)
>>>     * @addr: the address that the call site should be calling
>>>     *
>>>     * This is a very sensitive operation and great care needs
>>> @@ -520,9 +520,38 @@ static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
>>>    extern int ftrace_make_nop(struct module *mod,
>>>    			   struct dyn_ftrace *rec, unsigned long addr);
>>> +
>>> +/**
>>> + * ftrace_init_nop - initialize a nop call site
>>> + * @mod: module structure if called by module load initialization
>>> + * @rec: the call site record (e.g. 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 contain the contents created by
>>> + * the compiler
>> Nit: Will it be better to write it as "@rec->ip should store the adjusted
>> ftrace entry address of the call site" or something like that.
> 
> This was the specific wording requested by Steve, and it's trying to
> describe the instructions at rec->ip, rather than the value of rec->ip,
> so I think it's better to leave this as-is.
ok Its fine this way too. Actually from the comment, I could not 
understand which one of the compiler contents this points to as in this 
case there are 2 nops.
> 
>>> + *
>>> + * 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 inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
>>> +{
>>> +	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
>>> +}
>>> +#endif
>>> +
>> Now that ftrace_init_nop is also an arch implemented function so it may be
>> added in Documentation/trace/ftrace-design.rst along with ftrace_make_nop.
>> In general also, adding some description about patchable-function-entry
>> in kernel Documentation will be useful.
> 
> I agree that would be a good thing, but as Steve suggests I will leave
> this for subsequent rework, as I think that also implies more
> rework/renaming in the core code (e.g. to more cleanly separate the
> notion of a callsite from mcount specifically).
ok.

Thanks,
Amit
> 
> Steve, if you'd like help with that (or review), I'd be happy to help.
> 
> Thanks,
> Mark.
>

Amit Daniel Kachhap Nov. 5, 2019, 6:59 a.m. UTC | #9

On 11/4/19 6:41 PM, Steven Rostedt wrote:
> On Sat, 2 Nov 2019 17:49:00 +0530
> Amit Daniel Kachhap <[e-mail]> wrote:
> 
>> Now that ftrace_init_nop is also an arch implemented function so it may
>> be added in Documentation/trace/ftrace-design.rst along with
>> ftrace_make_nop.
>> In general also, adding some description about patchable-function-entry
>> in kernel Documentation will be useful.
> 
> I think this part is outside the scope of this patch set. Honestly, I
> need to chisel out some time to rewrite the ftrace-design document, as
> that's been long needed. But that can come at a later time. I'm
> currently rewriting some of it now, so it will be best to not waste
> effort to update a document that will soon become stale. ;-)
Yes it makes sense.

Thanks,
Amit
> 
> -- Steve
>

Mark Rutland Nov. 6, 2019, 2:15 p.m. UTC | #10

On Tue, Nov 05, 2019 at 12:17:26PM +0530, Amit Kachhap wrote:
> On 11/4/19 7:06 PM, Mark Rutland wrote:
> > On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
> > > On 10/29/19 10:28 PM, Mark Rutland wrote:
> > > > +/**
> > > > + * ftrace_init_nop - initialize a nop call site
> > > > + * @mod: module structure if called by module load initialization
> > > > + * @rec: the call site record (e.g. 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 contain the contents created by
> > > > + * the compiler
> > > Nit: Will it be better to write it as "@rec->ip should store the adjusted
> > > ftrace entry address of the call site" or something like that.
> > 
> > This was the specific wording requested by Steve, and it's trying to
> > describe the instructions at rec->ip, rather than the value of rec->ip,
> > so I think it's better to leave this as-is.
> ok Its fine this way too. Actually from the comment, I could not understand
> which one of the compiler contents this points to as in this case there are
> 2 nops.

We can't say what the compiler contents will be. An architecture may use
this callback if it's using mcount, mfentry, patchable-function-entry,
or some other mechanism we're not aware of today. Depending on the
architecture and mechanism, the callsite could contain a number of
distinct things.

All the comment is trying to say is that when ftrace_init_nop() is
called, the callsite has not been modified in any way since being
compiled, so we can expect the contents to be whatever the compiler
generated.

Thanks,
Mark.

Amit Daniel Kachhap Nov. 7, 2019, 4:40 a.m. UTC | #11

On 11/6/19 7:45 PM, Mark Rutland wrote:
> On Tue, Nov 05, 2019 at 12:17:26PM +0530, Amit Kachhap wrote:
>> On 11/4/19 7:06 PM, Mark Rutland wrote:
>>> On Sat, Nov 02, 2019 at 05:49:00PM +0530, Amit Daniel Kachhap wrote:
>>>> On 10/29/19 10:28 PM, Mark Rutland wrote:
>>>>> +/**
>>>>> + * ftrace_init_nop - initialize a nop call site
>>>>> + * @mod: module structure if called by module load initialization
>>>>> + * @rec: the call site record (e.g. 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 contain the contents created by
>>>>> + * the compiler
>>>> Nit: Will it be better to write it as "@rec->ip should store the adjusted
>>>> ftrace entry address of the call site" or something like that.
>>>
>>> This was the specific wording requested by Steve, and it's trying to
>>> describe the instructions at rec->ip, rather than the value of rec->ip,
>>> so I think it's better to leave this as-is.
>> ok Its fine this way too. Actually from the comment, I could not understand
>> which one of the compiler contents this points to as in this case there are
>> 2 nops.
> 
> We can't say what the compiler contents will be. An architecture may use
> this callback if it's using mcount, mfentry, patchable-function-entry,
> or some other mechanism we're not aware of today. Depending on the
> architecture and mechanism, the callsite could contain a number of
> distinct things.
> 
> All the comment is trying to say is that when ftrace_init_nop() is
> called, the callsite has not been modified in any way since being
> compiled, so we can expect the contents to be whatever the compiler
> generated.

ok. Your details seems reasonable.

Thanks,
Amit Daniel
> 
> Thanks,
> Mark.
>

Patch

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8a8cb3c401b2..9867d90d635e 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -499,7 +499,7 @@  static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
 /**
  * ftrace_make_nop - convert code into nop
  * @mod: module structure if called by module load initialization
- * @rec: the mcount call site record
+ * @rec: the call site record (e.g. mcount/fentry)
  * @addr: the address that the call site should be calling
  *
  * This is a very sensitive operation and great care needs
@@ -520,9 +520,38 @@  static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
 extern int ftrace_make_nop(struct module *mod,
 			   struct dyn_ftrace *rec, unsigned long addr);
 
+
+/**
+ * ftrace_init_nop - initialize a nop call site
+ * @mod: module structure if called by module load initialization
+ * @rec: the call site record (e.g. 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 contain the contents created 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 inline int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
+{
+	return ftrace_make_nop(mod, rec, MCOUNT_ADDR);
+}
+#endif
+
 /**
  * ftrace_make_call - convert a nop call site into a call to addr
- * @rec: the mcount call site record
+ * @rec: the call site record (e.g. mcount/fentry)
  * @addr: the address that the call site should call
  *
  * This is a very sensitive operation and great care needs
@@ -545,7 +574,7 @@  extern int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr);
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 /**
  * ftrace_modify_call - convert from one addr to another (no nop)
- * @rec: the mcount call site record
+ * @rec: the call site record (e.g. mcount/fentry)
  * @old_addr: the address expected to be currently called to
  * @addr: the address to change to
  *
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f296d89be757..5259d4dea675 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2494,14 +2494,14 @@  struct dyn_ftrace *ftrace_rec_iter_record(struct ftrace_rec_iter *iter)
 }
 
 static int
-ftrace_code_disable(struct module *mod, struct dyn_ftrace *rec)
+ftrace_nop_initialize(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 +2943,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_nop_initialize(mod, p))
 				break;
 
 			update_cnt++;