On Wed, Aug 28, 2024 at 08:50:02PM +0200, Daniel Vetter wrote:
> On Tue, Aug 27, 2024 at 07:48:38PM -0700, Matthew Brost wrote:
> > +int drm_gpusvm_migrate_to_sram(struct drm_gpusvm *gpusvm,
> > + struct drm_gpusvm_range *range,
> > + const struct drm_gpusvm_ctx *ctx)
> > +{
> > + u64 start = range->va.start, end = range->va.end;
> > + struct mm_struct *mm = gpusvm->mm;
> > + struct vm_area_struct *vas;
> > + int err;
> > + bool retry = false;
> > +
> > + if (!ctx->mmap_locked) {
> > + if (!mmget_not_zero(mm)) {
> > + err = -EFAULT;
> > + goto err_out;
> > + }
> > + if (ctx->trylock_mmap) {
> > + if (!mmap_read_trylock(mm)) {
> > + err = drm_gpusvm_evict_to_sram(gpusvm, range);
> > + goto err_mmput;
> > + }
> > + } else {
> > + mmap_read_lock(mm);
> > + }
> > + }
> > +
> > + mmap_assert_locked(mm);
> > +
> > + /*
> > + * Loop required to find all VMA area structs for the corner case when
> > + * VRAM backing has been partially unmapped from MM's address space.
> > + */
> > +again:
> > + vas = find_vma(mm, start);
>
> So a hiliarous case that amdkfd gets a bit better but still not entirely
> is that the original vma might entirely gone. Even when you can still get
> at the mm of that process. This happens with cow (or shared too I think)
> mappings in forked child processes, or also if you play fun mremap games.
>
> I think that outside of the ->migrate_to_ram callback migration/eviction
> to sram cannot assume there's any reasonable vma around and has to
> unconditionally go with the drm_gpusvm_evict_to_sram path.
>
See my response here [1]. Let me drop the whole trylock thing and
convert to an 'evict' flag which calls drm_gpusvm_evict_to_sram in
places where Xe needs to evict VRAM. Or maybe just export that function
and call it directly. That way the only place the VMA is looked up for
SRAM -> VRAM is upon CPU page fault.
[1] https://patchwork.freedesktop.org/patch/610955/?series=137870&rev=1#comment_1111164
> Also in the migrate_to_ram case the vma is essentially nothing else that
> informational about which ranges we might need if we prefault a bit (in
> case the child changed the vma compared to the original one). So it's good
> to as parameter for migrate_vma_setup, but absolutely nothing else.
>
> amdkfd almost gets this right by being entirely based on their svm_range
> structures, except they still have the lingering check that the orignal mm
> is still alive. Of course you cannot ever use that memory on the gpu
> anymore, but the child process could get very pissed if their memory is
> suddenly gone. Also the eviction code has the same issue as yours and
> limits itself to vma that still exist in the original mm, leaving anything
> that's orphaned in children or remaps stuck in vram. At least that's my
> understanding, I might very well be wrong.
>
> So probably want a bunch of these testcases too to make sure that all
> works, and we're not stuck with memory allocations in vram that we can't
> move out.
When writing some additional test cases, let me add hooks in my IGTs to
be able to verify we are not orphaning VRAM too.
Matt
> -Sima
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
