diff mbox series

[net,1/3] selftests: tc: set timeout to 15 minutes

Message ID 20230713-tc-selftests-lkft-v1-1-1eb4fd3a96e7@tessares.net (mailing list archive)
State Accepted
Commit fda05798c22a354efde09a76bdfc276b2d591829
Delegated to: Netdev Maintainers
Headers show
Series selftests: tc: increase timeout and add missing kconfig | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Matthieu Baerts July 13, 2023, 9:16 p.m. UTC
When looking for something else in LKFT reports [1], I noticed that the
TC selftest ended with a timeout error:

  not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds

The timeout had been introduced 3 years ago, see the Fixes commit below.

This timeout is only in place when executing the selftests via the
kselftests runner scripts. I guess this is not what most TC devs are
using and nobody noticed the issue before.

The new timeout is set to 15 minutes as suggested by Pedro [2]. It looks
like it is plenty more time than what it takes in "normal" conditions.

Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout per test")
Cc: stable@vger.kernel.org
Link: https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20230711/testrun/18267241/suite/kselftest-tc-testing/test/tc-testing_tdc_sh/log [1]
Link: https://lore.kernel.org/netdev/0e061d4a-9a23-9f58-3b35-d8919de332d7@tessares.net/T/ [2]
Suggested-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 tools/testing/selftests/tc-testing/settings | 1 +
 1 file changed, 1 insertion(+)

Comments

shaozhengchao July 14, 2023, 2:25 a.m. UTC | #1
On 2023/7/14 5:16, Matthieu Baerts wrote:
> When looking for something else in LKFT reports [1], I noticed that the
> TC selftest ended with a timeout error:
> 
>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
> 
> The timeout had been introduced 3 years ago, see the Fixes commit below.
> 
> This timeout is only in place when executing the selftests via the
> kselftests runner scripts. I guess this is not what most TC devs are
> using and nobody noticed the issue before.
> 
> The new timeout is set to 15 minutes as suggested by Pedro [2]. It looks
> like it is plenty more time than what it takes in "normal" conditions.
> 
> Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout per test")
> Cc: stable@vger.kernel.org
> Link: https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20230711/testrun/18267241/suite/kselftest-tc-testing/test/tc-testing_tdc_sh/log [1]
> Link: https://lore.kernel.org/netdev/0e061d4a-9a23-9f58-3b35-d8919de332d7@tessares.net/T/ [2]
> Suggested-by: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>   tools/testing/selftests/tc-testing/settings | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/tc-testing/settings b/tools/testing/selftests/tc-testing/settings
> new file mode 100644
> index 000000000000..e2206265f67c
> --- /dev/null
> +++ b/tools/testing/selftests/tc-testing/settings
> @@ -0,0 +1 @@
> +timeout=900
> 
I remember last year when I tested all the tdc cases(qdisc + filter +
action + infra) in my vm machine, it took me nearly 20 minutes.
So I think it should be more than 1200 seconds if all cases need to be
tested.

Maybe we should really optimize the parallel execution process of tdc.

Zhengchao Shao
Pedro Tammela July 14, 2023, 5:49 p.m. UTC | #2
On 13/07/2023 23:25, shaozhengchao wrote:
> 
> 
> On 2023/7/14 5:16, Matthieu Baerts wrote:
>> When looking for something else in LKFT reports [1], I noticed that the
>> TC selftest ended with a timeout error:
>>
>>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>
>> The timeout had been introduced 3 years ago, see the Fixes commit below.
>>
>> This timeout is only in place when executing the selftests via the
>> kselftests runner scripts. I guess this is not what most TC devs are
>> using and nobody noticed the issue before.
>>
>> The new timeout is set to 15 minutes as suggested by Pedro [2]. It looks
>> like it is plenty more time than what it takes in "normal" conditions.
>>
>> Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second 
>> timeout per test")
>> Cc: stable@vger.kernel.org
>> Link: 
>> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20230711/testrun/18267241/suite/kselftest-tc-testing/test/tc-testing_tdc_sh/log [1]
>> Link: 
>> https://lore.kernel.org/netdev/0e061d4a-9a23-9f58-3b35-d8919de332d7@tessares.net/T/ [2]
>> Suggested-by: Pedro Tammela <pctammela@mojatatu.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>>   tools/testing/selftests/tc-testing/settings | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/tc-testing/settings 
>> b/tools/testing/selftests/tc-testing/settings
>> new file mode 100644
>> index 000000000000..e2206265f67c
>> --- /dev/null
>> +++ b/tools/testing/selftests/tc-testing/settings
>> @@ -0,0 +1 @@
>> +timeout=900
>>
> I remember last year when I tested all the tdc cases(qdisc + filter +
> action + infra) in my vm machine, it took me nearly 20 minutes.
> So I think it should be more than 1200 seconds if all cases need to be
> tested.
> 
> Maybe we should really optimize the parallel execution process of tdc.

Let's try to spend some cycles improving the tdc code performance first.
TDC boils down essentially to:
- Setup namespace (if needed)
- Setup network interfaces
- Spawn a few processes
- Match a regex
- Bring down namespace

Nothing above screams expensive, so I'm sure there are some low hanging 
fruits to improve the overall wall time even in debug kernels.
Matthieu Baerts July 17, 2023, 8:32 a.m. UTC | #3
Hi Zhengchao Shao,

