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

[Eugene Loh]

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)				\