[DTrace-devel] [PATCH] Use default action ONLY if the clause is empty
Eugene Loh
eugene.loh at oracle.com
Thu Aug 20 12:02:31 PDT 2020
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.
> 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
More information about the DTrace-devel
mailing list