Re: [PATCH drm-misc-next v5 4/6] drm/gpuvm: track/lock/validate external/evicted objects

[Danilo Krummrich]

On 10/3/23 11:11, Thomas Hellström wrote:

<snip>

> > > +
> > > +/**
> > > + * drm_gpuvm_bo_evict() - add / remove a &drm_gpuvm_bo to / from the &drm_gpuvms
> > > + * evicted list
> > > + * @vm_bo: the &drm_gpuvm_bo to add or remove
> > > + * @evict: indicates whether the object is evicted
> > > + *
> > > + * Adds a &drm_gpuvm_bo to or removes it from the &drm_gpuvms evicted list.
> > > + */
> > > +void
> > > +drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, bool evict)
> > > +{
> > > +    struct drm_gem_object *obj = vm_bo->obj;
> > > +
> > > +    dma_resv_assert_held(obj->resv);
> > > +
> > > +    /* Always lock list transactions, even if DRM_GPUVM_RESV_PROTECTED is
> > > +     * set. This is required to protect multiple concurrent calls to
> > > +     * drm_gpuvm_bo_evict() with BOs with different dma_resv.
> > > +     */
> >
> > This doesn't work. The RESV_PROTECTED case requires the evicted flag we discussed before. The list is either protected by the spinlock or the resv. Otherwise a list add could race with a list removal elsewhere.

I think it does unless I miss something, but it might be a bit subtle though.

Concurrent drm_gpuvm_bo_evict() are protected by the spinlock. Additionally, when
drm_gpuvm_bo_evict() is called we hold the dma-resv of the corresponding GEM object.

In drm_gpuvm_validate() I assert that we hold *all* dma-resv, which implies that no
one can call drm_gpuvm_bo_evict() on any of the VM's objects and no one can add a new
one and directly call drm_gpuvm_bo_evict() on it either.

> > Thanks,
> >
> > Thomas