[DTrace-devel] [PATCH v2 05/11] speculations: use a destructor rather than hand-freeing

[Nick Alcock]

On 22 Oct 2021, Eugene Loh spake thusly:

> Another unconfident:
>     Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> And a few comments/questions...

I'll do another round of the whole lot, I think :)

> On 10/20/21 2:53 PM, Nick Alcock wrote:
>> @@ -1980,7 +1997,7 @@ dt_print_trace(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
>>    *  - Speculations drain from the perf ring buffer into dt_spec_buf_datas, one
>>    *    dt_spec_buf_data per ring-buffer of data fetched from the kernel with a
>>    *    nonzero specid (one speculated clause); these are chained into
>> - *    dt_spec_bufs, one per speculation ID, and these are chained into
>> + *    dt_spec_bufs, one per speculation ID, and these are put into
>>    *    dtp->dt_spec_bufs instead of being consumed.
>
> I'm having trouble digesting this comment block.  There is no 
> "dt_spec_buf_datas."  We do not chain into dt_spec_bufs.  Or by 
> dt_spec_bufs do you mean some dtsb_dsbd_list?

Maybe '... into dt_spec_buf_data_ts' would be clearer, to indicate that
this is an English plural of a type name. Ugh that looks awful too, and
dt_spec_buf_data_t's looks like a misapostrophe to me.

... rephrased a bit to avoid the confusing plural.

>> +/*
>> + * Remove a speculation's buffers, and the speculation itself, and the record of
>> + * it in the BPF specs map (which frees the ID for reuse).
>
> Remove first "and"?

... that does read better.

>> + *
>> + * Note: if the speculation is being committed, it will also be interned on the
>> + * dtp->dt_spec_bufs_draining list.  Such entries must be removed first.
>> + */
>
>
> Does discussion of dt_spec_bufs_draining really belong here?

It's giving a precondition for calling dt_spec_buf_destroy, and thus for
deleting elements from the dt_spec_bufs list. I'd say we definitely need
it somewhere like this, given that a bit later we were doing:

	dt_htab_delete(dtp->dt_spec_bufs, dtsb);
	dt_list_delete(&dtp->dt_spec_bufs_draining, dtsd);
	dt_free(dtp, dtsd);

which is, uh, the wrong way round! (Fixed. Harmless but still there was
a pointer to freed memory there for a bit.)

>> +		dt_bpf_map_delete(idp->di_id, &dtsb->dtsb_id);
>> +	dt_free(dtp, dtsb);
>>   }
>> -
>>   /*
>>    * Peeking flags (values for the peekflag parameter for functions that have
>>    * one).
>
> Restore that blank line after dt_spec_buf_destroy()'s closing brace.

How did that get deleted? I swear sometimes Emacs has a mind of its own.

Actually make that 'always'.

-- 
NULL && (void)