From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: Memory eviction in ttm Date: Fri, 14 Sep 2012 15:36:55 +0200 Message-ID: <505332F7.9000101@canonical.com> References: <50508493.6060304@canonical.com> <50508E04.4090702@vmware.com> <5051F985.8020804@canonical.com> <50520CB9.8000607@vmware.com> <50521433.5070708@canonical.com> <50524966.5080805@vmware.com> <5052D8ED.7000206@canonical.com> <50530BE7.90700@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 2D71D9EEE2 for ; Fri, 14 Sep 2012 06:36:58 -0700 (PDT) In-Reply-To: <50530BE7.90700@vmware.com> 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: =?ISO-8859-1?Q?Thomas_Hellstr=F6m?= Cc: linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Op 14-09-12 12:50, Thomas Hellstr=F6m schreef: > Hi Maarten! > > Broadening the audience a bit.. > > On 9/14/12 9:12 AM, Maarten Lankhorst wrote: >> Op 13-09-12 23:00, Thomas Hellstrom schreef: >>> On 09/13/2012 07:13 PM, Maarten Lankhorst wrote: >>>> Hey >>>> >>>> Op 13-09-12 18:41, Thomas Hellstrom schreef: >>>>> On 09/13/2012 05:19 PM, Maarten Lankhorst wrote: >>>>>> Hey, >>>>>> >>>>>> Op 12-09-12 15:28, Thomas Hellstrom schreef: >>>>>>> On 09/12/2012 02:48 PM, Maarten Lankhorst wrote: >>>>>>>> Hey Thomas, >>>>>>>> >>>>>>>> I'm playing around with moving reservations from ttm to global, bu= t how ttm >>>>>>>> ttm is handling reservations is getting in the way. The code want= s to move >>>>>>>> the bo from the lru lock at the same time a reservation is made, b= ut that >>>>>>>> seems to be slightly too strict. It would really help me if that g= uarantee >>>>>>>> is removed. >>>>>>> Hi, Maarten. >>>>>>> >>>>>>> Removing that restriction is not really possible at the moment. >>>>>>> Also the memory accounting code depends on this, and may cause rese= rvations >>>>>>> in the most awkward places. Since these reservations don't have a t= icket >>>>>>> they may and will cause deadlocks. So in short the restriction is t= here >>>>>>> to avoid deadlocks caused by ticketless reservations. >>>>>> I have finished the lockdep annotations now which seems to catch alm= ost >>>>>> all abuse I threw at it, so I'm feeling slightly more confident abou= t moving >>>>>> the locking order and reservations around. >>>>> Maarten, moving reservations in TTM out of the lru lock is incorrect = as the code is >>>>> written now. If we want to move it out we need something for ticketle= ss reservations >>>>> >>>>> I've been thinking of having a global hash table of tickets with the = task struct pointer as the key, >>>>> but even then, we'd need to be able to handle EBUSY errors on every o= peration that might try to >>>>> reserve a buffer. >>>>> >>>>> The fact that lockdep doesn't complain isn't enough. There *will* be = deadlock use-cases when TTM is handed >>>>> the right data-set. >>>>> >>>>> Isn't there a way that a subsystem can register a callback to be perf= ormed to remove stuff from LRU and >>>>> to take a pre-reservation lock? >>>> What if multiple subsystems need those? You will end up with a deadloc= k again. >>>> >>>> I think it would be easier to change the code in ttm_bo.c to not assum= e the first >>>> item on the lru list is really the least recently used, and assume the= first item >>>> that can be reserved without blocking IS the least recently used inste= ad. >>> So what would happen then is that we'd spin on the first item on the LR= U list, since >>> when reserving we must release the LRU lock, and if reserving fails, we= thus >>> need to restart LRU traversal. Typically after a schedule(). That's bad. >>> >>> So let's take a step back and analyze why the LRU lock has become a pro= blem. >>> From what I can tell, it's because you want to use per-object lock whe= n reserving instead of a >>> global reservation lock (that TTM could use as the LRU lock). Is that c= orrect? >>> and in that case, in what situation do you envision such a global lock = being contended >>> to the extent that it hurts performance? >>> >>>>>> Lockdep WILL complain about trying to use multiple tickets, doing ti= cketed >>>>>> and unticketed blocking reservations mixed, etc. >>>>>> >>>>>> I want to remove the global fence_lock and make it a per buffer lock= , with some >>>>>> lockdep annotations it's perfectly legal to grab obj->fence_lock and= obj2->fence_lock >>>>>> if you have a reservation, but it should complain loudly about tryin= g to take 2 fence_locks >>>>>> at the same time without a reservation. >>>>> Yes, TTM was previously using per buffer fence locks, and that works = fine from a deadlock perspective, but >>>>> it hurts performance. Fencing 200 buffers in a command submission (go= ogle-earth for example) will mean >>>>> 198 unnecessary locks, each discarding the processor pipelines. Locki= ng is a *slow* operation, particularly >>>>> on systems with many processors, and I don't think it's a good idea t= o change that back, without analyzing >>>>> the performance impact. There are reasons people are writing stuff li= ke RCU to avoid locking... >>>> So why don't we simply use RCU for fence pointers and get rid of the f= ence locking? :D >>>> danvet originally suggested it as a joke but if you think about it, it= would make a lot of sense for this usecase. >>> I thought of that before, but the problem is you'd still need a spinloc= k to change the buffer's fence pointer, >>> even if reading it becomes quick. >> Actually, I changed lockdep annotations a bit to distinguish between the >> cases where ttm_bo_wait is called without reservation, and ttm_bo_wait >> is called with, as far as I can see there are only 2 places that do it w= ithout, >> at least if I converted my git tree properly.. >> >> http://cgit.freedesktop.org/~mlankhorst/linux/log/?h=3Dv10-wip >> >> First one is nouveau_bo_vma_del, this can be fixed easily. >> Second one is ttm_bo_cleanup_refs and ttm_bo_cleanup_refs_or_queue, >> if reservation is done first before ttm_bo_wait, the fence_lock could be >> dropped entirely by adding smb_mb() in reserve and unreserve, functional= ly >> there would be no difference. So if you can verify my lockdep annotation= s are >> correct in the most recent commit wrt what's using ttm_bo_wait without r= eservation >> we could remove the fence_lock entirely. >> >> ~Maarten > Being able to wait for buffer idle or get the fence pointer without reser= ving is a fundamental property of TTM. Reservation is a long-term lock. The= fence lock is a very short term lock. If I were to choose, I'd rather acce= pt per-object fence locks than removing this property, but see below. I was trying to say the ONLY place where this is used in ttm is in ttm_bo_c= leanup_refs(or *_or_queue). No driver depends on this, they all take a reservation first. As such fence= _lock can be removed if you change the order of the reservation and waiting around, making it behave th= e same as the rest of ttm. http://cgit.freedesktop.org/~mlankhorst/linux/commit/?h=3Dv10-wip&id=3Db879= ac51810408809d6424d26da4013171d1bc15 fixes up those 2 calls, then removes the fence_lock altogether. > Likewise, to be able to guarantee that a reserved object is not on any LR= U list is also an important property. Removing that property will, in addit= ion to the spin wait we've already discussed make understanding TTM locking= even more difficult, and I'd really like to avoid it. This guarantee will no longer be true if reservations for buffer objects ar= e going to be shared across devices, other users of the reservations may have their own requirem= ents. Why is this property important though? If it could be changed to 'likely un= reserved' instead of guaranteed not on the lru list the few places that rely on this = could be fixed to attempt a nonblocking reserve first. Is it just ttm_mem_evict_first / ttm_bo_swapout / ttm_bo_force_list_clean that would break down? If so those could do a simple iteration over the list to find the first unreserved buffer instead, or do the default thing those = functions do when the list is empty if they traversed the full list without finding = anything that could be reserved. > If this were a real performance problem we were trying to solve it would = be easier to motivate changes in this area, but if it's just trying to avoi= d a global reservation lock and a global fence lock that will rarely if eve= r see any contention, I can't see the point. On the contrary, having per-ob= ject locks will be very costly when reserving / fencing many objects. As me= ntioned before, in the fence lock case it's been tried and removed, so I'd = like to know the reasoning behind introducing it again, and in what situati= ons you think the global locks will be contended. Because I'm trying to keep things as simple as possible, and this whole res= ervation business is already giving me a headache so I would really like to avoid any special lock that interacts with reserv= ations in a complex way and the easiest way would be to instead accept that there reservation and your lru removal call may not = be atomic. I hope to eventually support eviction though, so some notification that the buffer was reserved succesfully is going to b= e likely, I just don't think it needs to be done atomically.