Re: [PATCH v18 11/17] fprobe: Rewrite fprobe on function-graph tracer

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 1 Nov 2024 15:28:44 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 26 Oct 2024 13:37:43 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> > index ef609bcca0f9..686b30ce48b4 100644
> > --- a/include/linux/fprobe.h
> > +++ b/include/linux/fprobe.h
> > @@ -5,10 +5,11 @@
> >  
> >  #include <linux/compiler.h>
> >  #include <linux/ftrace.h>
> > -#include <linux/rethook.h>
> > +#include <linux/rcupdate.h>
> > +#include <linux/refcount.h>
> > +#include <linux/slab.h>
> >  
> >  struct fprobe;
> > -
> >  typedef int (*fprobe_entry_cb)(struct fprobe *fp, unsigned long entry_ip,
> >  			       unsigned long ret_ip, struct ftrace_regs *regs,
> >  			       void *entry_data);
> > @@ -17,35 +18,57 @@ typedef void (*fprobe_exit_cb)(struct fprobe *fp, unsigned long entry_ip,
> >  			       unsigned long ret_ip, struct ftrace_regs *regs,
> >  			       void *entry_data);
> >  
> > +/**
> > + * strcut fprobe_hlist_node - address based hash list node for fprobe.
> 
>       struct
> 

oops, thanks.

> > + *
> > + * @hlist: The hlist node for address search hash table.
> > + * @addr: The address represented by this.
> 
>   What is "this" in the above?

it should be `by this node.`

> 
> > + * @fp: The fprobe which owns this.
> > + */
> > +struct fprobe_hlist_node {
> > +	struct hlist_node	hlist;
> > +	unsigned long		addr;
> > +	struct fprobe		*fp;
> > +};
> > +
> > +/**
> > + * struct fprobe_hlist - hash list nodes for fprobe.
> > + *
> > + * @hlist: The hlist node for existence checking hash table.
> > + * @rcu: rcu_head for RCU deferred release.
> > + * @fp: The fprobe which owns this fprobe_hlist.
> > + * @size: The size of @array.
> > + * @array: The fprobe_hlist_node for each address to probe.
> > + */
> > +struct fprobe_hlist {
> > +	struct hlist_node		hlist;
> > +	struct rcu_head			rcu;
> > +	struct fprobe			*fp;
> > +	int				size;
> > +	struct fprobe_hlist_node	array[];
> 
> Should the above have __counted_by(size) ?

Yes. Thanks!


> 
> -- Steve
> 
> > +};
> > +
> >  /**
> >   * struct fprobe - ftrace based probe.
> > - * @ops: The ftrace_ops.
> > + *
> >   * @nmissed: The counter for missing events.
> >   * @flags: The status flag.
> > - * @rethook: The rethook data structure. (internal data)
> >   * @entry_data_size: The private data storage size.
> > - * @nr_maxactive: The max number of active functions.
> > + * @nr_maxactive: The max number of active functions. (*deprecated)
> >   * @entry_handler: The callback function for function entry.
> >   * @exit_handler: The callback function for function exit.
> > + * @hlist_array: The fprobe_hlist for fprobe search from IP hash table.
> >   */
> >  struct fprobe {
> > -#ifdef CONFIG_FUNCTION_TRACER
> > -	/*
> > -	 * If CONFIG_FUNCTION_TRACER is not set, CONFIG_FPROBE is disabled too.
> > -	 * But user of fprobe may keep embedding the struct fprobe on their own
> > -	 * code. To avoid build error, this will keep the fprobe data structure
> > -	 * defined here, but remove ftrace_ops data structure.
> > -	 */
> > -	struct ftrace_ops	ops;
> > -#endif
> >  	unsigned long		nmissed;
> >  	unsigned int		flags;
> > -	struct rethook		*rethook;
> >  	size_t			entry_data_size;
> >  	int			nr_maxactive;
> >  
> >  	fprobe_entry_cb entry_handler;
> >  	fprobe_exit_cb  exit_handler;
> > +
> > +	struct fprobe_hlist	*hlist_array;
> >  };
> >  


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>