On Wed, Dec 20, 2023 at 04:49:02PM -0500, Eugene Loh via DTrace-devel wrote:
> On 12/20/23 12:09, Kris Van Hees wrote:
>
> > On Tue, Sep 05, 2023 at 12:11:42AM -0400, eugene.loh at oracle.com wrote:
> > > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > I do not understand this patch, or rather why you rename
> > dt_cg_tramp_prologue_act
> > to be
> > dt_cg_tramp_prologue_sub
> > yet the implementation does not change?
>
> Yeah, good question. I no longer remember. Possibly I did something else
> to motivate the name change and then changed my mind and forgot to get rid
> of the name change. Okay: back to _act.
>
> > In all, I think it might be better to add a 2nd argument to
> > dt_cg_tramp_prologue() to indicate whether CPU filtering should be done (and
> > those who do it by means of how probes are attched can disable that). Maybe
> > an argument named 'no_cpu' to be 1 to skip CPU filtering if -xcpu is set.
>
>
> I don't understand in what sense this is better.
Yes, I don't like the argument approach either, which is why I suggested the
following...
> > Or alterntively, add a new dt_cg_tramp_prologue_cpu(dt_pcb_t *pcb) that simply
> > does dt_cg_tramp_prologue_act(pcb, DT_ACTIVITY_ACTIVE);
>
> I definitely do not understand this. This sounds like the name change that
> puzzled you except that _cpu is an even worse name than _sub. You must mean
> something different from what I'm reading here.
It is not a name change as such but rather introducing a new main function that
providers can use:
dt_cg_tramp_prologue()
which will check whether the cpu option was set and if so, generate
code to filter out probe firings unless they are on the specified CPU
dt_cg_tramp_prologue_cpu()
which is to be used by providers that do their own handling of the
cpu option, and this therefore does not generate code to filter probe
firings
Then logically, it makes sense that dt_cg_tramp_prologue() calls the new
dt_cg_tramp_prologue_cpu() because it has just generated code to do the cpu
filtering (if needed) and therefore it is now correct to use the trampoline
generation code for the 'already-dealt-with-cpu' case. And that one then uses
the underlying dt_cg_tramp_prologue_act() that deals with the activity state.
Only the dtrace provider was ever meant to mess with activity state itself.
> and then change the
> > dt_cg_tramp_prologue() function to generate the CPU filtering if -xcpu is set,
> > and then call dt_cg_tramp_prologue_cpu(). That was, as much as of this as can
> > be done remains hidden from the provider code.
>
> I do not see how having to choose between no_cpu = 0 and 1 hides any more
> than having to choose between trampoline and trampoline_act.
>
> I must be missing what you're trying to say.
>
> > > diff --git a/CODING-STYLE b/CODING-STYLE
> > > index c8013f05..f6382c65 100644
> > > --- a/CODING-STYLE
> > > +++ b/CODING-STYLE
> > > @@ -26,7 +26,7 @@ E.g.,
> > > uint_t
> > > dt_cg_tramp_prologue(dt_pcb_t *pcb)
> > > {
> > > - return dt_cg_tramp_prologue_act(pcb, DT_ACTIVITY_ACTIVE);
> > > + return dt_cg_tramp_prologue_sub(pcb, DT_ACTIVITY_ACTIVE);
> > > }
> > > In an "if...else if...else" statement, omit braces for the final branch if
> > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > index 0ed662f9..a79b8352 100644
> > > --- a/libdtrace/dt_cg.c
> > > +++ b/libdtrace/dt_cg.c
> > > @@ -49,7 +49,7 @@ static void dt_cg_node(dt_node_t *, dt_irlist_t *, dt_regset_t *);
> > > * %r9 contains a pointer to dctx
> > > */
> > > void
> > > -dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > > +dt_cg_tramp_prologue_sub(dt_pcb_t *pcb, dt_activity_t act)
> > > {
> > > dtrace_hdl_t *dtp = pcb->pcb_hdl;
> > > dt_irlist_t *dlp = &pcb->pcb_ir;
> > > @@ -274,7 +274,24 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > > void
> > > dt_cg_tramp_prologue(dt_pcb_t *pcb)
> > > {
> > > - dt_cg_tramp_prologue_act(pcb, DT_ACTIVITY_ACTIVE);
> > > + dtrace_hdl_t *dtp = pcb->pcb_hdl;
> > > +
> > > + /* Check if we are on the specified CPU (if any). */
> > > + if (dtp->dt_options[DTRACEOPT_CPU] != DTRACEOPT_UNSET) {
> > > + dt_irlist_t *dlp = &pcb->pcb_ir;
> > > +
> > > + emit(dlp, BPF_MOV_REG(BPF_REG_8, BPF_REG_1));
> > > +
> > > + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_smp_processor_id));
> > > + emit(dlp, BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0,
> > > + dtp->dt_options[DTRACEOPT_CPU],
> > > + pcb->pcb_exitlbl));
> > > +
> > > + emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_8));
> > > + }
> > > +
> > > + /* Call the rest of the prologue code generation. */
> > > + dt_cg_tramp_prologue_sub(pcb, DT_ACTIVITY_ACTIVE);
> > > }
> > > /*
> > > @@ -421,8 +438,8 @@ dt_cg_tramp_copy_args_from_regs(dt_pcb_t *pcb, int called)
> > > * So put the PC in both arg0 and arg1, test the PC, and then zero out
> > > * either arg0 or arg1, as apropriate.
> > > *
> > > - * The caller must ensure that %r7 and %r8 contain the values set by
> > > - * the dt_cg_tramp_prologue*() functions.
> > > + * The caller must ensure that %r7 and %r8 contain the values set by the
> > > + * dt_cg_tramp_prologue*() functions.
> > > */
> > > void
> > > dt_cg_tramp_copy_pc_from_regs(dt_pcb_t *pcb)
> > > diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
> > > index 0ced6dd2..1a40b2de 100644
> > > --- a/libdtrace/dt_cg.h
> > > +++ b/libdtrace/dt_cg.h
> > > @@ -20,7 +20,7 @@ extern "C" {
> > > extern void dt_cg(dt_pcb_t *, dt_node_t *);
> > > extern void dt_cg_xsetx(dt_irlist_t *, dt_ident_t *, uint_t, int, uint64_t);
> > > extern dt_irnode_t *dt_cg_node_alloc(uint_t, struct bpf_insn);
> > > -extern void dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act);
> > > +extern void dt_cg_tramp_prologue_sub(dt_pcb_t *pcb, dt_activity_t act);
> > > extern void dt_cg_tramp_prologue(dt_pcb_t *pcb);
> > > extern void dt_cg_tramp_clear_regs(dt_pcb_t *pcb);
> > > extern void dt_cg_tramp_copy_regs(dt_pcb_t *pcb);
> > > diff --git a/libdtrace/dt_prov_cpc.c b/libdtrace/dt_prov_cpc.c
> > > index 9e0f5542..706d729a 100644
> > > --- a/libdtrace/dt_prov_cpc.c
> > > +++ b/libdtrace/dt_prov_cpc.c
> > > @@ -395,7 +395,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > int i;
> > > dt_irlist_t *dlp = &pcb->pcb_ir;
> > > - dt_cg_tramp_prologue(pcb);
> > > + dt_cg_tramp_prologue_sub(pcb, DT_ACTIVITY_ACTIVE);
> > > /*
> > > * After the dt_cg_tramp_prologue() call, we have:
> > > diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
> > > index a76534f8..277edae2 100644
> > > --- a/libdtrace/dt_prov_dtrace.c
> > > +++ b/libdtrace/dt_prov_dtrace.c
> > > @@ -119,10 +119,10 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > key = DT_STATE_ENDEDON;
> > > }
> > > - dt_cg_tramp_prologue_act(pcb, act);
> > > + dt_cg_tramp_prologue_sub(pcb, act);
> > > /*
> > > - * After the dt_cg_tramp_prologue_act() call, we have:
> > > + * After the dt_cg_tramp_prologue_sub() call, we have:
> > > * // (%r7 = dctx->mst)
> > > * // (%r8 = dctx->ctx)
> > > */
> > > diff --git a/libdtrace/dt_prov_profile.c b/libdtrace/dt_prov_profile.c
> > > index f3f3bf23..08216b08 100644
> > > --- a/libdtrace/dt_prov_profile.c
> > > +++ b/libdtrace/dt_prov_profile.c
> > > @@ -220,7 +220,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > > int i;
> > > dt_irlist_t *dlp = &pcb->pcb_ir;
> > > - dt_cg_tramp_prologue(pcb);
> > > + dt_cg_tramp_prologue_sub(pcb, DT_ACTIVITY_ACTIVE);
> > > /*
> > > * After the dt_cg_tramp_prologue() call, we have:
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel