This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC][PATCH 1/2] uprobes: utrace-based user-space probes


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]