[DTrace-devel] [PATCH 3/3] Alternative handling of tracemem's dynamic size.

[Kris Van Hees]

On Thu, Feb 23, 2023 at 10:28:43PM -0500, Kris Van Hees via DTrace-devel wrote:
> On Mon, Jan 30, 2023 at 04:36:16PM -0500, eugene.loh--- via DTrace-devel wrote:
> > From: Eugene Loh <eugene.loh at oracle.com>
> > 
> > The tracemem() action takes a constant size argument that gives the size
> > of the buffer used.
> > 
> > Optionally, it also takes another size argument that is dynamically
> > computed.  Currently, in this DTrace port, this dsize is used only by the
> > consumer, to limit how much data is displayed.  The documentation is
> > unclear if dsize should also limit how much data is written into the
> > buffer in the first place.  Arguably, it should:
> > 
> > *)  Copying less data might mean faster run time, although the
> >     savings would be very small and would incur the cost of extra
> >     BPF instructions.
> > 
> > *)  Copying less data might protect against accessing memory
> >     illegally.
> > 
> > Using dsize to cap how much data is written into the buffer is
> > implemented in this patch.
> > 
> > Since any changes in behavior would not be seen in our testing, no test
> > changes are made.
> > 
> > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > ---
> >  libdtrace/dt_cg.c | 29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 17843415..30c48155 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2028,9 +2028,34 @@ dt_cg_act_tracemem(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >  	off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_TRACEMEM, nsiz->dn_value, 1, NULL,
> >  	    dsiz ? DTRACE_TRACEMEM_DYNAMIC : DTRACE_TRACEMEM_STATIC);
> >  
> > -	if (dsiz != NULL)
> > -		dt_cg_store_val(pcb, dsiz, DTRACEACT_TRACEMEM, NULL,
> > +	if (dsiz != NULL) {
> > +		uint_t	off_dsiz;
> > +		size_t	wid_dsiz = ctf_type_size(dsiz->dn_ctfp, dsiz->dn_type);
> > +		uint_t	Lokay = dt_irlist_label(dlp);
> > +
> > +		assert(wid_dsiz > 0 && wid_dsiz <= 8 && (wid_dsiz & (wid_dsiz - 1)) == 0);
> > +
> > +		/* compute and record dsiz at the appropriate offset */
> > +		dt_cg_node(dsiz, dlp, drp);
> > +		off_dsiz = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_TRACEMEM, wid_dsiz, wid_dsiz, NULL,
> >  		    dsiz->dn_flags & DT_NF_SIGNED ? DTRACE_TRACEMEM_SSIZE : DTRACE_TRACEMEM_SIZE);
> > +		emit(dlp, BPF_STORE(ldstw[wid_dsiz], BPF_REG_9, off_dsiz, dsiz->dn_reg));
> > +
> > +		/*
> > +		 * Decide whether to reduce the size of the data that is
> > +		 * actually being recorded.  We do this only if the dynamic
> > +		 * size is non-negative and less than the nsiz.  Note that the
> > +		 * dsiz->dn_reg value has been sign-extended to 64 bits and we
> > +		 * will never have an nsiz that is 1<<63 or greater.  So, just
> > +		 * treat dsiz->dn_reg as an unsigned value.
> > +		 */
> > +		emit(dlp,  BPF_BRANCH_REG(BPF_JLE, nsiz->dn_reg, dsiz->dn_reg, Lokay));
> > +		emit(dlp,  BPF_MOV_REG(nsiz->dn_reg, dsiz->dn_reg));
> > +		emitl(dlp, Lokay,
> > +			   BPF_NOP());
> > +
> > +		dt_regset_free(drp, dsiz->dn_reg);
> 
> I think it would be much cleaner to do things in a different order:
> 
> 	1. compute dsiz (dt_cg_node)
> 	2. decide whether to reduce nsiz based on diz
> 	3. use dt_cg_store_val to store the dsiz in the output buffer
> 
> That avoids the manual operations you do to record dsiz in the buffer.

Hm, on second thought, the problem of course is that dt_cg_store_val() itself
does the dt_cg_node() so that would mean it gets evaluated twice, which can be
an issue.

Then again, we have a very tight code path here that we can (ab)use...  We
know for certain that dt_cg_store_val() freed dsiz->dn_reg right before
returning to dt_cg_act_tracemem(), and dsiz->dn_reg still has the register
number in it.  Therefore, we can dt_regset_xalloc(drp, dsiz->dn_reg) right
after the dt_cg_store_val() call to "recover" the register and continue using
it.

I modified the patch that way and it works very nicely.  It is slightly
naughty but avoid the manual storing of the dsiz scalar in the output buffer
(which is a bit ugly).

> > +	}
> >  
> >  	if (dt_regset_xalloc_args(drp) == -1)
> >  		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > -- 
> > 2.18.4
> > 
> > 
> > _______________________________________________
> > 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