[DTrace-devel] [PATCH] bpf: ensure speculations drops are reported when needed

[Eugene Loh]

Anyhow, while I don't understand what the compiler used to be doing with 
the code, this patch seems correct.  So:

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

although again a goto rather than break (as mentioned below) might be good.

On 5/30/23 16:07, Eugene Loh wrote:
> So simple, but I'm puzzled.  Yes, the old code was wrong, but wasn't 
> it unambiguously wrong, regardless of any (legal) compiler 
> optimization?  That is, if a D script emitted a bunch of 
> "speculation()" calls, it would use up all nspec speculations. The 
> next speculation() would march through the SEARCH() until it exceeded 
> NSPEC, and then it would return 0, without registering SPEC_UNAVAIL.  
> I don't see how "lucky" compiler optimization would "help" get around 
> this problem.  I guess I'm missing something.
>
> Also, instead of a "break", how about a goto to the return statement 
> to shave off the subsequent, fruitless searches.  Not a big deal, but 
> maybe cleaner, both for code execution and for reading/understanding 
> the code.
>
> On 5/30/23 14:25, Kris Van Hees via DTrace-devel wrote:
>> The unrolled loop in dt_speculation() would return without recording
>> a speculation drop if an iteration was attempted beyond the configured
>> NSPEC value.  Depending on how the compiler optimized the code, that
>> could lead to an early return from the function without ever calling
>> dt_no_spec().
>>
>> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>> ---
>>   bpf/speculation.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/bpf/speculation.c b/bpf/speculation.c
>> index 82d09e1a..7f4ebd34 100644
>> --- a/bpf/speculation.c
>> +++ b/bpf/speculation.c
>> @@ -54,7 +54,7 @@ noinline uint32_t dt_speculation(void)
>>   #define SEARCH(n)                            \
>>       do {                                \
>>           if (n > (uint64_t) &NSPEC)                \
>> -            return 0;                    \
>> +            break;                        \
>>           id = (n);                        \
>>           if (bpf_map_update_elem(&specs, &id, &zero,        \
>>               BPF_NOEXIST) == 0)                \