Message-Id: <1195260891.8520.104.camel@dyn9047018096.beaverton.ibm.com>
Date:	Fri, 16 Nov 2007 16:54:51 -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

It'd be helpful to see others (especially kprobes maintainers) chime in
on this.  In particular, if doing kmalloc/kfree of GFP_ATOMIC data at
kretprobe-hit time is OK, as in Abhishek's approach, then we could also
use GFP_ATOMIC (or at least GFP_NOWAIT) allocations to make up the
difference when we run low on kretprobe_instances.

More comments below.

On Fri, 2007-11-16 at 23:20 +0530, Abhishek Sagar wrote:
> On Nov 16, 2007 2:46 AM, Jim Keniston <jkenisto@...ibm.com> wrote:
...
> 
> > > There are three problems in the "data pouch" approach, which is a
> > > generalized case of Srinivasa's timestamp case:
> > >
> > > 1. It bloats the size of each return instance to a run-time defined
> > > value. I'm certain that quite a few memory starved ARM kprobes users
> > > would certainly be wishing they could install more probes on their
> > > system without taking away too much from the precious memory pools
> > > which can impact their system performance. This is not a deal breaker
> > > though, just an annoyance.
> >
> > entry_info is, by default, a zero-length array, which adds nothing to
> > the size of a uretprobe_instance -- at least on the 3 architectures I've
> > tested on (i386, x86_64, powerpc).
> 
> Strange, because from what I could gather, the data pouch patch had
> the following in the kretprobe registration routine:
> 
> 
> for (i = 0; i < rp->maxactive; i++) {
> -		inst = kmalloc(sizeof(struct kretprobe_instance), GFP_KERNEL);
> +		inst = kmalloc((sizeof(struct kretprobe_instance)
> +				+ rp->entry_info_sz), GFP_KERNEL);
> 
> 
> rp->entry_info_sz is presumably the size of the private data structure
> of the registering module.

... which is zero for kretprobes that don't use the data pouch.

> This is the bloat I was referring to. But
> this difference should've showed up in your tests...?

What bloat?  On my 32-bit system, the pouch to hold struct prof_data in
your test_module example would be 20 bytes.  (For comparison,
sizeof(struct kretprobe_instance) = 28, btw.)  Except for functions like
schedule(), where a lot of tasks can be sleeping at the same time, an
rp->maxactive value of 5 or 10 is typically plenty.  That's 100-200
bytes of "bloat" spent at registration time (GFP_KERNEL), at least some
of which will be saved at probe-hit time (GFP_ATOMIC).  (And if somebody
says, "I always use a much higher value of rp->maxactive," then he/she's
probably not really worried about bloat.)

> 
> > >
> > > 2. Forces user entry/return handlers to use ri->entry_info (see
> > > Kevin's patch) even if their design only wanted such private data to
> > > be used in certain instances.
> >
> > No, it doesn't.  Providing a feature isn't the same as forcing people to
> > use the feature.
> 
> From the portion of the patch quoted above, it seems that there is
> private data allocation at registration time per-instance in one go.
> First of all, not all maxactive instances get used frequently. Even if
> they do, the fact that this private data would be used by the user
> handlers for a particular instance is also an assumption. So I guess
> it is better to leave allocation to the handlers and provide them with
> a data pointer in ri.

But as noted in my review of your sample module, hand-crafted, per-hit
allocation is more expensive both in terms of execution time and code
size/complexity.

> 
...
> > >
> > > 3. It's redundant. 'ri' can uniquely identify any entry-return handler
> > > pair. This itself solves your problem #2. It only moves the onus of
> > > private data allocation to user handlers.
> >
> > Having ri available at function entry time is definitely a win, but
> > maintaining separate data structures and using ri to map to the right
> > one is no trivial task.  
...
> 
> True, some kind of data pointer/pouch is essential.

Yes.  If the pouch idea is too weird, then the data pointer is a good
compromise.

With the above reservations, your enclosed patch looks OK.

You should provide a patch #2 that updates Documentation/kprobes.txt.
Maybe that will yield a little more review from other folks.

> 
> > I suggest you show us a probe module that captures data at function
> > entry and reports it upon return, exploiting your proposed patch.
> > Profiling would be a reasonable example, but something where multiple
> > values are captured might be more relevant.  (Your example below doesn't
> > count because it doesn't work.)  Then I'll code up the same example
> > using your enhancement + the data pouch enhancement, and we can compare.
> 
> I'll send a sample module which uses data pointer provided in ri in my
> next response.

Got it.  Thanks.  I've already commented.

Jim

> 
> > Of course, the data pouch enhancement would build nicely atop your
> > enhancement, so it could be proposed and considered separately.
> 
> Ok.
> 
> > >
> > > > Review of Abhishek's patch:
...
> Revising the patch with the suggested change:
> 
> ---
> Signed-off-by: Abhishek Sagar <sagar.abhishek@...il.com>
> 
> diff -upNr linux-2.6.24-rc2/include/linux/kprobes.h
> linux-2.6.24-rc2_kp/include/linux/kprobes.h
> --- linux-2.6.24-rc2/include/linux/kprobes.h	2007-11-07 03:27:46.000000000 +0530
> +++ linux-2.6.24-rc2_kp/include/linux/kprobes.h	2007-11-16
> 22:50:24.000000000 +0530
> @@ -152,6 +152,7 @@ static inline int arch_trampoline_kprobe
>  struct kretprobe {
>  	struct kprobe kp;
>  	kretprobe_handler_t handler;
> +	kretprobe_handler_t entry_handler;
>  	int maxactive;
>  	int nmissed;
>  	struct hlist_head free_instances;
> @@ -164,6 +165,7 @@ struct kretprobe_instance {
>  	struct kretprobe *rp;
>  	kprobe_opcode_t *ret_addr;
>  	struct task_struct *task;
> +	void *data;
>  };
> 
>  struct kretprobe_blackpoint {
> diff -upNr linux-2.6.24-rc2/kernel/kprobes.c
> linux-2.6.24-rc2_kp/kernel/kprobes.c
> --- linux-2.6.24-rc2/kernel/kprobes.c	2007-11-07 03:27:46.000000000 +0530
> +++ linux-2.6.24-rc2_kp/kernel/kprobes.c	2007-11-16 22:53:46.000000000 +0530
> @@ -699,6 +699,14 @@ static int __kprobes pre_handler_kretpro
>  				 struct kretprobe_instance, uflist);
>  		ri->rp = rp;
>  		ri->task = current;
> +		ri->data = NULL;
> +
> +		if (rp->entry_handler) {
> +			if (rp->entry_handler(ri, regs)) {
> +				spin_unlock_irqrestore(&kretprobe_lock, flags);
> +				return 0; /* voluntary miss */
> +			}
> +		}
>  		arch_prepare_kretprobe(ri, regs);
> 
>  		/* XXX(hch): why is there no hlist_move_head? */

-
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