Re: [PATCH net-next 1/4] selftests/net/forwarding: add slowwait functions

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 1/24/24 10:58, Hangbin Liu wrote:
> Add slowwait functions to wait for some operations that may need a long time
> to finish. The busywait executes the cmd too fast, which is kind of wasting
> cpu in this scenario. At the same time, if shell debugging is enabled with
> `set -x`. the busywait will output too much logs.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>   tools/testing/selftests/net/forwarding/lib.sh | 36 +++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 8a61464ab6eb..07faedc2071b 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -41,6 +41,7 @@ fi
>   # Kselftest framework requirement - SKIP code is 4.
>   ksft_skip=4
>   
> +# timeout in milliseconds
>   busywait()
>   {
>   	local timeout=$1; shift
> @@ -64,6 +65,32 @@ busywait()
>   	done
>   }
>   
> +# timeout in seconds
> +slowwait()
> +{
> +	local timeout=$1; shift
> +
> +	local start_time="$(date -u +%s)"
> +	while true
> +	do
> +		local out
> +		out=$("$@")
> +		local ret=$?
> +		if ((!ret)); then

it would be nice to have some exit code used (or just reserved) for
"operation failed, no need to wait, fail the test please"
similar to the xargs, eg:
               126    if the command cannot be run

> +			echo -n "$out"
> +			return 0
> +		fi
> +
> +		local current_time="$(date -u +%s)"
> +		if ((current_time - start_time > timeout)); then
> +			echo -n "$out"
> +			return 1
> +		fi
> +
> +		sleep 1

I see that `sleep 1` is simplest correct impl, but it's tempting to
suggest exponential back-off, perhaps with saturation at 15

(but then you will have to cap last sleep to don't exceed timeout by
more than 1).

> +	done
> +}
> +
>   ##############################################################################
>   # Sanity checks
>   
> @@ -505,6 +532,15 @@ busywait_for_counter()
>   	busywait "$timeout" until_counter_is ">= $((base + delta))" "$@"
>   }
>   
> +slowwait_for_counter()
> +{
> +	local timeout=$1; shift
> +	local delta=$1; shift
> +
> +	local base=$("$@")
> +	slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
> +}
> +
>   setup_wait_dev()
>   {
>   	local dev=$1; shift

just nitpicks so I will provide my RB in case you want to ignore ;)