[DTrace-devel] [PATCH 03/13] Compute lquantize() bin only once
Kris Van Hees
kris.van.hees at oracle.com
Tue Dec 8 07:56:27 PST 2020
On Wed, Dec 02, 2020 at 01:54:48PM -0500, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
We really should have a commit message here that explains that you are
introducing a pre-compiled BPF function for this (similar to the one you
did for quantize()). The message does not need to be as verbose since
you can refer to the quantize one, but mentioning that a new BPF function
was added to the library of pre-compiled functions is needed. You also
should add that this code will now use the dt_cg_agg_quantize_impl()
function to do the actual data update.
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> bpf/Build | 2 +-
> bpf/agg_lqbin.c | 24 +++++++++++
> libdtrace/dt_cg.c | 95 +++++++++-----------------------------------
> libdtrace/dt_dlibs.c | 1 +
> 4 files changed, 44 insertions(+), 78 deletions(-)
> create mode 100644 bpf/agg_lqbin.c
>
> diff --git a/bpf/Build b/bpf/Build
> index ae298f79..e9f2b1e2 100644
> --- a/bpf/Build
> +++ b/bpf/Build
> @@ -22,7 +22,7 @@ bpf_dlib_TARGET = dlibs/bpf_dlib
> bpf_dlib_DIR := $(current-dir)
> bpf_dlib_SRCDEPS = $(objdir)/include/.dir.stamp
> bpf_dlib_SOURCES = \
> - agg_qbin.c \
> + agg_lqbin.c agg_qbin.c \
> get_bvar.c \
> get_gvar.c set_gvar.c \
> get_tvar.c set_tvar.c \
> diff --git a/bpf/agg_lqbin.c b/bpf/agg_lqbin.c
> new file mode 100644
> index 00000000..3e981a65
> --- /dev/null
> +++ b/bpf/agg_lqbin.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
> + */
> +#include <linux/bpf.h>
> +#include <stdint.h>
> +#include <bpf-helpers.h>
> +
> +#ifndef noinline
> +# define noinline __attribute__((noinline))
> +#endif
> +
> +noinline uint64_t dt_agg_lqbin(int64_t val, int64_t base,
> + uint64_t levels, uint64_t steps)
steps -> step
(I.e. the 'step' between two consecutive levels rather than the number of
steps since that is levels.)
> +{
> + uint64_t level;
> +
> + if (steps == 0 || val < base)
> + return 0;
Note that steps can never be 0 (it is a positive integer constant). But
there is no harm to test here, so OK to leave that.
> + level = (val - base) / steps;
> + if (level > levels)
> + level = levels;
> + return level + 1;
> +}
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index ccf475fe..17159e24 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3315,80 +3315,6 @@ dt_cg_agg_llquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> }
>
> -static void
> -dt_cg_agg_lquantize_impl(dt_irlist_t *dlp, dt_regset_t *drp, int dreg,
> - int vreg, int ireg, int32_t base, uint16_t levels,
> - uint16_t step)
> -{
> - uint_t lbl_l1 = dt_irlist_label(dlp);
> - uint_t lbl_l2 = dt_irlist_label(dlp);
> - uint_t lbl_add = dt_irlist_label(dlp);
> -
> - TRACE_REGSET(" Impl: Begin");
> -
> - /*
> - * // (%rd = dreg -- agg data)
> - * // (%rv = val)
> - * // (%ri = incr)
> - * if (val >= base) // jge %rv, base, L1
> - * goto L1; //
> - *
> - * tmp = dreg; // mov %r0, %rd
> - * goto ADD; // ja ADD
> - */
> - emit(dlp, BPF_BRANCH_IMM(BPF_JSGE, vreg, base, lbl_l1));
> - dt_regset_xalloc(drp, BPF_REG_0);
> - emit(dlp, BPF_MOV_REG(BPF_REG_0, dreg));
> - emit(dlp, BPF_JUMP(lbl_add));
> -
> - /*
> - * L1: level = (val - base) / step;
> - * // L1:
> - * // mov %r0, %rv
> - * // sub %r0, base
> - * // div %r0, step
> - * if (level < levels) // jlt %r0, levels, L2
> - * goto L2;
> - *
> - * tmp = dreg + 8 * (levels + 1);
> - * // mov %r0, levels + 1
> - * // lsh %r0, 3
> - * // add %r0, %rd
> - * goto ADD; // ja ADD
> - */
> - emitl(dlp, lbl_l1,
> - BPF_MOV_REG(BPF_REG_0, vreg));
> - emit(dlp, BPF_ALU64_IMM(BPF_SUB, BPF_REG_0, base));
> - emit(dlp, BPF_ALU64_IMM(BPF_DIV, BPF_REG_0, step));
> - emit(dlp, BPF_BRANCH_IMM(BPF_JLT, BPF_REG_0, levels, lbl_l2));
> -
> - emit(dlp, BPF_MOV_IMM(BPF_REG_0, levels + 1));
> - emit(dlp, BPF_ALU64_IMM(BPF_LSH, BPF_REG_0, 3));
> - emit(dlp, BPF_ALU64_REG(BPF_ADD, BPF_REG_0, dreg));
> - emit(dlp, BPF_JUMP(lbl_add));
> -
> - /*
> - * L2: tmp = dreg + 8 * (level + 1);
> - * // L2:
> - * // add %r0, 1
> - * // lsh %r0, 3
> - * // add %r0, %rd
> - */
> - emitl(dlp, lbl_l2,
> - BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1));
> - emit(dlp, BPF_ALU64_IMM(BPF_LSH, BPF_REG_0, 3));
> - emit(dlp, BPF_ALU64_REG(BPF_ADD, BPF_REG_0, dreg));
> -
> - /*
> - * ADD: (*tmp) += incr; // xadd [%r0 + 0], %ri
> - */
> - emitl(dlp, lbl_add,
> - BPF_XADD_REG(BPF_DW, BPF_REG_0, 0, ireg));
> - dt_regset_free(drp, BPF_REG_0);
> -
> - TRACE_REGSET(" Impl: End ");
> -}
> -
> static void
> dt_cg_agg_lquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> dt_irlist_t *dlp, dt_regset_t *drp)
> @@ -3411,6 +3337,7 @@ dt_cg_agg_lquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> uint64_t step = 1;
> int64_t baseval, limitval;
> int sz, ireg;
> + dt_ident_t *idp;
>
> if (arg1->dn_kind != DT_NODE_INT)
> dnerror(arg1, D_LQUANT_BASETYPE, "lquantize( ) argument #1 "
> @@ -3526,6 +3453,21 @@ dt_cg_agg_lquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
>
> dt_cg_node(dnp->dn_aggfun->dn_args, dlp, drp);
>
> + /* quantize the value to a 0-based bin # using dt_agg_lqbin() */
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> + emit(dlp, BPF_MOV_REG(BPF_REG_1, dnp->dn_aggfun->dn_args->dn_reg));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_2, baseval));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_3, nlevels));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_4, step));
> + idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_agg_lqbin");
> + assert(idp != NULL);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> + dt_regset_free_args(drp);
> + emit(dlp, BPF_MOV_REG(dnp->dn_aggfun->dn_args->dn_reg, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
I think we have been trying to align the BPF instructions in a way where the
BPF_* argument lines up, so the ones with emit should have an extra space
before BPF_*.
> +
> if (incr == NULL) {
> if ((ireg = dt_regset_alloc(drp)) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> @@ -3536,9 +3478,8 @@ dt_cg_agg_lquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> ireg = incr->dn_reg;
> }
>
> - DT_CG_AGG_IMPL(aid, sz, dlp, drp, dt_cg_agg_lquantize_impl,
> - dnp->dn_aggfun->dn_args->dn_reg, ireg,
> - baseval, nlevels, step);
> + DT_CG_AGG_IMPL(aid, sz, dlp, drp, dt_cg_agg_quantize_impl,
> + dnp->dn_aggfun->dn_args->dn_reg, ireg, nlevels + 1);
>
> dt_regset_free(drp, dnp->dn_aggfun->dn_args->dn_reg);
> dt_regset_free(drp, ireg);
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index e3ab2231..f1d0b29a 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -52,6 +52,7 @@ static const dt_ident_t dt_bpf_symbols[] = {
> /* BPF built-in functions */
> DT_BPF_SYMBOL(dt_program, DT_IDENT_FUNC),
> /* BPF library (external) functions */
> + DT_BPF_SYMBOL(dt_agg_lqbin, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_agg_qbin, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_get_bvar, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_get_gvar, DT_IDENT_SYMBOL),
> --
> 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