[DTrace-devel] [PATCH 02/13] Add code for quantize() aggregation function
Kris Van Hees
kris.van.hees at oracle.com
Mon Dec 7 14:23:40 PST 2020
On Wed, Dec 02, 2020 at 01:54:47PM -0500, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
We relaly need to have a commit message here, especially since you are
using a pre-compiled BPF function to calculate the bin number. That is
worth mentioning here as a technique used to implement this functionality.
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> bpf/Build | 1 +
> bpf/agg_qbin.c | 40 +++++++++++++++++++++++
> libdtrace/dt_cg.c | 75 +++++++++++++++++++++++++++++++++++++++++++-
> libdtrace/dt_dlibs.c | 1 +
> 4 files changed, 116 insertions(+), 1 deletion(-)
> create mode 100644 bpf/agg_qbin.c
>
> diff --git a/bpf/Build b/bpf/Build
> index bd7683e0..ae298f79 100644
> --- a/bpf/Build
> +++ b/bpf/Build
> @@ -22,6 +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 \
> get_bvar.c \
> get_gvar.c set_gvar.c \
> get_tvar.c set_tvar.c \
> diff --git a/bpf/agg_qbin.c b/bpf/agg_qbin.c
> new file mode 100644
> index 00000000..741dfe27
> --- /dev/null
> +++ b/bpf/agg_qbin.c
> @@ -0,0 +1,40 @@
> +// 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 int16_t dt_agg_qbin(int64_t val)
> +{
> + int16_t off;
> + uint64_t tmp;
> +
> + if (val == 0)
> + return 63;
> + if (val == 0x8000000000000000)
> + return 0;
> +
> + tmp = val;
> + if (val < 0)
> + tmp *= -1;
> +
> + /* now, tmp has at least one 1, while the leading bit is 0 */
> + off = 1;
> + if (tmp & 0x7fffffff00000000) { off += 32; tmp >>= 32; }
> + if (tmp & 0x00000000ffff0000) { off += 16; tmp >>= 16; }
> + if (tmp & 0x000000000000ff00) { off += 8; tmp >>= 8; }
> + if (tmp & 0x00000000000000f0) { off += 4; tmp >>= 4; }
> + if (tmp & 0x000000000000000c) { off += 2; tmp >>= 2; }
> + if (tmp & 0x0000000000000002) { off += 1; }
> +
> + if (val < 0)
> + off *= -1;
> + off += 63;
> + return off;
> +}
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 178f82b0..ccf475fe 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3114,6 +3114,35 @@ dt_cg_agg_count(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> TRACE_REGSET(" AggCnt: End ");
> }
>
> +static void
> +dt_cg_agg_quantize_impl(dt_irlist_t *dlp, dt_regset_t *drp, int dreg, int vreg, int ireg, int maxbin)
> +{
> + uint_t L = dt_irlist_label(dlp);
> + int offreg;
> +
> + TRACE_REGSET(" Impl: Begin");
> +
> + if ((offreg = dt_regset_alloc(drp)) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + /* clip to in-bounds */
> + emit(dlp, BPF_MOV_IMM(offreg, maxbin));
What does this do?
> + emit(dlp, BPF_BRANCH_IMM(BPF_JGT, vreg, maxbin, L));
> + emit(dlp, BPF_MOV_IMM(offreg, 0));
What does this do?
> + emit(dlp, BPF_BRANCH_IMM(BPF_JLT, vreg, 0, L));
> + emit(dlp, BPF_MOV_REG(offreg, vreg));
Hm, so you test the bin number against the boundaries but whether or not
the test succeeds you are proceeding with the data update? I can see that
we can expect that the bin number will be valid *but* since we do not have
control over external modification of the bpf_lib.o file and therefore the
implementation details of the code that calculates the bin number (e.g what
if someone replaces that file with alternative implementations as a way to
be nasty), we really should not perform the data update if those tests fail.
Yes, that means we never update the data, but that is better than possibly
updating an unknown buffer location. And in the future we may be able to
have this trigger an ERROR probe, perhaps.
> + /* *(dest + 8 * off) += incr */
> + emitl(dlp, L,
> + BPF_ALU64_IMM(BPF_MUL, offreg, 8));
Label should not be here.
> + emit(dlp, BPF_ALU64_REG(BPF_ADD, offreg, dreg));
> + emit(dlp, BPF_XADD_REG(BPF_DW, offreg, 0, ireg));
Label should be at a NOP here.
> +
> + dt_regset_free(drp, offreg);
> +
> + TRACE_REGSET(" Impl: End ");
> +}
> +
> static void
> dt_cg_agg_llquantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> dt_irlist_t *dlp, dt_regset_t *drp)
> @@ -3541,13 +3570,57 @@ static void
> dt_cg_agg_quantize(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> dt_irlist_t *dlp, dt_regset_t *drp)
> {
> + dt_ident_t *idp;
> dt_node_t *incr;
> - int sz = DTRACE_QUANTIZE_NBUCKETS * sizeof(uint64_t);
> + int ireg, sz = DTRACE_QUANTIZE_NBUCKETS * sizeof(uint64_t);
> +
> + /*
> + * The quantize() implementation is currently hardwired for
> + * DTRACE_QUANTIZE_NBUCKETS 127
> + * DTRACE_QUANTIZE_ZEROBUCKET 63
> + * These values are defined in include/dtrace/actions_defines.h
> + */
> + assert(DTRACE_QUANTIZE_NBUCKETS == 127 &&
> + DTRACE_QUANTIZE_ZEROBUCKET == 63);
>
> incr = dt_cg_agg_opt_incr(dnp, dnp->dn_aggfun->dn_args, "quantize", 2);
>
> if (aid->di_offset == -1)
> aid->di_offset = DT_CG_AGG_OFFSET(pcb, sz);
> +
> + TRACE_REGSET(" AggQ : Begin");
> +
> + dt_cg_node(dnp->dn_aggfun->dn_args, dlp, drp);
> +
> + /* quantize the value to a 0-based bin # using dt_agg_qbin() */
> + 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));
> + idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_agg_qbin");
> + 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);
> +
> + if (incr == NULL) {
> + if ((ireg = dt_regset_alloc(drp)) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_MOV_IMM(ireg, 1));
> + } else {
> + dt_cg_node(incr, dlp, drp);
> + ireg = incr->dn_reg;
> + }
> +
> + DT_CG_AGG_IMPL(aid, sz, dlp, drp, dt_cg_agg_quantize_impl,
> + dnp->dn_aggfun->dn_args->dn_reg, ireg, 126);
> +
> + dt_regset_free(drp, dnp->dn_aggfun->dn_args->dn_reg);
> + dt_regset_free(drp, ireg);
> +
> + TRACE_REGSET(" AggQ : End ");
> }
>
> static void
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index 6c6e11eb..e3ab2231 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_qbin, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_get_bvar, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_get_gvar, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_get_string, 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