[DTrace-devel] [PATCH 7/9] Consolidate bpf_get_stack()-call code generation for stack and ustack()
Kris Van Hees
kris.van.hees at oracle.com
Wed Jan 24 06:59:32 UTC 2024
On Thu, Oct 05, 2023 at 05:14:05PM -0400, eugene.loh at oracle.com wrote:
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 141 ++++++++++++++++++++++++++++------------------
> 1 file changed, 85 insertions(+), 56 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 6cae8814..ba698d2f 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2225,33 +2225,90 @@ dt_cg_ustack_arg(dtrace_hdl_t *dtp, dt_node_t *dnp)
> return DTRACE_USTACK_ARG(nframes, strsize);
> }
>
> -static void
> -dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +/*
> + * Call the bpf_get_stack() helper function.
> + *
> + * dnp->dn_arg has optional, integer-constant sizing information.
> + *
> + * If reg>=0, reg is a register holding the output pointer.
> + * If reg<0, we write the stack to the output buffer in %r9.
> + *
> + * These cases treat "off" differently:
> + * - reg>=0: "off" tracks how far the pointer in "reg" has
> + * been advanced, both at function entry and return
> + *
> + * - reg<0: "off" is basically just a local variable since
> + * offsets with the %r9 buffer are managed by
> + * dt_rec_add()
> + *
> + * The "user" flag indicates whether a user (or kernel) stack is desired.
> + */
> +static int
> +dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, int user)
> {
> dtrace_hdl_t *dtp = pcb->pcb_hdl;
> dt_irlist_t *dlp = &pcb->pcb_ir;
> dt_regset_t *drp = pcb->pcb_regs;
> - int nframes;
> - int skip = 0;
> - uint_t off;
> + uint64_t arg;
> + int nframes, stacksize, prefsz, align = sizeof(uint64_t);
> uint_t lbl_valid = dt_irlist_label(dlp);
>
> - nframes = dt_cg_stack_nframes(dtp, dnp);
> + /* Get sizing information from dnp->dn_arg. */
> + if (user) {
> + arg = dt_cg_ustack_arg(dtp, dnp);
> + nframes = DTRACE_USTACK_NFRAMES(arg);
> + prefsz = sizeof(uint64_t);
> + } else {
> + nframes = dt_cg_stack_nframes(dtp, dnp);
> + arg = nframes;
> + prefsz = 0;
> + }
I think you might as well not have dt_cg_ustack_arg() and dt_cg_stack_nframes()
and instead inline what they do here. Especially since (modulo some constant
values) there is duplication for the handling of the first argument anyway.
In that sense, I don't think the [5/9] patch in this series is needed at all,
because that one introduces those two functions. But if we are not going to
be using them anyway per my suggestion above, there is no need to introduce
them. And that also makes [6/9] (moving one of those functions) unnecessary.
Besides, patch [5/9] mentions that it refactors dt_cg_act_stack() amd
dt_cg_act_ustack() so that what they do can be invoked by multiple callers,
but that is not really what that patch does. The new functions it introduces
are never called by more than a single caller, and in this patch [8/9] the
actual implementation of stack and ustack is moved into a function that then
can be called from multiple callers for real.
So, I think you can just use this patch (with modifications) to provide a
subroutine function that handles the real work of performing stack() and
ustack(), and then the next patch in the series hooks it up to the arglist
code to support using stack(), ustack(), etc as elements in a tuple.
> + stacksize = nframes * sizeof(uint64_t);
> +
> + /* Handle alignment and reserve space in the output buffer. */
> + if (reg >= 0) {
> + uint_t nextoff;
> + nextoff = (off + (align - 1)) & ~(align - 1);
> + if (off < nextoff)
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, nextoff - off));
> + off = nextoff + prefsz + stacksize;
> + } else {
> + off = dt_rec_add(dtp, dt_cg_fill_gap,
> + user ? DTRACEACT_USTACK : DTRACEACT_STACK,
> + prefsz + stacksize, align, NULL, arg);
> + }
>
> - /* Reserve space in the output buffer. */
> - off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_STACK,
> - sizeof(uint64_t) * nframes, sizeof(uint64_t),
> - NULL, nframes);
> + /* Write the tgid. */
> + if (user) {
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> + dt_regset_free_args(drp);
> + emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
> + if (reg >= 0)
> + emit(dlp, BPF_STORE(BPF_DW, reg, 0, BPF_REG_0));
> + else
> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, off, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> + }
>
> - /* Now call bpf_get_stack(ctx, buf, size, flags). */
> + /* Call bpf_get_stack(ctx, buf, size, flags). */
> if (dt_regset_xalloc_args(drp) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_CTX));
> - emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_9));
> - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, off));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_3, sizeof(uint64_t) * nframes));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_4, skip & BPF_F_SKIP_FIELD_MASK));
> + if (reg >= 0) {
> + emit(dlp, BPF_MOV_REG(BPF_REG_2, reg));
> + if (prefsz)
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, prefsz));
> + } else {
> + emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_9));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, off + prefsz));
> + }
> + emit(dlp, BPF_MOV_IMM(BPF_REG_3, stacksize));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_4, (0 & BPF_F_SKIP_FIELD_MASK)
> + | (user ? BPF_F_USER_STACK : 0)));
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_stack));
> dt_regset_free_args(drp);
> @@ -2260,6 +2317,18 @@ dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> dt_cg_probe_error(pcb, DTRACEFLT_BADSTACK, DT_ISIMM, 0);
> emitl(dlp, lbl_valid,
> BPF_NOP());
> +
> + /* Finish. */
> + if (reg >= 0)
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, prefsz + stacksize));
> +
> + return off;
> +}
> +
> +static void
> +dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +{
> + dt_cg_act_stack_sub(pcb, dnp, -1, 0, 0);
Make this:
dt_cg_act_stack_sub(pcb, dnp, -1, 0, kind == DTRACEACT_USTACK ? 1 : 0);
> }
>
> static void
> @@ -2417,47 +2486,7 @@ dt_cg_act_trunc(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> static void
> dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> {
> - dt_irlist_t *dlp = &pcb->pcb_ir;
> - dt_regset_t *drp = pcb->pcb_regs;
> - dtrace_hdl_t *dtp = pcb->pcb_hdl;
> - uint64_t arg;
> - int nframes, stacksize;
> - int skip = 0;
> - uint_t off;
> - uint_t lbl_valid = dt_irlist_label(dlp);
> -
> - arg = dt_cg_ustack_arg(dtp, dnp);
> - nframes = DTRACE_USTACK_NFRAMES(arg);
> - stacksize = nframes * sizeof(uint64_t);
> -
> - /* Reserve space in the output buffer. */
> - off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_USTACK,
> - 8 + stacksize, 8, NULL, arg);
> -
> - if (dt_regset_xalloc_args(drp) == -1)
> - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> - dt_regset_xalloc(drp, BPF_REG_0);
> -
> - /* Write the tgid. */
> - emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> - emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
> - emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, off, BPF_REG_0));
> -
> - /* Now call bpf_get_stack(ctx, buf, size, flags). */
> - emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> - emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_CTX));
> - emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_9));
> - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, off + 8));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_3, stacksize));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_4, (skip & BPF_F_SKIP_FIELD_MASK)
> - | BPF_F_USER_STACK));
> - emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_stack));
> - dt_regset_free_args(drp);
> - emit(dlp, BPF_BRANCH_IMM(BPF_JSGE, BPF_REG_0, 0, lbl_valid));
> - dt_regset_free(drp, BPF_REG_0);
> - dt_cg_probe_error(pcb, DTRACEFLT_BADSTACK, DT_ISIMM, 0);
> - emitl(dlp, lbl_valid,
> - BPF_NOP());
> + dt_cg_act_stack_sub(pcb, dnp, -1, 0, 1);
> }
Get rid of dt_cg_act_ustack(), and replace the entries for stack and ustack in
the _dt_cg_actions table with this:
[DT_ACT_IDX(DT_ACT_STACK)] = { &dt_cg_act_stack,
DTRACEACT_STACK },
[DT_ACT_IDX(DT_ACT_USTACK)] = { &dt_cg_act_stack,
DTRACEACT_USTACK },
(This is similar to what we do for printf-alike actions.)
> typedef void dt_cg_action_f(dt_pcb_t *, dt_node_t *, dtrace_actkind_t);
> --
> 2.18.4
>
>
More information about the DTrace-devel
mailing list