[PATCH i-g-t] tests/xe_evict|exec_threads: Use fd reopen to avoid corrupting global data

[Kamil Konieczny]

Hi Zbigniew,
On 2024-04-17 at 16:26:07 +0200, Zbigniew Kempczyński wrote:
> Device scanning and filtering was designed to work in multi-process
> environment. Due to that opening the device with drm_open_driver()
> in multiple threads lead to global device data corruption.
> 
> At the moment there's no easy way to redesign this so simplest thing
> we may do is to just acquire device fd in main process and reopen it
> (this doesn't involve any device scanning) in spawned threads.
> 
> Lets use drm_reopen_driver() to stop calling not thread-safe code.
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Cc: Matthew Brost <matthew.brost at intel.com>
> ---
>  tests/intel/xe_evict.c        |  8 ++++----
>  tests/intel/xe_exec_threads.c | 22 ++++++++--------------
>  2 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/tests/intel/xe_evict.c b/tests/intel/xe_evict.c
> index 8ef55211f7..9b97cccab2 100644
> --- a/tests/intel/xe_evict.c
> +++ b/tests/intel/xe_evict.c
> @@ -61,7 +61,7 @@ test_evict(int fd, struct drm_xe_engine_class_instance *eci,
>  	bo = calloc(n_execs / 2, sizeof(*bo));
>  	igt_assert(bo);
>  
> -	fd = drm_open_driver(DRIVER_XE);
> +	fd = drm_reopen_driver(fd);
>  

I checked xe_evict again just to be sure and there are
two opens (not counting one from fixture):
grep -n open_driver tests/intel/xe_evict.c
64:     fd = drm_reopen_driver(fd);
244:    fd = drm_open_driver(DRIVER_XE);

imho that second one could also be called in multi-threaded version
from thread() function, so line 244 should also be fixed into reopen.

You can keep my r-b with fixed version.

Regards,
Kamil

>  	vm = xe_vm_create(fd, 0, 0);
>  	if (flags & BIND_EXEC_QUEUE)
> @@ -769,21 +769,21 @@ igt_main
>  
>  	for (const struct section *s = sections; s->name; s++) {
>  		igt_subtest_f("evict-%s", s->name)
> -			test_evict(-1, hwe, s->n_exec_queues, s->n_execs,
> +			test_evict(fd, hwe, s->n_exec_queues, s->n_execs,
>  				   calc_bo_size(vram_size, s->mul, s->div),
>  				   s->flags, NULL);
>  	}
>  
>  	for (const struct section_cm *s = sections_cm; s->name; s++) {
>  		igt_subtest_f("evict-%s", s->name)
> -			test_evict_cm(-1, hwe, s->n_exec_queues, s->n_execs,
> +			test_evict_cm(fd, hwe, s->n_exec_queues, s->n_execs,
>  				      calc_bo_size(vram_size, s->mul, s->div),
>  				      s->flags, NULL);
>  	}
>  
>  	for (const struct section_threads *s = sections_threads; s->name; s++) {
>  		igt_subtest_f("evict-%s", s->name)
> -			threads(-1, hwe, s->n_threads, s->n_exec_queues,
> +			threads(fd, hwe, s->n_threads, s->n_exec_queues,
>  				 s->n_execs,
>  				 calc_bo_size(vram_size, s->mul, s->div),
>  				 s->flags);
> diff --git a/tests/intel/xe_exec_threads.c b/tests/intel/xe_exec_threads.c
> index 8083980f90..1e80842b98 100644
> --- a/tests/intel/xe_exec_threads.c
> +++ b/tests/intel/xe_exec_threads.c
> @@ -71,8 +71,8 @@ test_balancer(int fd, int gt, uint32_t vm, uint64_t addr, uint64_t userptr,
>  
>  	igt_assert(n_exec_queues <= MAX_N_EXEC_QUEUES);
>  
> -	if (!fd) {
> -		fd = drm_open_driver(DRIVER_XE);
> +	if (flags & FD) {
> +		fd = drm_reopen_driver(fd);
>  		owns_fd = true;
>  	}
>  
> @@ -273,8 +273,8 @@ test_compute_mode(int fd, uint32_t vm, uint64_t addr, uint64_t userptr,
>  
>  	igt_assert(n_exec_queues <= MAX_N_EXEC_QUEUES);
>  
> -	if (!fd) {
> -		fd = drm_open_driver(DRIVER_XE);
> +	if (flags & FD) {
> +		fd = drm_reopen_driver(fd);
>  		owns_fd = true;
>  	}
>  
> @@ -477,8 +477,8 @@ test_legacy_mode(int fd, uint32_t vm, uint64_t addr, uint64_t userptr,
>  
>  	igt_assert(n_exec_queues <= MAX_N_EXEC_QUEUES);
>  
> -	if (!fd) {
> -		fd = drm_open_driver(DRIVER_XE);
> +	if (flags & FD) {
> +		fd = drm_reopen_driver(fd);
>  		owns_fd = true;
>  	}
>  
> @@ -995,10 +995,7 @@ static void threads(int fd, int flags)
>  #define ADDRESS_SHIFT	39
>  		threads_data[i].addr = addr | (i << ADDRESS_SHIFT);
>  		threads_data[i].userptr = userptr | (i << ADDRESS_SHIFT);
> -		if (flags & FD)
> -			threads_data[i].fd = 0;
> -		else
> -			threads_data[i].fd = fd;
> +		threads_data[i].fd = fd;
>  		threads_data[i].vm_legacy_mode = vm_legacy_mode;
>  		threads_data[i].vm_compute_mode = vm_compute_mode;
>  		threads_data[i].eci = hwe;
> @@ -1046,10 +1043,7 @@ static void threads(int fd, int flags)
>  						threads_data[i].addr = addr;
>  					threads_data[i].userptr = userptr |
>  						(i << ADDRESS_SHIFT);
> -					if (flags & FD)
> -						threads_data[i].fd = 0;
> -					else
> -						threads_data[i].fd = fd;
> +					threads_data[i].fd = fd;
>  					threads_data[i].gt = gt;
>  					threads_data[i].vm_legacy_mode =
>  						vm_legacy_mode;
> -- 
> 2.34.1
>