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: Thu, 19 Jun 2014 10:48:29 +0100 Message-ID: <53A2B1ED.7060107@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 1F82C6E85B for ; Thu, 19 Jun 2014 02:48:31 -0700 (PDT) In-Reply-To: <1403109376-23452-2-git-send-email-bradley.d.volkin@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: bradley.d.volkin@intel.com, intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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. Are object sizes generally page aligned or all you've seen all sizes in the distribution? Either way, I am thinking whether some sort of rounding up would be more efficient? Would it cause a problem if slightly larger object was returned? Given that objects are only ever added to the pool, once max number is allocated and there are no free ones of exact size it nags userspace with EAGAIN and retires objects. But I wonder if the above points could reduce that behaviour? Could we get away without tracking the given out objects in a list and just keep a list of available ones? In which case if object can only ever be either in the free pool or on one of the existing GEM active/inactive lists the same list head could be used? Could it use its own locking just as easily? Just thinking if the future goal is to fine grain locking, this seems self contained enough to be doable straight away unless I am missing something. The above would make the pool more generic, but then I read Chris's reply which perhaps suggests to make it more specialised so I don't know. Regards, Tvrtko