[DTrace-devel] [PATCH v3] Add rodata support

[Eugene Loh]

On 8/17/23 22:04, Kris Van Hees wrote:

> On Thu, Aug 17, 2023 at 06:10:25PM -0400, Eugene Loh via DTrace-devel wrote:
>> On 8/17/23 10:53, Kris Van Hees via DTrace-devel wrote:
>>> @@ -711,13 +716,20 @@ gmap_create_strtab(dtrace_hdl_t *dtp)
>>>    	int		fd, rc, err;
>>>    	dtp->dt_strlen = dt_strtab_size(dtp->dt_ccstab);
>>> -	dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_strlen, 8);
>>> +	dtp->dt_rooffset = P2ROUNDUP(dtp->dt_strlen, 8);
>>> +	dtp->dt_rosize = dt_rodata_size(dtp->dt_rodata);
>>> +	dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_rooffset + dtp->dt_rosize, 8);
>>> +	dtp->dt_zerosize = 0;
>>> +	sz = dt_rodata_max_item_size(dtp->dt_rodata);
>> To me, it makes much more sense to put those last two lines after the next
>> comment.
> I see it more as first initializing these variables, and then we do some
> updating based on the conditions.

In particular, "sz" is simply a temporary variable that is used only for 
the convenience for that next block of code.  It has no use otherwise.  
It is local to that code block.

>> I'd make that second argument NULL.  (See my comment later about
>> dt_rodata_write().)
> I disagree (likewise, see below).
>
>>> diff --git a/libdtrace/dt_rodata.c b/libdtrace/dt_rodata.c
>>> @@ -0,0 +1,307 @@
>> I'd like a block comment like this near the top of this file:
> That seems really out of place.

Out of place because this file is so sparsely commented? Conceptually, 
all we're doing is concatenating data, but there is so much more than 
that going on.  Why ask new developers to reverse-engineer a file that 
is 100s of lines long?  I do not understand the objection to a comment 
block to explain what's going on in this file.

