Re: [PATCH v3 3/4] drm/ttm: convert to unified vma offset manager

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

Op 18-07-13 22:54, David Herrmann schreef:
> Hi
>
> On Thu, Jul 18, 2013 at 1:24 PM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> On 07/18/2013 01:07 PM, David Herrmann wrote:
>>> Hi
>>>
>>> On Thu, Jul 18, 2013 at 10:53 AM, Thomas Hellstrom
>>> <thellstrom@vmware.com> wrote:
>>>> A quick look, but not a full review:
>>>>
>>>> Looks mostly good, but it looks like the TTM vm lock isn't needed at all
>>>> anymore (provided the vma offset manager is properly protected), since
>>>> kref_get_unless_zero() is used when a reference after lookup is taken.
>>>> (please see the kref_get_unless_zero documentation examples). When
>>>> drm_vma_offset_remove() is called, the kref value is unconditionally
>>>> zero,
>>>> and that should block any successful lookup.
>>> If we drop vm_lock without modifying TTM, this will not work. Even
>>> kref_get_unless_zero() needs some guarantee that the object is still
>>> valid. Assume this scenario:
>>>
>>> drm_vma_offset_lookup() returns a valid node, however, before we call
>>> kref_get_*(), another thread destroys the last reference to the bo so
>>> it gets kfree()d. kref_get_unless_zero() will thus reference memory
>>> which can be _anything_ and is not guarantee to stay 0.
>>> (Documentation/kref.txt mentions this, too, and I guess it was you who
>>> wrote that).
>>>
>>> I cannot see any locking around the mmap call that could guarantee
>>> this except vm_lock.
>>
>> You're right. My apologies. Parental leave has taken its toll.
>>
>>
>>>> Otherwise, if the vma offset manager always needs external locking to
>>>> make
>>>> lookup + referencing atomic, then perhaps locking should be completely
>>>> left to the caller?
>>> I would actually prefer to keep it in the VMA manager. It allows some
>>> fast-paths which otherwise would need special semantics for the caller
>>> (for instance see the access-management patches which are based on
>>> this patchset or the reduced locking during setup/release). We'd also
>>> require the caller to use rwsems for good performance, which is not
>>> the case for gem.
>>>
>>> So how about Daniel's proposal to add an _unlocked() version and
>>> provide _lock_lookup() and _unlock_lookup() helpers which just wrap
>>> the read_lock() and read_unlock?
>>>
>>> Thanks!
>>> David
>>
>> I think that if there are good reasons to keep locking internal, I'm fine
>> with that, (And also, of course, with
>> Daniel's proposal). Currently the add / remove / lookup paths are mostly
>> used by TTM during object creation and
>> destruction.
>>
>> However, if the lookup path is ever used by pread / pwrite, that situation
>> might change and we would then like to
>> minimize the locking.
> I tried to keep the change as minimal as I could. Follow-up patches
> are welcome. I just thought pushing the lock into drm_vma_* would
> simplify things. If there are benchmarks that prove me wrong, I'll
> gladly spend some time optimizing that.
>
> Apart from this, I'd like to see someone ack the
> ttm_buffer_object_transfer() changes. I am not very confident with
> that. Everything else should be trivial.
>
The transfer object only exists to kill off the memory backing during asynchronous eviction,
by using the delayed destroyed mechanism. The re-initialization there looks correct to me.

~Maarten