Re: [RFC -mm][PATCH 0/6] kprobes x86 code unification and boosters (review part 3)

[Jim Keniston <jkenisto at us dot ibm dot com>]

Here's the rest of my review.  I didn't find anything broken, so I'm
willing to ack the patch set (in however many pieces it's posted).

Thanks for all your great work, Masami.

Jim
-----
+#define MIN_STACK_SIZE(ADDR) (((MAX_STACK_SIZE) < \
+       (((unsigned long)current_thread_info()) + THREAD_SIZE -
(unsigned long)(ADDR))) \
+       ? (MAX_STACK_SIZE) \
+       : (((unsigned long)current_thread_info()) + THREAD_SIZE -
(unsigned long)(ADDR)))

How about making MIN_STACK_SIZE(ADDR) an inline function?  Seems like
it'd clarify what we're trying to do.
static unsigned long saved_stack_size(unsigned long sp)
{
	unsigned long cur_stack_size =
		((unsigned long)current_thread_info())
		+ THREAD_SIZE - sp;
	return min(cur_stack_size, MAX_STACK_SIZE);
}
But maybe such a change should be packaged separately, and include s390.

+struct arch_specific_insn {
+	/* copy of the original instruction */
+	kprobe_opcode_t *insn;
+	/*
+	 * If this flag is 1, this kprobe can be boosted when its
+	 * post_handler and break_handler is not set.
+	 */

How about:
	/*
	 * boostable = -1: This instruction type is not boostable.
	 * boostable = 0: This instruction type is boostable.
	 * boostable = 1: This instruction has been boosted: we have
	 * added a relative jump after the instruction copy in insn,
	 * so no single-step and fixup are needed (unless there's
	 * a post_handler or break_handler).
	 */
+	int boostable;
+};

+	/*
+	 * It is possible to have multiple instances associated with a given
+	 * task either because an multiple functions in the call path
s/an multiple/multiple/
+	 * have a return probe installed on them, and/or more then one return
s/a return probe/return probes/
s/one return/one/  [Fix "return\nreturn"]
+	 * return probe was registered for a target function.
+	 *
+	 * We can handle this because:
+	 *     - instances are always inserted at the head of the list
s/inserted at/pushed onto/
+	 *     - when multiple return probes are registered for the same
+	 *       function, the first instance's ret_addr will point to the
+	 *       real return address, and all the rest will point to
+	 *       kretprobe_trampoline
How about this?
	 *	 function, the (chronologically) first instance's ret_addr
	 *	 will be the real return address, and all the rest will
	 *	 point to kretprobe_trampoline.
+	 */

+		 * we can also use npre/npostfault count for accouting
s/accouting/accounting/