Re: [PATCH] drm/ttm: remove fence_lock

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

Hey,

Op 18-10-12 09:59, Thomas Hellstrom schreef:
>
>
>
> On 10/18/2012 09:28 AM, Thomas Hellstrom wrote:
>> Hi, Maarten,
>>
>> As you know I have been having my doubts about this change.
>> To me it seems insane to be forced to read the fence pointer under a
>> reserved lock, simply because when you take the reserve lock, another
>> process may have it and there is a substantial chance that that process
>> will also be waiting for idle while holding the reserve lock.

I think it makes perfect sense, the only times you want to read the fence
is when you want to change the members protected by the reservation.

>> In essence, to read the fence pointer, there is a large chance you will
>> be waiting for idle to be able to access it. That's why it's protected by
>> a separate spinlock in the first place.
>>
>> So even if this change might not affect current driver much it's a change
>> to the TTM api that leads to an IMHO very poor design.

I would argue the opposite, no longer having a separate lock, with clear
semantics when fencing is allowed, gives you a way to clean up the core
of ttm immensely.

There were only 2 functions affected by fence lock removal and they were
on buffer destruction, ttm_bo_cleanup_refs (and *_or_queue). But since
the *_or_queue can simply change order around, you only have to worry about
ttm_bo_cleanup_refs. This function is already ugly for other reasons, and
the followup patch I was suggesting cleaned up the ugliness there too.

The only thing done differently is backing off from the reservation early.
With the cleanup it won't even try to get the reservation again, since
nobody should set a new fence on the bo when it's dead. Instead all
destruction is moved until list refcount drops to 0.

> One way we could perhaps improve on this, if you think this is necessary, is to
> build a bit on the initial rcu suggestion, still require reservation when the fence pointer is modified,
> but to also use rcu_assign_pointer() for the fence pointer.

This is a massive overkill when the only time you read the fence pointer
without reservation is during buffer destruction. RCU is only good if
there's ~10x more reads than writes, and for us it's simply 50% reads 50%
writes..

> Anyone that wants quick access to the fence pointer would then use rcu_dereference_x(), but
> if the fence is indeed signaled, that caller needs to avoid setting the bo fence pointer to NULL.
>
> A check for bo idle without taking any (bo) locks would then mean imply reading the fence pointer
> in this fashion, and if it's non-NULL check whether the fence is signaled.

Sure it may look easier, but you add more overhead and complexity. I thought
you wanted to avoid overhead in the reservation path? RCU won't be the way
to do that.

~Maarten