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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
>
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
