On Tue, 25 Mar 2025 14:41:11 -0400 Steven Rostedt <[email protected]> wrote:
> On Sun, 16 Mar 2025 21:21:42 +0900 > "Masami Hiramatsu (Google)" <[email protected]> wrote: > > > From: Masami Hiramatsu (Google) <[email protected]> > > > > Currently fprobe events are registered when it is defined. Thus it will > > give some overhead even if it is disabled. This changes it to register the > > fprobe only when it is enabled. > > > > Suggested-by: Steven Rostedt <[email protected]> > > Signed-off-by: Masami Hiramatsu (Google) <[email protected]> > > --- > > include/linux/fprobe.h | 8 + > > kernel/trace/fprobe.c | 29 +++-- > > kernel/trace/trace_fprobe.c | 234 > > +++++++++++++++++++++---------------------- > > 3 files changed, 140 insertions(+), 131 deletions(-) > > > > diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h > > index 702099f08929..9635a24d5a25 100644 > > --- a/include/linux/fprobe.h > > +++ b/include/linux/fprobe.h > > @@ -94,6 +94,8 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long > > *addrs, int num); > > int register_fprobe_syms(struct fprobe *fp, const char **syms, int num); > > int unregister_fprobe(struct fprobe *fp); > > bool fprobe_is_registered(struct fprobe *fp); > > +int fprobe_alloc_ip_list_from_filter(const char *filter, const char > > *notfilter, > > + unsigned long **addrs); > > #else > > static inline int register_fprobe(struct fprobe *fp, const char *filter, > > const char *notfilter) > > { > > @@ -115,6 +117,12 @@ static inline bool fprobe_is_registered(struct fprobe > > *fp) > > { > > return false; > > } > > +static inline int fprobe_alloc_ip_list_from_filter(const char *filter, > > + const char *notfilter, > > + unsigned long **addrs) > > +{ > > + return -EOPNOTSUPP; > > +} > > #endif > > > > /** > > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c > > index 33082c4e8154..05050f1c2239 100644 > > --- a/kernel/trace/fprobe.c > > +++ b/kernel/trace/fprobe.c > > @@ -486,6 +486,24 @@ static int ip_list_from_filter(const char *filter, > > const char *notfilter, > > return match.index ?: -ENOENT; > > } > > > > +#define FPROBE_IPS_MAX INT_MAX > > + > > +int fprobe_alloc_ip_list_from_filter(const char *filter, const char > > *notfilter, > > + unsigned long **addrs) > > +{ > > + int ret; > > + > > + /* Count the number of ips from filter. */ > > + ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX); > > + if (ret < 0) > > + return ret; > > + > > + *addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL); > > + if (!*addrs) > > + return -ENOMEM; > > + return ip_list_from_filter(filter, notfilter, *addrs, ret); > > This was in the old code, but I'm wondering. Does this code prevent modules > from being loaded and unloaded too? Ah, no. In that case we should do module_get() for each module found in module_kallsyms_on_each_symbol(), hmm. > > I'm asking because if we call the first instance of ip_list_from_filter() > and it finds a list of functions from a module, and then that module is > unloaded, the ip_list_from_filter() will return a failure, and *addrs would > be a memory leak. Good catch! Let me fix it. Thanks, > > -- Steve > > > +} > > + > > static void fprobe_fail_cleanup(struct fprobe *fp) > > { > > kfree(fp->hlist_array); > > @@ -528,8 +546,6 @@ static int fprobe_init(struct fprobe *fp, unsigned long > > *addrs, int num) > > return 0; > > } > > > > -#define FPROBE_IPS_MAX INT_MAX > > - > > /** > > * register_fprobe() - Register fprobe to ftrace by pattern. > > * @fp: A fprobe data structure to be registered. > > @@ -549,14 +565,7 @@ int register_fprobe(struct fprobe *fp, const char > > *filter, const char *notfilter > > if (!fp || !filter) > > return -EINVAL; > > > > - ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX); > > - if (ret < 0) > > - return ret; > > - > > - addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL); > > - if (!addrs) > > - return -ENOMEM; > > - ret = ip_list_from_filter(filter, notfilter, addrs, ret); > > + ret = fprobe_alloc_ip_list_from_filter(filter, notfilter, &addrs); > > if (ret > 0) > > ret = register_fprobe_ips(fp, addrs, ret); > > -- Masami Hiramatsu (Google) <[email protected]>
