Re: [RFC] Systemtap translator support for hw breakpoints on x86

[fche at redhat dot com (Frank Ch. Eigler)]

Prerna Saxena <prerna@linux.vnet.ibm.com> writes:

> [...]
>> Instead or in addition, the translator could make the whole probe point
>> type conditional on session.kernel_config("CONFIG_HAVE_HW_BREAKPOINT").
>
> I'll add this in addition to the pass 4 check. Will this check take
> care of cross compilation for a different kernel ? (I think it should)

Yes.

>>> +// Define macros for access type
>>> +  s.op->newline()<<  "#define HWBKPT_READ 0";
>>> +  s.op->newline()<<  "#define HWBKPT_WRITE 1";
>>> +  s.op->newline()<<  "#define HWBKPT_RW 2";
>>
>> (Are these necessary?  Could the translator not emit symbols like
>> HW_BREAKPOINT_W directly?)
>
> This had to be defined because some types are not defined on all
> architectures. X86 supports only HW_BREAKPOINT_W and HW_BREAKPOINT_RW,
> while ppc supports HW_BREAKPOINT_W and HW_BREAKPOINT_R. [...]

But does this support or lack of support manifest itself in a
different list of symbols defined by the kernel headers?  For example,
the symbol HW_BREAKPOINT_R is defined in linux/hw_breakpoint.h, so if
systemtap emits code that uses it, this will compile just fine on all
architectures.


> [...]  For now, I had designed the probes to error out if a user
> defined a length that was not supported by architecture (eg a length
> 3 on x86, as this supports lengths like 1,2,4 bytes only) [...]

My concern with the above was not the script language interface or
mappings, but only the cleanliness / simplicity of the generated code.
If the translator neglects to filter out unsupported lengths, and the
generated code propagates the value 3, will the kernel's registration
function check and reject such requests at run time?  If so, then
there is no need for the generated code to contain preemptive checks /
filters / error messages.  Just let it listen to the result code from
the kernel's registration function.

As to having the translator filter for parameters expected to be
valid, that is fine, but those checks need not show up redundantly in
generated C code.


>>> +  s.op->newline()<<  "     rc = PTR_ERR(stap_hwbkpt_ret_array[i]);";
>>> +  s.op->newline()<<  "      sdp->registered_p = 0;";
>>> +  s.op->newline(1)<<  "	    _stp_warn(\" Hwbkpt Probe %s : Registration error rc= %d, Addr = %p, name = %s\", probe_point, rc, addr, hwbkpt_symbol_name);";
>>> +  s.op->newline(-1)<<  "  }";
>>> +  s.op->newline()<<  "    else sdp->registered_p = 1;";
>>> +  s.op->newline()<<  "}";
>>> +  s.op->newline(-1)<<  "}"; // for loop
>>
>> Not so simple: if you get a failure on the sixth registration, you have to
>> unregister the prior ones.
> [...]
> However, in case less resources are available at runtime (say, only 3
> out of 4 debug registers are free on x86 when the stap-generated
> module is running), and the script has 4 hardware breakpoints -- the
> first 3 will get registered while _only_ the 4th request will fail at
> registration. I thought this was an acceptable behaviour.

OK, if you want to let the registration phase pass with only partial
successes, then the code must set rc = 0 around your stp_warn() call.
As the code is currently, rc != 0, so the registration loop assumes
that this particular derived_probe_group *has already cleaned up* any
partial registrations, and will not get a subsequent call.

- FChE