Re: [PATCH i-g-t] tests/intel/xe_exec_reset: Add a vm_unbind after the stress test completes

["Summers, Stuart" <stuart.summers@intel.com>]

On Thu, 2025-01-09 at 15:39 +0000, Summers, Stuart wrote:
> On Wed, 2025-01-08 at 23:38 +0000, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of
> > Stuart Summers
> > Sent: Wednesday, January 8, 2025 2:54 PM
> > Cc: igt-dev@lists.freedesktop.org; Summers, Stuart
> > <stuart.summers@intel.com>
> > Subject: [PATCH i-g-t] tests/intel/xe_exec_reset: Add a vm_unbind
> > after the stress test completes
> > > 
> > > The test is submitting workloads in a tight loop and then
> > > destroying the exec queue for each of these. There is a
> > > potential that the GuC IDs could get used up during these
> > > submissions and before a GT reset goes through. If that
> > > happens, the subsequent submissions will essentially be
> > > "lost" in that they aren't submitted to GuC and just wait
> > > for a submission timeout to happen, at which point we do
> > > a reset_async in the driver. What seems to be happening
> > > however is these workloads are lingering beyond the
> > > completion of the xe_exec_reset test. Then when the
> > > reset_async eventually goes through, the submissions
> > > from a next test can also hang (or possibly because
> > > they too ran into the guc_id exhaustion case).
> > > 
> > > Add an explicit vm_unbind before destroying the VM
> > > to give time for those submissions to complete
> > > before the test ends.
> > > 
> > > https://gitlab.freedesktop.org/drm/xe/kernel/issues/4015
> > 
> > I think this needs to be:
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/issues/4015
> 
> Ok I'll update. 
> 
> > But otherwise:
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> 
> Thanks!
> 
> > Well, assuming this works, at least.  I might be reading a bit too
> > far into the
> > commit message and kernel issue comment, but it seems you aren't
> > too
> > certain this will work?
> 
> Yeah before merging I need to make sure this fully fixes the issue :)
> This particular test was already passing so just need to review the
> logs and make sure the cleanup is consistently happening. I'll report
> back here with the details...

Ok I've done quite a bit of testing here over the past couple of days
and I can't reproduce this behavior on the latest without the IGT
change I have here. Let's drop this change for now. If this pops back
up we can revisit. Thanks for the look here Jonathan!

-Stuart

> 
> Thanks,
> Stuart
> 
> > -Jonathan Cavitt
> > 
> > > 
> > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > ---
> > >  tests/intel/xe_exec_reset.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/tests/intel/xe_exec_reset.c
> > > b/tests/intel/xe_exec_reset.c
> > > index a3eaf8bbf..ca5566d11 100644
> > > --- a/tests/intel/xe_exec_reset.c
> > > +++ b/tests/intel/xe_exec_reset.c
> > > @@ -645,6 +645,8 @@ static void submit_jobs(struct gt_thread_data
> > > *t)
> > >                 xe_exec_queue_destroy(fd, exec.exec_queue_id);
> > >         }
> > >  
> > > +       xe_vm_unbind_sync(fd, vm, 0, addr, bo_size);
> > > +
> > >         munmap(data, bo_size);
> > >         gem_close(fd, bo);
> > >         xe_vm_destroy(fd, vm);
> > > -- 
> > > 2.34.1
> > > 
> > > 
>