Message-Id: <1196124850.4862.7.camel@dyn9047018096.beaverton.ibm.com>
Date:	Mon, 26 Nov 2007 16:54:10 -0800
From:	Jim Keniston <jkenisto@...ibm.com>
To:	Abhishek Sagar <sagar.abhishek@...il.com>
Cc:	Srinivasa Ds <srinivasa@...ibm.com>, linux-kernel@...r.kernel.org,
	prasanna@...ibm.com, davem@...emloft.net,
	anil.s.keshavamurthy@...el.com,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>
Subject: Re: [PATCH][RFC] kprobes: Add user entry-handler in kretprobes

On Wed, 2007-11-21 at 15:50 +0530, Abhishek Sagar wrote:
> On 11/21/07, Jim Keniston <jkenisto@...ibm.com> wrote:
> > I have one minor suggestion on the code -- see below -- but I'm willing
> > to ack that with or without the suggested change.  Please also see
> > suggestions on kprobes.txt and the demo program.
> 
> Thanks...I've made the necessary changes. More comments below.
> 
> > It would be more consistent with the existing text in kprobes.txt to add
> > a subsection labeled "User's entry handler (rp->entry_handler):" and
> > document the entry_handler there.
> 
> I've moved almost all of the entry_handler discussion in a separate section.
> 
> > >  static struct kretprobe my_kretprobe = {
> > > -     .handler = ret_handler,
> > > -     /* Probe up to 20 instances concurrently. */
> > > -     .maxactive = 20
> > > +     .handler = return_handler,
> > > +     .entry_handler = entry_handler,
> > > +     .data_size = sizeof(struct my_data),
> > > +     .maxactive = 1, /* profile one invocation at a time */
> >
> > I don't like the idea of setting maxactive = 1 here.  That's not normal
> > kretprobes usage, which is what we're trying to illustrate here.  This
> > is no place for splitting hairs about profiling recursive functions.
> 
> I've reverted back to ".maxactive = 20". In any case, there is no need
> to protect against recursive instances of sys_open.
> 
> > > @@ -699,6 +699,14 @@ static int __kprobes pre_handler_kretpro
> > >                                struct kretprobe_instance, uflist);
> > >               ri->rp = rp;
> > >               ri->task = current;
> > > +
> > > +             if (rp->entry_handler) {
> > > +                     if (rp->entry_handler(ri, regs)) {
> >
> > Could also be
> >        if (rp->entry_handler && rp->entry_handler(ri, regs)) {
> 
> Done.
> 
> ---
> Signed-off-by: Abhishek Sagar <sagar.abhishek@...il.com>

Acked-by: Jim Keniston <jkenisto@...ibm.com>

This works for me with the revised kretprobe-example.c and with another
test program I had lying around.

Jim

> 
> diff -upNr linux-2.6.24-rc3/Documentation/kprobes.txt
> linux-2.6.24-rc3_kp/Documentation/kprobes.txt
> --- linux-2.6.24-rc3/Documentation/kprobes.txt	2007-11-17
> 10:46:36.000000000 +0530
> +++ linux-2.6.24-rc3_kp/Documentation/kprobes.txt	2007-11-21
> 15:20:53.000000000 +0530
> @@ -96,7 +96,9 @@ or in registers (e.g., for x86_64 or for
>  The jprobe will work in either case, so long as the handler's
>  prototype matches that of the probed function.
> 
> -1.3 How Does a Return Probe Work?
> +1.3 Return Probes
> +
> +1.3.1 How Does a Return Probe Work?
> 
>  When you call register_kretprobe(), Kprobes establishes a kprobe at
>  the entry to the function.  When the probed function is called and this
> @@ -107,7 +109,7 @@ At boot time, Kprobes registers a kprobe
> 
>  When the probed function executes its return instruction, control
>  passes to the trampoline and that probe is hit.  Kprobes' trampoline
> -handler calls the user-specified handler associated with the kretprobe,
> +handler calls the user-specified return handler associated with the kretprobe,
>  then sets the saved instruction pointer to the saved return address,
>  and that's where execution resumes upon return from the trap.
> 
> @@ -131,6 +133,30 @@ zero when the return probe is registered
>  time the probed function is entered but there is no kretprobe_instance
>  object available for establishing the return probe.
> 
> +1.3.2 Kretprobe entry-handler
> +
> +Kretprobes also provides an optional user-specified handler which runs
> +on function entry. This handler is specified by setting the entry_handler
> +field of the kretprobe struct. Whenever the kprobe placed by kretprobe at the
> +function entry is hit, the user-defined entry_handler, if any, is invoked.
> +If the entry_handler returns 0 (success) then a corresponding return handler
> +is guaranteed to be called upon function return. If the entry_handler
> +returns a non-zero error then Kprobes leaves the return address as is, and
> +the kretprobe has no further effect for that particular function instance.
> +
> +Multiple entry and return handler invocations are matched using the unique
> +kretprobe_instance object associated with them. Additionally, a user
> +may also specify per return-instance private data to be part of each
> +kretprobe_instance object. This is especially useful when sharing private
> +data between corresponding user entry and return handlers. The size of each
> +private data object can be specified at kretprobe registration time by
> +setting the data_size field of the kretprobe struct. This data can be
> +accessed through the data field of each kretprobe_instance object.
> +
> +In case probed function is entered but there is no kretprobe_instance
> +object available, then in addition to incrementing the nmissed count,
> +the user entry_handler invocation is also skipped.
> +
>  2. Architectures Supported
> 
>  Kprobes, jprobes, and return probes are implemented on the following
> @@ -273,6 +299,8 @@ of interest:
>  - ret_addr: the return address
>  - rp: points to the corresponding kretprobe object
>  - task: points to the corresponding task struct
> +- data: points to per return-instance private data; see "Kretprobe entry-
> +  handler" for details.
> 
>  The regs_return_value(regs) macro provides a simple abstraction to
>  extract the return value from the appropriate register as defined by
> @@ -555,23 +583,46 @@ report failed calls to sys_open().
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/kprobes.h>
> +#include <linux/ktime.h>
> 
>  static const char *probed_func = "sys_open";
> 
> -/* Return-probe handler: If the probed function fails, log the return value. */
> -static int ret_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
> +/* per-instance private data */
> +struct my_data {
> +	ktime_t entry_stamp;
> +};
> +
> +static int entry_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
> +{
> +	struct my_data *data;
> +
> +	if(!current->mm)
> +		return 1; /* skip kernel threads */
> +
> +	data = (struct my_data *)ri->data;
> +	data->entry_stamp = ktime_get();
> +	return 0;
> +}
> +
> +static int return_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
>  {
>  	int retval = regs_return_value(regs);
> -	if (retval < 0) {
> -		printk("%s returns %d\n", probed_func, retval);
> -	}
> +	struct my_data *data = (struct my_data *)ri->data;
> +	s64 delta;
> +	ktime_t now = ktime_get();
> +
> +	delta = ktime_to_ns(ktime_sub(now, data->entry_stamp));
> +	if (retval < 0) /* probed function failed; log retval and duration */
> +		printk("%s: return val = %d (duration = %lld ns)\n",
> +		       probed_func, retval, delta);
>  	return 0;
>  }
> 
>  static struct kretprobe my_kretprobe = {
> -	.handler = ret_handler,
> -	/* Probe up to 20 instances concurrently. */
> -	.maxactive = 20
> +	.handler = return_handler,
> +	.entry_handler = entry_handler,
> +	.data_size = sizeof(struct my_data),
> +	.maxactive = 20, /* probe up to 20 instances concurrently */
>  };
> 
>  static int __init kretprobe_init(void)
> @@ -580,10 +631,10 @@ static int __init kretprobe_init(void)
>  	my_kretprobe.kp.symbol_name = (char *)probed_func;
> 
>  	if ((ret = register_kretprobe(&my_kretprobe)) < 0) {
> -		printk("register_kretprobe failed, returned %d\n", ret);
> +		printk("Failed to register kretprobe!\n");
>  		return -1;
>  	}
> -	printk("Planted return probe at %p\n", my_kretprobe.kp.addr);
> +	printk("Kretprobe active on %s\n", my_kretprobe.kp.symbol_name);
>  	return 0;
>  }
> 
> @@ -591,7 +642,7 @@ static void __exit kretprobe_exit(void)
>  {
>  	unregister_kretprobe(&my_kretprobe);
>  	printk("kretprobe unregistered\n");
> -	/* nmissed > 0 suggests that maxactive was set too low. */
> +	/* nmissed > 0 suggests that maxactive was set too low */
>  	printk("Missed probing %d instances of %s\n",
>  		my_kretprobe.nmissed, probed_func);
>  }
> diff -upNr linux-2.6.24-rc3/include/linux/kprobes.h
> linux-2.6.24-rc3_kp/include/linux/kprobes.h
> --- linux-2.6.24-rc3/include/linux/kprobes.h	2007-11-17 10:46:36.000000000 +0530
> +++ linux-2.6.24-rc3_kp/include/linux/kprobes.h	2007-11-21
> 13:40:48.000000000 +0530
> @@ -152,8 +152,10 @@ static inline int arch_trampoline_kprobe
>  struct kretprobe {
>  	struct kprobe kp;
>  	kretprobe_handler_t handler;
> +	kretprobe_handler_t entry_handler;
>  	int maxactive;
>  	int nmissed;
> +	size_t data_size;
>  	struct hlist_head free_instances;
>  	struct hlist_head used_instances;
>  };
> @@ -164,6 +166,7 @@ struct kretprobe_instance {
>  	struct kretprobe *rp;
>  	kprobe_opcode_t *ret_addr;
>  	struct task_struct *task;
> +	char data[0];
>  };
> 
>  struct kretprobe_blackpoint {
> diff -upNr linux-2.6.24-rc3/kernel/kprobes.c
> linux-2.6.24-rc3_kp/kernel/kprobes.c
> --- linux-2.6.24-rc3/kernel/kprobes.c	2007-11-17 10:46:36.000000000 +0530
> +++ linux-2.6.24-rc3_kp/kernel/kprobes.c	2007-11-21 13:41:57.000000000 +0530
> @@ -699,6 +699,12 @@ static int __kprobes pre_handler_kretpro
>  				 struct kretprobe_instance, uflist);
>  		ri->rp = rp;
>  		ri->task = current;
> +
> +		if (rp->entry_handler && rp->entry_handler(ri, regs)) {
> +				spin_unlock_irqrestore(&kretprobe_lock, flags);
> +				return 0;
> +		}
> +
>  		arch_prepare_kretprobe(ri, regs);
> 
>  		/* XXX(hch): why is there no hlist_move_head? */
> @@ -745,7 +751,8 @@ int __kprobes register_kretprobe(struct
>  	INIT_HLIST_HEAD(&rp->used_instances);
>  	INIT_HLIST_HEAD(&rp->free_instances);
>  	for (i = 0; i < rp->maxactive; i++) {
> -		inst = kmalloc(sizeof(struct kretprobe_instance), GFP_KERNEL);
> +		inst = kmalloc(sizeof(struct kretprobe_instance) +
> +			       rp->data_size, GFP_KERNEL);
>  		if (inst == NULL) {
>  			free_rp_inst(rp);
>  			return -ENOMEM;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists