Re: [PATCH i-g-t] tests/xe_exec_threads: Fill in GT field for second balancer thread

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Mar 21, 2024 at 01:01:22AM -0500, Lucas De Marchi wrote:
> On Thu, Mar 21, 2024 at 01:04:37AM +0000, Matthew Brost wrote:
> > On Wed, Mar 20, 2024 at 03:47:57PM -0500, Lucas De Marchi wrote:
> > > On Wed, Mar 20, 2024 at 12:23:56PM -0700, Matt Roper wrote:
> > > > The balancer subtests spawn two pthreads per engine class if there are
> > > > multiple instances of the class.  The GT field of the data structure is
> > > > filled in properly for the first thread, but not for the second,
> > > > effectively leaving it set to "0."
> > > >
> > > > For platforms with standalone media, this will result in failures when
> > > > the thread tries to find the instances of a media class on GT0 and trips
> > > > the "igt_assert(num_placements > 1)" assertion in test_balancer().
> > > >
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > 
> > > 
> > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > 
> > > do you know why we unrolled a loop in there and did a wrong copy and
> > 
> > I think the for_gt loop was added after the original version of this.
> > Likely an ommision when that was added.
> > 
> > > paste? That together with vertical spacing on flow control seems to be a
> > > good source of bugs. Good find.
> > > 
> > > The only difference between the first and second thread data I'm seeing
> > > is flags VIRTUAL vs PARALLEL. Matt Brost, is this test from you? Could
> > 
> > Yes, I wrote this one. Not my finest work in terms of copy / pasting,
> > indeed the only difference should be VIRTUAL vs. PARALLEL. I think this
> > should be consolidated into a loop.
> 
> 
> sounds good. Something like this on top of this patch? (completely untested, it builds)
> 

Yes, that looks about right.

>   diff --git a/tests/intel/xe_exec_threads.c b/tests/intel/xe_exec_threads.c
>   index 977e8c600..b95870714 100644
>   --- a/tests/intel/xe_exec_threads.c
>   +++ b/tests/intel/xe_exec_threads.c
>   @@ -1030,6 +1030,7 @@ static void threads(int fd, int flags)
>                   xe_for_each_gt(fd, gt)
>                           xe_for_each_engine_class(class) {
>                                   int num_placements = 0;
>   +                               int *data_flags = (int[]){ VIRTUAL, PARALLEL, -1 };
>                                   xe_for_each_engine(fd, hwe) {
>                                           if (hwe->engine_class != class ||
>   @@ -1038,36 +1039,10 @@ static void threads(int fd, int flags)
>                                           ++num_placements;
>                                   }
>   -                               if (num_placements > 1) {
>   -                                       threads_data[i].mutex = &mutex;
>   -                                       threads_data[i].cond = &cond;
>   -                                       if (flags & SHARED_VM)
>   -                                               threads_data[i].addr = addr |
>   -                                                       (i << ADDRESS_SHIFT);
>   -                                       else
>   -                                               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].gt = gt;
>   -                                       threads_data[i].vm_legacy_mode =
>   -                                               vm_legacy_mode;
>   -                                       threads_data[i].class = class;
>   -                                       threads_data[i].n_exec_queue = N_EXEC_QUEUE;
>   -                                       threads_data[i].n_exec = N_EXEC;
>   -                                       threads_data[i].flags = flags;
>   -                                       threads_data[i].flags &= ~BALANCER;
>   -                                       threads_data[i].flags |= VIRTUAL;
>   -                                       threads_data[i].go = &go;
>   -
>   -                                       ++n_threads;
>   -                                       pthread_create(&threads_data[i].thread, 0,
>   -                                                      thread, &threads_data[i]);
>   -                                       ++i;
>   +                               if (num_placements <= 1)
>   +                                       continue;
>   +                               while (*data_flags >= 0) {
>                                           threads_data[i].mutex = &mutex;
>                                           threads_data[i].cond = &cond;
>                                           if (flags & SHARED_VM)
>   @@ -1089,13 +1064,14 @@ static void threads(int fd, int flags)
>                                           threads_data[i].n_exec = N_EXEC;
>                                           threads_data[i].flags = flags;
>                                           threads_data[i].flags &= ~BALANCER;
>   -                                       threads_data[i].flags |= PARALLEL;
>   +                                       threads_data[i].flags |= *data_flags;
>                                           threads_data[i].go = &go;
>                                           ++n_threads;
>                                           pthread_create(&threads_data[i].thread, 0,
>                                                          thread, &threads_data[i]);
>                                           ++i;
>   +                                       data_flags++;
>                                   }
>                           }
>           }
> 
> > 
> > > you add an overview to its documentation? The boilerplate doc added
> > > there after the fact looks less than helpful.
> > > 
> > 
> > What kinda of documentation were you thinking of? Most all the IGTs are
> > light on doc but updating the doc for ones I wrote is on my todo list...
> 
> It would be good to have an overview on what the test is checking and
> how the different subtests are different from each other.
> 
> thanks
> Lucas De Marchi

Ok let me see if I can write something up.

Matt

> 
> > 
> > Matt
> > 
> > > Lucas De Marchi
> > > 
> > > > ---
> > > > tests/intel/xe_exec_threads.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/tests/intel/xe_exec_threads.c b/tests/intel/xe_exec_threads.c
> > > > index 55907e2b3..977e8c600 100644
> > > > --- a/tests/intel/xe_exec_threads.c
> > > > +++ b/tests/intel/xe_exec_threads.c
> > > > @@ -1081,6 +1081,7 @@ static void threads(int fd, int flags)
> > > > 						threads_data[i].fd = 0;
> > > > 					else
> > > > 						threads_data[i].fd = fd;
> > > > +					threads_data[i].gt = gt;
> > > > 					threads_data[i].vm_legacy_mode =
> > > > 						vm_legacy_mode;
> > > > 					threads_data[i].class = class;
> > > > --
> > > > 2.43.0
> > > >