This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC][PATCH 1/2] uprobes: utrace-based user-space probes
- From: Jim Keniston <jkenisto at us dot ibm dot com>
- To: Ernie Petrides <petrides at redhat dot com>
- Cc: "Frank Ch. Eigler" <fche at redhat dot com>, Linda Wang <lwang at redhat dot com>, systemtap at sources dot redhat dot com
- Date: Mon, 07 May 2007 11:42:58 -0700
- Subject: Re: [RFC][PATCH 1/2] uprobes: utrace-based user-space probes
- References: <200705050107.l4517fGO018580@pasta.boston.redhat.com>
On Fri, 2007-05-04 at 21:07 -0400, Ernie Petrides wrote:
> On Friday, 20-Apr-2007 at 15:08 PDT, Jim Keniston wrote:
>
> > Here's the latest rev of our i386 implementation of user-space
> > probes (uprobes), based on Roland's utrace infrastructure....
[snip]
>
> Hello, Jim. I've been asked to look over your uprobes patches by
> Frank and Linda. While I don't have the detailed arch-specific or
> utrace background for a thorough review, I do have a couple of higher
> level comments.
>
> Firstly, you have my compliments on a great idea and sound
implementation.
> The value of this as a fantastic debugging facility (with systemtap)
is
> obvious, but I also envision this getting used for applying temporary
> fixes or work-arounds to applications in the field.
>
> Because of this latter possibility, consider that a 3rd-party uprobe
> module has been deployed at a customer site, but then a later update
> to the uprobe infrastructure becomes desirable. It's possible that a
> change to the "uprobe" structure would be needed for whatever reason,
> but that could break compatibility with the 3rd-party module (i.e.,
> the uprobes facility consumer).
>
> Thus, I'd recommend finding a way to isolate the
consumer/infrastructure
> interface from the uprobes-internal data. The best way would be to
make
> the "uprobe" contain only the "pid", "vaddr", and "handler" fields and
> then add an opaque (void *) to point to a 2nd internal-use-only
struct.
Yeah, you're not the first person to suggest that. That was the
original intent of struct uprobe_kimg, but that struct quickly morphed
into something that would be better called struct uprobe_probepoint
(which can be associated with multiple uprobes). uprobe_kimg needs
to be split into uprobe_kimg + uprobe_probepoint. The main reason
I haven't already done it is inertia (e.g., over 200 refs to "uk"
in kernel/uprobes.c).
I need to reconsider based on your point about binary compatibility.
> Alternatives would be to put a flags field, version number, or struct
> size field at the beginning of the struct, or else to require a
dynamic
> allocation of the struct.
>
> Secondly, I'd recommend that everything except the
consumer/infrastructure
> interface be moved from include/linux/uprobes.h to kernel/uprobes.c in
> order to prevent a consumer module from using/modifying/depending on
> anything that it shouldn't. This is simply basic "information hiding"
> to prevent future incompatibility issues.
arch/*/kernel/uprobes.c typically needs access to certain internal
data structures (uprobe_kimg, uprobe_task), so I think the best we
could do for some of those data structures is to move them to a
separate "implementation" header. I don't see much precedent for
that, but I'm open to examples.
>
> Lastly, I think that the register_uprobe() and unregister_uprobe()
> declarations in uprobes.h should use "extern"
Yes.
> and that the init_module()
> and cleanup_module() functions in the documentation example should use
> __init/__exit.
OK. There doesn't seem to be anything close to unanimity about this in
the kernel source, but a slight majority of init_module/cleanup_module
uses use __init/__exit. Do you happen to know under what circumstances
this makes a difference for a module?
>
> I'll reply to your 2nd patch with one final comment.
>
> (Note that I'm not on the systemtap@sources.redhat.com list.)
>
> Cheers. -ernie
Thanks very much for your review.
Jim