netdev - Re: [PATCH net] selftests: net: cope with slow env in gro.sh test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3e5f9040-53a4-4906-819a-ef38500f9eb6@kernel.org>
Date: Wed, 7 Feb 2024 15:45:44 +0100
From: Matthieu Baerts <matttbe@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Shuah Khan <shuah@...nel.org>, Willem de Bruijn <willemb@...gle.com>,
 Coco Li <lixiaoyan@...gle.com>, linux-kselftest@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH net] selftests: net: cope with slow env in gro.sh test

Hi Paolo,

On 07/02/2024 15:35, Paolo Abeni wrote:
> On Wed, 2024-02-07 at 12:16 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 06/02/2024 16:27, Paolo Abeni wrote:
>>> The gro self-tests sends the packets to be aggregated with
>>> multiple write operations.
>>>
>>> When running is slow environment, it's hard to guarantee that
>>> the GRO engine will wait for the last packet in an intended
>>> train.
>>>
>>> The above causes almost deterministic failures in our CI for
>>> the 'large' test-case.
>>>
>>> Address the issue explicitly ignoring failures for such case
>>> in slow environments (KSFT_MACHINE_SLOW==true).
>>
>> To what value is KSFT_MACHINE_SLOW set in the CI?
> 
> AFAIK, the CI initialize KSFT_MACHINE_SLOW (to true) only on slow env.

Should be good, then!

>> Is it set to a different value if the machine is not slow? e.g.
>>
>>   KSFT_MACHINE_SLOW == false
>>
>> (please see below)
>>
>>> Fixes: 7d1575014a63 ("selftests/net: GRO coalesce test")
>>> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
>>> ---
>>> Note that the fixes tag is there mainly to justify targeting the net
>>> tree, and this is aiming at net to hopefully make the test more stable
>>> ASAP for both trees.
>>>
>>> I experimented with a largish refactory replacing the multiple writes
>>> with a single GSO packet, but exhausted by time budget before reaching
>>> any good result.
>>> ---
>>>  tools/testing/selftests/net/gro.sh | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
>>> index 19352f106c1d..114b5281a3f5 100755
>>> --- a/tools/testing/selftests/net/gro.sh
>>> +++ b/tools/testing/selftests/net/gro.sh
>>> @@ -31,6 +31,10 @@ run_test() {
>>>        1>>log.txt
>>>      wait "${server_pid}"
>>>      exit_code=$?
>>> +    if [ ${test} == "large" -a -n "${KSFT_MACHINE_SLOW}" ]; then
>>
>> Maybe best to avoid using:
>>
>>   -n "${KSFT_MACHINE_SLOW}"
>>
>> Otherwise, we have the same behaviour if KSFT_MACHINE_SLOW is set to
>> 1/yes/true or 0/no/false.
> 
> For consistency, I followed the logic already in place in commit
> c41dfb0dfbec ("selftests/net: ignore timing errors in so_txtime if
> KSFT_MACHINE_SLOW").

I only checked code in -net, I forgot to look at net-next. Thanks for
the pointer! I thought it was "fragile", but if that's how we are
supposed to use this env var, that's OK then :)

>> But maybe it is fine like that, and what is just missing is adding
>> somewhere how KSFT_MACHINE_SLOW is supposed to be set/used? :)
>>
>>
>> Not linked to that, but a small detail about the new line, just in case
>> you need to send a v2: it looks like it is better to avoid using '-a':
>>
>>   https://www.shellcheck.net/wiki/SC2166
> 
> Thank for the pointer, I was not aware of that. 
> 
> I guess a v2 dropping '-a' would be better.

I'm not even sure a v2 is really needed. "-a" seems OK if you don't use
(or don't plan to use) "!" or "-" in the expression from what I read.

Another way to fix this is to use [[ ]]:

  [[ ${test} == "large" && -n "${KSFT_MACHINE_SLOW}" ]]

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.