Re: Controlling probe overhead

[David Smith <dsmith at redhat dot com>]

Frank Ch. Eigler wrote:
> David Smith <dsmith@redhat.com> writes:
>
> > [...] BTW, I had to rework the STP_TIMING code a very small bit to make it
> > work correctly with the STP_OVERLOAD code.  The STP_TIMING code was
> > storing cycle counts as 32-bit values, where the STP_OVERLOAD code
> > wanted 64-bit cycle counts.  The STP_TIMING code now truncates down to
> > 32-bits a little later than it did originally.
>
> Note that the current code doesn't (intend to) truncate cycle counts,
> just individual samples of the get_cycles() values.

Right, but the STP_OVERLOAD stuff needed full 64-bit values.  So, now we 
keep 64-bit get_cycles() values, but truncate down when figuring out the 
elapsed time for STP_TIMING purposes.  Since the STP_OVERLOAD_INTERVAL 
is greater than the max value of a 32-bit value, I needed full 64-bit 
get_cycles() values.

> > [...] I've have one stress test (that Frank wrote) that will make a
> > RHEL5 system non-responsive.  The system doesn't crash - just
> > decides to no longer take any input.  The overload code kills the
> > script in less than 3 minutes.
>
> 3 minutes is almost certainly too long for a default overload
> detection interval.  I would expect something on the order of a few
> seconds.

Sorry, I wrote that from memory.  I went back and ran the test again, 
and I get the overload message in roughly 2 seconds.  It takes another 
60 seconds or so to deregister all the probes.

I was pleasantly surprised that this code didn't bother any of the test 
suite tests or my other stress tests - just the problem one.

> > Note that I haven't implemented the new error probes you and Frank
> > discussed.  I'd like to get the current code in (since it is quite
> > useful in its current state) before thinking about error probes.
>
> Indeed, they are independent ideas.
>
>
> > [...]
> > +    << "   -O         turn off automatic probe overload handling" << endl
>
> IMO, there is no need for this option.  Overload detection should
> always be present, and tunable with the (documented?) -D parameters.

I see your point and the option would be easy enough to remove.  I just 
thought it would be easier to make one knob to turn everything off if 
the STP_OVERLOAD bothered your systemtap use.

Hmm, another way to completely disable the STP_OVERLOAD stuff could be 
to do "stap -DSTP_OVERLOAD=0" (if I make one small change to the patch). 
 I'll do that and delete the option itself.

> If this code depends on the STP_TIMING stuff in the probe
> prologues/epilogues, than most of that code too could be on also,
> (with -t just controlling whether the final timing report is printed).

This code doesn't depend on the STP_TIMING stuff, it just uses the same 
cycles_atstart/cycles_atend counters (since it seemed silly to have two 
sets of counters doing exactly the same thing).

You can still turn STP_TIMING on/off independently of STP_OVERLOAD (and 
vice versa).

> > -  o->newline(1) << "int32_t cycles_atend = (int32_t) get_cycles ();";
> > -  // Handle 32-bit wraparound.
> > [...]
>
> Perhaps you could excerpt the actual generated overload/timing code
> here.  It looks like there may be more being done here than necessary.

You want to see the generated code?  OK, here goes:

Ignoring all the support code (variable decls and #defines), in the 
original enter_kprpobe_probe() function you'll see this:

===================
  #ifdef STP_TIMING
  int32_t cycles_atstart = (int32_t) get_cycles ();
  #endif

...

  #ifdef STP_TIMING
  {
    int32_t cycles_atend = (int32_t) get_cycles ();
    int32_t cycles_elapsed = (cycles_atend > cycles_atstart)
      ? (cycles_atend - cycles_atstart)
      : (~(int32_t)0) - cycles_atstart + cycles_atend + 1;
      if (likely (c->statp)) _stp_stat_add(*c->statp, cycles_elapsed);
  }
  #endif

===================

In the new enter_kprpobe_probe() function, you'll see this:

===================
  #if defined(STP_TIMING) || defined(STP_OVERLOAD)
  cycles_t cycles_atstart = get_cycles ();
  #endif

...

  #if defined(STP_TIMING) || defined(STP_OVERLOAD)
  {
    cycles_t cycles_atend = get_cycles ();
    int32_t cycles_elapsed = ((int32_t)cycles_atend > 
(int32_t)cycles_atstart)
      ? ((int32_t)cycles_atend - (int32_t)cycles_atstart)
      : (~(int32_t)0) - (int32_t)cycles_atstart + (int32_t)cycles_atend 
+ 1;
    #ifdef STP_TIMING
    if (likely (c->statp)) _stp_stat_add(*c->statp, cycles_elapsed);
    #endif
    #ifdef STP_OVERLOAD
    {
      cycles_t interval = (cycles_atend > c->cycles_base)
        ? (cycles_atend - c->cycles_base)
        : (STP_OVERLOAD_INTERVAL + 1);
      c->cycles_sum += cycles_elapsed;
      if (interval > STP_OVERLOAD_INTERVAL) {
        if (c->cycles_sum > STP_OVERLOAD_THRESHOLD) {
          _stp_error ("probe overhead exceeded threshold");
          atomic_set (&session_state, STAP_SESSION_ERROR);
        }
        c->cycles_base = cycles_atend;
        c->cycles_sum = 0;
      }
    }
    #endif
  }
  #endif
=================

I tried to mess with the STP_TIMING code as little as possible.

> > +  o->newline() << "#ifndef STP_OVERLOAD_INTERVAL";
> > +  o->newline() << "#define STP_OVERLOAD_INTERVAL 1000000000LL";
> > +  o->newline() << "#endif";
> > +  o->newline() << "#ifndef STP_OVERLOAD_THRESHOLD";
> > +  o->newline() << "#define STP_OVERLOAD_THRESHOLD 500000000LL";
> > +  o->newline() << "#endif";
>
> These quantities should probably depend on the processor, so that
> overload intervals are measured in units of time rather than cycles.

Hmm, those quantities do depend a bit on the processor, since cycles 
depend on the processor.  However, trying to map time into cycles is 
typically a losing game, especially on systems with variable clock speeds.

--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)