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

[Zbigniew Kempczyński]

On Mon, Apr 22, 2024 at 03:56:04PM +0200, Kamil Konieczny wrote:
> 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.

Eh, good catch. I'll fix it and resend.

--
Zbigniew

> 
> 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
> >