Thomas Hellström
2023-Oct-03 12:25 UTC
[Nouveau] [PATCH drm-misc-next v5 4/6] drm/gpuvm: track/lock/validate external/evicted objects
Hi, Boris, On Tue, 2023-10-03 at 12:05 +0200, Boris Brezillon wrote:> Hello Thomas, > > On Tue, 3 Oct 2023 10:36:10 +0200 > Thomas Hellstr?m <thomas.hellstrom at linux.intel.com> wrote: > > > > +/** > > > + * get_next_vm_bo_from_list() - get the next vm_bo element > > > + * @__gpuvm: The GPU VM > > > + * @__list_name: The name of the list we're iterating on > > > + * @__local_list: A pointer to the local list used to store > > > already iterated items > > > + * @__prev_vm_bo: The previous element we got from > > > drm_gpuvm_get_next_cached_vm_bo() > > > + * > > > + * This helper is here to provide lockless list iteration. > > > Lockless as in, the > > > + * iterator releases the lock immediately after picking the > > > first element from > > > + * the list, so list insertion deletion can happen concurrently. > > > + * > > > + * Elements popped from the original list are kept in a local > > > list, so removal > > > + * and is_empty checks can still happen while we're iterating > > > the list. > > > + */ > > > +#define get_next_vm_bo_from_list(__gpuvm, __list_name, > > > __local_list, __prev_vm_bo)?????\ > > > +???????({??????????????????????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????struct drm_gpuvm_bo *__vm_bo > > > NULL;????????????????????????????????????\ > > > +???????????????????????????????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????drm_gpuvm_bo_put(__prev_vm_bo);?????????????????? > > > ???????????????????????\ > > > +???????????????????????????????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????spin_lock(&(__gpuvm)- > > > >__list_name.lock);????????????????????????????????\? > > > > Here we unconditionally take the spinlocks while iterating, and the > > main > > point of DRM_GPUVM_RESV_PROTECTED was really to avoid that? > > > > > > > +???????????????if (!(__gpuvm)- > > > >__list_name.local_list)?????????????????????????????????\ > > > +???????????????????????(__gpuvm)->__list_name.local_list > > > __local_list;???????????????\ > > > +???????????????else????????????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????????????WARN_ON((__gpuvm)->__list_name.local_list > > > != __local_list);?????\ > > > +???????????????????????????????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????while (!list_empty(&(__gpuvm)->__list_name.list)) > > > {?????????????????????\ > > > +???????????????????????__vm_bo = list_first_entry(&(__gpuvm)- > > > >__list_name.list,????????\ > > > +????????????????????????????????????????????????? struct > > > drm_gpuvm_bo,?????????????????\ > > > +????????????????????????????????????????????????? > > > list.entry.__list_name);?????????????\ > > > +???????????????????????if (kref_get_unless_zero(&__vm_bo->kref)) > > > {? > > And unnecessarily grab a reference in the RESV_PROTECTED case. > > > ????????????????????????\ > > > +???????????????????????????????list_move_tail(&(__vm_bo)- > > > >list.entry.__list_name,??????\ > > > +????????????????????????????????????????????? > > > __local_list);???????????????????????????\ > > > +???????????????????????????????break;??????????????????????????? > > > ???????????????????????\ > > > +???????????????????????} else > > > {????????????????????????????????????????????????????????\ > > > +???????????????????????????????list_del_init(&(__vm_bo)- > > > >list.entry.__list_name);??????\ > > > +???????????????????????????????__vm_bo > > > NULL;?????????????????????????????????????????\ > > > +???????????????????????}???????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????}???????????????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????spin_unlock(&(__gpuvm)- > > > >__list_name.lock);??????????????????????????????\ > > > +???????????????????????????????????????????????????????????????? > > > ???????????????????????\ > > > +???????????????__vm_bo;????????????????????????????????????????? > > > ???????????????????????\ > > > +???????})? > > > > IMHO this lockless list iteration looks very complex and should be > > pretty difficult to maintain while moving forward, also since it > > pulls > > the gpuvm_bos off the list, list iteration needs to be protected by > > an > > outer lock anyway. > > As being partly responsible for this convoluted list iterator, I must > say I agree with you. There's so many ways this can go wrong if the > user doesn't call it the right way, or doesn't protect concurrent > list > iterations with a separate lock (luckily, this is a private > iterator). I > mean, it works, so there's certainly a way to get it right, but gosh, > this is so far from the simple API I had hoped for. > > > Also from what I understand from Boris, the extobj > > list would typically not need the fine-grained locking; only the > > evict > > list? > > Right, I'm adding the gpuvm_bo to extobj list in the ioctl path, when > the GEM and VM resvs are held, and I'm deferring the > drm_gpuvm_bo_put() > call to a work that's not in the dma-signalling path. This being > said, > I'm still not comfortable with the > > gem = drm_gem_object_get(vm_bo->gem); > dma_resv_lock(gem->resv); > drm_gpuvm_bo_put(vm_bo); > dma_resv_unlock(gem->resv); > drm_gem_object_put(gem); > > dance that's needed to avoid a UAF when the gpuvm_bo is the last GEM > owner, not to mention that drm_gpuva_unlink() calls > drm_gpuvm_bo_put() > after making sure the GEM gpuvm_list lock is held, but this lock > might > differ from the resv lock (custom locking so we can call > gpuvm_unlink() in the dma-signalling path). So we now have paths > where > drm_gpuvm_bo_put() are called with the resv lock held, and others > where > they are not, and that only works because we're relying on the the > fact > those drm_gpuvm_bo_put() calls won't make the refcount drop to zero, > because the deferred vm_bo_put() work still owns a vm_bo ref.I'm not sure I follow to 100% here, but in the code snippet above it's pretty clear to me that it needs to hold an explicit gem object reference when calling dma_resv_unlock(gem->resv). Each time you copy a referenced pointer (here from vm_bo->gem to gem) you need to up the refcount unless you make sure (by locks or other means) that the source of the copy has a strong refcount and stays alive, so that's no weird action to me. Could possibly add a drm_gpuvm_bo_get_gem() to access the gem member (and that also takes a refcount) for driver users to avoid the potential pitfall.> > All these tiny details add to the overall complexity of this common > layer, and to me, that's not any better than the > get_next_vm_bo_from_list() complexity you were complaining about > (might > be even worth, because this sort of things leak to users). > > Having an internal lock partly solves that, in that the locking of > the > extobj list is now entirely orthogonal to the GEM that's being > removed > from this list, and we can lock/unlock internally without forcing the > caller to take weird actions to make sure things don't explode. Don't > get me wrong, I get that this locking overhead is not acceptable for > Xe, but I feel like we're turning drm_gpuvm into a white elephant > that > only few people will get right.I tend to agree, but to me the big complication comes from the async (dma signalling path) state updates. Let's say for example we have a lower level lock for the gem object's gpuvm_bo list. Some drivers grab it from the dma fence signalling path, other drivers need to access all vm's of a bo to grab their dma_resv locks using a WW transaction. There will be problems, although probably solveable.> > This is just my personal view on this, and I certainly don't want to > block or delay the merging of this patchset, but I thought I'd share > my > concerns. As someone who's been following the evolution of this > drm_gpuva/vm series for weeks, and who's still sometimes getting > lost, > I can't imagine how new drm_gpuvm users would feel...> > > Also it seems that if we are to maintain two modes here, for > > reasonably clean code we'd need two separate instances of > > get_next_bo_from_list(). > > > > For the !RESV_PROTECTED case, perhaps one would want to consider > > the > > solution used currently in xe, where the VM maintains two evict > > lists. > > One protected by a spinlock and one protected by the VM resv. When > > the > > VM resv is locked to begin list traversal, the spinlock is locked > > *once* > > and the spinlock-protected list is looped over and copied into the > > resv > > protected one. For traversal, the resv protected one is used. > > Oh, so you do have the same sort of trick where you move the entire > list to another list, such that you can let other paths update the > list > while you're iterating your own snapshot. That's interesting...Yes, it's instead of the "evicted" bool suggested here. I thought the latter would be simpler. Although that remains to be seen after all use-cases are implemented. But in general I think the concept of copying from a staging list to another with different protection rather than traversing the first list and unlocking between items is a good way of solving the locking inversion problem with minimal overhead. We use it also for O(1) userptr validation. /Thomas> > Regards, > > Boris