Re: [PATCH i-g-t] tests/amdgpu: add timeout for queue reset

[vitaly prosyak <vprosyak@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 4028 bytes --]

The change looks good to me.

Please do some tiny modifications below.
It is related to explicitly having a code block for timeout avoid checking:

if (sh_mem->reset_flags == 0)

This prevents a potential race condition between processes.
Since a timeout implies that we do not need to check the flags, this check can be safely omitted.

Reviewed-by: Vitaly Prosyak vitaly.prosyak@amd.com


On 2024-08-14 06:11, Jesse.zhang@amd.com wrote:
> 1.If the test case cannot trigger any reset on some ASIC,
> It should be considered a failure.
>
> 2. Fix code style
>
> Signed-off-by: Jesse Zhang <jesse.zhang@amd.com>
> ---
>  tests/amdgpu/amd_queue_reset.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/tests/amdgpu/amd_queue_reset.c b/tests/amdgpu/amd_queue_reset.c
> index 6819892e0..249676407 100644
> --- a/tests/amdgpu/amd_queue_reset.c
> +++ b/tests/amdgpu/amd_queue_reset.c
> @@ -30,6 +30,7 @@
>  #define SHARED_CHILD_DESCRIPTOR 3
>  
>  #define SHARED_MEM_NAME  "/queue_reset_shm"
> +#define TEST_TIMEOUT 100 //100 seconds
>  
>  enum  process_type {
>  	PROCESS_UNKNOWN,
> @@ -49,6 +50,7 @@ enum error_code_bits {
>  };
>  
>  enum reset_code_bits {
> +	NO_RESET_SET_BIT,
>  	QUEUE_RESET_SET_BIT,
>  	GPU_RESET_BEGIN_SET_BIT,
>  	GPU_RESET_END_SUCCESS_SET_BIT,

Change to

ALL_RESET_BITS = 0x1f,

Since we have now 5 states after you added NO_RESET_SET_BIT

> @@ -307,6 +309,7 @@ static void set_next_test_to_run(struct shmbuf *sh_mem, unsigned int error,
>  	sync_point_enter(sh_mem);
>  	wait_for_complete_iteration(sh_mem);
>  	sync_point_exit(sh_mem);
> +	igt_assert_neq(sh_mem->reset_flags, 1U << NO_RESET_SET_BIT);
>  }
>  
>  static int
> @@ -473,6 +476,9 @@ run_monitor_child(amdgpu_device_handle device, amdgpu_context_handle *arr_contex
>  	int state_machine = 0;
>  	int error_code;
>  	unsigned int flags;
> +	int64_t cnt = 0;
> +	time_t start, end;
> +	double elapsed = 0;
>  
>  	after_reset_state = after_reset_hangs = 0;
>  	init_flags = in_process_flags = 0;
> @@ -487,7 +493,8 @@ run_monitor_child(amdgpu_device_handle device, amdgpu_context_handle *arr_contex
>  		error_code = 0;
>  		flags = 0;
>  		set_reset_state(sh_mem, false, ALL_RESET_BITS);
> -		while (1) {
Please, keep while(1)
> +		time(&start);
> +		while (elapsed < TEST_TIMEOUT) {
>  			if (state_machine == 0) {
>  				amdgpu_cs_query_reset_state2(arr_context[test_counter], &init_flags);
>  
> @@ -533,7 +540,15 @@ run_monitor_child(amdgpu_device_handle device, amdgpu_context_handle *arr_contex
>  					break;
>  				}
>  			}
> +			cnt++;
> +			if (cnt % 1000000 == 0) {
> +				time(&end);
> +				elapsed = difftime(end, start);

                                               if ( elapsed >= TEST_TIMEOUT) {

				   set_reset_state(sh_mem, true, NO_RESET_SET_BIT);
				   break;		

                                               }                                               

> +			}
>  		}
> +		elapsed = 0;
remove 2 lines below:
> +		if (sh_mem->reset_flags == 0) //remove this
> +			set_reset_state(sh_mem, true, NO_RESET_SET_BIT);

>  		sync_point_exit(sh_mem);
>  		num_of_tests--;
>  		test_counter++;
> @@ -1000,7 +1015,7 @@ igt_main
>  			igt_describe("Stressful-and-multiple-cs-of-bad and good length-operations-using-multiple-processes");
>  			igt_subtest_with_dynamic_f("amdgpu-%s-%s", ip_tests[i] == AMD_IP_COMPUTE ? "COMPUTE":"GRAFIX", it->name) {
>  				if (arr_cap[ip_tests[i]] && get_next_rings(ring_id_good, info, &ring_id_good, &ring_id_bad, i)) {
> -					igt_dynamic_f("amdgpu-%s-ring-good-%d-bad-%d-%s", it->name,ring_id_good, ring_id_bad, ip_tests[i] == AMD_IP_COMPUTE ? "COMPUTE":"GRAFIX")
> +					igt_dynamic_f("amdgpu-%s-ring-good-%d-bad-%d-%s", it->name, ring_id_good, ring_id_bad, ip_tests[i] == AMD_IP_COMPUTE ? "COMPUTE":"GRAFIX")
>  					set_next_test_to_run(sh_mem, it->test, ip_background, ip_tests[i], ring_id_good, ring_id_bad);
>  				}
>  			}

[-- Attachment #2: Type: text/html, Size: 6168 bytes --]