[DTrace-devel] Reviewed: Do not use BPF_REG_0 in dt_cg_agg_buf_prepare()

[Eugene Loh]

I'm confused...

On 11/20/2020 02:15 PM, kris.van.hees at oracle.com wrote:

> [Commit 8a11eab0d47aee9d48a55506113801af4f59c3fb]
>> Do not use BPF_REG_0 in dt_cg_agg_buf_prepare()
>>
>> The function dt_cg_agg_buf_prepare() uses BPF_REG_0.  Perhaps this is
>> all right, but it is not ideal.  The specific register is hardcoded in,
>> and we do not check locally if the register is in use.
> It is correct to use %r0 and I do not understand why you think it is not ideal.

Not ideal because it involves reasoning about correctness that someone 
like me could get wrong.  In contrast, one can just ask regset_alloc() 
for a free register and be safe.

> Since it is the de facto return value register it is actually a good choice for
> scratch register when a temporary value is needed.  It should not be used when
> the code generation calls dt_cg_node() of course, since that may cause it to be
> (re)used incorrectly.  That's why we should always allocate it before we use it
> (and I still need to add that in some places, just to be consistent).

Well, since one is allocating a register, might as well ask for a free 
register rather than for a particular register.

> But, as you show in the code below, we actually do not need to use %r0 here,
> and I agree there is no need to use more registers than needed.
>
>> Further, the comments suggest that the sequence latch should be
>> incremented before computing the pointer.
> Actually, no.

"No" as in "No that's not what supposed to happen" or as in "No that's 
not what the comments say"?  The comments for the function seem to say 
"update the latch, then determine the offset."  As for what's *supposed* 
to happen, it would seem like that's kind of arbitrary in that the 
consumer is not yet written.  I would think that, given a latch number, 
the consumer is equally happy using either latch&1 or the other one.  
Again, kind of arbitrary.

> The sequence latch is incremented to signal the consuemr that
> it should now read from the 'other' copy.  But the pointer is to be computed
> based on the old value because the pointer indicates the copy to which we are
> going to apply the update.  So:
> 	new value -> where the consumer should read from
> 	old value -> where we will apply the update

I'm confused.  The producer is running behind the consumer?  Or maybe 
the use of "new" and "old" is confusing?

> In other words, while we are updating copy A, the consumer should be directed
> to copy B (and vice versa).

Okay, that part is certainly clear.  But that leaves open multiple ways 
of doing this.

>> Make minor changes so that the code agrees with the comments and so
>> that BPF_REG_0 is not explicitly needed.
>>
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>>   libdtrace/dt_cg.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index fea08138..7d0580a4 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -2925,26 +2925,26 @@ dt_cg_agg_buf_prepare(dt_ident_t *aid, int size, dt_irlist_t *dlp,
>>   	assert(rx != -1 && ry != -1);
>>   
>>   	/*
>> -	 *				//     (%rX = register for agg ptr)
>> -	 *				//     (%rY = register for agg data)
>> -	 *	agd = dctx->agg;
>> +	 *	agd = dctx->agg;	//     (%rX = register for agg ptr)
>>   	 */
> This comment block should probably be:
>
> 	/*
> 	 *	uint64_t off;		//     (%rY = data offset)
> 	 *	char *agd;		//     (%rX = agg data ptr)
> 	 *
> 	 *	agd = dctx->agg;	// lddw %rY, [%fp + DT_STK_DCTX]
> 	 *				// lddw %rX, [%rY + DCTX_AGG]
> 	 */
>
>> -	BPFCODE(BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_FP, DT_STK_DCTX))
>> -	BPFCODE(BPF_LOAD(BPF_DW, rx, BPF_REG_0, DCTX_AGG))
>> +	BPFCODE(BPF_LOAD(BPF_DW, ry, BPF_REG_FP, DT_STK_DCTX))
>> +	BPFCODE(BPF_LOAD(BPF_DW, rx, ry, DCTX_AGG))
> OK.
>
> As explained above, the rest of the patch is not to be included.
>
>>   
>>   	/*
>> -	 *	off = (*agd % 2) * size
>> -	 *	      + aid->di_offset
>> -	 *	      + sizeof(uint64_t);
>>   	 *	(*agd)++;
>> +	 */
>> +	BPFCODE(BPF_MOV_IMM(ry, 1))
>> +	BPFCODE(BPF_XADD_REG(BPF_DW, rx, 0, ry))
>> +
>> +	/*
>> +	 *	off = (*agd & 1) * size
>> +	 *	      + aid->di_offset + sizeof(uint64_t);
>>   	 *	agd += off;
>>   	 */
>>   	BPFCODE(BPF_LOAD(BPF_DW, ry, rx, 0))
>>   	BPFCODE(BPF_ALU64_IMM(BPF_AND, ry, 1))
>>   	BPFCODE(BPF_ALU64_IMM(BPF_MUL, ry, size))
>>   	BPFCODE(BPF_ALU64_IMM(BPF_ADD, ry, aid->di_offset + sizeof(uint64_t)))
>> -	BPFCODE(BPF_MOV_IMM(BPF_REG_0, 1))
>> -	BPFCODE(BPF_XADD_REG(BPF_DW, rx, 0, BPF_REG_0))
>>   	BPFCODE(BPF_ALU64_REG(BPF_ADD, rx, ry))
>>   
>>   	dt_regset_free(drp, ry);
>> -- 
>> 2.28.0
>>