[DTrace-devel] [PATCH v4] Implement the lockstat provider

[Eugene Loh]

On 5/24/23 23:44, Kris Van Hees wrote:

> On Wed, May 24, 2023 at 08:17:45PM -0400, Eugene Loh via DTrace-devel wrote:
>> Why does lockstat not report a function (e.g., dtrace -lP lockstat)?  I
>> thought that some probes instrumented source code (e.g., lockstat) and
>> therefore reported a module and function; others (like profile, cpc, dtrace)
>> did not and therefore did not. It appears that lockstat is reporting a
>> module but not a function, but clearly lockstat is instrumenting source
>> code.
> No, lockstat is not instrumenting source code.  It is providing probes that
> allow instrumentation of locking primitives in the kernel.  The fact that the
> legacy version reported multiple probes (each with a function name listed) for
> SDT probes is simply an artifact of the implementation requiring multiple
> probes locations for the same (conceptual) probe.  That is no longer needed
> because we overlay multiple probe locations with a single probe.  Which is
> perfectly valid because we are not probing location but rather logic.

So, should lockstat *NOT* report a module?  The documentation says:

         module
         If this probe corresponds to a specific program location,
         the name of the kernel module, library, or user-space
         program in which the probe is located.

         function
         If this probe corresponds to a specific program location,
         the name of the program function in which the probe is located.

So if we're changing lockstat from instrumenting specific program 
locations to being conceptual points, then no module should be reported.

Also, if lockstat should not report functions (or modules), the tests 
should check that behavior.  Instead, the .r.p file is allowing either 
behavior, even though apparently the new behavior is no function should 
appear.

>> On 5/24/23 17:56, Kris Van Hees via DTrace-devel wrote:
>>
>>> diff --git a/test/unittest/lockstat/tst.lv-adaptive-acquire-error.r.p b/test/unittest/lockstat/tst.lv-adaptive-acquire-error.r.p
>>> new file mode 100755
>>> +#!/usr/bin/awk -f
>>> +NR == 1 { next; }
>>> +NR == 2 && NF == 5 { $1 = "PROBE"; $4 = ""; gsub(/  +/, " "); print; next; }
>>> +NR == 2 && NF == 4 { $1 = "PROBE"; gsub(/  +/, " "); print; next; }
>>> +/^ *[0-9]+/ { exit; }
>>> +{ print; }
>> Since this is nontrivial (as Nick already mentioned) and awk can be
>> commented, how about a few comments to explain what's going on?
> Even though you and Nick) mention it is non-trivial, it is clearly easy enough
> to understand if you know awk.  And with your suggestion below I think it is
> even more clear.  I don't see what comments can make it more obvious than it
> already is.
>
>> The two NR==2 lines could perhaps be reduced to:
>>          NR==2 { print "PROBEID", $2, $3, $NF; next; }
> Sure.
>
>> What's the exit line for?
> If this is used e.g. on the legacy version where the .r files were generated,
> you do not want multiple occurence (due to multiple probe locations) to be
> listed because they all will have the same argument declarations anyway.  So,
> we bail if there is a 2nd probe listed.

I'm confused.  If a second probe is listed, that would be an error, 
right?  We should report an error if a second probe is listed.  The .r.p 
file should not be for automating generation of .r files on legacy 
systems;  the .r.p file should be for testing correctness of the current 
bits.