This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC V2] Systemtap translator support for kernel hardware breakpoints


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

> [...]
> Thanks for the prompt review. Attaching a new version of patches that
> address your comments.

Thanks for the quick turnaround.

> Perf allows multiplexing only for non-pinned breakpoints, not the
> pinned ones used by systemtap. If this script is allowed to run with a
> warning, the runtime will register the first 'n' possible requests and
> reject the later ones. Is it not better to notify to the user that
> they are making more requests than the maximum limit imposed by
> hardware ? (more on this later)

I didn't say "don't notify", just "don't make it a translate-time
error".  It is possible that in the future, some of these limitations
will be relaxed by virtualization or improvements in hardware.  By
hard-coding them in systemtap, we are forcing ourselves to worry about
this again in the future.


> [...]
>> Are you sure this CONFIG_arch* stuff is needed?  Let the kernel
>> try 8-byte watchpoints, and let us handle the error.
>>
>
> I cannot foresee the advantage of requesting an erroneous breakpoint,
> only to see the request fail later. Could you pls let me know if there
> are any specific issues wrt above, apart from the hassle of
> arch-specific checks?

This becomes a porting burden in systemtap, when we could just rely on
the existing porting effort in the kernel.  The kernel will already
make the same checks and report an error.  Checks on our own side,
made immediately before the kernel checks, are redundant.


>>> +  s.op->newline()<<  "if ( !IS_ERR(stap_hwbkpt_ret_array[i]) ) {";
>>> +  s.op->newline()<<  " stap_hwbkpt_ret_array[i] = register_wide_hw_breakpoint( hp, (void *)&enter_hwbkpt_probe );";
>>> +  s.op->newline()<<  " if (IS_ERR(stap_hwbkpt_ret_array[i])) {";
>>> +  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
>>
>> I mentioned this in the previous review, but this is insufficient.
>> (Please review my earlier comments in case something else was missed.)
>> If the [i]th registration attempt fails, and you return an rc != 0 to
>> the wrapping code, this code right here must ensure that all
>> already-registered entries ([0..i-1]) have been unregistered.

> I beg to differ here. As is the case with dwarfless probes, is it
> not better to ignore an erroneous probe(that failed registration)
> and continue running the other probes that got successfully
> registered ? [...]

Sure, but then your code must keep rc zeroed instead of setting it to
an nonzero PTR_ERR.


- FChE


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]