Re: [DTrace-devel] [PATCH v2 15/19] Ignore clauses: some clauses are impossible regardless of uprp

[Kris Van Hees <kris.van.hees@oracle.com>]

On Thu, Oct 24, 2024 at 03:30:13PM -0400, Eugene Loh via DTrace-devel wrote:
> 
> On 10/23/24 16:28, Kris Van Hees wrote:
> > On Tue, Sep 24, 2024 at 04:25:54PM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > In ignore_clauses, for an underlying probe uprp, we try to
> > > decide if we can safely ignore clause n.
> > > 
> > > Meanwhile, for some clauses, the probe description tells us the
> > > clause will not be called for any USDT probe, regardless of the
> > > underlying probe.  For example, "syscall::write:" can safely be
> > > ignored, for all uprp.
> > > 
> > > Add a dtsd_usdt variable to each statement to track status:
> > > 
> > >      USDT_FLAG_UNINITIALIZED  not yet initialized
> > > 
> > >      USDT_FLAG_POSSIBLE       clause could possibly be called
> > >                               for some USDT probe
> > > 
> > >      USDT_FLAG_IGNORE         clause can safely be ignored for
> > >                               all USDT probes
> > I think it would be better to use the dtsd_clauseflags member for this.
> > You can add dt_stmt_set_flag() and dt_stmt_test_flag() (or similar names)
> > to set and test for specific bits in the dtsd_clauseflags member using
> > DT_CLSFLAG_* constants.  You should be OK with just 2, one to indicate
> > POSSIBLE and one to indicate IGNORE (neither being set obviously meansnot
> > yet initialized).
> > 
> > I don't really know what symbol names would be best... perhaps for now use
> > DT_CLSFLAG_USDT_INCLUDE and DT_CLSFLAG_USDT_EXCLUDE?
> > 
> > Main thing I would like to accomplish here is simply to access flags on the
> > statements through functions rather than accessing them directly.
> 
> I'd like to push back here.
> 
> First of all, the patch exposes only dtsd_usdt at the dtrace.h level.  That
> variable can hide whatever it is that USDT wants to do with it.  Your
> proposal would expose at least USDT_INCLUDE/EXCLUDE (or POSSIBLE/IGNORE or
> whatever we want to call them) in dtrace.h, and I do not know if future USDT
> would have even more stuff it wants to play with, meaning even more changes
> outside of the provider.

My objection is primarily with adding a member to the statement struct for a
particular provider.  Especially when there is no need for that.  There is a
flags member already that can be used for these purposes.

We never know whether there might be future needs that might require changes
but that is not really a reason to make changes now that are not needed.

> Second, you say the main point is to access flags through functions rather
> than directly, but to date we've been accessing flags directly.  The
> set/test functions would be new.  If we want to stop the direct accesses,
> that would seem to be a different patch.

My point here is that doing this from *providers* is a bit different than
accessing things from the libdtrace core.  Providers have been written to be
more separated from the core of libdtrace, or at least I have been trying to
do so.  This particular use case seems to be a perfect situation where using
accessor functions makes a lot of sense.  Especially because you are changing
data in structures that are at the core of libdtrace - not just usiing it.

Alternatively, I'd propose simply not doing this caching of state concerning
which clauses to ignore, and having clause_ignore() determine it for a given
statement without using cached information (since these flags really amount to
caching of information for optinization purposes).  Perhaps that is the better
option right now?

	Kris