[DTrace-devel] [PATCH v2 02/08] The exit() action must set the activity state to DRAINING

[Eugene Loh]

On 09/14/2020 12:50 PM, Kris Van Hees wrote:

> Now that activity state is supported, the exit() action needs to move
> the activity state to DRAINING to prevent further probe firings from
> being processed.
>
> The recording of the exit code is now done using the dt_cg_store_val()
> function since that is the preferred manner of storing values into the
> output buffer.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_cg.c | 54 ++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 44 insertions(+), 10 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index e94a605f..14e11f06 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -676,7 +676,9 @@ dt_cg_act_discard(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>   }
>   
>   /*
> - * Signal that tracing should be terminated with the given return code.
> + * Signal that tracing should be terminated with the given return code.  This
> + * action also forces the activity state to be set to DRAINING to signal that
> + * tracing is ending.
>    *
>    * Stores:
>    *	integer (4 bytes)		-- return code
> @@ -684,19 +686,51 @@ dt_cg_act_discard(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>   static void
>   dt_cg_act_exit(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>   {
> -	struct bpf_insn	instr;
>   	dt_irlist_t	*dlp = &pcb->pcb_ir;
> -	uint_t		off;
> -
> -	dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
> +	dt_ident_t	*state = dt_dlib_get_map(pcb->pcb_hdl, "state");
> +	struct bpf_insn	instr;
>   
> -	off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_EXIT,
> -			 sizeof(uint32_t), sizeof(uint32_t), NULL,
> -			 DT_ACT_EXIT);
> +	/* Record the exit code. */
> +	dt_cg_store_val(pcb, dnp->dn_args, DTRACEACT_EXIT, NULL, DT_ACT_EXIT);
>   
> -	instr = BPF_STORE(BPF_W, BPF_REG_9, off, dnp->dn_args->dn_reg);
> +	/*
> +	 * Force the activity state to DRAINING.
> +	 *
> +	 *	key = DT_STATE_ACTIVITY;// stw [%fp + DT_STK_SPILL(0)],
> +	 *				//		DT_STATE_ACTIVITY
> +	 *	val = DT_ACTIVITY_DRAINING;
> +	 *				// stw [%fp + DT_STK_SPILL(1)],
> +	 *				//		DT_ACTIVITY_DRAINING
> +	 *	rc = 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
> +	 *				//     (%r1 ... %r5 clobbered)
> +	 *				//     (%r0 = map value)

r0 is not a map value but a return status.  Anyhow, we just drop that 
value on the floor, so I would think the comment can be discarded entirely.

In a similar vein, I don't understand the value of the "clobbered" comment.

Meanwhile, something puzzles me and so let me ask a lazy question. 
Should we be allocating (and freeing) registers 0-5 before (and after) 
the helper call?  I don't trust myself to think about these things 
correctly and so would appreciate any comments.

> +	 */
> +	instr = BPF_STORE_IMM(BPF_W, BPF_REG_FP, DT_STK_SPILL(0),
> +			      DT_STATE_ACTIVITY);
> +	dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> +	instr = BPF_STORE_IMM(BPF_W, BPF_REG_FP, DT_STK_SPILL(1),
> +			      DT_ACTIVITY_DRAINING);
> +	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));
> -	dt_regset_free(pcb->pcb_regs, dnp->dn_args->dn_reg);
>   }
>   
>   static void