| Message ID | 20191029165832.33606-2-mark.rutland@arm.com |
|---|---|
| State | Accepted |
| Commit | fbf6c73c5b264c25484fa9f449b5546569fe11f0 |
| Headers | show |
| Series | arm64: ftrace cleanup + FTRACE_WITH_REGS | expand |
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 <mark.rutland@arm.com> > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Torsten Duwe <duwe@suse.de> Reviewed-by: Miroslav Benes <mbenes@suse.cz> M
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 <mark.rutland@arm.com> > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Torsten Duwe <duwe@suse.de> > --- > 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++; >
On Sat, 2 Nov 2019 17:49:00 +0530 Amit Daniel Kachhap <amit.kachhap@arm.com> 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
On Tue, 29 Oct 2019 16:58:25 +0000 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_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 <mark.rutland@arm.com> > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Steven Rostedt <rostedt@goodmis.org> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > Cc: Torsten Duwe <duwe@suse.de> > --- > 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++;
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 <mark.rutland@arm.com> > > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Torsten Duwe <duwe@suse.de> > > --- > > 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.
On Mon, Nov 04, 2019 at 08:16:20AM -0500, Steven Rostedt wrote: > On Tue, 29 Oct 2019 16:58:25 +0000 > 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_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 <mark.rutland@arm.com> > > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Thanks! Just to check, are you happy if this were to go via the arm64 tree with the rest of the patches? Mark.
On Mon, 4 Nov 2019 13:38:36 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > 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
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 <mark.rutland@arm.com> >>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> Cc: Ingo Molnar <mingo@redhat.com> >>> Cc: Steven Rostedt <rostedt@goodmis.org> >>> Cc: Torsten Duwe <duwe@suse.de> >>> --- >>> 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. >
On 11/4/19 6:41 PM, Steven Rostedt wrote: > On Sat, 2 Nov 2019 17:49:00 +0530 > Amit Daniel Kachhap <amit.kachhap@arm.com> 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 >
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.
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. >
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++;