Re: [Intel-gfx] [PATCH 15/26] drm/i915: Create page table allocators
On Tue, Mar 18, 2014 at 09:14:09AM +, 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/26] drm/i915: Create page table allocators
On Sat, Mar 22, 2014 at 01:21:39PM -0700, Ben Widawsky wrote: On Tue, Mar 18, 2014 at 09:14:09AM +, 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/26] drm/i915: Create page table allocators
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? Otherwise, patch does as you say. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
