[DTrace-devel] [PATCH] Convert dvars to use the default block of zeros

[Eugene Loh]

On 8/4/22 19:10, Kris Van Hees wrote:

> On Thu, Aug 04, 2022 at 12:41:40PM -0700, Eugene Loh wrote:
>> On 8/4/22 12:25, Kris Van Hees wrote:
>>> On Thu, Aug 04, 2022 at 10:03:13AM -0700, Eugene Loh via DTrace-devel wrote:
>>>> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>>>> I'm getting confused about the order of patches, but in dt_bpf.c I thought
>>>> there was a block of code that computed dt_zerosize, bumping it up depending
>>>> on dt_maxtuplesize, dt_aggsiz, etc.  That would seem to be the right place
>>>> to bump it up per strsize + 1 and dt_maxdvarsize as well.  As it is, the
>>>> current patch seems to compute dt_zerosize based only on dt_maxdvarsize and
>>>> strsize+1, ignoring any previous information about dt_zerosize (which would
>>>> have incorporated info on dt_maxtuplesize and dt_aggsiz).
>>> No, the patch that introduces the block of zeros never assigns a value to
>>> zerosize (not in your patch, and not in my version).  Since this patch is the
>>> first use of that block, it introduces determining the value of zerosize.
>>>
>>> Later uses (in your patches) will need to add further logic to determine the
>>> size needed.
>> Okay, but if this patch is going to go in before the others, how about
>> putting the dt_zerosize= statement earlier by itself.  For this particular
>> patch, maybe it's okay to insert that one-liner into the middle of some
>> other dense code, but since we already know how this code will evolve we
>> might as well put the dt_zerosize= line earlier, by itself, so that the code
>> can be patched more cleanly. It is often hard to predict how code will
>> evolve.  In this case, we already can look several patches ahead.
> It doesn't really matter, does it?  This code will indeed evolve, in that
> future patches will modify the way zerosize is determined, so this code
> will eventually become something like:
>
> 	dtp->dt_strlen = dt_strtab_size(dtp->dt_ccstab);
> 	dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_strlen, 8);
> 	dtp->dt_zerosize = MAX(dtp->dt_maxdvarsize, strsize + 1);
> 	if (dtp->dt_zerosize < dtp->dt_aggsiz)
> 		dtp->dt_zerosize = dtp->dt_aggsiz;
> 	if (dtp->dt_zerosize < dtp->dt_maxtuplesize)
> 		dtp->dt_zerosize = dtp->dt_maxtuplesize;
>
> 	sz = dtp->dt_zerooffset + dtp->dt_zerosize;
> 	strtab = dt_zalloc(dtp, sz);
>
> I do not see where that is a problem.

It's just subjective.  As the zerosize computation grows more 
complicated, it's nice (in my opinion) to give it its own block of code, 
versus having one line with zerooffset, then six lines of zerosize, then 
back to zerooffset.  Again, it's subjective.

>
>>>> On 8/4/22 06:55, Kris Van Hees via DTrace-devel wrote:
>>>>> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
>>>>> @@ -431,7 +431,8 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>>>>>     	 */
>>>>>     	dtp->dt_strlen = dt_strtab_size(dtp->dt_ccstab);
>>>>>     	dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_strlen, 8);
>>>>> -	sz = dtp->dt_zerooffset + MAX(strsize + 1, dtp->dt_zerosize);
>>>>> +	dtp->dt_zerosize = MAX(dtp->dt_maxdvarsize, strsize + 1);
>>>>> +	sz = dtp->dt_zerooffset + dtp->dt_zerosize;
>>>>>     	strtab = dt_zalloc(dtp, sz);
>>>>>     	if (strtab == NULL)
>>>>>     		return dt_set_errno(dtp, EDT_NOMEM);