[DTrace-devel] [PATCH v3 03/08] Record CPU that BEGIN and END probes run on in the 'state' BPF map

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

FWIW, minor comments below.


On 09/17/2020 03:06 PM, Kris Van Hees wrote:
> On Thu, Sep 17, 2020 at 02:50:46PM -0700, Eugene Loh wrote:
>> On 09/16/2020 04:11 PM, Kris Van Hees wrote:
>>> diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
>>> @@ -102,6 +113,47 @@ static void trampoline(dt_pcb_t *pcb)
>>>    
>>>    	dt_cg_tramp_prologue_act(pcb, lbl_exit, act);
>>>    
>>> +	if (key) {
>>> +		/*
>>> +		 *     key = DT_STATE_BEGANON;
>>> +		 *			// stw [%fp + DT_STK_SPILL(0)],
>>> +		 *			//			DT_STATE_BEGANON
>> The references to DT_STATE_BEGANON are simply wrong.  "key" was set
>> earlier, whether to DT_STATE_BEGANON or DT_STATE_ENDEDON.  At this
>> point, we are simply preparing key (whatever its value) to be passed
>> into a function call.  I know we disagree on how this pseudocode should
>> look, but such comments typically describe the actual code rather
>> closely and I thought you said you would make some change. Could it be
>> that patch changes got dropped between the cracks?
> I forgot to update the comment to add an explanation that this shows the code
> as it is generated for BEGANON and that ENDEDON is identical aside from the
> key value.

Thanks for cleaning that up.

> So, again, no the references to DT_STATE_BEGANON are not simply wrong.  The
> comments show the pseudo code for the BPF code being generated.  There is no
> BPF code being generated earlier that sets key to some value.  Do not confuse
> the C code that generates the BPF code with the pseudo code that shows the BPF
> code being generated.  For the purpose of the pseudo code (and the BPF code)
> one of the inportant things here is to understand that we're generating static
> code for the specific probe we're dealing with.

The BPF is not picking DT_STATE_[BEGAN|ENDED]ON.  It's simply storing an 
IMM value.  What that value is was set by earlier C code.  Anyhow, 
thanks for the patch.  I just wanted to say where I was coming from.  
It's great to see this functionality heading out the door.