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: Ernie Petrides <petrides at redhat dot com>
- To: Jim Keniston <jkenisto at us dot ibm dot com>
- Cc: "Frank Ch. Eigler" <fche at redhat dot com>, Linda Wang <lwang at redhat dot com>, Ernie Petrides <petrides at redhat dot com>, systemtap at sources dot redhat dot com
- Date: Fri, 04 May 2007 21:07:41 -0400
- Subject: Re: [RFC][PATCH 1/2] uprobes: utrace-based user-space probes
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. Uprobes
> supplements utrace and kprobes, enabling a kernel module to probe
> user-space applications in much the same way that a kprobes-based
> module probes the kernel. See Documentation/uprobes.txt in the patch
> for details.
>
> This patch should apply to any recent -mm kernel (i.e., any kernel
> that includes utrace). Comments welcome.
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.
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.
Lastly, I think that the register_uprobe() and unregister_uprobe()
declarations in uprobes.h should use "extern" and that the init_module()
and cleanup_module() functions in the documentation example should use
__init/__exit.
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