Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 8/2/24 11:58, Martin KaFai Lau wrote:
> On 8/1/24 9:31 PM, Kui-Feng Lee wrote:
>>
>>
>> On 8/1/24 20:29, Martin KaFai Lau wrote:
>>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>>> Add functions that capture packets and print log in the background. 
>>>> They
>>>> are supposed to be used for debugging flaky network test cases. A 
>>>> monitored
>>>> test case should call traffic_monitor_start() to start a thread to 
>>>> capture
>>>> packets in the background for a given namespace and call
>>>> traffic_monitor_stop() to stop capturing. (Or, option '-m' 
>>>> implemented by
>>>> the later patches.)
>>>>
>>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 68, 
>>>> ifindex 1, SYN
>>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 60, 
>>>> ifindex 1, SYN, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 60, 
>>>> ifindex 1, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, 
>>>> ifindex 1, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:48165 -> 127.0.0.1:36707, len 52, 
>>>> ifindex 1, FIN, ACK
>>>>      IPv4 TCP packet: 127.0.0.1:36707 -> 127.0.0.1:48165, len 52, 
>>>> ifindex 1, RST, ACK
>>>
>>> nit. Instead of ifindex, it should be ifname now.
>>
>> Sure! I will update it.
>>
>>>
>>>>      Packet file: packets-2172-86-select_reuseport:sockhash-test.log
>>>>      #280/87 select_reuseport/sockhash IPv4/TCP LOOPBACK 
>>>> test_detach_bpf:OK
>>>>
>>>> The above is the output of an example. It shows the packets of a 
>>>> connection
>>>> and the name of the file that contains captured packets in the 
>>>> directory
>>>> /tmp/tmon_pcap. The file can be loaded by tcpdump or wireshark.
>>>>
>>>> This feature only works if TRAFFIC_MONITOR variable has been passed to
>>>> build BPF selftests. For example,
>>>>
>>>>    make TRAFFIC_MONITOR=1 -C tools/testing/selftests/bpf
>>>>
>>>> This command will build BPF selftests with this feature enabled.
>>>>
>>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>>> ---
>>>>   tools/testing/selftests/bpf/Makefile     |   5 +
>>>>   tools/testing/selftests/bpf/test_progs.c | 432 
>>>> +++++++++++++++++++++++
>>>
>>> In the cover letter, it mentioned the traffic monitoring 
>>> implementation is moved from the network_helpers.c to test_progs.c.
>>>
>>> Can you share more about the reason?
>>
>> network_helpers.c has been used by several test programs.
>> However, they don't have env that we found in test_progs.c.
>> That means we could not access env directly. Instead, the caller
>> have to pass the test name and subtest name to the function.
>> Leter, we also need to check if a test name matches the patterns. It is
>> inconvient for users. So, I move these functions to test_progs.c to make
>> user's life eaiser.
>>
>>
>>>
>>> Is it because the traffic monitor now depends on the test_progs's 
>>> test name, should_tmon...etc ? Can the test name and should_tmon be 
>>> exported for the network_helpers to use?
>>
>> Yes! And in later patches, we also introduce a list of patterns.
> 
> The list of patterns matching is summarized in "should_tmon" which can 
> be exported through a function?

Yes! Even with a functio, it still depends on test_progs.c.

> 
> or I have missed another criteria when deciding tmon should be enabled 
> for a test?
> 
>>>
>>> What other compilation issues did it hit if the traffic monitor codes 
>>> stay in the network_helpers.c? Some individual binaries (with main()) 
>>> like test_tcp_check_syncookie_user that links to network_helpers.o 
>>> but not to test_progs.o?
>>
>> Yes, they are problems as well. These binary also need to link to
>> libpcap even they don't use it although this is not an important issue.
> 
> I don't think linking the non test_progs binaries to libpcap or not is 
> important.
> 
> I am positive there are ways out of it without adding the networking 
> codes to the test_progs.c. It sounds like an unnecessary nit now but I 
> believe it is useful going forward when making changes and extension to 
> the traffic monitoring. May be brainstorm a little to see if there is an 
> way out.
> 
> One way could be putting them in a new traffic_monitor.c such that the 
> non test_progs binaries won't link to it. and exports the test name and 
> shmod_tmon in test_progs.h (e.g. through function).
> 
> Another way (better and my preference if it works out) is to ask the 
> traffic_monitor_start() to take the the pcap file name args and makeup a 
> reasonable default if no filename is given.  Not that I am promoting non 
> test_progs tests, traffic_monitor_start() can then be reused by others 
> for legit reason. The test_progs's tests usually should not use 
> traffic_monitor_start() directly and they should stay with the 
> netns_{new, free}. I think only netns_new needs the env to figure out 
> the should_tmon and the pcap filename. May be netns_new() can stay in 
> test_progs.c, or rename it to test__netns_new().
> 
> wdyt?

How about put two ideas together?
Have traffic_monitor.c and macros in test_progs.h to collect
data from env, and pass the data to netns_new() in traffic_monitor.c.

For example,

#define test__netns_new(ns) netns_new(ns, env.test->should_tmon || \
             (env.subtest_state && env.subtest_state->should_tmon), \
             env.test->test_name,                                   \
             env.subtest_state ? env.subtest_state->name: NULL)