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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxx>
---
  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)





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux