Re: [PATCH] tests/intel/xe_create: Remove the elapsed time validation

["Vivekanandan, Balasubramani" <balasubramani.vivekanandan@intel.com>]

On 02.08.2024 07:12, Zbigniew Kempczyński wrote:
> On Thu, Aug 01, 2024 at 06:03:12PM +0530, Balasubramani Vivekanandan wrote:
> > Time check used is incorrect. Time limit used for validation is
> > arbitrarily chosen as 5 seconds with no real resoning behind it.
> > With different execution environments taking different execution times,
> > limit can't be fixed and leads to unnecessary hacks.
> > So remove this time check completely.
> 
> MAXTIME picked in the past was chosen as a fraction of created/destroyed
> exec queues in some (safe) time period. Up to relatively small number
> of exec queues (up to 512) there was no issue observed. But with larger
> number of exec queues we noticed kernel lockup (iirc guc related).

Time limit of 5 seconds is not fitting all platforms and execution
environments. I would like to avoid hacks to workaround those.
We can validate the test case by ensuring there were no GPU hangs or wait
timeouts instead of checking the time taken.
Measuring the time is more a performance validation rather than
functionality validation. Better to isolate performance tests from
functionality tests.

> 
> I think keeping some time regime for those operations is good idea, we
> may notice performance / stability drop if something unexpected will
> happen in kmd/guc. MAXTIME/MAXEXECQUEUES were chosen as safe values
> which should be easily handled. If you're removing this check you may
> delete subtest as well.

Keeping subtest without time check will ensure the use case works fine
without causing GPU hangs, kernel lockups or even dmesg warnings.

Regards,
Bala

> 
> --
> Zbigniew
> 
> > 
> > Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> > ---
> >  tests/intel/xe_create.c | 17 +----------------
> >  1 file changed, 1 insertion(+), 16 deletions(-)
> > 
> > diff --git a/tests/intel/xe_create.c b/tests/intel/xe_create.c
> > index 76ffdcef2..80de07789 100644
> > --- a/tests/intel/xe_create.c
> > +++ b/tests/intel/xe_create.c
> > @@ -157,7 +157,6 @@ enum vm_count {
> >  };
> >  
> >  #define MAXEXECQUEUES 2048
> > -#define MAXTIME 5
> >  
> >  /**
> >   * SUBTEST: create-execqueues-%s
> > @@ -175,10 +174,8 @@ enum vm_count {
> >  static void create_execqueues(int fd, enum exec_queue_destroy ed,
> >  			      enum vm_count vc)
> >  {
> > -	struct timespec tv = { };
> >  	uint32_t num_engines, exec_queues_per_process, vm;
> > -	int nproc = sysconf(_SC_NPROCESSORS_ONLN), seconds;
> > -	int real_timeout = MAXTIME * (vc == SHARED ? 4 : 1);
> > +	int nproc = sysconf(_SC_NPROCESSORS_ONLN);
> >  
> >  	if (vc == SHARED) {
> >  		fd = drm_reopen_driver(fd);
> > @@ -189,8 +186,6 @@ static void create_execqueues(int fd, enum exec_queue_destroy ed,
> >  	exec_queues_per_process = max_t(uint32_t, 1, MAXEXECQUEUES / nproc);
> >  	igt_debug("nproc: %u, exec_queues per process: %u\n", nproc, exec_queues_per_process);
> >  
> > -	igt_nsec_elapsed(&tv);
> > -
> >  	igt_fork(n, nproc) {
> >  		struct drm_xe_engine *engine;
> >  		uint32_t exec_queue, exec_queues[exec_queues_per_process];
> > @@ -236,16 +231,6 @@ static void create_execqueues(int fd, enum exec_queue_destroy ed,
> >  		xe_vm_destroy(fd, vm);
> >  		drm_close_driver(fd);
> >  	}
> > -
> > -	seconds = igt_seconds_elapsed(&tv);
> > -	if (seconds > real_timeout) {
> > -		if (igt_run_in_simulation())
> > -			igt_info("Creating %d exec_queues took too long: %d [limit: %d] seconds\n",
> > -				 MAXEXECQUEUES, seconds, real_timeout);
> > -		else
> > -			igt_assert_f(false, "Creating %d exec_queues took too long: %d [limit: %d] seconds\n",
> > -				     MAXEXECQUEUES, seconds, real_timeout);
> > -	}
> >  }
> >  
> >  /**
> > -- 
> > 2.34.1
> >