Re: [PATCH v5 bpf-next 7/7] selftests/bpf: Test bpf_sock_destroy

From: Martin KaFai Lau <martin.lau@linux.dev>
To: Aditi Ghag <aditi.ghag@isovalent.com>
Cc: bpf@vger.kernel.org, kafai@fb.com, edumazet@google.com,
	Stanislav Fomichev <sdf@google.com>
Subject: Re: [PATCH v5 bpf-next 7/7] selftests/bpf: Test bpf_sock_destroy
Date: Mon, 3 Apr 2023 18:41:01 -0700	[thread overview]
Message-ID: <4f5913d0-8271-5676-569b-366fc6def385@linux.dev> (raw)
In-Reply-To: <144D865C-07D6-4665-85F8-A5AF511ED44A@isovalent.com>

On 4/3/23 5:15 PM, Aditi Ghag wrote:
> 
> 
>> On Apr 3, 2023, at 10:35 AM, Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 4/3/23 8:55 AM, Aditi Ghag wrote:
>>>> On Mar 31, 2023, at 3:32 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>
>>>> On 3/30/23 11:46 AM, Stanislav Fomichev wrote:
>>>>>> +void test_sock_destroy(void)
>>>>>> +{
>>>>>> +    struct sock_destroy_prog *skel;
>>>>>> +    int cgroup_fd = 0;
>>>>>> +
>>>>>> +    skel = sock_destroy_prog__open_and_load();
>>>>>> +    if (!ASSERT_OK_PTR(skel, "skel_open"))
>>>>>> +        return;
>>>>>> +
>>>>>> +    cgroup_fd = test__join_cgroup("/sock_destroy");
>>>>
>>>> Please run this test in its own netns also to avoid affecting other tests as much as possible.
>>> Is it okay if I defer this nit to a follow-up patch? It's not conflicting with other tests at the moment.
>>
>> Is it sure it won't affect other tests when running in parallel (test -j)?
>> This test is iterating all in the current netns and only checks for port before aborting.
>>
>> It is easy to add. eg. ~10 lines of codes can be borrowed from prog_tests/mptcp.c which has recently done the same in commit 02d6a057c7be to run the test in its own netns to set a sysctl.
> 
> I haven't run the tests in parallel, but the tests are not using hard-coded ports anymore. Anyway I'm willing to run it in a separate netns as a follow-up for additional guards, but do you still think it's a blocker for this patch?

Testing port is not good enough. It is only like ~10 lines of codes that can be 
borrowed from other existing tests that I mentioned earlier. What is the reason 
to cut corners here? The time spent in replying on this topic is more than 
enough to add the netns support. I don't want to spend time to figure out why 
other tests running in parallel become flaky while waiting for the follow up,
so no.

Please run the test in its own netns. All new network tests must run in its own 
netns.

btw, since I don't hear any comment on patch 5 regarding to restricting the 
destroy kfunc to BPF_TRACE_ITER only. It is the major piece missing. I am 
putting some pseudo code that is more flexible than adding 
BTF_KFUNC_HOOK_TRACING_ITER that I mentioned earlier to see how it may look 
like. Will update that patch's thread soon.