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

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

On 7/25/24 3:47 PM, Kui-Feng Lee wrote:
>>> +/* Start to monitor the network traffic in the given network namespace.
>>> + *
>>> + * netns: the name of the network namespace to monitor. If NULL, the
>>> + * current network namespace is monitored.
>>> + *
>>> + * This function will start a thread to capture packets going through NICs
>>> + * in the give network namespace.
>>> + */
>>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns)
>>
>> There is opportunity to make the traffic monitoring easier for tests that 
>> create its own netns which I hope most of the networking tests fall into this 
>> bucket now. Especially for tests that create multiple netns such that the test 
>> does not have to start/stop for each individual netns.
>>
>> May be adding an API like "struct nstoken *netns_new(const char *netns_name)". 
>> The netns_new() will create the netns and (optionally) start the monitoring 
>> thread also. It will need another "void netns_free(struct nstoken *nstoken)" 
>> to stop the thread and remove the netns. The "struct tmonitor_ctx" probably 
>> makes sense to be embedded into "struct nstoken" if we go with this new API.
> 
> Agree! But, I think we need another type rather than to reuse "struct
> netns". People may accidentally call close_netns() on the nstoken
> returned by this function.

ah. Good point. close_netns() does free the nstoken also...
yep. probably make sense to have another type for netns create/destroy which 
start/stop the monitoring automatically based on the on/off in the libpcap.list.

> 
>>
>> This will need some changes to the tests creating netns but it probably should 
>> be obvious change considering most test do "ip netns add..." and then 
>> open_netns(). It can start with the flaky test at hand first like tc_redirect.
>>
>> May be a little more changes for the test using "unshare(CLONE_NEWNET)" but 
>> should not be too bad either. This can be done only when we need to turn on 
>> libpcap to debug that test.
>>
>> Also, when the test is flaky, make it easier for people not familiar with the 
>> codes of the networking test to turn on traffic monitoring without changing 
>> the test code. May be in a libpcap.list file (in parallel to the existing 
>> DENYLIST)?
>>
>> For the tests without having its own netns, they can either move to netns 
>> (which I think it is a good thing to do) or use the 
>> traffic_monitor_start/stop() manually by changing the testing code,
>> or a better way is to ask test_progs do it for the host netns (init_netns) 
>> automatically for all tests in the libpcap.list.
> 
> Agree! I will start move some tests to netns, and use libpcap.list to
> enable them.

The tc_redirect test should be in netns already. It seems the select_reuseport 
and the sockmap_listen test, that this patchset is touching, are not in netns. I 
hope the netns migration changes should be obvious for them. Other than those 
two flaky tests, I would separate other netns moving work to another effort.