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

[Eugene Loh]

On 09/02/2020 01:56 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.

I don't understand this patch.  In a previous patch, we told END to 
short circuit if the activity state is not ACTIVE.  So, if exit() 
changes the activity state to DRAINING, then END will not execute its 
clause.  That seems wrong.

Consider:

     # dtrace -n 'BEGIN {exit(0)} END{trace(1234)}'
     dtrace: description 'BEGIN ' matched 2 probes
CPU     ID                    FUNCTION:NAME

Okay, now what?  With DTv1:

       7      1                           :BEGIN
       7      2                             :END      1234

Both probes fire and report.  Good.  What about DTv2?  Well, with your 
new patch:

       1      1                           :BEGIN

while before your patch:

       1      1                           :BEGIN
       1      1                           :BEGIN

Grrr.  Very confusing.  Let me throw in a quick hack just so we can make 
sense of what's going on.  In dt_consume.c:

     @@ -2535,10 +2535,11 @@ dt_consume_cpu(dtrace_hdl_t *dtp, FILE *fp, 
int cpu, dt_peb_t *peb,
                  rval = dt_consume_one(dtp, fp, cpu, event, &pdat,
                                efunc, rfunc, flow, quiet, &last,
                                arg);
     +            tail += hdr->size;
     +            ring_buffer_write_tail(rb_page, tail);
     +
                  if (rval != DTRACE_WORKSTATUS_OKAY)
                      return rval;
     -
     -            tail += hdr->size;
              } while (tail != head);

              ring_buffer_write_tail(rb_page, tail);

That's just to clean up the reporting of the last probe.  Okay, so 
hacked, I get (with your patch):

       1      1                           :BEGIN

while before your patch:

       1      1                           :BEGIN
       1      2                             :END        1234

I can imagine your "Wow, that's a lot of writing just to say it's 
wrong."  Sorry, I'm trying to think these things out.  It seems to me 
your patch breaks things by exit() disabling the ERROR probe's clause.

As you say, patches should include tests.  I do not know what tests were 
done here.  The patch does not indicate any tests.

> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_cg.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 01e8707f..d289e02b 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -674,7 +674,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
> @@ -682,12 +684,54 @@ 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;
> +	dt_ident_t	*state = dt_dlib_get_map(pcb->pcb_hdl, "state");
> +	struct bpf_insn	instr;
>   	uint_t		off;
>   
> +	/* Evaluate the exit code expression. */
>   	dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
>   
> +	/*
> +	 * 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)
> +	 */
> +	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));
> +
> +	/* Record the exit code. */
>   	off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_EXIT,
>   			 sizeof(uint32_t), sizeof(uint32_t), NULL,
>   			 DT_ACT_EXIT);