[DTrace-devel] [PATCH 7/9] Consolidate bpf_get_stack()-call code generation for stack and ustack()

[Eugene Loh]

On 1/24/24 01:59, Kris Van Hees wrote:

> On Thu, Oct 05, 2023 at 05:14:05PM -0400, eugene.loh at oracle.com wrote:
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> @@ -2225,33 +2225,90 @@ dt_cg_ustack_arg(dtrace_hdl_t *dtp, dt_node_t *dnp)
>> -static void
>> -dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>> +/*
>> + * Call the bpf_get_stack() helper function.
>> + *
>> + * dnp->dn_arg has optional, integer-constant sizing information.
>> + *
>> + * If reg>=0, reg is a register holding the output pointer.
>> + * If reg<0, we write the stack to the output buffer in %r9.
>> + *
>> + * These cases treat "off" differently:
>> + *   - reg>=0: "off" tracks how far the pointer in "reg" has
>> + *             been advanced, both at function entry and return
>> + *
>> + *   - reg<0: "off" is basically just a local variable since
>> + *            offsets with the %r9 buffer are managed by
>> + *            dt_rec_add()
>> + *
>> + * The "user" flag indicates whether a user (or kernel) stack is desired.
>> + */
>> +static int
>> +dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, int user)
>>   {
>>   	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
>>   	dt_irlist_t	*dlp = &pcb->pcb_ir;
>>   	dt_regset_t	*drp = pcb->pcb_regs;
>> -	int		nframes;
>> -	int		skip = 0;
>> -	uint_t		off;
>> +	uint64_t	arg;
>> +	int		nframes, stacksize, prefsz, align = sizeof(uint64_t);
>>   	uint_t		lbl_valid = dt_irlist_label(dlp);
>>   
>> -	nframes = dt_cg_stack_nframes(dtp, dnp);
>> +	/* Get sizing information from dnp->dn_arg. */
>> +	if (user) {
>> +		arg = dt_cg_ustack_arg(dtp, dnp);
>> +		nframes = DTRACE_USTACK_NFRAMES(arg);
>> +		prefsz = sizeof(uint64_t);
>> +	} else {
>> +		nframes = dt_cg_stack_nframes(dtp, dnp);
>> +		arg = nframes;
>> +		prefsz = 0;
>> +	}
> I think you might as well not have dt_cg_ustack_arg() and dt_cg_stack_nframes()
> and instead inline what they do here.  Especially since (modulo some constant
> values) there is duplication for the handling of the first argument anyway.
>
> In that sense, I don't think the [5/9] patch in this series is needed at all,
> because that one introduces those two functions.  But if we are not going to
> be using them anyway per my suggestion above, there is no need to introduce
> them.  And that also makes [6/9] (moving one of those functions) unnecessary.
>
> Besides, patch [5/9] mentions that it refactors dt_cg_act_stack() amd
> dt_cg_act_ustack() so that what they do can be invoked by multiple callers,
> but that is not really what that patch does.  The new functions it introduces
> are never called by more than a single caller, and in this patch [8/9] the
> actual implementation of stack and ustack is moved into a function that then
> can be called from multiple callers for real.
>
> So, I think you can just use this patch (with modifications) to provide a
> subroutine function that handles the real work of performing stack() and
> ustack(), and then the next patch in the series hooks it up to the arglist
> code to support using stack(), ustack(), etc as elements in a tuple.

Thanks for your comments on the @[stack()] patch series.

I've been trying to digest and incorporate your feedback, but at some 
point I get stuck.

I think the linchpin for the remaining issues are your comments here.  
Yes, the function(s) that determine arg/nframes are used by the 
integrated (stack and ustack) dt_cg_act_stack_sub(), and that one 
function is used both for the u/stack() actions and for arglist.

But that does not mean that the dt_cg_ustack_arg() and 
dt_cg_stack_nframes() functions are called from only one spot.  The 8/9 
patch adds a call site:  after the cg for the agg's arglist is finished, 
we then have to add size information for the agg.  At this point, we 
make the arg/nframes calls.  So, inlining the arg/nframes stuff would 
mean having to inline in two places:  once where you indicate and again 
at the new site/s introduced by the 8/9 patch.

But maybe I miss what you're getting at.

>> +	stacksize = nframes * sizeof(uint64_t);
>> +
>> +	/* Handle alignment and reserve space in the output buffer. */
>> +	if (reg >= 0) {
>> +		uint_t	nextoff;
>> +		nextoff = (off + (align - 1)) & ~(align - 1);
>> +		if (off < nextoff)
>> +			emit(dlp,  BPF_ALU64_IMM(BPF_ADD, reg, nextoff - off));
>> +		off = nextoff + prefsz + stacksize;
>> +	} else {
>> +		off = dt_rec_add(dtp, dt_cg_fill_gap,
>> +				 user ? DTRACEACT_USTACK : DTRACEACT_STACK,
>> +				 prefsz + stacksize, align, NULL, arg);
>> +	}
>>