From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 15/26] drm/i915: Create page table allocators Date: Sat, 22 Mar 2014 21:10:03 +0000 Message-ID: <20140322211003.GE4366@nuc-i3427.alporthouse.com> References: <1395121738-29126-1-git-send-email-benjamin.widawsky@intel.com> <1395121738-29126-16-git-send-email-benjamin.widawsky@intel.com> <20140318091409.GF18530@nuc-i3427.alporthouse.com> <20140322202139.GD18765@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id CC9BA6E279 for ; Sat, 22 Mar 2014 14:10:06 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140322202139.GD18765@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ben Widawsky Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Sat, Mar 22, 2014 at 01:21:39PM -0700, Ben Widawsky wrote: > On Tue, Mar 18, 2014 at 09:14:09AM +0000, Chris Wilson wrote: > > On Mon, Mar 17, 2014 at 10:48:47PM -0700, Ben Widawsky wrote: > > > As we move toward dynamic page table allocation, it becomes much easier > > > to manage our data structures if break do things less coarsely by > > > breaking up all of our actions into individual tasks. This makes the > > > code easier to write, read, and verify. > > > > > > Aside from the dissection of the allocation functions, the patch > > > statically allocates the page table structures without a page directory. > > > This remains the same for all platforms, > > > > > > The patch itself should not have much functional difference. The primary > > > noticeable difference is the fact that page tables are no longer > > > allocated, but rather statically declared as part of the page directory. > > > This has non-zero overhead, but things gain non-trivial complexity as a > > > result. > > > > We increase overhead for increased complexity. What's the selling point > > of this patch then? > > I'd argue about the complexity. Personally, I think the result is easier > to read. > > I'll add this all to the commit message, but hopefully you agree: > > 1. Splitting out the functions allows easily combining GEN6 and GEN8 > code. Page tables have no difference based on GEN8. As we'll see in a > future patch when we add the dma mappings to the allocations, it > requires only one small change to make work, and error handling should > just fall into place. > > 2. Unless we always want to allocate all page tables under a given PDE, > we'll have to eventually break this up into an array of pointers (or > pointer to pointer). > > 3. Having the discrete functions is easier to review, and understand. > All allocations and frees now take place in just a couple of locations. > Reviewing, and cathcing leaks should be easy. > > 4. Less important: the gfp flags are confined to one location, which > makes playing around with such things trivial.o > > Hopefully you're more convinced after you went through more of the patch > series. Right, the patches and the resulting code look good. I just felt the changelog here was self-contradictory. And now we have a great spiel to include ;-) > If you want to try to optimize the overhead of managing the page tables, > I think this is a worthy thing to do (for instance, not statically > declaring the array). It takes a little more work though, and I'd prefer > to do it after the code is doing what it's supposed to do. Agreed. I'm still watching PT pages come and go with great joy, and not yet worrying about the impact. (Though I think I did notices some side-effect when a reap of the userspace cache lead to a sudden release of lots of pages, and a dip in throughput.) -Chris -- Chris Wilson, Intel Open Source Technology Centre