| Message ID | 164311270629.1933078.4596694198103138848.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fprobe: Introduce fprobe function entry/exit probe | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 25 Jan 2022 21:11:46 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > From: Jiri Olsa <jolsa@redhat.com> > > Adding ftrace_set_filter_ips function to be able to set filter on > multiple ip addresses at once. > > With the kprobe multi attach interface we have cases where we need to > initialize ftrace_ops object with thousands of functions, so having > single function diving into ftrace_hash_move_and_update_ops with > ftrace_lock is faster. > > The functions ips are passed as unsigned long array with count. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/ftrace.h | 3 +++ > kernel/trace/ftrace.c | 53 ++++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 47 insertions(+), 9 deletions(-) > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 9999e29187de..60847cbce0da 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -512,6 +512,8 @@ struct dyn_ftrace { > > int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, > int remove, int reset); > +int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips, > + unsigned int cnt, int remove, int reset); > int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf, > int len, int reset); > int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf, > @@ -802,6 +804,7 @@ static inline unsigned long ftrace_location(unsigned long ip) > #define ftrace_regex_open(ops, flag, inod, file) ({ -ENODEV; }) > #define ftrace_set_early_filter(ops, buf, enable) do { } while (0) > #define ftrace_set_filter_ip(ops, ip, remove, reset) ({ -ENODEV; }) > +#define ftrace_set_filter_ips(ops, ips, cnt, remove, reset) ({ -ENODEV; }) > #define ftrace_set_filter(ops, buf, len, reset) ({ -ENODEV; }) > #define ftrace_set_notrace(ops, buf, len, reset) ({ -ENODEV; }) > #define ftrace_free_filter(ops) do { } while (0) > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index be5f6b32a012..39350aa38649 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -4958,7 +4958,7 @@ ftrace_notrace_write(struct file *file, const char __user *ubuf, > } > > static int > -ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > +__ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > { > struct ftrace_func_entry *entry; > > @@ -4976,9 +4976,25 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > return add_hash_entry(hash, ip); > } > > +static int > +ftrace_match_addr(struct ftrace_hash *hash, unsigned long *ips, > + unsigned int cnt, int remove) > +{ > + unsigned int i; > + int err; > + > + for (i = 0; i < cnt; i++) { > + err = __ftrace_match_addr(hash, ips[i], remove); > + if (err) > + return err; On error should we revert what was done? goto err; > + } > + return 0; err: for (i--; i >= 0; i--) __ftrace_match_addr(hash, ips[i], !remove); return err; Although it may not matter as it looks like it is only used on a temporary hash. But either it should be commented that is the case, or we do the above just to be more robust. > +} > + > static int > ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > - unsigned long ip, int remove, int reset, int enable) > + unsigned long *ips, unsigned int cnt, > + int remove, int reset, int enable) > { > struct ftrace_hash **orig_hash; > struct ftrace_hash *hash; > @@ -5008,8 +5024,8 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > ret = -EINVAL; > goto out_regex_unlock; > } > - if (ip) { > - ret = ftrace_match_addr(hash, ip, remove); > + if (ips) { > + ret = ftrace_match_addr(hash, ips, cnt, remove); > if (ret < 0) > goto out_regex_unlock; > } > @@ -5026,10 +5042,10 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > } > > static int > -ftrace_set_addr(struct ftrace_ops *ops, unsigned long ip, int remove, > - int reset, int enable) > +ftrace_set_addr(struct ftrace_ops *ops, unsigned long *ips, unsigned int cnt, > + int remove, int reset, int enable) > { > - return ftrace_set_hash(ops, NULL, 0, ip, remove, reset, enable); > + return ftrace_set_hash(ops, NULL, 0, ips, cnt, remove, reset, enable); > } > > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > @@ -5634,10 +5650,29 @@ int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, > int remove, int reset) > { > ftrace_ops_init(ops); > - return ftrace_set_addr(ops, ip, remove, reset, 1); > + return ftrace_set_addr(ops, &ip, 1, remove, reset, 1); > } > EXPORT_SYMBOL_GPL(ftrace_set_filter_ip); > > +/** > + * ftrace_set_filter_ips - set a functions to filter on in ftrace by addresses - set functions to filter on ... -- Steve > + * @ops - the ops to set the filter with > + * @ips - the array of addresses to add to or remove from the filter. > + * @cnt - the number of addresses in @ips > + * @remove - non zero to remove ips from the filter > + * @reset - non zero to reset all filters before applying this filter. > + * > + * Filters denote which functions should be enabled when tracing is enabled > + * If @ips array or any ip specified within is NULL , it fails to update filter. > + */ > +int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips, > + unsigned int cnt, int remove, int reset) > +{ > + ftrace_ops_init(ops); > + return ftrace_set_addr(ops, ips, cnt, remove, reset, 1); > +} > +EXPORT_SYMBOL_GPL(ftrace_set_filter_ips); > + > /** > * ftrace_ops_set_global_filter - setup ops to use global filters > * @ops - the ops which will use the global filters > @@ -5659,7 +5694,7 @@ static int > ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len, > int reset, int enable) > { > - return ftrace_set_hash(ops, buf, len, 0, 0, reset, enable); > + return ftrace_set_hash(ops, buf, len, NULL, 0, 0, reset, enable); > } > > /**
On Tue, Jan 25, 2022 at 11:06:59AM -0500, Steven Rostedt wrote: SNIP > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index be5f6b32a012..39350aa38649 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -4958,7 +4958,7 @@ ftrace_notrace_write(struct file *file, const char __user *ubuf, > > } > > > > static int > > -ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > > +__ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > > { > > struct ftrace_func_entry *entry; > > > > @@ -4976,9 +4976,25 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > > return add_hash_entry(hash, ip); > > } > > > > +static int > > +ftrace_match_addr(struct ftrace_hash *hash, unsigned long *ips, > > + unsigned int cnt, int remove) > > +{ > > + unsigned int i; > > + int err; > > + > > + for (i = 0; i < cnt; i++) { > > + err = __ftrace_match_addr(hash, ips[i], remove); > > + if (err) > > + return err; > > On error should we revert what was done? > > goto err; > > + } > > + return 0; > > err: > for (i--; i >= 0; i--) > __ftrace_match_addr(hash, ips[i], !remove); > return err; > > Although it may not matter as it looks like it is only used on a temporary > hash. But either it should be commented that is the case, or we do the above > just to be more robust. yes, that's the case.. it populates just the hash at this point and if __ftrace_match_addr fails, the thehash is relased after jumping to out_regex_unlock > > > +} > > + > > static int > > ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > > - unsigned long ip, int remove, int reset, int enable) > > + unsigned long *ips, unsigned int cnt, > > + int remove, int reset, int enable) > > { > > struct ftrace_hash **orig_hash; > > struct ftrace_hash *hash; > > @@ -5008,8 +5024,8 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > > ret = -EINVAL; > > goto out_regex_unlock; > > } > > - if (ip) { > > - ret = ftrace_match_addr(hash, ip, remove); > > + if (ips) { > > + ret = ftrace_match_addr(hash, ips, cnt, remove); > > if (ret < 0) > > goto out_regex_unlock; > > } > > @@ -5026,10 +5042,10 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > > } > > > > static int > > -ftrace_set_addr(struct ftrace_ops *ops, unsigned long ip, int remove, > > - int reset, int enable) > > +ftrace_set_addr(struct ftrace_ops *ops, unsigned long *ips, unsigned int cnt, > > + int remove, int reset, int enable) > > { > > - return ftrace_set_hash(ops, NULL, 0, ip, remove, reset, enable); > > + return ftrace_set_hash(ops, NULL, 0, ips, cnt, remove, reset, enable); > > } > > > > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > > @@ -5634,10 +5650,29 @@ int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, > > int remove, int reset) > > { > > ftrace_ops_init(ops); > > - return ftrace_set_addr(ops, ip, remove, reset, 1); > > + return ftrace_set_addr(ops, &ip, 1, remove, reset, 1); > > } > > EXPORT_SYMBOL_GPL(ftrace_set_filter_ip); > > > > +/** > > + * ftrace_set_filter_ips - set a functions to filter on in ftrace by addresses > > - set functions to filter on ... will fix, thanks, jirka
Hi Jiri, On Tue, 25 Jan 2022 17:46:07 +0100 Jiri Olsa <jolsa@redhat.com> wrote: > On Tue, Jan 25, 2022 at 11:06:59AM -0500, Steven Rostedt wrote: > > SNIP > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > > index be5f6b32a012..39350aa38649 100644 > > > --- a/kernel/trace/ftrace.c > > > +++ b/kernel/trace/ftrace.c > > > @@ -4958,7 +4958,7 @@ ftrace_notrace_write(struct file *file, const char __user *ubuf, > > > } > > > > > > static int > > > -ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > > > +__ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > > > { > > > struct ftrace_func_entry *entry; > > > > > > @@ -4976,9 +4976,25 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) > > > return add_hash_entry(hash, ip); > > > } > > > > > > +static int > > > +ftrace_match_addr(struct ftrace_hash *hash, unsigned long *ips, > > > + unsigned int cnt, int remove) > > > +{ > > > + unsigned int i; > > > + int err; > > > + > > > + for (i = 0; i < cnt; i++) { > > > + err = __ftrace_match_addr(hash, ips[i], remove); > > > + if (err) > > > + return err; > > > > On error should we revert what was done? > > > > goto err; > > > + } > > > + return 0; > > > > err: > > for (i--; i >= 0; i--) > > __ftrace_match_addr(hash, ips[i], !remove); > > return err; > > > > Although it may not matter as it looks like it is only used on a temporary > > hash. But either it should be commented that is the case, or we do the above > > just to be more robust. > > yes, that's the case.. it populates just the hash at this point > and if __ftrace_match_addr fails, the thehash is relased after > jumping to out_regex_unlock > > > > > > +} > > > + > > > static int > > > ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > > > - unsigned long ip, int remove, int reset, int enable) > > > + unsigned long *ips, unsigned int cnt, > > > + int remove, int reset, int enable) > > > { > > > struct ftrace_hash **orig_hash; > > > struct ftrace_hash *hash; > > > @@ -5008,8 +5024,8 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > > > ret = -EINVAL; > > > goto out_regex_unlock; > > > } > > > - if (ip) { > > > - ret = ftrace_match_addr(hash, ip, remove); > > > + if (ips) { > > > + ret = ftrace_match_addr(hash, ips, cnt, remove); > > > if (ret < 0) > > > goto out_regex_unlock; > > > } > > > @@ -5026,10 +5042,10 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, > > > } > > > > > > static int > > > -ftrace_set_addr(struct ftrace_ops *ops, unsigned long ip, int remove, > > > - int reset, int enable) > > > +ftrace_set_addr(struct ftrace_ops *ops, unsigned long *ips, unsigned int cnt, > > > + int remove, int reset, int enable) > > > { > > > - return ftrace_set_hash(ops, NULL, 0, ip, remove, reset, enable); > > > + return ftrace_set_hash(ops, NULL, 0, ips, cnt, remove, reset, enable); > > > } > > > > > > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > > > @@ -5634,10 +5650,29 @@ int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, > > > int remove, int reset) > > > { > > > ftrace_ops_init(ops); > > > - return ftrace_set_addr(ops, ip, remove, reset, 1); > > > + return ftrace_set_addr(ops, &ip, 1, remove, reset, 1); > > > } > > > EXPORT_SYMBOL_GPL(ftrace_set_filter_ip); > > > > > > +/** > > > + * ftrace_set_filter_ips - set a functions to filter on in ftrace by addresses > > > > - set functions to filter on ... > > will fix, So, I wrote a below change for the next version. Is that OK for you? Thank you, diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index f305e18f699f..a28b1bdb234a 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -4985,8 +4985,13 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned long *ips, for (i = 0; i < cnt; i++) { err = __ftrace_match_addr(hash, ips[i], remove); - if (err) + if (err) { + /* + * This expects the @hash is a temporary hash and if this + * fails the caller must free the @hash. + */ return err; + } } return 0; } @@ -5649,7 +5654,7 @@ int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, EXPORT_SYMBOL_GPL(ftrace_set_filter_ip); /** - * ftrace_set_filter_ips - set a functions to filter on in ftrace by addresses + * ftrace_set_filter_ips - set functions to filter on in ftrace by addresses * @ops - the ops to set the filter with * @ips - the array of addresses to add to or remove from the filter. * @cnt - the number of addresses in @ips > > thanks, > jirka >
On Fri, Jan 28, 2022 at 11:05:23AM +0900, Masami Hiramatsu wrote: SNIP > > So, I wrote a below change for the next version. Is that OK for you? > > Thank you, > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index f305e18f699f..a28b1bdb234a 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -4985,8 +4985,13 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned long *ips, > > for (i = 0; i < cnt; i++) { > err = __ftrace_match_addr(hash, ips[i], remove); > - if (err) > + if (err) { > + /* > + * This expects the @hash is a temporary hash and if this > + * fails the caller must free the @hash. > + */ > return err; > + } > } > return 0; > } > @@ -5649,7 +5654,7 @@ int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, > EXPORT_SYMBOL_GPL(ftrace_set_filter_ip); > > /** > - * ftrace_set_filter_ips - set a functions to filter on in ftrace by addresses > + * ftrace_set_filter_ips - set functions to filter on in ftrace by addresses > * @ops - the ops to set the filter with > * @ips - the array of addresses to add to or remove from the filter. > * @cnt - the number of addresses in @ips looks good, thanks jirka
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 9999e29187de..60847cbce0da 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -512,6 +512,8 @@ struct dyn_ftrace { int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, int remove, int reset); +int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips, + unsigned int cnt, int remove, int reset); int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf, int len, int reset); int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf, @@ -802,6 +804,7 @@ static inline unsigned long ftrace_location(unsigned long ip) #define ftrace_regex_open(ops, flag, inod, file) ({ -ENODEV; }) #define ftrace_set_early_filter(ops, buf, enable) do { } while (0) #define ftrace_set_filter_ip(ops, ip, remove, reset) ({ -ENODEV; }) +#define ftrace_set_filter_ips(ops, ips, cnt, remove, reset) ({ -ENODEV; }) #define ftrace_set_filter(ops, buf, len, reset) ({ -ENODEV; }) #define ftrace_set_notrace(ops, buf, len, reset) ({ -ENODEV; }) #define ftrace_free_filter(ops) do { } while (0) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index be5f6b32a012..39350aa38649 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -4958,7 +4958,7 @@ ftrace_notrace_write(struct file *file, const char __user *ubuf, } static int -ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) +__ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) { struct ftrace_func_entry *entry; @@ -4976,9 +4976,25 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned long ip, int remove) return add_hash_entry(hash, ip); } +static int +ftrace_match_addr(struct ftrace_hash *hash, unsigned long *ips, + unsigned int cnt, int remove) +{ + unsigned int i; + int err; + + for (i = 0; i < cnt; i++) { + err = __ftrace_match_addr(hash, ips[i], remove); + if (err) + return err; + } + return 0; +} + static int ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, - unsigned long ip, int remove, int reset, int enable) + unsigned long *ips, unsigned int cnt, + int remove, int reset, int enable) { struct ftrace_hash **orig_hash; struct ftrace_hash *hash; @@ -5008,8 +5024,8 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, ret = -EINVAL; goto out_regex_unlock; } - if (ip) { - ret = ftrace_match_addr(hash, ip, remove); + if (ips) { + ret = ftrace_match_addr(hash, ips, cnt, remove); if (ret < 0) goto out_regex_unlock; } @@ -5026,10 +5042,10 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char *buf, int len, } static int -ftrace_set_addr(struct ftrace_ops *ops, unsigned long ip, int remove, - int reset, int enable) +ftrace_set_addr(struct ftrace_ops *ops, unsigned long *ips, unsigned int cnt, + int remove, int reset, int enable) { - return ftrace_set_hash(ops, NULL, 0, ip, remove, reset, enable); + return ftrace_set_hash(ops, NULL, 0, ips, cnt, remove, reset, enable); } #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS @@ -5634,10 +5650,29 @@ int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip, int remove, int reset) { ftrace_ops_init(ops); - return ftrace_set_addr(ops, ip, remove, reset, 1); + return ftrace_set_addr(ops, &ip, 1, remove, reset, 1); } EXPORT_SYMBOL_GPL(ftrace_set_filter_ip); +/** + * ftrace_set_filter_ips - set a functions to filter on in ftrace by addresses + * @ops - the ops to set the filter with + * @ips - the array of addresses to add to or remove from the filter. + * @cnt - the number of addresses in @ips + * @remove - non zero to remove ips from the filter + * @reset - non zero to reset all filters before applying this filter. + * + * Filters denote which functions should be enabled when tracing is enabled + * If @ips array or any ip specified within is NULL , it fails to update filter. + */ +int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips, + unsigned int cnt, int remove, int reset) +{ + ftrace_ops_init(ops); + return ftrace_set_addr(ops, ips, cnt, remove, reset, 1); +} +EXPORT_SYMBOL_GPL(ftrace_set_filter_ips); + /** * ftrace_ops_set_global_filter - setup ops to use global filters * @ops - the ops which will use the global filters @@ -5659,7 +5694,7 @@ static int ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len, int reset, int enable) { - return ftrace_set_hash(ops, buf, len, 0, 0, reset, enable); + return ftrace_set_hash(ops, buf, len, NULL, 0, 0, reset, enable); } /**