From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Volkin, Bradley D" Subject: Re: [RFC 1/4] drm/i915: Implement a framework for batch buffer pools Date: Fri, 20 Jun 2014 08:30:39 -0700 Message-ID: <20140620153039.GA24124@bdvolkin-ubuntu-desktop> References: <1403109376-23452-1-git-send-email-bradley.d.volkin@intel.com> <1403109376-23452-2-git-send-email-bradley.d.volkin@intel.com> <53A2B1ED.7060107@linux.intel.com> <20140619173544.GA16660@bdvolkin-ubuntu-desktop> <53A43664.4060806@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 493D86E1E4 for ; Fri, 20 Jun 2014 08:29:46 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53A43664.4060806@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Tvrtko Ursulin Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Fri, Jun 20, 2014 at 06:25:56AM -0700, Tvrtko Ursulin wrote: > > On 06/19/2014 06:35 PM, Volkin, Bradley D wrote: > > On Thu, Jun 19, 2014 at 02:48:29AM -0700, Tvrtko Ursulin wrote: > >> > >> Hi Brad, > >> > >> On 06/18/2014 05:36 PM, bradley.d.volkin@intel.com wrote: > >>> From: Brad Volkin > >>> > >>> This adds a small module for managing a pool of batch buffers. > >>> The only current use case is for the command parser, as described > >>> in the kerneldoc in the patch. The code is simple, but separating > >>> it out makes it easier to change the underlying algorithms and to > >>> extend to future use cases should they arise. > >>> > >>> The interface is simple: alloc to create an empty pool, free to > >>> clean it up; get to obtain a new buffer, put to return it to the > >>> pool. Note that all buffers must be returned to the pool before > >>> freeing it. > >>> > >>> The pool has a maximum number of buffers allowed due to some tests > >>> (e.g. gem_exec_nop) creating a very large number of buffers (e.g. > >>> ___). Buffers are purgeable while in the pool, but not explicitly > >>> truncated in order to avoid overhead during execbuf. > >>> > >>> Locking is currently based on the caller holding the struct_mutex. > >>> We already do that in the places where we will use the batch pool > >>> for the command parser. > >>> > >>> Signed-off-by: Brad Volkin > >>> --- > >>> > >>> r.e. pool capacity > >>> My original testing showed something like thousands of buffers in > >>> the pool after a gem_exec_nop run. But when I reran with the max > >>> check disabled just now to get an actual number for the commit > >>> message, the number was more like 130. I developed and tested the > >>> changes incrementally, and suspect that the original run was before > >>> I implemented the actual copy operation. So I'm inclined to remove > >>> or at least increase the cap in the final version. Thoughts? > >> > >> Some random thoughts: > >> > >> Is it strictly necessary to cap the pool size? I ask because it seems to > >> be introducing a limit where so far there wasn't an explicit one. > > > > No, I only added it because there were a huge number of buffers in the > > pool at one point. But that seems to have been an artifact of my > > development process, so unless someone says they really want to keep > > the cap, I'm going to drop it in the next rev. > > Cap or no cap (I am for no cap), but the pool is still "grow only" at > the moment, no? So one allocation storm and objects on the pool inactive > list end up wasting memory forever. Oh, so what happens is that when you put() an object back in the pool, we set obj->madv = I915_MADV_DONTNEED, which should tell the shrinker that it can drop the backing storage for the object if we need space. When you get() an object, we set obj->madv = I915_MADV_WILLNEED and get new backing pages. So the number of objects grows (capped or not), but the memory used can be controlled. Brad > > Unless my novice eyes are missing something hidden? But it can't be > since then there would have to be a mechanism letting the pool know that > some objects got expired. > > Regards, > > Tvrtko > >