| Message ID | 20211008091336.33616-8-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | x86/ftrace: Add direct batch interface | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, 8 Oct 2021 11:13:35 +0200 Jiri Olsa <jolsa@redhat.com> wrote: > + /* > + * Shutdown the ops, change 'direct' pointer for each > + * ops entry in direct_functions hash and startup the > + * ops back again. > + * > + * Note there is no callback called for @ops object after > + * this ftrace_shutdown call until ftrace_startup is called > + * later on. > + */ > + err = ftrace_shutdown(ops, 0); > + if (err) > + goto out_unlock; I believe I said before that we can do this by adding a stub ops that match all the functions with the direct ops being modified. This will cause the loop function to be called, which will call the direct function helper, which will then call the direct function that is found. That is, there is no "pause" in calling the direct callers. Either the old direct is called, or the new one. When the function returns, all are calling the new one. That is, instead of: [ Changing direct call from my_direct_1 to my_direct_2 ] <traced_func>: call my_direct_1 |||||||||||||||||||| vvvvvvvvvvvvvvvvvvvv <traced_func>: nop |||||||||||||||||||| vvvvvvvvvvvvvvvvvvvv <traced_func>: call my_direct_2 We have it do: <traced_func>: call my_direct_1 |||||||||||||||||||| vvvvvvvvvvvvvvvvvvvv <traced_func>: call ftrace_caller <ftrace_caller>: [..] call ftrace_ops_list_func ftrace_ops_list_func() { ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2 } call rax (to either my_direct_1 or my_direct_2 |||||||||||||||||||| vvvvvvvvvvvvvvvvvvvv <traced_func>: call my_direct_2 I did this on top of this patch: Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/ftrace.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 30120342176e..7ad1e8ae5855 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi); */ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) { - struct ftrace_hash *hash = ops->func_hash->filter_hash; + struct ftrace_hash *hash; struct ftrace_func_entry *entry, *iter; + static struct ftrace_ops tmp_ops = { + .func = ftrace_stub, + .flags = FTRACE_OPS_FL_STUB, + }; int i, size; int err; @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) return -EINVAL; mutex_lock(&direct_mutex); - mutex_lock(&ftrace_lock); + + /* Enable the tmp_ops to have the same functions as the direct ops */ + ftrace_ops_init(&tmp_ops); + tmp_ops.func_hash = ops->func_hash; + + err = register_ftrace_function(&tmp_ops); + if (err) + goto out_direct; /* - * Shutdown the ops, change 'direct' pointer for each - * ops entry in direct_functions hash and startup the - * ops back again. - * - * Note there is no callback called for @ops object after - * this ftrace_shutdown call until ftrace_startup is called - * later on. + * Now the ftrace_ops_list_func() is called to do the direct callers. + * We can safely change the direct functions attached to each entry. */ - err = ftrace_shutdown(ops, 0); - if (err) - goto out_unlock; + mutex_lock(&ftrace_lock); + hash = ops->func_hash->filter_hash; size = 1 << hash->size_bits; for (i = 0; i < size; i++) { hlist_for_each_entry(iter, &hash->buckets[i], hlist) { @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) } } - err = ftrace_startup(ops, 0); + /* Removing the tmp_ops will add the updated direct callers to the functions */ + unregister_ftrace_function(&tmp_ops); out_unlock: mutex_unlock(&ftrace_lock); + out_direct: mutex_unlock(&direct_mutex); return err; }
On Thu, Oct 14, 2021 at 04:28:19PM -0400, Steven Rostedt wrote: > On Fri, 8 Oct 2021 11:13:35 +0200 > Jiri Olsa <jolsa@redhat.com> wrote: > > > + /* > > + * Shutdown the ops, change 'direct' pointer for each > > + * ops entry in direct_functions hash and startup the > > + * ops back again. > > + * > > + * Note there is no callback called for @ops object after > > + * this ftrace_shutdown call until ftrace_startup is called > > + * later on. > > + */ > > + err = ftrace_shutdown(ops, 0); > > + if (err) > > + goto out_unlock; > > I believe I said before that we can do this by adding a stub ops that match > all the functions with the direct ops being modified. This will cause the > loop function to be called, which will call the direct function helper, > which will then call the direct function that is found. That is, there is > no "pause" in calling the direct callers. Either the old direct is called, > or the new one. When the function returns, all are calling the new one. > > That is, instead of: > > [ Changing direct call from my_direct_1 to my_direct_2 ] > > <traced_func>: > call my_direct_1 > > |||||||||||||||||||| > vvvvvvvvvvvvvvvvvvvv > > <traced_func>: > nop > > |||||||||||||||||||| > vvvvvvvvvvvvvvvvvvvv > > <traced_func>: > call my_direct_2 > > > We have it do: > > <traced_func>: > call my_direct_1 > > |||||||||||||||||||| > vvvvvvvvvvvvvvvvvvvv > > <traced_func>: > call ftrace_caller > > > <ftrace_caller>: > [..] > call ftrace_ops_list_func > > > ftrace_ops_list_func() > { > ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2 > } > > call rax (to either my_direct_1 or my_direct_2 nice! :) I did not see that as a problem and something that can be done later, thanks for doing this > > |||||||||||||||||||| > vvvvvvvvvvvvvvvvvvvv > > <traced_func>: > call my_direct_2 > > > I did this on top of this patch: ATM I'm bit stuck on the bpf side of this whole change, I'll test it with my other changes when I unstuck myself ;-) thanks, jirka > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/ftrace.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 30120342176e..7ad1e8ae5855 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi); > */ > int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > { > - struct ftrace_hash *hash = ops->func_hash->filter_hash; > + struct ftrace_hash *hash; > struct ftrace_func_entry *entry, *iter; > + static struct ftrace_ops tmp_ops = { > + .func = ftrace_stub, > + .flags = FTRACE_OPS_FL_STUB, > + }; > int i, size; > int err; > > @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > return -EINVAL; > > mutex_lock(&direct_mutex); > - mutex_lock(&ftrace_lock); > + > + /* Enable the tmp_ops to have the same functions as the direct ops */ > + ftrace_ops_init(&tmp_ops); > + tmp_ops.func_hash = ops->func_hash; > + > + err = register_ftrace_function(&tmp_ops); > + if (err) > + goto out_direct; > > /* > - * Shutdown the ops, change 'direct' pointer for each > - * ops entry in direct_functions hash and startup the > - * ops back again. > - * > - * Note there is no callback called for @ops object after > - * this ftrace_shutdown call until ftrace_startup is called > - * later on. > + * Now the ftrace_ops_list_func() is called to do the direct callers. > + * We can safely change the direct functions attached to each entry. > */ > - err = ftrace_shutdown(ops, 0); > - if (err) > - goto out_unlock; > + mutex_lock(&ftrace_lock); > > + hash = ops->func_hash->filter_hash; > size = 1 << hash->size_bits; > for (i = 0; i < size; i++) { > hlist_for_each_entry(iter, &hash->buckets[i], hlist) { > @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > } > } > > - err = ftrace_startup(ops, 0); > + /* Removing the tmp_ops will add the updated direct callers to the functions */ > + unregister_ftrace_function(&tmp_ops); > > out_unlock: > mutex_unlock(&ftrace_lock); > + out_direct: > mutex_unlock(&direct_mutex); > return err; > } > -- > 2.31.1 >
On Fri, 15 Oct 2021 14:05:25 +0200 Jiri Olsa <jolsa@redhat.com> wrote: > ATM I'm bit stuck on the bpf side of this whole change, I'll test > it with my other changes when I unstuck myself ;-) If you want, I'll apply this as a separate change on top of your patch set. As I don't see anything wrong with your current code. And when you are satisfied with this, just give me a "tested-by" and I'll push it too. -- Steve
On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote: > On Fri, 15 Oct 2021 14:05:25 +0200 > Jiri Olsa <jolsa@redhat.com> wrote: > > > ATM I'm bit stuck on the bpf side of this whole change, I'll test > > it with my other changes when I unstuck myself ;-) > > If you want, I'll apply this as a separate change on top of your patch set. > As I don't see anything wrong with your current code. > > And when you are satisfied with this, just give me a "tested-by" and I'll > push it too. sounds great, thanks jirka
On Sat, 16 Oct 2021 13:39:55 +0200 Jiri Olsa <jolsa@redhat.com> wrote: > On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote: > > On Fri, 15 Oct 2021 14:05:25 +0200 > > Jiri Olsa <jolsa@redhat.com> wrote: > > > > > ATM I'm bit stuck on the bpf side of this whole change, I'll test > > > it with my other changes when I unstuck myself ;-) > > > > If you want, I'll apply this as a separate change on top of your patch set. > > As I don't see anything wrong with your current code. > > > > And when you are satisfied with this, just give me a "tested-by" and I'll > > push it too. > > sounds great, thanks > jirka Would you want to ack/review this? -- Steve From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Subject: [PATCH] ftrace/direct: Do not disable when switching direct callers Currently to switch a set of "multi" direct trampolines from one trampoline to another, a full shutdown of the current set needs to be done, followed by an update to what trampoline the direct callers would call, and then re-enabling the callers. This leaves a time when the functions will not be calling anything, and events may be missed. Instead, use a trick to allow all the functions with direct trampolines attached will always call either the new or old trampoline while the switch is happening. To do this, first attach a "dummy" callback via ftrace to all the functions that the current direct trampoline is attached to. This will cause the functions to call the "list func" instead of the direct trampoline. The list function will call the direct trampoline "helper" that will set the function it should call as it returns back to the ftrace trampoline. At this moment, the direct caller descriptor can safely update the direct call trampoline. The list function will pick either the new or old function (depending on the memory coherency model of the architecture). Now removing the dummy function from each of the locations of the direct trampoline caller, will put back the direct call, but now to the new trampoline. A better visual is: [ Changing direct call from my_direct_1 to my_direct_2 ] <traced_func>: call my_direct_1 |||||||||||||||||||| vvvvvvvvvvvvvvvvvvvv <traced_func>: call ftrace_caller <ftrace_caller>: [..] call ftrace_ops_list_func ftrace_ops_list_func() { ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2 } call rax (to either my_direct_1 or my_direct_2 |||||||||||||||||||| vvvvvvvvvvvvvvvvvvvv <traced_func>: call my_direct_2 Link: https://lore.kernel.org/all/20211014162819.5c85618b@gandalf.local.home/ Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/ftrace.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 30120342176e..7ad1e8ae5855 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi); */ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) { - struct ftrace_hash *hash = ops->func_hash->filter_hash; + struct ftrace_hash *hash; struct ftrace_func_entry *entry, *iter; + static struct ftrace_ops tmp_ops = { + .func = ftrace_stub, + .flags = FTRACE_OPS_FL_STUB, + }; int i, size; int err; @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) return -EINVAL; mutex_lock(&direct_mutex); - mutex_lock(&ftrace_lock); + + /* Enable the tmp_ops to have the same functions as the direct ops */ + ftrace_ops_init(&tmp_ops); + tmp_ops.func_hash = ops->func_hash; + + err = register_ftrace_function(&tmp_ops); + if (err) + goto out_direct; /* - * Shutdown the ops, change 'direct' pointer for each - * ops entry in direct_functions hash and startup the - * ops back again. - * - * Note there is no callback called for @ops object after - * this ftrace_shutdown call until ftrace_startup is called - * later on. + * Now the ftrace_ops_list_func() is called to do the direct callers. + * We can safely change the direct functions attached to each entry. */ - err = ftrace_shutdown(ops, 0); - if (err) - goto out_unlock; + mutex_lock(&ftrace_lock); + hash = ops->func_hash->filter_hash; size = 1 << hash->size_bits; for (i = 0; i < size; i++) { hlist_for_each_entry(iter, &hash->buckets[i], hlist) { @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) } } - err = ftrace_startup(ops, 0); + /* Removing the tmp_ops will add the updated direct callers to the functions */ + unregister_ftrace_function(&tmp_ops); out_unlock: mutex_unlock(&ftrace_lock); + out_direct: mutex_unlock(&direct_mutex); return err; }
On Mon, Oct 18, 2021 at 10:10:15PM -0400, Steven Rostedt wrote: > On Sat, 16 Oct 2021 13:39:55 +0200 > Jiri Olsa <jolsa@redhat.com> wrote: > > > On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote: > > > On Fri, 15 Oct 2021 14:05:25 +0200 > > > Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > ATM I'm bit stuck on the bpf side of this whole change, I'll test > > > > it with my other changes when I unstuck myself ;-) > > > > > > If you want, I'll apply this as a separate change on top of your patch set. > > > As I don't see anything wrong with your current code. > > > > > > And when you are satisfied with this, just give me a "tested-by" and I'll > > > push it too. > > > > sounds great, thanks > > jirka > > Would you want to ack/review this? hum, do you have it in some branch already? I'm getting: patching file kernel/trace/ftrace.c Hunk #1 succeeded at 5521 with fuzz 1 (offset -40 lines). Hunk #2 FAILED at 5576. Hunk #3 succeeded at 5557 (offset -44 lines). 1 out of 3 hunks FAILED -- saving rejects to file kernel/trace/ftrace.c.rej when trying to apply on top of my changes thanks, jirka > > -- Steve > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > Subject: [PATCH] ftrace/direct: Do not disable when switching direct callers > > Currently to switch a set of "multi" direct trampolines from one > trampoline to another, a full shutdown of the current set needs to be > done, followed by an update to what trampoline the direct callers would > call, and then re-enabling the callers. This leaves a time when the > functions will not be calling anything, and events may be missed. > > Instead, use a trick to allow all the functions with direct trampolines > attached will always call either the new or old trampoline while the > switch is happening. To do this, first attach a "dummy" callback via > ftrace to all the functions that the current direct trampoline is attached > to. This will cause the functions to call the "list func" instead of the > direct trampoline. The list function will call the direct trampoline > "helper" that will set the function it should call as it returns back to > the ftrace trampoline. > > At this moment, the direct caller descriptor can safely update the direct > call trampoline. The list function will pick either the new or old > function (depending on the memory coherency model of the architecture). > > Now removing the dummy function from each of the locations of the direct > trampoline caller, will put back the direct call, but now to the new > trampoline. > > A better visual is: > > [ Changing direct call from my_direct_1 to my_direct_2 ] > > <traced_func>: > call my_direct_1 > > |||||||||||||||||||| > vvvvvvvvvvvvvvvvvvvv > > <traced_func>: > call ftrace_caller > > <ftrace_caller>: > [..] > call ftrace_ops_list_func > > ftrace_ops_list_func() > { > ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2 > } > > call rax (to either my_direct_1 or my_direct_2 > > |||||||||||||||||||| > vvvvvvvvvvvvvvvvvvvv > > <traced_func>: > call my_direct_2 > > Link: https://lore.kernel.org/all/20211014162819.5c85618b@gandalf.local.home/ > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/ftrace.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 30120342176e..7ad1e8ae5855 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi); > */ > int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > { > - struct ftrace_hash *hash = ops->func_hash->filter_hash; > + struct ftrace_hash *hash; > struct ftrace_func_entry *entry, *iter; > + static struct ftrace_ops tmp_ops = { > + .func = ftrace_stub, > + .flags = FTRACE_OPS_FL_STUB, > + }; > int i, size; > int err; > > @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > return -EINVAL; > > mutex_lock(&direct_mutex); > - mutex_lock(&ftrace_lock); > + > + /* Enable the tmp_ops to have the same functions as the direct ops */ > + ftrace_ops_init(&tmp_ops); > + tmp_ops.func_hash = ops->func_hash; > + > + err = register_ftrace_function(&tmp_ops); > + if (err) > + goto out_direct; > > /* > - * Shutdown the ops, change 'direct' pointer for each > - * ops entry in direct_functions hash and startup the > - * ops back again. > - * > - * Note there is no callback called for @ops object after > - * this ftrace_shutdown call until ftrace_startup is called > - * later on. > + * Now the ftrace_ops_list_func() is called to do the direct callers. > + * We can safely change the direct functions attached to each entry. > */ > - err = ftrace_shutdown(ops, 0); > - if (err) > - goto out_unlock; > + mutex_lock(&ftrace_lock); > > + hash = ops->func_hash->filter_hash; > size = 1 << hash->size_bits; > for (i = 0; i < size; i++) { > hlist_for_each_entry(iter, &hash->buckets[i], hlist) { > @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > } > } > > - err = ftrace_startup(ops, 0); > + /* Removing the tmp_ops will add the updated direct callers to the functions */ > + unregister_ftrace_function(&tmp_ops); > > out_unlock: > mutex_unlock(&ftrace_lock); > + out_direct: > mutex_unlock(&direct_mutex); > return err; > } > -- > 2.31.1 >
On Tue, Oct 19, 2021 at 03:19:48PM +0200, Jiri Olsa wrote: > On Mon, Oct 18, 2021 at 10:10:15PM -0400, Steven Rostedt wrote: > > On Sat, 16 Oct 2021 13:39:55 +0200 > > Jiri Olsa <jolsa@redhat.com> wrote: > > > > > On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote: > > > > On Fri, 15 Oct 2021 14:05:25 +0200 > > > > Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > > ATM I'm bit stuck on the bpf side of this whole change, I'll test > > > > > it with my other changes when I unstuck myself ;-) > > > > > > > > If you want, I'll apply this as a separate change on top of your patch set. > > > > As I don't see anything wrong with your current code. > > > > > > > > And when you are satisfied with this, just give me a "tested-by" and I'll > > > > push it too. > > > > > > sounds great, thanks > > > jirka > > > > Would you want to ack/review this? > > hum, do you have it in some branch already? I'm getting: > > patching file kernel/trace/ftrace.c > Hunk #1 succeeded at 5521 with fuzz 1 (offset -40 lines). > Hunk #2 FAILED at 5576. > Hunk #3 succeeded at 5557 (offset -44 lines). > 1 out of 3 hunks FAILED -- saving rejects to file kernel/trace/ftrace.c.rej > > > when trying to apply on top of my changes I updated my ftrace/direct branch, it actually still had the previous version.. sorry, perhaps this is the cause of fuzz jirka > > thanks, > jirka > > > > > -- Steve > > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > Subject: [PATCH] ftrace/direct: Do not disable when switching direct callers > > > > Currently to switch a set of "multi" direct trampolines from one > > trampoline to another, a full shutdown of the current set needs to be > > done, followed by an update to what trampoline the direct callers would > > call, and then re-enabling the callers. This leaves a time when the > > functions will not be calling anything, and events may be missed. > > > > Instead, use a trick to allow all the functions with direct trampolines > > attached will always call either the new or old trampoline while the > > switch is happening. To do this, first attach a "dummy" callback via > > ftrace to all the functions that the current direct trampoline is attached > > to. This will cause the functions to call the "list func" instead of the > > direct trampoline. The list function will call the direct trampoline > > "helper" that will set the function it should call as it returns back to > > the ftrace trampoline. > > > > At this moment, the direct caller descriptor can safely update the direct > > call trampoline. The list function will pick either the new or old > > function (depending on the memory coherency model of the architecture). > > > > Now removing the dummy function from each of the locations of the direct > > trampoline caller, will put back the direct call, but now to the new > > trampoline. > > > > A better visual is: > > > > [ Changing direct call from my_direct_1 to my_direct_2 ] > > > > <traced_func>: > > call my_direct_1 > > > > |||||||||||||||||||| > > vvvvvvvvvvvvvvvvvvvv > > > > <traced_func>: > > call ftrace_caller > > > > <ftrace_caller>: > > [..] > > call ftrace_ops_list_func > > > > ftrace_ops_list_func() > > { > > ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2 > > } > > > > call rax (to either my_direct_1 or my_direct_2 > > > > |||||||||||||||||||| > > vvvvvvvvvvvvvvvvvvvv > > > > <traced_func>: > > call my_direct_2 > > > > Link: https://lore.kernel.org/all/20211014162819.5c85618b@gandalf.local.home/ > > > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > --- > > kernel/trace/ftrace.c | 33 ++++++++++++++++++++------------- > > 1 file changed, 20 insertions(+), 13 deletions(-) > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index 30120342176e..7ad1e8ae5855 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi); > > */ > > int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > > { > > - struct ftrace_hash *hash = ops->func_hash->filter_hash; > > + struct ftrace_hash *hash; > > struct ftrace_func_entry *entry, *iter; > > + static struct ftrace_ops tmp_ops = { > > + .func = ftrace_stub, > > + .flags = FTRACE_OPS_FL_STUB, > > + }; > > int i, size; > > int err; > > > > @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > > return -EINVAL; > > > > mutex_lock(&direct_mutex); > > - mutex_lock(&ftrace_lock); > > + > > + /* Enable the tmp_ops to have the same functions as the direct ops */ > > + ftrace_ops_init(&tmp_ops); > > + tmp_ops.func_hash = ops->func_hash; > > + > > + err = register_ftrace_function(&tmp_ops); > > + if (err) > > + goto out_direct; > > > > /* > > - * Shutdown the ops, change 'direct' pointer for each > > - * ops entry in direct_functions hash and startup the > > - * ops back again. > > - * > > - * Note there is no callback called for @ops object after > > - * this ftrace_shutdown call until ftrace_startup is called > > - * later on. > > + * Now the ftrace_ops_list_func() is called to do the direct callers. > > + * We can safely change the direct functions attached to each entry. > > */ > > - err = ftrace_shutdown(ops, 0); > > - if (err) > > - goto out_unlock; > > + mutex_lock(&ftrace_lock); > > > > + hash = ops->func_hash->filter_hash; > > size = 1 << hash->size_bits; > > for (i = 0; i < size; i++) { > > hlist_for_each_entry(iter, &hash->buckets[i], hlist) { > > @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) > > } > > } > > > > - err = ftrace_startup(ops, 0); > > + /* Removing the tmp_ops will add the updated direct callers to the functions */ > > + unregister_ftrace_function(&tmp_ops); > > > > out_unlock: > > mutex_unlock(&ftrace_lock); > > + out_direct: > > mutex_unlock(&direct_mutex); > > return err; > > } > > -- > > 2.31.1 > >
On Tue, 19 Oct 2021 15:26:21 +0200 Jiri Olsa <jolsa@redhat.com> wrote: > > when trying to apply on top of my changes > > I updated my ftrace/direct branch, it actually still had the previous > version.. sorry, perhaps this is the cause of fuzz I just pushed it (including your patches) here: git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git ftrace/core This is where I keep my WIP code. It should not be used to base anything off of, as I rebase it constantly. But it has the current version I plan on testing. You can make sure the patches in there have your latest version, as you can review my patch. I'll update the tags if you give me one. -- Steve
On Tue, Oct 19, 2021 at 09:32:16AM -0400, Steven Rostedt wrote: > On Tue, 19 Oct 2021 15:26:21 +0200 > Jiri Olsa <jolsa@redhat.com> wrote: > > > > when trying to apply on top of my changes > > > > I updated my ftrace/direct branch, it actually still had the previous > > version.. sorry, perhaps this is the cause of fuzz > > I just pushed it (including your patches) here: > > git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git > > ftrace/core > > > This is where I keep my WIP code. It should not be used to base anything > off of, as I rebase it constantly. But it has the current version I plan on > testing. > > You can make sure the patches in there have your latest version, as you can > review my patch. I'll update the tags if you give me one. I'm getting error when compiling: CC kernel/trace/ftrace.o kernel/trace/ftrace.c: In function ‘modify_ftrace_direct_multi’: kernel/trace/ftrace.c:5608:2: error: label ‘out_unlock’ defined but not used [-Werror=unused-label] 5608 | out_unlock: | ^~~~~~~~~~ looks like out_unlock is nolonger needed, I removed it jirka
On Tue, 19 Oct 2021 16:03:03 +0200 Jiri Olsa <jolsa@redhat.com> wrote: > > You can make sure the patches in there have your latest version, as you can > > review my patch. I'll update the tags if you give me one. > > I'm getting error when compiling: > > CC kernel/trace/ftrace.o > kernel/trace/ftrace.c: In function ‘modify_ftrace_direct_multi’: > kernel/trace/ftrace.c:5608:2: error: label ‘out_unlock’ defined but not used [-Werror=unused-label] Ah, I don't think I've been hit by the "-Werror" yet ;-) > 5608 | out_unlock: > | ^~~~~~~~~~ > > looks like out_unlock is nolonger needed, I removed it My tests would have found this, as it has a check for "new warnings". Anyway, was this in your latest patch, or did I pull in and older one? That is, should I expect a v2 from you? -- Steve
On Tue, Oct 19, 2021 at 10:44:11AM -0400, Steven Rostedt wrote: > On Tue, 19 Oct 2021 16:03:03 +0200 > Jiri Olsa <jolsa@redhat.com> wrote: > > > > You can make sure the patches in there have your latest version, as you can > > > review my patch. I'll update the tags if you give me one. > > > > I'm getting error when compiling: > > > > CC kernel/trace/ftrace.o > > kernel/trace/ftrace.c: In function ‘modify_ftrace_direct_multi’: > > kernel/trace/ftrace.c:5608:2: error: label ‘out_unlock’ defined but not used [-Werror=unused-label] > > Ah, I don't think I've been hit by the "-Werror" yet ;-) > > > > 5608 | out_unlock: > > | ^~~~~~~~~~ > > > > looks like out_unlock is nolonger needed, I removed it > > My tests would have found this, as it has a check for "new warnings". > > Anyway, was this in your latest patch, or did I pull in and older one? > > That is, should I expect a v2 from you? it's on top of your ftrace/core, in your change: e62d91d8206e ftrace/direct: Do not disable when switching direct callers just removing the label will fix it also you can add my ack Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index ba5d02ba8166..c15b767f39cf 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -318,6 +318,8 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry, unsigned long ftrace_find_rec_direct(unsigned long ip); int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr); int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr); +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr); + #else struct ftrace_ops; # define ftrace_direct_func_count 0 @@ -357,6 +359,10 @@ static inline int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigne { return -ENODEV; } +static inline int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) +{ + return -ENODEV; +} #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */ #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index f9df7bffb770..d92f2591c3fc 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -5547,6 +5547,68 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) return err; } EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi); + +/** + * modify_ftrace_direct_multi - Modify an existing direct 'multi' call + * to call something else + * @ops: The address of the struct ftrace_ops object + * @addr: The address of the new trampoline to call at @ops functions + * + * This is used to unregister currently registered direct caller and + * register new one @addr on functions registered in @ops object. + * + * Note there's window between ftrace_shutdown and ftrace_startup calls + * where there will be no callbacks called. + * + * Returns: zero on success. Non zero on error, which includes: + * -EINVAL - The @ops object was not properly registered. + */ +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) +{ + struct ftrace_hash *hash = ops->func_hash->filter_hash; + struct ftrace_func_entry *entry, *iter; + int i, size; + int err; + + if (check_direct_multi(ops)) + return -EINVAL; + if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) + return -EINVAL; + + mutex_lock(&direct_mutex); + mutex_lock(&ftrace_lock); + + /* + * Shutdown the ops, change 'direct' pointer for each + * ops entry in direct_functions hash and startup the + * ops back again. + * + * Note there is no callback called for @ops object after + * this ftrace_shutdown call until ftrace_startup is called + * later on. + */ + err = ftrace_shutdown(ops, 0); + if (err) + goto out_unlock; + + size = 1 << hash->size_bits; + for (i = 0; i < size; i++) { + hlist_for_each_entry(iter, &hash->buckets[i], hlist) { + entry = __ftrace_lookup_ip(direct_functions, iter->ip); + if (!entry) + continue; + entry->direct = addr; + } + } + + err = ftrace_startup(ops, 0); + + out_unlock: + mutex_unlock(&ftrace_lock); + mutex_unlock(&direct_mutex); + return err; +} +EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi); #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */ /**
Adding interface to modify registered direct function for ftrace_ops. Adding following function: modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr) The function changes the currently registered direct function for all attached functions. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/linux/ftrace.h | 6 ++++ kernel/trace/ftrace.c | 62 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)