[DTrace-devel] [PATCH] Use default action ONLY if the clause is empty
Kris Van Hees
kris.van.hees at oracle.com
Thu Aug 20 12:35:38 PDT 2020
On Thu, Aug 20, 2020 at 12:02:31PM -0700, Eugene Loh wrote:
> On 08/20/2020 11:10 AM, Kris Van Hees wrote:
>
> > On Thu, Aug 20, 2020 at 01:13:30PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> If a clause is empty, use the default clause; this was working.
> >> On the other hand, if a clause is not empty, do not use the default
> >> clause, even if the clause traces no data. This was not working
> >> in DTv2. Arguably, the new behavior was more sensible, but it
> >> breaks precedent, including in some very important and common
> >> cases, such as clauses that simply manipulate D variables.
> >>
> >> Restore the legacy behavior.
> >>
> >> Specifically, split the epilogue into two pieces:
> >> - If we explicitly wrote any data or if the clause was empty,
> >> output the buffer.
> >> - Exit.
> > I am not sure whether this is the correct approach. Because of some of the
> > limitations in what we can do with BPF in the kernel, I anticipate that we
> > need to actually have some of these clauses that do not generate data still be
> > reported in the trace output buffer. So, I think instead of this approach we
> > need to somehow mark the clause (perhaps in the datadesc in some way) as one
> > that should not display output.
>
> Thanks for taking a look.
>
> If I understand what you're saying, you propose writing the output
> buffer in each case, but simply suppressing the printing on the consumer
> end. I'm uncomfortable with that approach. A case I'm interested in is
> a probe that fires very often, updating some important D state, and a
> different probe that fires rarely, reporting something of interest
> depending on that state. It's a poor-man's speculative tracing, if you
> will. You do not want the frequent probe to send anything to user space
> since it may incur lots of overhead. Does that make sense?
>
> Maybe you're saying there some some cases where we are forced to push
> more records to user space to move some processing there since we cannot
> do something in the kernel due to BPF limitations. Fine, but using that
> machinery to fix the default-action bug seems to me like a different
> issue and the wrong fix to this particular bug.
I am not suggesting using any machinery for anything to fix the default action
issue. I am pointing out that the fact that a clause does not record any data
(i.e. it does not contain any statements that generate probe data records) is
not a conclusive condition to determine that we do not need to write the default
data (epid) to the output buffer. There will be cases where we need an entry
to be written to the output buffer without any associated data records.
One way could be to set a flag on the statement (or the pcb) when we generate
code for an action that generates data. We can then use that same flag for
actions that need the epid to be recorded (even when there are no data records).
There is legacy code that used to investigate actions (when a new one gets
appended to a statement, I think) to ensure that the new action is compatible
with the already compiled actions for that statement. That one also did the
destructive-action flag processing.
Now that we compile the entire clause as a single unit, we can perform the same
kind of functionality as we compile code, and update and/or test flags. That
is a good use to accomplish what you are doing here, and it combines the need
for the non-data-generating action case you are solving and the need to still
be able to have clauses that do not generate data have the epid recorded in the
output buffer.
> > Can you look at ways we could do that? Maybe we can stick a flag somewhere
> > to signal this? It needs to be something that can be easily checked for in
> > the buffer processing in dt_consume.
> >
> >> Do not modify the prologue. A few instructions in the prologue could
> >> be eliminated for clauses that will not write the output buffer, but
> >> the instructions are inconsequential and would be difficult to eliminate.
> >>
> >> To check if we "wrote any data" it is not enough to see how much data
> >> was written. E.g., a printf("format") with no formatting attributes
> >> will write no explicit data. So, we have to check the number of records.
> >>
> >> The test test/unittest/funcs/tst.default.d was too mild to catch
> >> this problem. Add test/unittest/actions/default/tst.default.d .
> >>
> >> Orabug: 31772287
> >> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> >> ---
> >> libdtrace/dt_cg.c | 42 ++++++++++++++++-----
> >> test/unittest/actions/default/tst.default.d | 40 ++++++++++++++++++++
> >> test/unittest/actions/default/tst.default.r | 7 ++++
> >> 3 files changed, 79 insertions(+), 10 deletions(-)
> >> create mode 100644 test/unittest/actions/default/tst.default.d
> >> create mode 100644 test/unittest/actions/default/tst.default.r
> >>
> >> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> >> index d5532c55..821e5b18 100644
> >> --- a/libdtrace/dt_cg.c
> >> +++ b/libdtrace/dt_cg.c
> >> @@ -259,15 +259,14 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> >> }
> >>
> >> /*
> >> - * Generate the function epilogue:
> >> + * Generate the function epilogue (output the buffer):
> >> * 4. If a fault was flagged, return 0.
> >> * 5. Submit the buffer to the perf event output buffer for the current
> >> * cpu.
> >> - * 6. Return 0
> >> * }
> >> */
> >> static void
> >> -dt_cg_epilogue(dt_pcb_t *pcb)
> >> +dt_cg_epilogue_output(dt_pcb_t *pcb)
> >> {
> >> dt_irlist_t *dlp = &pcb->pcb_ir;
> >> dt_ident_t *buffers = dt_dlib_get_map(pcb->pcb_hdl, "buffers");
> >> @@ -304,11 +303,6 @@ dt_cg_epilogue(dt_pcb_t *pcb)
> >> * // mov %r5, pcb->pcb_bufoff
> >> * // add %r4, 4
> >> * // call bpf_perf_event_output
> >> - *
> >> - * exit:
> >> - * return 0; // mov %r0, 0
> >> - * // exit
> >> - * }
> >> */
> >> instr = BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX);
> >> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> >> @@ -326,11 +320,30 @@ dt_cg_epilogue(dt_pcb_t *pcb)
> >> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> >> instr = BPF_CALL_HELPER(BPF_FUNC_perf_event_output);
> >> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> >> + TRACE_REGSET("Epilogue: End ");
> >> +}
> >> +
> >> +/*
> >> + * Generate the function epilogue (exit):
> >> + * 6. Return 0
> >> + * }
> >> + */
> >> +static void
> >> +dt_cg_epilogue_exit(dt_pcb_t *pcb)
> >> +{
> >> + dt_irlist_t *dlp = &pcb->pcb_ir;
> >> + struct bpf_insn instr;
> >> +
> >> + /*
> >> + * exit:
> >> + * return 0; // mov %r0, 0
> >> + * // exit
> >> + * }
> >> + */
> >> instr = BPF_MOV_IMM(BPF_REG_0, 0);
> >> dt_irlist_append(dlp, dt_cg_node_alloc(pcb->pcb_exitlbl, instr));
> >> instr = BPF_RETURN();
> >> dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> >> - TRACE_REGSET("Epilogue: End ");
> >> }
> >>
> >> /*
> >> @@ -2968,8 +2981,10 @@ dt_cg(dt_pcb_t *pcb, dt_node_t *dnp)
> >> dt_cg_node(dnp, &pcb->pcb_ir, pcb->pcb_regs);
> >> } else if (dnp->dn_kind == DT_NODE_CLAUSE) {
> >> dt_irlist_t *dlp = &pcb->pcb_ir;
> >> + int start_nrecs;
> >>
> >> dt_cg_prologue(pcb, dnp->dn_pred);
> >> + start_nrecs = pcb->pcb_ddesc->dtdd_nrecs;
> >>
> >> for (act = dnp->dn_acts; act != NULL; act = act->dn_list) {
> >> pcb->pcb_dret = act->dn_expr;
> >> @@ -2992,7 +3007,14 @@ dt_cg(dt_pcb_t *pcb, dt_node_t *dnp)
> >> }
> >> }
> >>
> >> - dt_cg_epilogue(pcb);
> >> + /*
> >> + * If we put something in the buffer or we have the default
> >> + * action, output the buffer.
> >> + */
> >> + if (pcb->pcb_ddesc->dtdd_nrecs != start_nrecs ||
> >> + dnp->dn_acts == NULL)
> >> + dt_cg_epilogue_output(pcb);
> >> + dt_cg_epilogue_exit(pcb);
> >> } else if (dnp->dn_kind == DT_NODE_TRAMPOLINE) {
> >> assert(pcb->pcb_probe != NULL);
> >>
> >> diff --git a/test/unittest/actions/default/tst.default.d b/test/unittest/actions/default/tst.default.d
> >> new file mode 100644
> >> index 00000000..26469c40
> >> --- /dev/null
> >> +++ b/test/unittest/actions/default/tst.default.d
> >> @@ -0,0 +1,40 @@
> >> +/*
> >> + * Oracle Linux DTrace.
> >> + * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
> >> + * Licensed under the Universal Permissive License v 1.0 as shown at
> >> + * http://oss.oracle.com/licenses/upl.
> >> + */
> >> +
> >> +/*
> >> + * ASSERTION: The default action is used for the empty clause but not
> >> + * for a non-empty clause, even if it does not trace anything.
> >> + * A special case is a printf() with no attributes, which is
> >> + * a trace record with no extra data in the tracing record;
> >> + * make sure the printf() occurs.
> >> + *
> >> + * SECTION: Actions and Subroutines/default
> >> + */
> >> +
> >> +/* should not trace, but should update n */
> >> +BEGIN
> >> +{ n = 1; }
> >> +
> >> +/* should trace, "hello world" should appear */
> >> +tick-14
> >> +{ printf("hello world"); }
> >> +
> >> +/* should not trace, but should update n */
> >> +tick-13
> >> +{ n += 2; }
> >> +
> >> +/* should trace */
> >> +tick-12
> >> +{ }
> >> +
> >> +/* should not trace, but should update n */
> >> +tick-11
> >> +{ n += 4; }
> >> +
> >> +/* should trace, and n should be 1+2+4=7 */
> >> +tick-10
> >> +{ trace(n); exit(0) }
> >> diff --git a/test/unittest/actions/default/tst.default.r b/test/unittest/actions/default/tst.default.r
> >> new file mode 100644
> >> index 00000000..ef0e5257
> >> --- /dev/null
> >> +++ b/test/unittest/actions/default/tst.default.r
> >> @@ -0,0 +1,7 @@
> >> + FUNCTION:NAME
> >> + :tick-14 hello world
> >> + :tick-12
> >> + :tick-10 7
> >> +
> >> +-- @@stderr --
> >> +dtrace: script 'test/unittest/actions/default/tst.default.d' matched 6 probes
> >> --
> >> 2.18.4
> >>
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
>
>
> _______________________________________________
> 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