On 11/09, Yonghong Song wrote:
>
> This patch extends the emulation to "push <reg>"
> insns. These insns are typical in the beginning
> of the function. For example, bcc
> in https://github.com/iovisor/bcc repo provides
> tools to measure funclantency, detect memleak, etc.
> The tools will place uprobes in the beginning of
> function and possibly uretprobes at the end of function.
> This patch is able to reduce the trap overhead for
> uprobe from 2 to 1.
OK. but to be honest I do not like the implementation, please see below.
> +enum uprobe_insn_t {
> + UPROBE_BRANCH_INSN = 0,
> + UPROBE_PUSH_INSN = 1,
> +};
> +
> struct uprobe_xol_ops;
>
> struct arch_uprobe {
> @@ -42,6 +47,7 @@ struct arch_uprobe {
> };
>
> const struct uprobe_xol_ops *ops;
> + enum uprobe_insn_t insn_class;
Why?
I'd suggest to leave branch_xol_ops alone and add the new push_xol_ops{},
the code will look much simpler.
The only thing they can share is branch_post_xol_op() which is just
regs->sp += sizeof_long();
return -ERESTART;
I think a bit of code duplication would be fine in this case.
And. Do you really need ->post_xol() method to emulate "push"? Why we can't
simply execute it out-of-line if copy_to_user() fails?
branch_post_xol_op() is needed because we can't execute "call" out-of-line,
we need to restart and try again if copy_to_user() fails, but I don not
understand why it is needed to emulate "push".
Oleg.
