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

[Kris Van Hees]

On Thu, Sep 17, 2020 at 02:50:46PM -0700, Eugene Loh wrote:
> I'm confused what the difference is between v1 and v3.  In particular, ...
> 
> 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.

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.

> Incidentally, there is something else I do not understand.  You use 
> spill slots to stage arguments for function calls.  (E.g., something 
> similar in staging the exit() action activity state.)  But don't we need 
> to worry about if those spill slots are in use for some registers?

There is no code that can be using spill slots at this point because we're
generating the trampoline code.  At this point it is guaranteed that no
register spilling can have occured.

> > +		 *     val = bpf_get_smp_processor_id();
> > +		 *			// call bpf_get_smp_processor_id
> > +		 *			// stw [%fp + DT_STK_SPILL(1)], %r0
> > +		 *     bpf_map_update_elem(state, &key, &val, BPF_ANY);
> > +		 *			// lddw %r1, &state
> > +		 *			// mov %r2, %fp
> > +		 *			// add %r2, DT_STK_SPILL(0)
> > +		 *			// mov %r3, %fp
> > +		 *			// add %r3, DT_STK_SPILL(1)
> > +		 *			// mov %r4, BPF_ANY
> > +		 *			// call bpf_map_update_elem
> > +		 */
> > +		dt_ident_t	*state = dt_dlib_get_map(pcb->pcb_hdl, "state");
> > +
> > +		instr = BPF_STORE_IMM(BPF_W, BPF_REG_FP, DT_STK_SPILL(0), key);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		instr = BPF_CALL_HELPER(BPF_FUNC_get_smp_processor_id);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		instr = BPF_STORE(BPF_W, BPF_REG_FP, DT_STK_SPILL(1),
> > +				  BPF_REG_0);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		dt_cg_xsetx(dlp, state, DT_LBL_NONE, BPF_REG_1, state->di_id);
> > +		instr = BPF_MOV_REG(BPF_REG_2, BPF_REG_FP);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		instr = BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DT_STK_SPILL(0));
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		instr = BPF_MOV_REG(BPF_REG_3, BPF_REG_FP);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		instr = BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, DT_STK_SPILL(1));
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		instr = BPF_MOV_IMM(BPF_REG_4, BPF_ANY);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +		instr = BPF_CALL_HELPER(BPF_FUNC_map_update_elem);
> > +		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> > +	}
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel