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. 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. > > Otherwise, patch does as you say. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
