[DTrace-devel] [PATCH] Use default action ONLY if the clause is empty

[Eugene Loh]

I am quite stuck on this...

On 08/20/2020 08:58 PM, Kris Van Hees wrote:

> On Thu, Aug 20, 2020 at 03:20:28PM -0700, Eugene Loh wrote:
>> 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?
> The case you mention is a case of a data recording action, so of course it
> makes sense to use a data record.  In fact, I implemented that zero size
> record descriptor in commit a03231b19.  As I mention, there are needs to have
> a clause record its EPID (the minimum data ever recorded) when there is no
> data recording taking place.  The commit() action is an example of that case.

We're going in circles here.  Again, if a clause needs to records its 
EPID, a zero-size record does the job.

Regarding commit(), I realize that speculation is not yet implemented, 
but here is the placeholder code I see so far:

     dt_cg_act_commit(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t 
kind) {
         [...]
         off = dt_rec_add(..., DTRACEACT_COMMIT,
                          sizeof(uint64_t), sizeof(uint64_t), NULL, 
DT_ACT_COMMIT);
         instr = BPF_STORE(BPF_DW, BPF_REG_9, off, BPF_REG_0);
         dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
     }

It's already adding a record.  As do speculate and discard.

>>> 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).
> As I mention above, that mechanism that I introduced is for data recording
> actions.  That is why it is valid to create a data record descriptor with a
> zero size for it.
>
> I don't think that there is a need to have the full details laid out already
> in order to account for the need for actions like commit().  It should suffice
> to know that there is a need to allow for probe firings to be recorded in the
> output buffer (recording of the EPID) for clauses that do not contain any
> data recording actions.  Therefore, we shouldn't make it conditional on
> whether there are data record descriptors but rather based on a flag that
> indicates that we need to write the generated trace record into the output
> buffer.

Whatever would set such a flag could as easily indicate a zero-size record.

I'm curious:  do you suggest that data-recording actions should also set 
that flag?  Or would the epilogue check both the number of records and 
the new flag?

> I am not hiding details from you - I do not have a completed design yet for
> things like speculative tracing but I do know it will need to have the EPID
> recorded in the output buffer even though there will not be any data recording
> actions in the clause.  Again, the full details of that do not need to be
> defined yet to establish the need.

And I am not trying to be difficult.  Quite the contrary, I'd like to be 
done with this.  But it's hard for me to figure out what substitute you 
want for the mechanism that is already there.

>>> 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().