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

[Martin KaFai Lau <martin.lau@linux.dev>]

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?

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?