Message-ID: <cdb528b1-b53f-bff6-fdf9-260ee4bd897a@redhat.com>
Date:   Wed, 4 Oct 2023 20:24:20 +0200
From:   Danilo Krummrich <dakr@...hat.com>
To:     Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>, airlied@...il.com,
        daniel@...ll.ch, matthew.brost@...el.com, sarah.walker@...tec.com,
        donald.robson@...tec.com, boris.brezillon@...labora.com,
        christian.koenig@....com, faith@...strand.net
Cc:     dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH drm-misc-next v5 4/6] drm/gpuvm: track/lock/validate
 external/evicted objects



On 10/4/23 19:57, Thomas Hellström wrote:
> On Wed, 2023-10-04 at 19:17 +0200, Danilo Krummrich wrote:
>> On 10/4/23 17:29, Thomas Hellström wrote:
>>>
>>> On Wed, 2023-10-04 at 14:57 +0200, Danilo Krummrich wrote:
>>>> 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.
>>>
>>> But translated into how the data (the list in this case) is
>>> protected
>>> it becomes
>>>
>>> "Either the spinlock and the bo resv of a single list item OR the
>>> bo
>>> resvs of all bos that can potentially be on the list",
>>>
>>> while this is certainly possible to assert, any new / future code
>>> that
>>> manipulates the evict list will probably get this wrong and as a
>>> result
>>> the code becomes pretty fragile. I think drm_gpuvm_bo_destroy()
>>> already
>>> gets it wrong in that it, while holding a single resv, doesn't take
>>> the
>>> spinlock.
>>
>> That's true and I don't like it either. Unfortunately, with the dma-
>> resv
>> locking scheme we can't really protect the evict list without the
>> drm_gpuvm_bo::evicted trick properly.
>>
>> But as pointed out in my other reply, I'm a bit worried about the
>> drm_gpuvm_bo::evicted trick being too restrictive, but maybe it's
>> fine
>> doing it in the RESV_PROTECTED case.
> 
> Ah, indeed. I misread that as discussing the current code rather than
> the drm_gpuvm_bo::evicted trick. If validating only a subset, or a
> range, then with the drm_gpuvm_bo::evicted trick would be valid only
> for that subset.
> 
> But the current code would break because the condition of locking "the
> resvs of all bos that can potentially be on the list" doesn't hold
> anymore, and you'd get list corruption.
> 
> What *would* work, though, is the solution currently in xe, The
> original evict list, and a staging evict list whose items are copied
> over on validation. The staging evict list being protected by the
> spinlock, the original evict list by the resv, and they'd use separate
> list heads in the drm_gpuvm_bo, but that is yet another complication.
> 
> But I think if this becomes an issue, those VMs (perhaps OpenGL UMD
> VMs) only wanting to validate a subset, would simply initially rely on
> the current non-RESV solution. It looks like it's only a matter of
> flipping the flag on a per-vm basis.

If such a driver locks a range it can also just validate all locked
objects I guess.

And for everything else, we still have the spinlock protected variant,
where drivers can freely move things around by just taking the spinlock.

I think I will go ahead and add drm_gpuvm_bo::evicted, plus the helpers
I mentioned.

> 
> /Thomas
> 
> 
>>
>>>
>>> So I think that needs fixing, and if keeping that protection I
>>> think it
>>> needs to be documented with the list member and ideally an assert.
>>> But
>>> also note that lockdep_assert_held will typically give false true
>>> for
>>> dma_resv locks; as long as the first dma_resv lock locked in a
>>> drm_exec
>>> sequence  remains locked, lockdep thinks *all* dma_resv locks are
>>> held.
>>> (or something along those lines), so the resv lockdep asserts are
>>> currently pretty useless.
>>>
>>> /Thomas
>>>
>>>
>>>
>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Thomas
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

Powered by blists - more mailing lists