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

[Kris Van Hees]

On Thu, Sep 17, 2020 at 12:48:27PM -0700, Eugene Loh wrote:
> 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.

Copy-and-paste error - thanks for noticing.  I'm keeping the comment because
because of ... (see below)
> 
> In a similar vein, I don't understand the value of the "clobbered" comment.

It simply states that %r1 through %r5 are clobbered due to the helper call.

> 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.

Yes indeed, we should.  I shamelessly copied this code from the trampoline
where I don't (yet) because that is code that doesn't call dynamic code
generation functions - i.e. we have total control over the regs usage.

I'll add the alloc/free of call regs.

A future patch will add alloc/free for regs in the prologue/epilogue code
generation also, for consistemcy sake.

> > +	 */
> > +	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
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel