[DTrace-devel] [PATCH 5/5] Add -xcpu support for general providers
Kris Van Hees
kris.van.hees at oracle.com
Thu Dec 21 01:03:54 UTC 2023
On Wed, Dec 20, 2023 at 07:19:39PM -0500, Eugene Loh via DTrace-devel wrote:
> On 12/20/23 18:34, Kris Van Hees wrote:
>
> > 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.
>
> Sorry, but how is this different from the proposed patch outside of using
> _cpu instead of _sub? If I changed the patch to rename _act to _cpu instead
> of to _sub, would that implement your proposal?
No because you changed providers to use the *_act() version (though you called
it *_sub()) which includes the activity state argument which it should not.
That should stay out of providers (except for the dtrace provider).
> > Only the dtrace provider was ever meant to mess with activity state itself.
>
> Yeah, I suspect that's why I changed the name from _act to _sub. I'm not
> saying that _sub is a good name, but only that the function _act suddenly
> became less about activity than about something else, even though the
> function itself did not change.
>
> > > > 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
>
> _______________________________________________
> 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