Op 28-11-12 20:21, Thomas Hellstrom schreef:
> On 11/28/2012 07:32 PM, Maarten Lankhorst wrote:
>> Op 28-11-12 16:10, Thomas Hellstrom schreef:
>>> On 11/28/2012 03:46 PM, Maarten Lankhorst wrote:
>>>> Op 28-11-12 15:23, Thomas Hellstrom schreef:
>>>>> On 11/28/2012 02:55 PM, Maarten Lankhorst wrote:
>>>>>> Op 28-11-12 14:21, Thomas Hellstrom schreef:
>>>>>>> On 11/28/2012 01:15 PM, Maarten Lankhorst wrote:
>>>>>>>> Op 28-11-12 12:54, Thomas Hellstrom schreef:
>>>>>>>>> On 11/28/2012 12:25 PM, Maarten Lankhorst wrote:
>>>>>>>>>> By removing the unlocking of lru and retaking it immediately, a race
>>>>>>>>>> is
>>>>>>>>>> removed where the bo is taken off the swap list or the lru list
>>>>>>>>>> between
>>>>>>>>>> the unlock and relock. As such the cleanup_refs code can be
>>>>>>>>>> simplified,
>>>>>>>>>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>>>>>>>>>> it will drop the locks and perform a blocking wait, or return an
>>>>>>>>>> error
>>>>>>>>>> if no_wait_gpu was set.
>>>>>>>>>>
>>>>>>>>>> The need for looping is also eliminated, since swapout and
>>>>>>>>>> evict_mem_first
>>>>>>>>>> will always follow the destruction path, so no new fence is allowed
>>>>>>>>>> to be attached. As far as I can see this may already have been the
>>>>>>>>>> case,
>>>>>>>>>> but the unlocking / relocking required a complicated loop to deal
>>>>>>>>>> with
>>>>>>>>>> re-reservation.
>>>>>>>>>>
>>>>>>>>>> The downside is that ttm_bo_cleanup_memtype_use is no longer called
>>>>>>>>>> with
>>>>>>>>>> reservation held, so drivers must be aware that move_notify with a
>>>>>>>>>> null
>>>>>>>>>> parameter doesn't require a reservation.
>>>>>>>>> Why can't we unreserve *after* ttm_bo_cleanup_memtype_use? That's not
>>>>>>>>> immediately clear from this patch.
>>>>>>>> Because we would hold the reservation while waiting and with the
>>>>>>>> object still
>>>>>>>> on swap and lru lists still, that would defeat the whole purpose of
>>>>>>>> keeping
>>>>>>>> the object on multiple lists, plus break current code that assumes
>>>>>>>> bo's on the
>>>>>>>> those lists can always be reserved.
>>>>>>>>
>>>>>>>> the if (ret && !no_wait_gpu) path has to drop the reservation and lru
>>>>>>>> lock, and
>>>>>>>> isn't guaranteed to be able to retake it. Maybe it could be guaranteed
>>>>>>>> now, but
>>>>>>>> I'm sure that would not be the case if the reservations were shared
>>>>>>>> across
>>>>>>>> devices.
>>>>>>> The evict path removes the BO from the LRU lists, drops the LRU lock
>>>>>>> but hangs on to the reservation,
>>>>>>> and in case the wait goes wrong, re-adds the bo to the LRU lists and
>>>>>>> returns an error.
>>>>>> If you really want to, we could hang on to the !no_wait_gpu path, wait
>>>>>> shouldn't ever fail there, so I suppose
>>>>>> leaving it off the lru lists and not re-add on any list in case of wait
>>>>>> fail is fine. It's still on the ddestroy list in that
>>>>>> case, so not adding it back to the other lists is harmless.
>>>>>>
>>>>> Well I'm a bit afraid that theoretically, other callers may have a bo
>>>>> reserved, while cleanup_refs_and_unlock
>>>>> more or less runs the whole destroy path on that buffer. Sure, we have
>>>>> control over those other reservers,
>>>>> but it may come back and bite us.
>>>> That's why initially I moved all the destruction to ttm_bo_release_list,
>>>> to have all destruction in
>>>> only 1 place. But even now it's serialized with the lru lock, while the
>>>> destruction may not happen
>>>> right away, it still happens before last list ref to the bo is dropped.
>>>>
>>>> But it's your call, just choose the approach you want and I'll resubmit
>>>> this. :-)
>>>>
>>>>> Also the wait might fail if a signal is hit, so it's definitely possible,
>>>>> and even likely in the case of the X server process.
>>>>>
>>>>> Anyway, I prefer if we could try to keep the reservation across the
>>>>> ttm_cleanup_memtype_use function, and as far
>>>>> as I can tell, the only thing preventing that is the reservation release
>>>>> in the (!no_wait_gpu) path. So if we alter that to
>>>>> do the same as the evict path I think without looking to deeply into the
>>>>> consequences that we should be safe.
>>>> I think returning success early without removing off ddestroy list if
>>>> re-reserving fails
>>>> with lru lock held would be better.
>>>>
>>>> We completed the wait and attempt to reserve the bo, which failed. Without
>>>> the lru
>>>> lock atomicity, this can't happen since the only places that would do it
>>>> call this with
>>>> the lru lock held.
>>>>
>>>> With the atomicity removal, the only place that could do this is
>>>> ttm_bo_delayed_delete
>>>> with remove_all set to true. And even if that happened the destruction
>>>> code would run
>>>> *anyway* since we completed the waiting part already, it would just not
>>>> necessarily be
>>>> run from this thread, but that guarantee didn't exist anyway.
>>>>> Then we should be able to skip patch 2 as well.
>>>> If my tryreserve approach sounds sane, second patch should still be
>>>> skippable. :-)
>>> Sure, Lets go for that approach.
>> Ok updated patch below, you only need to check if you like the approach or
>> not,
>> since I haven't tested it yet beyond compiling. Rest of series (minus patch
>> 2)
>> should still apply without modification.
>>
>> drm/ttm: call ttm_bo_cleanup_refs with reservation and lru lock held, v2
>> By removing the unlocking of lru and retaking it immediately, a
>> race is
>> removed where the bo is taken off the swap list or the lru list between
>> the unlock and relock. As such the cleanup_refs code can be simplified,
>> it will attempt to call ttm_bo_wait non-blockingly, and if it fails
>> it will drop the locks and perform a blocking wait, or return an error
>> if no_wait_gpu was set.
>> The need for looping is also eliminated, since swapout and
>> evict_mem_first
>> will always follow the destruction path, no new fence is allowed
>> to be attached. As far as I can see this may already have been the case,
>> but the unlocking / relocking required a complicated loop to deal with
>> re-reservation.
>> Changes since v1:
>> - Simplify no_wait_gpu case by folding it in with empty ddestroy.
>> - Hold a reservation while calling ttm_bo_cleanup_memtype_use again.
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at
>> canonical.com>
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 202fc20..e9f01fe 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -488,12 +488,16 @@ static void ttm_bo_cleanup_memtype_use(struct
>> ttm_buffer_object *bo)
>> ttm_bo_mem_put(bo, &bo->mem);
>> atomic_set(&bo->reserved, 0);
>> + wake_up_all(&bo->event_queue);
>> /*
>> - * Make processes trying to reserve really pick it up.
>> + * Since the final reference to this bo may not be dropped by
>> + * the current task we have to put a memory barrier here to make
>> + * sure the changes done in this function are always visible.
>> + *
>> + * This function only needs protection against the final kref_put.
>> */
>> - smp_mb__after_atomic_dec();
>> - wake_up_all(&bo->event_queue);
>> + smp_mb__before_atomic_dec();
>> }
>> static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>> @@ -543,68 +547,95 @@ static void ttm_bo_cleanup_refs_or_queue(struct
>> ttm_buffer_object *bo)
>> }
>> /**
>> - * function ttm_bo_cleanup_refs
>> + * function ttm_bo_cleanup_refs_and_unlock
>> * If bo idle, remove from delayed- and lru lists, and unref.
>> * If not idle, do nothing.
>> *
>> + * Must be called with lru_lock and reservation held, this function
>> + * will drop both before returning.
>> + *
>> * @interruptible Any sleeps should occur interruptibly.
>> - * @no_wait_reserve Never wait for reserve. Return -EBUSY instead.
>> * @no_wait_gpu Never wait for gpu. Return -EBUSY instead.
>> */
>> -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
>> - bool interruptible,
>> - bool no_wait_reserve,
>> - bool no_wait_gpu)
>> +static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
>> + bool interruptible,
>> + bool no_wait_gpu)
>> {
>> struct ttm_bo_device *bdev = bo->bdev;
>> + struct ttm_bo_driver *driver = bdev->driver;
>> struct ttm_bo_global *glob = bo->glob;
>> int put_count;
>> - int ret = 0;
>> + int ret;
>> -retry:
>> spin_lock(&bdev->fence_lock);
>> - ret = ttm_bo_wait(bo, false, interruptible, no_wait_gpu);
>> - spin_unlock(&bdev->fence_lock);
>> + ret = ttm_bo_wait(bo, false, false, true);
>> - if (unlikely(ret != 0))
>> - return ret;
>> + if (ret && !no_wait_gpu) {
>> + void *sync_obj;
>> -retry_reserve:
>> - spin_lock(&glob->lru_lock);
>> + /*
>> + * Take a reference to the fence and unreserve,
>> + * at this point the buffer should be dead, so
>> + * no new sync objects can be attached.
>> + */
>> + sync_obj = driver->sync_obj_ref(&bo->sync_obj);
>> + spin_unlock(&bdev->fence_lock);
>> - if (unlikely(list_empty(&bo->ddestroy))) {
>> + put_count = ttm_bo_del_from_lru(bo);
>> +
>> + atomic_set(&bo->reserved, 0);
>> + wake_up_all(&bo->event_queue);
>> spin_unlock(&glob->lru_lock);
>> - return 0;
>> - }
>> - ret = ttm_bo_reserve_locked(bo, false, true, false, 0);
>> + ttm_bo_list_ref_sub(bo, put_count, true);
>> - if (unlikely(ret == -EBUSY)) {
>> - spin_unlock(&glob->lru_lock);
>> - if (likely(!no_wait_reserve))
>> - ret = ttm_bo_wait_unreserved(bo, interruptible);
>> - if (unlikely(ret != 0))
>> + ret = driver->sync_obj_wait(sync_obj, false, interruptible);
>> + driver->sync_obj_unref(&sync_obj);
>> + if (ret) {
>> + /*
>> + * Either the wait returned -ERESTARTSYS, or -EDEADLK
>> + * (radeon lockup) here. No effort is made to re-add
>> + * this bo to any lru list. Instead the bo only
>> + * appears on the delayed destroy list.
>> + */
>> return ret;
>> + }
> Actually, we *must* re-add the bo to LRU lists here, because otherwise when a
> driver needs
> to evict a memory type completely, there's a large chance it will miss this
> bo.
>
> So I think either we need to keep the reservation, or keep the bo on the LRU
> lists.
The second option is what v1 did, except I never bothered to re-take the
reservation. ;-)
It shouldn't cause troubles to leave it on the lru lists if we drop the the
reservation,
we can keep handling re-reservation failure in the same way as in v2.
In that case would v3 be the same as v2 of this patch, except with those 2
lines from the
ret && !no_wait_gpu branch removed:
put_count = ttm_bo_del_from_lru(bo);
ttm_bo_list_ref_sub(bo, put_count, true);
And of course the comment after sync_obj_wait failure would no longer apply.
~Maarten
