[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <629eaabc-0a30-44e6-aeda-60dbbbe1a637@oracle.com>
Date: Tue, 6 Aug 2024 13:38:54 +0100
From: Alan Maguire <alan.maguire@...cle.com>
To: Alexis Lothoré <alexis.lothore@...tlin.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau
<martin.lau@...ux.dev>,
Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>
Cc: ebpf@...uxfoundation.org, Thomas Petazzoni
<thomas.petazzoni@...tlin.com>,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next 2/4] selftests/bpf: convert test_cgroup_storage
to test_progs
On 01/08/2024 10:21, Alexis Lothoré wrote:
> On 8/1/24 10:27, Alan Maguire wrote:
>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
>>> test_cgroup_storage is currently a standalone program which is not run
>>> when executing test_progs.
>>>
>>> Convert it to the test_progs framework so it can be automatically executed
>>> in CI. The conversion led to the following changes:
>>> - converted the raw bpf program in the userspace test file into a dedicated
>>> test program in progs/ dir
>>> - reduced the scope of cgroup_storage test: the content from this test
>>> overlaps with some other tests already present in test_progs, most
>>> notably netcnt and cgroup_storage_multi*. Those tests already check
>>> extensively local storage, per-cpu local storage, cgroups interaction,
>>> etc. So the new test only keep the part testing that the program return
>>> code (based on map content) properly leads to packet being passed or
>>> dropped.
>>>
>>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@...tlin.com>
>>
>> Two small things below, but
>>
>> Reviewed-by: Alan Maguire <alan.maguire@...cle.com>
>
> [...]
>
>>> +#define PING_CMD "ping localhost -c 1 -W 1 -q"
>>
>> other tests seem to redirect ping stdout output to /dev/null ; might be
>> worth doing that too.
>
> That's in fact performed automatically by SYS_NOFAIL :)
> > #define SYS_NOFAIL(fmt, ...) \
> ({ \
> char cmd[1024]; \
> int n; \
> n = snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__); \
> if (n < sizeof(cmd) && sizeof(cmd) - n >= sizeof(ALL_TO_DEV_NULL)) \
> strcat(cmd, ALL_TO_DEV_NULL); \
> system(cmd); \
> })
>
> [...]
>
Perfect, I missed that.
>>> +{
>>> + __u64 *counter;
>>> +
>>> + counter = bpf_get_local_storage(&cgroup_storage, 0);
>>
>> don't we need a NULL check for counter here? Or does the verifier know
>> bpf_get_local_storage never fails?
>
> Good question. Since the verifier accepted the prog during my tests, I indeed
> assume that the returned pointer is always valid. Amongst all calls to this
> function in progs involved in selftests, I found only one performing a check
> before using the value (lsm_cgroup.c). So I guess it is fine ?
>
Looks like the prototype for the helper specifies a return type of
RET_PTR_TO_MAP_VALUE ; if it was RET_PTR_TO_MAP_VALUE_OR_NULL we'd need
the NULL check, but because it's a guaranteed map ptr we are good here
without a NULL check.
Thanks!
Alan
Powered by blists - more mailing lists