>>      The function dt_rodata_insert(dp, ptr, len) takes an input block of data,
>>      described by ptr and len, and appends it to a monolithic buffer described
>>      by dp.  The function returns the offset in the buffer where the data is
>>      stored.
>>
>>      Actually, dp also has a hash table so that if the data block already appears
>>      in the buffer, an offset to the pre-existing copy is returned and no new
>>      copy is added.
>>
>>      Further, dp actually stores the buffer in a growable set of smaller chunks,
>>      each chunk of size originally specified in dt_rodata_create(bufsz).
>>      The buffer can be written out to a single, linear block of memory using
>>      dt_rodata_write().
>>> +dt_rodata_t *
>>> +dt_rodata_create(size_t bufsz)
>>> +{
>>> +	dt_rodata_t	*dp = malloc(sizeof(dt_rodata_t));
>>> +	size_t		nbuckets = _dtrace_strbuckets;
>>> +
>>> +	assert(bufsz != 0);
>>> +
>>> +	if (dp == NULL)
>>> +		return NULL;
>>> +
>>> +	memset(dp, 0, sizeof(dt_rodata_t));
>>> +	dp->hash = malloc(nbuckets * sizeof(dt_rohash_t *));
>>> +	if (dp->hash == NULL)
>>> +		goto err;
>>> +
>>> +	memset(dp->hash, 0, nbuckets * sizeof(dt_rohash_t *));
>>> +	dp->hashsz = nbuckets;
>>> +	dp->bufs = NULL;
>>> +	dp->ptr = NULL;
>>> +	dp->nbufs = 0;
>>> +	dp->bufsz = bufsz;
>>> +
>>> +	if (dt_rodata_grow(dp) == -1)
>>> +		goto err;
>> Why not let the first dt_rodata_grow() be made by the first "insert"?
> Since we want a first NULL element.

Why?  And why not insert it with an insert call?

>>> +
>>> +	/* Pre-populate the read-only data with a 0-byte at offset 0.  */
>>> +	*dp->ptr++ = 0;
>>> +	dp->size = 1;
>>> +	dp->count = 1;
>> Why prepopulate?  And why not with an "insert" call?
> To guarantee a NULL element at offset 0.
>
>
>
>>> +ssize_t
>>> +dt_rodata_index(dt_rodata_t *dp, const char *ptr, size_t len)
>>> +{
>>> +	ssize_t		rc;
>>> +	uint32_t	h;
>>> +
>>> +	if (ptr == NULL || ptr[0] == '\0')
>>> +		return 0;	/* NULL or a single 0-byte are at offset 0. */
>> Why the comment and the ptr[0]=='\0' check?
> Because we have a prepopulated entry at offset 0 to represent a NULL or empty
> memory block (considered to be the same as NULL).

I'm still not getting it.  Why are we hardwiring this case?  And why are 
we treating all blocks of data whose first byte is 0 all the same?  
E.g., let's say we have a strtab and its first byte just happens to be 
0.  Now we try to insert this strtab into the rodata area.  We call 
insert.  The insert function checks the first byte and sees it's 0.  So, 
the entire strtab is dropped on the floor. And then when I try to use 
strtab, I get garbage.  Or what am I missing?

>>> +
>>> +	h = dt_gen_hval(ptr, len, len) % dp->hashsz;
>>> +	rc = dt_rodata_xindex(dp, ptr, len, h);
>>> +
>>> +	return rc;
>>> +}
>>> +
>>> +ssize_t
>>> +dt_rodata_insert(dt_rodata_t *dp, const char *ptr, size_t len)
>>> +{
>>> +	dt_rohash_t	*hp;
>>> +	ssize_t		off;
>>> +	uint32_t	h;
>>> +
>>> +	if (ptr == NULL || ptr[0] == '\0')
>>> +		return 0;	/* NULL or a single 0-byte are at offset 0. */
>>> +
>> Why the comment and the ptr[0]=='\0' check?
> See above.
>
>>> +	h = dt_gen_hval(ptr, len, len) % dp->hashsz;
>>> +	off = dt_rodata_xindex(dp, ptr, len, h);
>>> +	if (off != -1)
>>> +		return off;
>>> +
>>> +	/*
>>> +	 * Create a new hash bucket, initialize it, and insert it at the front
>>> +	 * of the hash chain for the appropriate bucket.
>>> +	 */
>>> +	if ((hp = malloc(sizeof(dt_rohash_t))) == NULL)
>>> +		return -1L;
>>> +
>>> +	hp->data = dp->ptr;
>>> +	hp->buf = dp->nbufs - 1;
>>> +	hp->off = dp->size;
>>> +	hp->len = len;
>>> +	hp->next = dp->hash[h];
>>> +
>>> +	/*
>>> +	 * Now copy the data into our buffer list, and then update the global
>>> +	 * counts of items and bytes.  Return the item's byte offset.
>>> +	 */
>>> +	if (dt_rodata_copyin(dp, ptr, len) == -1) {
>>> +		free(hp);
>>> +		return -1L;
>>> +	}
>>> +
>>> +	dp->count++;
>>> +	dp->size += len;
>>> +	dp->hash[h] = hp;
>>> +	if (dp->item_size < len)
>>> +		dp->item_size = len;
>>> +
>>> +	return hp->off;
>>> +}
>>> +
>>> +/*
>>> + * Copy data from the compile-time read-only data into a DIFO-style read-only
>> What do "compile-time" and "DIFO-style" mean?
> Explaining that is beyond the scope of this source code file.  That is matter
> for the overall internals of DTrace.

All right.  So a different question:  why are "compile-time" and 
"DIFO-style" being mentioned at all?  What do they have to do with 
anything here?  There is some linear buffer that's being stored in some 
funny way peculiar to this file and we're going to write it out as one, 
long, serial buffer... the sort of thing the rest of the world expects 
when handed a ptr and a len.  Why should anyone care about 
"compile-time" or "DIFO-style"?

>>> + * storage memory block.
>>> + */
>>> +ssize_t
>>> +dt_rodata_copy(const char *s, size_t n, size_t off, char *buf)
>> Why not just inline this function?  There is no other copy function to use.
> But we can support multiple ones, and in fact, when strtab is implemented based
> on rodata, we can use a single writer function and supply it with a custom
> copy function (distinct from what rodata needs).

What would that function look like?  Today, dt_strtab_copystr() is 
simply a memcpy.

>>> +{
>>> +	memcpy(buf + off, s, n);
>>> +	return n;
>>> +}
>>> +
>>> +ssize_t
>>> +dt_rodata_write(const dt_rodata_t *dp, dt_rodata_copy_f *func, void *private)
>> Why make the caller specify func?  In particular, the only func used is
>> dt_rodata_copy(), which is in this file but the callers are outside of this
>> file.  At the very least, insert "if (func == NULL) func = dt_rodata_copy"
>> so that a caller does not HAVE to specify func.
> See above.  I don't like the concept of a default function because that leads
> to needing to go look at the writer function to figure out what the default is.
> There is no real benefit from using a default.

Maybe it would help if I knew what other functions were being considered.

Right now, my understanding is that rodata is some big linear space 
where I can dump blocks of data.  I'm given the linear offset to each 
block.  There is protection against duplicates.  The "write" function 
presumably dumps that linear space to somewhere designated by a 
pointer.  Why should I need to specify anything other than that 
pointer?  Why should I have to go looking at the writer function to 
figure out what the func argument is supposed to be?

> +{
> +	ssize_t	res, total = 0;
> +	size_t	i;
> +	size_t	n;
> +
> +	for (i = 0; i < dp->nbufs; i++, total += res) {
> +		if (i == dp->nbufs - 1)
> +			n = dp->ptr - dp->bufs[i];
> +		else
> +			n = dp->bufsz;
> +
> +		if ((res = func(dp->bufs[i], n, total, private)) <= 0)
> +			break;
> +	}
> +
> +	if (total == 0 && dp->size != 0)
> +		return -1;
>> Why not just (total != dp->size)?
> Consistency with the strtab code.

I thought that was deprecated code.  Why would we want consistency with 
deprecated code?