From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [RFC 1/4] drm/i915: Implement a framework for batch buffer pools Date: Fri, 20 Jun 2014 16:41:08 +0100 Message-ID: <53A45614.3070406@linux.intel.com> 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> <20140620153039.GA24124@bdvolkin-ubuntu-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id C96F86E1F1 for ; Fri, 20 Jun 2014 08:41:28 -0700 (PDT) In-Reply-To: <20140620153039.GA24124@bdvolkin-ubuntu-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Volkin, Bradley D" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On 06/20/2014 04:30 PM, Volkin, Bradley D wrote: > 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. Education time for me I see. :) So the object is in pool->inactive_list _and_ in some other list so shrinker can find it? Thanks, Tvrtko