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

[Kris Van Hees]

On Fri, Jan 26, 2024 at 12:43:30PM -0500, Eugene Loh via DTrace-devel wrote:
> 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.

No, you are right.  I overlooked the 2nd use.  But in that case, yes I think
splitting this into a function makes sense, but since there is quite some
overlap between dt_cg_stack_nframes() and dt_cg_ustack_arg() when it comes to
the first ragument, doing this in a single function at least still makes sense.
But yes, it would still need to be a function because inlining it in two
places makes no sense.  Thanks for catching that.

The merging of those two functions is actually nicely supported by the fact
that the 'arg' for stack is equivalent to DTRACE_USTACK_ARG(nframes, 0) as
far as I ccan see.

> > > +	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);
> > > +	}
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel