[DTrace-devel] [PATCH] The exit() action must set the activity state to DRAINING
Kris Van Hees
kris.van.hees at oracle.com
Fri Sep 4 23:45:29 PDT 2020
On Fri, Sep 04, 2020 at 09:43:07PM -0700, Eugene Loh wrote:
> 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.
And yet, it is the right thing to do. The trouble is that it won't show it
is the right thing until some other patches go in that further modify the
handling of BEGIN, END, exit, and the buffer consumption.
Yes, this patch causes regression, but since it will not be applied without
the upcoming companion patches (bundled as a nice patch-set) it won't matter.
It cannot be avoided.
> 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);
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list