[DTrace-devel] [PATCH] Replace dt_memcpy() with bpf_probe_read()

[Kris Van Hees]

On Fri, Aug 20, 2021 at 01:12:59PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> Two nits below:
> 
> On 8/20/21 2:46 AM, Kris Van Hees wrote:
> > The dt_memcpy() functionality can be provided by the bpf_probe_read()
> > helper.  Thanks to Eugene Loh for pointing this out.
> >
> > Signed-off-by: Kris Van Hees<kris.van.hees at oracle.com>
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -725,17 +725,14 @@ dt_cg_fill_gap(dt_pcb_t *pcb, int gap)
> >   static void
> >   dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
> >   {
> > -	dt_ident_t	*idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_memcpy");
> > -
> > -	assert(idp != NULL);
> >   	if (dt_regset_xalloc_args(drp) == -1)
> >   		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> >   
> >   	emit(dlp,  BPF_MOV_REG(BPF_REG_1, dst));
> > -	emit(dlp,  BPF_MOV_REG(BPF_REG_2, src));
> > -	emit(dlp,  BPF_MOV_IMM(BPF_REG_3, size));
> > +	emit(dlp,  BPF_MOV_IMM(BPF_REG_2, size));
> > +	emit(dlp,  BPF_MOV_REG(BPF_REG_3, src));
> >   	dt_regset_xalloc(drp, BPF_REG_0);
> > -	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> > +	emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> >   	dt_regset_free_args(drp);
> >   	/* FIXME: check BPF_REG_0 for error? */
> >   	dt_regset_free(drp, BPF_REG_0);
> 
> Fine by me, but if you want to have consistent indentation on the emit() 
> statements, here is another such opportunity.

Ah yes, I missed that one.

> > @@ -875,19 +872,17 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   		 * output buffer at [%r9 + off].  The amount of bytes copied is
> >   		 * the lesser of the data size and the maximum string size.
> >   		 */
> > -		emit(dlp,   BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
> > -		emit(dlp,   BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> > -		emit(dlp,   BPF_MOV_REG(BPF_REG_2, dnp->dn_reg));
> > +		emit(dlp,  BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
> > +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> > +		emit(dlp,  BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> >   		dt_regset_free(drp, dnp->dn_reg);
> > -		emit(dlp,   BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
> > +		emit(dlp,  BPF_MOV_REG(BPF_REG_2, BPF_REG_0));
> >   		dt_regset_free(drp, BPF_REG_0);
> > -		emit(dlp,   BPF_BRANCH_IMM(BPF_JLT, BPF_REG_3, size, vcopy));
> > -		emit(dlp,   BPF_MOV_IMM(BPF_REG_3, size));
> > -		idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_memcpy");
> > -		assert(idp != NULL);
> > +		emit(dlp,  BPF_BRANCH_IMM(BPF_JLT, BPF_REG_2, size, vcopy));
> > +		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, size));
> >   		dt_regset_xalloc(drp, BPF_REG_0);
> > -		emitle(dlp, vcopy,
> > -			    BPF_CALL_FUNC(idp->di_id), idp);
> > +		emitl(dlp, vcopy,
> > +			   BPF_CALL_HELPER(BPF_FUNC_probe_read));
> >   		dt_regset_free_args(drp);
> >   		dt_regset_free(drp, BPF_REG_0);
> 
> Okay, but we normally load func args in order from r1 to r5.  In fact, 
> since you have to switch r2 and r3, you do switch the order at the other 
> call site.  So, maybe do so here as well?  It means jiggling that jump 
> label, but the placement of that jump label is now just a historical 
> vestige.

The reason why they are out of order is not so much necause of the original
code but because I wanted to free dnp->dn_reg earlier.  But it doesn't matter
so I'll switch to numeric arg order for this code.  It will not always be the
best choice to do in numeric order though, so one should not expect that to be
the case.  There are times where a different order yields better code (esp. if
we need to be mindful of how many registers we use).