[PATCH 04/10] drm/ttm: change fence_lock to inner lock, v3

[Maarten Lankhorst]

Op 20-11-12 12:33, Maarten Lankhorst schreef:
> Op 20-11-12 08:48, Thomas Hellstrom schreef:
>> On 11/19/2012 04:33 PM, Maarten Lankhorst wrote:
>>> Op 19-11-12 16:04, Thomas Hellstrom schreef:
>>>> On 11/19/2012 03:17 PM, Thomas Hellstrom wrote:
>>>>> Hi,
>>>>>
>>>>> This patch looks mostly good, although I think ttm_bo_cleanup_refs becomes overly complicated:
>>>>> Could this do, or am I missing something?
>>>>>
>>>> Actually, my version is bad, because ttm_bo_wait() is called with the lru lock held.
>>>>
>>>> /Thomas
>>> Oh digging through it made me remember why I had to release the reservation early and
>>> had to allow move_notify to be called without reservation.
>>>
>>> Fortunately move_notify has a NULL parameter, which is the only time that happens,
>>> so you can still check do BUG_ON(mem != NULL && !ttm_bo_reserved(bo)); in your
>>> move_notify handler.
>>>
>>> 05/10 removed the loop and assumed no new fence could be attached after the driver has
>>> declared the bo dead.
>>>
>>> However, at that point it may no longer hold a reservation to confirm this, that's why
>>> I moved the cleanup to be done in the release_list handler. It could still be done in
>>> ttm_bo_release, but we no longer have a reservation after we waited. Getting
>>> a reservation can fail if the bo is imported for example.
>>>
>>> While it would be true that in that case a new fence may be attached as well, that
>>> would be less harmful since that operation wouldn't involve this device, so the
>>> ttm bo can still be removed in that case. When that time comes I should probably
>>> fix up that WARN_ON(ret) in ttm_bo_cleanup_refs. :-)
>>>
>>> I did add a WARN_ON(!atomic_read(&bo->kref.refcount)); to
>>> ttm_bo_reserve and ttm_eu_reserve_buffers to be sure nothing is done on the device
>>> itself. If that is too paranoid, those WARN_ON's could be dropped. I prefer to leave them
>>> in for a kernel release or 2. But according to the rules that would be the only time you
>>> could attach a new fence and trigger the WARN_ON for now..
>> Hmm, I'd appreciate if you could group patches with functional changes that depend on eachother togeteher,
>> and "this is done because ...", which makes it much easier to review, (and to follow the commit history in case
>> something goes terribly wrong and we need to revert).
>>
>> Meanwhile I'll take a look at the final ttm_bo.c and see if I can spot any culprits.
>>
>> In general, as long as a bo is on a LRU list, we must be able to attach fences because of accelerated eviction.
> I thought it was deliberately designed in such a way that it was kept on the lru list,
> but since it's also on the ddestroy list it won't start accelerated eviction,
> since it branches into cleanup_refs early, and lru_lock still protects all the list entries.
>
> Of course any previous acceleration may still happen, but since we take a reservation first before waiting,
> we're already sure that any previous acceleration command has finished fencing, and no new one can
> start since it appears on the ddestroy list which would force it to perform the same wait.
>
> The wait is legal, and no new fences can be attached.
>
> I do agree all those patches probably needs a lot longer commit message to explain it though. :-)
>
Or maybe an alternative patch..

We could move the checks. There are only 2 places that are allowed to hold
reservations at that point right?

ttm_bo_swapout and evict_mem_first.

If cleanup_refs_or_queue fails because reservation fails, it must mean it's in one of those 2 places.
If it succeeds, we can remove it from the lru list and swap list, and if wait fails move it to ddestroy list.

unreserve in swapout doesn't add it back to any lists. No special handling needed there.
unreserve in evict_mem_first does, but we could take the lock before unreserve, and only
re-add it to the swap/lru list when it's not on ddestroy.

That way we wouldn't need to call ttm_bo_cleanup_refs from multiple places,
and the cleanup would only ever need to be done in the ttm_bo_delayed_delete without race.

I thought it was a feature that it still appeared on the lru list after death, so evict_mem_first could
wait on it, but if it's an annoyance it could be easily fixed like that.

But even if it's a feature to be preserved, evict_mem_first and swapout could be modified to check
the ddestroy list first for buffers to destroy. In that case those functions would explicitly prefer waiting for
destruction of bo's before queueing new work to swapout or evict bo's.

~Maarten