Re: [PATCH 15/26] drm/i915: Create page table allocators

[Chris Wilson <chris@chris-wilson.co.uk>]

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