[DTrace-devel] [PATCH] Add support for built-in variable walltimestamp

[Kris Van Hees]

On Mon, Apr 26, 2021 at 04:40:05PM -0400, Eugene Loh wrote:
> On 4/24/21 10:07 PM, Kris Van Hees wrote:
> 
> > On Fri, Apr 23, 2021 at 09:50:39PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> Use bpf_ktime_get_ns() to get the time (in nsec) since boot.  This
> >> value must be adjusted to POSIX time.  So, when we load a built-in
> >> variable, check if it is walltimestamp.  If so, add BPF instructions
> >> to add the adjustment.
> >>
> >> It would be nice if the support for walltimestamp were more localized,
> >> but the BPF code that gets built-in variables is compiled at build
> >> time while the adjustment is determined at run time.
> > Why not store the adjustment as a value in the 'state' map as we already
> > do for some struct offsets that we need in get_bvar()?  That way you can
> > keep the walltimestamp handling all in the precompiler BPF code.  All you
> > need to do is populate an element in the 'state' map woth the correct
> > delta.
> 
> 
> That strikes me as unnecessarily complicated.  Not a huge deal -- none 
> of this is rocket science or particularly expensive -- I'm just saying 
> it's more complicated than it needs to be.  The code generator needs to 
> know about this stuff anyhow, since it has to figure out the adjustment 
> and then do *something* with that information.  In the current approach, 
> if the adjustment is needed, the cg figures it out and uses it.  Done.  
> What you propose means also storing this somewhere where BPF can find it 
> and then having BPF retrieve it before adding it.  (And currently, 
> get_bvar gets stuff from the state map with a bpf_map_lookup_elem() 
> call, which is a few lines of C that compile to about 7 BPF instructions.)
> 
> I agree it'd be nice if the walltimestamp stuff were all confined to 
> get_bvar.  That'd be nice.  But, again, cg needs to know this stuff 
> anyhow.  Putting the adjustment into get_bvar as well doesn't change 
> that, requires more instructions to retrieve it, and requires adding a 
> place to store this stuff.
> 
> The struct offsets aren't a fair comparison.  BPF needs to know those 
> offsets.
> 
> Anyhow, you asked.  :^)

How about using the newly discovered and (ab)used approach of a symbol like
STBSZ that gets resolved at load time?  That avoids adding custom code to the
code generator for this particular D variable, and it also avoids the extra
complexity of pulling a value out of a map.  (Come to think of it, we might
want to deprecate putting the offsets needed for ppid into that state map and
do that using a resolved symbol as well - but that is a separate issue).

This gives us a solution that solves both concerns.

> >> A few more walltimestamp tests pass now, but existing walltimestamp
> >> tests are rather lenient.  Add a new, more stringent test.  In particular,
> >> timestamp is supposed to be fixed for an entire clause while walltimestamp
> >> is fresh with every call.
> > What about timezones?  Does the CLOCK_REALTIME provide a timespec in UTC or
> > in local tiemzone?  The walltimestamp D variable needs to report in UTC.
> 
> I don't think timezones are relevant.  Neither one is actually reporting 
> a timestamp.  Both CLOCK_REALTIME and D walltimestamp are reporting 
> elapsed time since some well defined and universal instant (Unix 
> Epoch).  (Modulo relativistic effects!) one could just as well be in 
> outer space.

OK.