[DTrace-devel] [PATCH] Use default action ONLY if the clause is empty
Eugene Loh
eugene.loh at oracle.com
Thu Aug 20 15:20:28 PDT 2020
On 08/20/2020 12:35 PM, Kris Van Hees wrote:
> 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.
Good to know, but what is an example of that?
Maybe I can supply an example: printf("format"), with no format
conversions (no real data to return to user space... just indicate that
the probe has fired). But this is already handled: we say that a
record of zero size is being written. That's why the code checks the
number of records rather than the number of bytes. So, again, this case
is already being handled. Would this existing mechanism suffice for the
case you imagine?
> 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).
Again, the existing mechanism is to say that there is a record of zero
size. Plus, without any examples of what case you're concerned about, I
wouldn't know how to use "the mechanism" regardless of what that
mechanism is (pcb flag or whatever).
> 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.
I assume that's the big FIXME section of dt_stmt_append().
> That one also did the destructive-action flag processing.
Perhaps that's besides the point. Anyhow, FWIW, I think (if we're
talking about the same code) it doesn't so much do "the"
destructive-action flag processing. Rather, it makes sure that the new
action is compatible with the others (as you say), in particular making
sure that there are no conflicts with respect to speculation.
Specifically, destructive actions may not be speculative. So, e.g.,
this code has nothing to do with -w processing. Its focus is currently
more just speculation. Again, however, all this may be besides the point.
> 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
More information about the DTrace-devel
mailing list