On 14/07/2023 04:25, shaozhengchao wrote:
> 
> 
> On 2023/7/14 5:16, Matthieu Baerts wrote:
>> When looking for something else in LKFT reports [1], I noticed that the
>> TC selftest ended with a timeout error:
>>
>>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>
>> The timeout had been introduced 3 years ago, see the Fixes commit below.
>>
>> This timeout is only in place when executing the selftests via the
>> kselftests runner scripts. I guess this is not what most TC devs are
>> using and nobody noticed the issue before.
>>
>> The new timeout is set to 15 minutes as suggested by Pedro [2]. It looks
>> like it is plenty more time than what it takes in "normal" conditions.
>>
>> Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
>> timeout per test")
>> Cc: stable@vger.kernel.org
>> Link:
>> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20230711/testrun/18267241/suite/kselftest-tc-testing/test/tc-testing_tdc_sh/log [1]
>> Link:
>> https://lore.kernel.org/netdev/0e061d4a-9a23-9f58-3b35-d8919de332d7@tessares.net/T/ [2]
>> Suggested-by: Pedro Tammela <pctammela@mojatatu.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>>   tools/testing/selftests/tc-testing/settings | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/tc-testing/settings
>> b/tools/testing/selftests/tc-testing/settings
>> new file mode 100644
>> index 000000000000..e2206265f67c
>> --- /dev/null
>> +++ b/tools/testing/selftests/tc-testing/settings
>> @@ -0,0 +1 @@
>> +timeout=900
>>
> I remember last year when I tested all the tdc cases(qdisc + filter +
> action + infra) in my vm machine, it took me nearly 20 minutes.
> So I think it should be more than 1200 seconds if all cases need to be
> tested.

Thank you for your feedback!

Be careful that here, it is the timeout to run "tdc.sh" only which is
currently limited to:

  ./tdc.py -c actions --nobuildebpf
  ./tdc.py -c qdisc

(not "filter", nor "infra" then)

I guess for this, 15 minutes is more than enough, no?

At least on my side, I ran it in a i386 VM without KVM and it took less
than 3 minutes [1].

Cheers,
Matt

[1]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWHb7PJfqkUX1m8rLu3GXbsHE0/logs?format=html
shaozhengchao July 18, 2023, 1:43 a.m. UTC | #4
On 2023/7/17 16:32, Matthieu Baerts wrote:
> Hi Zhengchao Shao,
> 
> On 14/07/2023 04:25, shaozhengchao wrote:
>>
>>
>> On 2023/7/14 5:16, Matthieu Baerts wrote:
>>> When looking for something else in LKFT reports [1], I noticed that the
>>> TC selftest ended with a timeout error:
>>>
>>>     not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>>
>>> The timeout had been introduced 3 years ago, see the Fixes commit below.
>>>
>>> This timeout is only in place when executing the selftests via the
>>> kselftests runner scripts. I guess this is not what most TC devs are
>>> using and nobody noticed the issue before.
>>>
>>> The new timeout is set to 15 minutes as suggested by Pedro [2]. It looks
>>> like it is plenty more time than what it takes in "normal" conditions.
>>>
>>> Fixes: 852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second
>>> timeout per test")
>>> Cc: stable@vger.kernel.org
>>> Link:
>>> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20230711/testrun/18267241/suite/kselftest-tc-testing/test/tc-testing_tdc_sh/log [1]
>>> Link:
>>> https://lore.kernel.org/netdev/0e061d4a-9a23-9f58-3b35-d8919de332d7@tessares.net/T/ [2]
>>> Suggested-by: Pedro Tammela <pctammela@mojatatu.com>
>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>> ---
>>>    tools/testing/selftests/tc-testing/settings | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/tools/testing/selftests/tc-testing/settings
>>> b/tools/testing/selftests/tc-testing/settings
>>> new file mode 100644
>>> index 000000000000..e2206265f67c
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/tc-testing/settings
>>> @@ -0,0 +1 @@
>>> +timeout=900
>>>
>> I remember last year when I tested all the tdc cases(qdisc + filter +
>> action + infra) in my vm machine, it took me nearly 20 minutes.
>> So I think it should be more than 1200 seconds if all cases need to be
>> tested.
> 
> Thank you for your feedback!
> 
Hi Matthieu:
> Be careful that here, it is the timeout to run "tdc.sh" only which is
> currently limited to:
> 
>    ./tdc.py -c actions --nobuildebpf
>    ./tdc.py -c qdisc
> 
> (not "filter", nor "infra" then)
> 
> I guess for this, 15 minutes is more than enough, no?
> 
15 minutes is enough for qdisc and actions. Thanks.

> At least on my side, I ran it in a i386 VM without KVM and it took less
> than 3 minutes [1].
> 
> Cheers,
> Matt
> 
> [1]
> https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWHb7PJfqkUX1m8rLu3GXbsHE0/logs?format=html


Reviewed-by: Zhengchao Shao <shaozhengchao@huawei.com>
diff mbox series

Patch

diff --git a/tools/testing/selftests/tc-testing/settings b/tools/testing/selftests/tc-testing/settings
new file mode 100644
index 000000000000..e2206265f67c
--- /dev/null
+++ b/tools/testing/selftests/tc-testing/settings
@@ -0,0 +1 @@ 
+timeout=900