Re: [PATCH bpf] bpf/selftests: fix test_tcpnotify_user

[Stanislav Fomichev <stfomichev@gmail.com>]

On 08/15, Matt Bobrowski wrote:
> Based on a bisect, it appears that commit 7ee988770326 ("timers:
> Implement the hierarchical pull model") has somehow inadvertently
> broken BPF selftest test_tcpnotify_user. The error that is being
> generated by this test is as follows:
> 
> 	FAILED: Wrong stats Expected 10 calls, got 8
> 
> It looks like the change allows timer functions to be run on CPUs
> different from the one they are armed on. The test had pinned itself
> to CPU 0, and in the past the retransmit attempts also occurred on CPU
> 0. The test had set the max_entries attribute for
> BPF_MAP_TYPE_PERF_EVENT_ARRAY to 2 and was calling
> bpf_perf_event_output() with BPF_F_CURRENT_CPU, so the entry was
> likely to be in range. With the change to allow timers to run on other
> CPUs, the current CPU tasked with performing the retransmit might be
> bumped and in turn fall out of range, as the event will be filtered
> out via __bpf_perf_event_output() using:
> 
>     if (unlikely(index >= array->map.max_entries))
>             return -E2BIG;

[..]

> A possible change would be to explicitly set the max_entries attribute
> for perf_event_map in test_tcpnotify_kern.c to a value that's at least
> as large as the number of CPUs. As it turns out however, if the field
> is left unset, then the BPF selftest library will determine the number
> of CPUs available on the underlying system and update the max_entries
> attribute accordingly.

nit: the max_entries is set by libbpf in map_set_def_max_entries. 'BPF
selftest library' seems a bit vague. But not a reason for respin.
 
> A further problem with the test is that it has a thread that continues
> running up until the program exits. The main thread cleans up some
> LIBBPF data structures, while the other thread continues to use them,
> which inevitably will trigger a SIGSEGV. This can be dealt with by
> telling the thread to run for as long as necessary and doing a
> pthread_join on it before exiting the program.
> 
> Finally, I don't think binding the process to CPU 0 is meaningful for
> this test any more, so get rid of that.
> 
> Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>