Re: [v2 31/31] drm/xe/svm: Migration from sram to vram for system allocator

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

On Fri, Jun 07, 2024 at 06:18:57PM +0000, Matthew Brost wrote:
> On Fri, Jun 07, 2024 at 11:22:42AM -0600, Zeng, Oak wrote:
> > Hi Matt,
> > 
> > > -----Original Message-----
> > > From: Brost, Matthew <matthew.brost@intel.com>
> > > Sent: Wednesday, April 10, 2024 10:55 PM
> > > To: Zeng, Oak <oak.zeng@intel.com>
> > > Cc: intel-xe@lists.freedesktop.org; Ghimiray, Himal Prasad
> > > <himal.prasad.ghimiray@intel.com>; Bommu, Krishnaiah
> > > <krishnaiah.bommu@intel.com>; Thomas.Hellstrom@linux.intel.com; Welty,
> > > Brian <brian.welty@intel.com>
> > > Subject: Re: [v2 31/31] drm/xe/svm: Migration from sram to vram for system
> > > allocator
> > > 
> > > On Tue, Apr 09, 2024 at 04:17:42PM -0400, Oak Zeng wrote:
> > > > If applicable, migrate a vma from sram to vram for system allocator.
> > > > Traditional userptr is not migrated. Only userptr created during
> > > > fault (aka userptr splitted from system allocator vma) can be
> > > > migrated.
> > > >
> > > > FIXME: The migration should be conditional on user memory attributes
> > > > setting. Add this logic when memory attributes are supported
> > > >
> > > > Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_gt_pagefault.c | 9 ++++++++-
> > > >  drivers/gpu/drm/xe/xe_vm.c           | 4 ----
> > > >  2 files changed, 8 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > index 668984f0769e..c6ba00049964 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > > > @@ -20,6 +20,7 @@
> > > >  #include "xe_guc_ct.h"
> > > >  #include "xe_migrate.h"
> > > >  #include "xe_trace.h"
> > > > +#include "xe_svm.h"
> > > >  #include "xe_vm.h"
> > > >
> > > >  struct pagefault {
> > > > @@ -209,12 +210,18 @@ static int handle_pagefault(struct xe_gt *gt,
> > > struct pagefault *pf)
> > > >
> > > >  	if (xe_vma_is_userptr(vma) && write_locked) {
> > > >  		struct xe_userptr_vma *uvma = to_userptr_vma(vma);
> > > > +		struct xe_userptr *userptr = &uvma->userptr;
> > > >
> > > >  		spin_lock(&vm->userptr.invalidated_lock);
> > > > -		list_del_init(&uvma->userptr.invalidate_link);
> > > > +		list_del_init(&userptr->invalidate_link);
> > > >  		spin_unlock(&vm->userptr.invalidated_lock);
> > > >
> > > > +		mmap_read_lock(userptr->notifier.mm);
> > > > +		/**FIXME: Add migration policy here*/
> > > > +		if (xe_vma_is_fault_userptr(vma))
> > > > +			xe_svm_migrate_vma_to_vram(vm, vma, tile);
> > > 
> > > Agree we need a policy here...
> > > 
> > > See my comments about locking in [1] thinking if we migrate we likely
> > > want to hold the mmap lock until at least the bind being issued to
> > > prevent races with the CPU fault handler, at least initially.
> > 
> > As explained in [1], I will continue to use mmap read lock for now. And the read lock only covers migration and userptr-pin-pages but not the bind. The bind correctness is guaranteed by a retry mechanism and a userptr's notifier_lock.
> > 
> 
> See my reply, I think to avoid races VRAM migration and grabbing VRAM
> pages via hmm_range_fault either need the write lock or a some core
> refactoring needs to be done to avoid races.
> 
> One more possibly wrt to these races, maybe we simply don't care and
> racing access between the CPU and GPU of shared memory and this is akin
> to a user fault (i.e. the user must synchronize access or the behavior
> is undefined possibly resulting in a segfault). If I recall correctly,
> my tests showed when these races occured either a memory corruption
> happened or the user got a segfault. 
> 
> FWIW, Cuda seems to have synchronize calls in their SVM documentation
> (e.g. pass a malloc address to a shader, execute shader, call
> synchronize, then access malloc address in user program). I can dig up
> this documentation if you like.
>

Sorry one more comment again. I haven't looked at the level0 spec for
SVM, we should also look at that and perhap spec it out as CPU and GPU
access of SVM memory is mutually exclusive and concurent access is
undefined.

Also missed a comment below.
 
> Agree binds certainly do not need the mmap lock.
> 
> Matt
> 
> > > 
> > > [1] https://patchwork.freedesktop.org/patch/588542/?series=132229&rev=1
> > > 
> > > >  		ret = xe_vma_userptr_pin_pages(uvma);
> > > > +		mmap_read_unlock(userptr->notifier.mm);
> > > >  		if (ret)
> > > >  			goto unlock_vm;
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > > index 498b36469d00..8a58fe144a02 100644
> > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > @@ -71,16 +71,12 @@ int xe_vma_userptr_pin_pages(struct
> > > xe_userptr_vma *uvma)
> > > >  	struct xe_vma *vma = &uvma->vma;
> > > >  	struct xe_vm *vm = xe_vma_vm(vma);
> > > >  	struct xe_device *xe = vm->xe;
> > > > -	struct xe_userptr *userptr;
> > > >  	int ret;
> > > >
> > > >  	lockdep_assert_held(&vm->lock);
> > > >  	xe_assert(xe, xe_vma_is_userptr(vma));
> > > >
> > > > -	userptr = &uvma->userptr;
> > > > -	mmap_read_lock(userptr->notifier.mm);
> > > >  	ret = xe_userptr_populate_range(uvma);
> > > > -	mmap_read_unlock(userptr->notifier.mm);
> > > 
> > > Now you won't have the lock here other callers of this function...
> > > Probably need to have locked / unlocked version or arguments here.
> > 
> > This is addressed in v3.
> > 

Misses this. Also to be clear, in you design Xe shouldn't touch the MMAP
lock either, that lock should be owned by the DRM layer.

Matt

> > Oak
> > 
> > > 
> > > Matt
> > > 
> > > >
> > > >  	return ret;
> > > >  }
> > > > --
> > > > 2.26.3
> > > >