This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC -mm][PATCH 0/6] kprobes x86 code unification and boosters (review part 2)
- From: Jim Keniston <jkenisto at us dot ibm dot com>
- To: Masami Hiramatsu <mhiramat at redhat dot com>
- Cc: Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>, Roland McGrath <roland at redhat dot com>, Arjan van de Ven <arjan at infradead dot org>, prasanna at in dot ibm dot com, anil dot s dot keshavamurthy at intel dot com, davem at davemloft dot net, systemtap-ml <systemtap at sources dot redhat dot com>
- Date: Thu, 13 Dec 2007 17:25:32 -0800
- Subject: Re: [RFC -mm][PATCH 0/6] kprobes x86 code unification and boosters (review part 2)
- References: <475DC33E.8080907@redhat.com> <1197421379.3876.12.camel@dyn9047018096.beaverton.ibm.com> <47601717.8040003@redhat.com>
FYI, here are review comments I sent privately to Masami yesterday. (I
was in a hurry and didn't cc everybody.) He has already addressed them.
Jim
-----
Patch #5:
+#define C ,
This is a creative solution to the problem of test_bit() wanting
an unsigned long* arg. It may be a little TOO creative for some
reviewers. If so, remember that you can make the tables all u64
or u32, and just add (const unsigned long*) casts to the test_bit()
calls.
In kprobes_64.c...
+#undef C
s/ / /
+/*
+ * Interrupts are disabled on entry as trap3 is an interrupt gate and
they
+ * remain disabled thorough out this function.
+ */
+static int __kprobes kprobe_handler(struct pt_regs *regs)
I assume you're confident about this. If not, verify with somebody
like Andi Kleen. I tried to verify this by sifting through traps_64.c
and desc_64.c and the AMD manuals, but gave up after a while.
Patch #6:
Regarding the historical comments in kprobes.c and kprobes.h...
1. What's the current policy regarding such comments? Should they just
be removed? If not...
2. Seems like historical notes for kprobe boosters and kretprobe
boosters
should be added. (Modesty should not obscure history. :-))
3. You could probably remove the historical comments from kprobes.h,
and/or add a ref to the (more nearly complete) history in kprobes.c.
4. Since each file reflects both i386 and x86_64 now, you need to
clarify some comments:
a. In kprobes.h, the 2004-Oct comment should end with something
like "x86_64 adopted from i386".
b. In kprobes.c, the 2005-May comment about Rusty Lynch should say
"Added x86_64 function-return probes". Also, it should follow the
other 2005-May comment, since we implemented i386 first.
5. s/kenistoj/jkenisto/