From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 14/20] drm/radeon: multiple ring allocator v2 Date: Tue, 08 May 2012 12:23:09 +0200 Message-ID: <4FA8F40D.5080400@vodafone.de> References: <1336390975-30945-1-git-send-email-deathsimple@vodafone.de> <1336390975-30945-15-git-send-email-deathsimple@vodafone.de> <4FA7FC26.7040504@vodafone.de> <4FA832D8.3090205@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 2622F9EB6F for ; Tue, 8 May 2012 03:23:12 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 07.05.2012 23:28, Jerome Glisse wrote: > On Mon, May 7, 2012 at 4:38 PM, Christian K=F6nig wrote: >> On 07.05.2012 20:52, Jerome Glisse wrote: >>> On Mon, May 7, 2012 at 1:59 PM, Jerome Glisse wr= ote: >>>>> 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 skeepi= ng >>>>>> 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 ju= st >> 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, whi= ch >> 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. 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.