[DTrace-devel] [PATCH 5/9] Handle stack() and ustack() args in separate functions
Kris Van Hees
kris.van.hees at oracle.com
Tue Jan 23 21:31:01 UTC 2024
Working through this patch and the 3 following ones but it s taking some
time because I am having some trouble following all logic. For one, the
splitting out of the arg handling and the moving of the ustack arg handling
can just be a single patch. The code movement patch [6/9] is not really
adding anything by being in its own patch.
The [8/9] patch introduces a dt_cg_nonvoid_act() function that contains a
switch statement that is then repeated 3 times in the caller, which seems
a bit odd. I really think there ought to be a nicer way to do that. At a
minimum, it seems to me that the dt_cg_nonvoid_act() function isn't really
needed. An additional conditional with the switch statements in the caller
should be sufficient.
I am also struggling through the handling of tuplesize and nextoff because
I expect it ought to be correct but it doesn't look right :) I think that
the complexity might need some restructuring. E.g. when you construct the
actual tuple data, you have the switch (dt_cg_nonvoid_act(...)) and handle
the cases, where some cause an immediate contiue (start the next element)
whereas others do something and then continue after the switch with a
conditional that reads:
if (dt_node_is_scalar(...) || dt_node_is_float(...) || dt_cg_nonvoid_act(...))
I think that can be streamlined by perhaps setting a flag that indicates
whether a scalar-style value is being stored or not, rather than calling that
dt_cg_nonvoid_act() function again (if we retain it) because we keep doing
all these idp->di_id checks over and over again.
I'm trying to see if there is a way to do this with just two switch statements
and some flags.
On Thu, Oct 05, 2023 at 05:14:03PM -0400, eugene.loh at oracle.com wrote:
>
> 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 separate functions. (Actually,
> that handling does not even involve any code generation.)
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 61 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index cef41e6a..389e4dba 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2163,17 +2163,11 @@ dt_cg_act_speculate(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> dt_regset_free(drp, dnp->dn_reg);
> }
>
> -static void
> -dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +static int
> +dt_cg_stack_nframes(dtrace_hdl_t *dtp, dt_node_t *dnp)
> {
> - 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];
> - int skip = 0;
> - uint_t off;
> - uint_t lbl_valid = dt_irlist_label(dlp);
>
> if (nframes == DTRACEOPT_UNSET)
> nframes = _dtrace_stackframes;
> @@ -2192,6 +2186,22 @@ dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> if (nframes > dtp->dt_options[DTRACEOPT_MAXFRAMES])
> nframes = dtp->dt_options[DTRACEOPT_MAXFRAMES];
>
> + return nframes;
> +}
> +
> +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;
> + int nframes;
> + int skip = 0;
> + uint_t off;
> + uint_t lbl_valid = dt_irlist_label(dlp);
> +
> + nframes = dt_cg_stack_nframes(dtp, dnp);
> +
> /* Reserve space in the output buffer. */
> off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_STACK,
> sizeof(uint64_t) * nframes, sizeof(uint64_t),
> @@ -2368,20 +2378,13 @@ dt_cg_act_trunc(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> dt_cg_store_val(pcb, trunc, DTRACEACT_LIBACT, NULL, DT_ACT_TRUNC);
> }
>
> -static void
> -dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +static uint64_t
> +dt_cg_ustack_arg(dtrace_hdl_t *dtp, dt_node_t *dnp)
> {
> - 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;
> - int skip = 0;
> - uint_t off;
> - uint_t lbl_valid = dt_irlist_label(dlp);
>
> if (nframes == DTRACEOPT_UNSET)
> nframes = _dtrace_ustackframes;
> @@ -2398,7 +2401,6 @@ dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> 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) &&
> @@ -2409,11 +2411,28 @@ dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> strsize = arg1->dn_value;
> }
>
> - /* Reserve space in the output buffer. */
> + return DTRACE_USTACK_ARG(nframes, strsize);
> +}
> +
> +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,
> - 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
>
>
More information about the DTrace-devel
mailing list