Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <74d79ced-e811-bed9-6fb0-db694428c10f@redhat.com>
Date:   Wed, 4 Oct 2023 19:17:25 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH drm-misc-next v5 4/6] drm/gpuvm: track/lock/validate
 external/evicted objects
Content-Language: en-US
To:     =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= 
        <thomas.hellstrom@linux.intel.com>, airlied@gmail.com,
        daniel@ffwll.ch, matthew.brost@intel.com, sarah.walker@imgtec.com,
        donald.robson@imgtec.com, boris.brezillon@collabora.com,
        christian.koenig@amd.com, faith@gfxstrand.net
Cc:     dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20230928191624.13703-1-dakr@redhat.com>
 <20230928191624.13703-5-dakr@redhat.com>
 <e4e68970-c7c9-55e2-9483-01252f38c956@linux.intel.com>
 <6b16ab6f-b1a2-efdb-04bf-5af4c3de381b@linux.intel.com>
 <6489f31f-8929-3e59-fbef-a22049cccbe3@redhat.com>
 <36233651a7675ab894134e41fc711fdcc71eefec.camel@linux.intel.com>
From:   Danilo Krummrich <dakr@redhat.com>
Organization: RedHat
In-Reply-To: <36233651a7675ab894134e41fc711fdcc71eefec.camel@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

> 
> 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
>>>>
>>>>
>>>
>>
>