[PATCH 14/20] drm/radeon: multiple ring allocator v2

[Jerome Glisse]

On Tue, May 8, 2012 at 6:23 AM, Christian König <deathsimple at vodafone.de> wrote:
> On 07.05.2012 23:28, Jerome Glisse wrote:
>>
>> On Mon, May 7, 2012 at 4:38 PM, Christian König<deathsimple at vodafone.de>
>>  wrote:
>>>
>>> On 07.05.2012 20:52, Jerome Glisse wrote:
>>>>
>>>> On Mon, May 7, 2012 at 1:59 PM, Jerome Glisse<j.glisse at gmail.com>
>>>>  wrote:
>>>>>>
>>>>>> On 07.05.2012 17:23, Jerome Glisse wrote:
>>>>>>>
>>>>>>> Your patch here can enter in infinite loop and never return holding
>>>>>>> the lock. See below.
>>>>>>>
>>>>>>> [SNIP]
>>>>>>>
>>>>>>>> +               } while (radeon_sa_bo_next_hole(sa_manager,
>>>>>>>> fences));
>>>>>>>
>>>>>>> Here you can infinite loop, in the case there is a bunch of hole in
>>>>>>> the allocator but none of them allow to full fill the allocation.
>>>>>>> radeon_sa_bo_next_hole will keep returning true looping over and over
>>>>>>> on all the all. That's why i only restrict my patch to 2 hole
>>>>>>> skeeping
>>>>>>> and then fails the allocation or try to wait. I believe sadly we need
>>>>>>> an heuristic and 2 hole skeeping at most sounded like a good one.
>>>>>>
>>>>>> Nope, that can't be an infinite loop, cause radeon_sa_bo_next_hole in
>>>>>> conjunction with radeon_sa_bo_try_free are eating up the opportunities
>>>>>> for
>>>>>> holes.
>>>>>>
>>>>>> Look again, it probably will never loop more than RADEON_NUM_RINGS +
>>>>>> 1,
>>>>>> with
>>>>>> the exception for allocating in a complete scattered buffer, and even
>>>>>> then
>>>>>> it will never loop more often than halve the number of current
>>>>>> allocations
>>>>>> (and that is really really unlikely).
>>>>>>
>>>>>> Cheers,
>>>>>> Christian.
>>>>>
>>>>> I looked again and yes it can loop infinitly, think of hole you can
>>>>> never free ie radeon_sa_bo_try_free can't free anything. This
>>>>> situation can happen if you have several thread allocating sa bo at
>>>>> the same time while none of them are yet done with there sa_bo (ie
>>>>> none have call sa_bo_free yet). I updated a v3 that track oldest and
>>>>> fix all things you were pointing out above.
>>>
>>> No that isn't a problem, radeon_sa_bo_next_hole takes the firsts entries
>>> of
>>> the flist, so it only considers holes that have a signaled fence and so
>>> can
>>> be freed.
>>>
>>> Having multiple threads allocate objects that can't be freed yet will
>>> just
>>> result in empty flists, and so radeon_sa_bo_next_hole will return false,
>>> resulting in calling radeon_fence_wait_any with an empty fence list,
>>> which
>>> in turn will result in an ENOENT and abortion of allocation (ok maybe we
>>> should catch that and return -ENOMEM instead).
>>>
>>> So even the corner cases should now be handled fine.
>>
>> No, there is still infinite loop possible with gpu lockup, i am
>> against the while (next_hole) using for on 2 iteration looks a lot
>> better and it avoids sa allocator possibly looping too much (because
>> it can loop a lot more than RADEON_NUM_RINGS, the maximum number of
>> loop is sa_manager->size/4).
>
> I'm still pretty sure that there isn't the possibility for an infinite loop,
> so please explain further where exactly the problem is.
> radeon_sa_bo_next_hole will return true as long as it can find AND remove an
> allocation with an already signaled fence, and since nobody else can add
> allocations while we are in the loop we sooner or later run out of
> allocations and so the loop ends.

Yeah you right

> Also what the loop does is just cleaning up all the already signaled
> allocations, and it doesn't matter if there is one allocation or a million,
> we need to clean them up anyway. So aborting the loop and trying to wait for
> anything to be signaled makes no sense at all, and aborting the whole
> allocation at this point makes even less sense, cause that only delays the
> work that needs to be done anyway (freeing the allocations) to a later call
> to radeon_sa_bo_new.
>
> What could make sense is limiting how often we are waiting for some fences,
> since while waiting we release the lock and then other processes can jump in
> and grab what we wanted to have by waiting for something to happen.
>
> Cheers,
> Christian.

Still i don't want to loop more than necessary, it's bad, i am ok with :
http://people.freedesktop.org/~glisse/reset5/0001-drm-radeon-multiple-ring-allocator-v3.patch

If there is fence signaled it will retry 2 times at most, otherwise it
will go to wait and
that way better. Because with your while loop the worst case is
something proportional to
the manager size given it's 1Mo it can loop for a long long time.

Cheers,
Jerome