Controlling probe overhead

[David Smith]

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)