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

[Eugene Loh]

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?

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?

> +		 *     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));
> +	}