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 3)
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