[DTrace-devel] [PATCH 5/5] Add -xcpu support for general providers
Eugene Loh
eugene.loh at oracle.com
Thu Dec 21 00:19:39 UTC 2023
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?
> 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
More information about the DTrace-devel
mailing list