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

[Masami Hiramatsu <mhiramat at redhat dot com>]

Hello Jim,

Jim Keniston wrote:
> 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.

I agree, and I think it might better merge it after unification
for reducing modification and side-effects.

> +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;
> +};

Thanks, it's good to me.

> 
> +	/*
> +	 * 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/

OK, I'll fix it.

> 
> 

Thank you very much!

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com