[DTrace-devel] [PATCH v2 5/9] Handle stack() and ustack() args in a separate function
Kris Van Hees
kris.van.hees at oracle.com
Mon Jan 29 18:56:08 UTC 2024
On Sat, Jan 27, 2024 at 03:12:27PM -0500, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> Currently, dt_cg_act_stack() and dt_cg_act_ustack() generate BPF code
> to implement the stack() and ustack() actions. However, it should also
> be possible to use stack() and ustack() as aggregation keys. So, it
> would be nice to move the main work of the code-generation functions
> to separate functions that can be used by different callers.
>
> Therefore, start refactoring these cg functions so that what they do
> can be invoked by multiple callers. Here, simply move the handling of
> the stack() and ustack() args out into a separate function. (Actually,
> that handling does not even involve any code generation.)
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 119 ++++++++++++++++++++++++++--------------------
> 1 file changed, 68 insertions(+), 51 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 1e4bb707..084157a0 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2501,34 +2501,78 @@ dt_cg_act_speculate(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> dt_regset_free(drp, dnp->dn_reg);
> }
>
> +static int
> +dt_cg_stack_nframes(dtrace_hdl_t *dtp, dt_node_t *arg0, int indopt,
> + int def, int inderr, const char *fname)
> +{
> + int nframes;
> +
> + /* Get the default number. */
> + nframes = dtp->dt_options[indopt];
> + if (nframes == DTRACEOPT_UNSET)
> + nframes = _dtrace_ustackframes;
This should be
nframes = def;
but see below about this function in general.
> +
> + /* Get the specified value, if any. */
> + if (arg0 != NULL) {
> + if (!dt_node_is_posconst(arg0))
> + dnerror(arg0, inderr,
> + "%s( ) argument #1 must be a non-zero positive integer constant\n",
> + fname);
> + nframes = arg0->dn_value;
> + }
> +
> + /* If more frames are requested than allowed, silently reduce nframes. */
> + if (nframes > dtp->dt_options[DTRACEOPT_MAXFRAMES])
> + nframes = dtp->dt_options[DTRACEOPT_MAXFRAMES];
> +
> + return nframes;
> +}
While there are 4 parameters passed for the different cases, in the end, there
really are only 2 cases: DTRACEACT_STACK and DTRACEACT_USTACK. And the values
are constants anyway, so I would just inline this in dt_cg_stack_arg() below,
and not have dt_cg_stack_nframes(). I don't think it adds anything to have
this in its own function. Below, you would just look at what case you are
dealing with, set the constants, and then have the code above inlined.
> +
> +static uint64_t
> +dt_cg_stack_arg(dtrace_hdl_t *dtp, dt_node_t *dnp, int user)
For clarity in reading this code, I would pass in kind (DTRACEACT_STACK or
DTRACEACT_USTACK) and explicitly check for that rather than using 'user'.
It doesn't matter much here, but the caller passing 1 or 0 as the last argument
doesn't really show what this argument means at the call site. Passing the
symbolic constant makes reading the code in the caller easier.
> +{
> + int nframes;
> + int strsize = 0;
> + dt_node_t *arg0 = dnp->dn_args;
> + dt_node_t *arg1 = arg0 != NULL ? arg0->dn_list : NULL;
> +
> + if (user)
> + nframes = dt_cg_stack_nframes(dtp, arg0, DTRACEOPT_USTACKFRAMES,
> + _dtrace_ustackframes, D_USTACK_FRAMES, "ustack");
> + else
> + nframes = dt_cg_stack_nframes(dtp, arg0, DTRACEOPT_STACKFRAMES,
> + _dtrace_stackframes, D_STACK_SIZE, "stack");
> +
> + if (!user)
> + return DTRACE_USTACK_ARG(nframes, 0);
> +
> + if (arg1 != NULL) {
> + if (arg1->dn_kind != DT_NODE_INT ||
> + ((arg1->dn_flags & DT_NF_SIGNED) &&
> + (int64_t)arg1->dn_value < 0))
> + dnerror(arg1, D_USTACK_STRSIZE, "ustack( ) argument #2 "
> + "must be a positive integer constant\n");
> +
> + strsize = arg1->dn_value;
> + }
> +
> + return DTRACE_USTACK_ARG(nframes, strsize);
> +}
> +
> static void
> dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> {
> dtrace_hdl_t *dtp = pcb->pcb_hdl;
> dt_irlist_t *dlp = &pcb->pcb_ir;
> dt_regset_t *drp = pcb->pcb_regs;
> - dt_node_t *arg = dnp->dn_args;
> - int nframes = dtp->dt_options[DTRACEOPT_STACKFRAMES];
> + uint64_t arg;
> + int nframes;
> int skip = 0;
> uint_t off;
> uint_t lbl_valid = dt_irlist_label(dlp);
>
> - if (nframes == DTRACEOPT_UNSET)
> - nframes = _dtrace_stackframes;
> -
> - if (arg != NULL) {
> - if (!dt_node_is_posconst(arg))
> - dnerror(arg, D_STACK_SIZE, "stack( ) argument #1 must "
> - "be a non-zero positive integer constant\n");
> -
> - nframes = arg->dn_value;
> - }
> -
> - /*
> - * If more frames are requested than allowed, silently reduce nframes.
> - */
> - if (nframes > dtp->dt_options[DTRACEOPT_MAXFRAMES])
> - nframes = dtp->dt_options[DTRACEOPT_MAXFRAMES];
> + arg = dt_cg_stack_arg(dtp, dnp, 0);
arg = dt_cg_stack_arg(dtp, dnp, DTRACEACT_STACK);
> + nframes = DTRACE_USTACK_NFRAMES(arg);
>
> /* Reserve space in the output buffer. */
> off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_STACK,
> @@ -2711,46 +2755,19 @@ 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;
> - int nframes = dtp->dt_options[DTRACEOPT_USTACKFRAMES];
> - int strsize = 0;
> - int stacksize;
> - dt_node_t *arg0 = dnp->dn_args;
> - dt_node_t *arg1 = arg0 != NULL ? arg0->dn_list : NULL;
> + uint64_t arg;
> + int nframes, stacksize;
> int skip = 0;
> uint_t off;
> uint_t lbl_valid = dt_irlist_label(dlp);
>
> - if (nframes == DTRACEOPT_UNSET)
> - nframes = _dtrace_ustackframes;
> -
> - if (arg0 != NULL) {
> - if (!dt_node_is_posconst(arg0))
> - dnerror(arg0, D_USTACK_FRAMES, "ustack( ) argument #1 "
> - "must be a non-zero positive integer "
> - "constant\n");
> -
> - nframes = arg0->dn_value;
> - }
> -
> - if (nframes > dtp->dt_options[DTRACEOPT_MAXFRAMES])
> - nframes = dtp->dt_options[DTRACEOPT_MAXFRAMES];
> -
> - /* FIXME: for now, accept non-zero strsize, but it does nothing */
> - if (arg1 != NULL) {
> - if (arg1->dn_kind != DT_NODE_INT ||
> - ((arg1->dn_flags & DT_NF_SIGNED) &&
> - (int64_t)arg1->dn_value < 0))
> - dnerror(arg1, D_USTACK_STRSIZE, "ustack( ) argument #2 "
> - "must be a positive integer constant\n");
> -
> - strsize = arg1->dn_value;
> - }
> + arg = dt_cg_stack_arg(dtp, dnp, 1);
arg = dt_cg_stack_arg(dtp, dnp, DTRACEACT_USTACK);
> + nframes = DTRACE_USTACK_NFRAMES(arg);
> + stacksize = nframes * sizeof(uint64_t);
>
> /* Reserve space in the output buffer. */
> - stacksize = nframes * sizeof(uint64_t);
> off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_USTACK,
> - 8 + stacksize, 8, NULL,
> - DTRACE_USTACK_ARG(nframes, strsize));
> + 8 + stacksize, 8, NULL, arg);
>
> if (dt_regset_xalloc_args(drp) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> --
> 2.18.4
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list