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

[Kris Van Hees]

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

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.

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

It is beside the point, but yes, that code actually does perform the
destructive-action flag processing that I was referring to.  Not the '-w'
destructive execution option but rather the destructive-action flags that are
embedded in the action code.