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

[Kris Van Hees]

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.

> +	}
>  
>  	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