[v4,4/4] Try inlining sg_next()

Commit Message

Dave Gordon May 20, 2016, 12:31 a.m. UTC

Signed-off-by: Dave Gordon <[e-mail]>
Cc: Chris Wilson <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Chris Wilson May 20, 2016, 7:35 a.m. UTC | #1

On Fri, May 20, 2016 at 01:31:30AM +0100, Dave Gordon wrote:
> Signed-off-by: Dave Gordon <[e-mail]>
> Cc: Chris Wilson <[e-mail]>

Much better. The effect of the inline

         gem:exec:fault:1MiB:    +4.90%
  gem:exec:fault:1MiB:forked:    +7.99%
        gem:exec:fault:16MiB:   +22.94%
 gem:exec:fault:16MiB:forked:   +19.96%
       gem:exec:fault:256MiB:   +27.45%
gem:exec:fault:256MiB:forked:   +36.89%

And it brings this series into parity with mine.

----

Avoiding the out-of-line call to sg_next() reduces the kernel execution
overhead by 10% in some workloads (for example the Unreal Engine 4 demo
Atlantis on 2GiB GTTs) which are dominated by the cost of inserting PTEs
due to texture thrashing. We can demonstrate this in a microbenchmark
that forces us to rebind the object on every execbuf, where we can
measure a 25% improvement, in the time required to execute an execbuf
requiring a texture to be rebound, for inlining the sg_next() for large
texture sizes.

Benchmark: igt/benchmarks/gem_exec_fault
Benchmark: igt/benchmarks/gem_exec_trace/Atlantis
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01cde0f..53ff499 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2287,6 +2287,25 @@  __sgt_iter(struct scatterlist *sgl, bool dma)
 }
 
 /**
+ * __sg_next - return the next scatterlist entry in a list
+ * @sg:		The current sg entry
+ *
+ * Description:
+ *   If the entry is the last, return NULL; otherwise, step to the next
+ *   element in the array (@sg@+1). If that's a chain pointer, follow it;
+ *   otherwise just return the pointer to the current element.
+ **/
+static inline struct scatterlist *__sg_next(struct scatterlist *sg)
+{
+#ifdef CONFIG_DEBUG_SG
+	BUG_ON(sg->sg_magic != SG_MAGIC);
+#endif
+	return sg_is_last(sg) ? NULL :
+		likely(!sg_is_chain(++sg)) ? sg :
+		sg_chain_ptr(sg);
+}
+
+/**
  * for_each_sgt_dma - iterate over the DMA addresses of the given sg_table
  * @__dmap:	DMA address (output)
  * @__iter:	'struct sgt_iter' (iterator state, internal)
@@ -2296,7 +2315,7 @@  __sgt_iter(struct scatterlist *sgl, bool dma)
 	for ((__iter) = __sgt_iter((__sgt)->sgl, true);			\
 	     ((__dmap) = (__iter).ix.dma + (__iter).curr);		\
 	     (((__iter).curr += PAGE_SIZE) < (__iter).max) ||		\
-		((__iter) = __sgt_iter(sg_next((__iter).sgp), true), 0))
+		((__iter) = __sgt_iter(__sg_next((__iter).sgp), true), 0))
 
 /**
  * for_each_sgt_page - iterate over the pages of the given sg_table
@@ -2309,7 +2328,7 @@  __sgt_iter(struct scatterlist *sgl, bool dma)
 	     ((__pp) = (__iter).ix.pfn == 0 ? NULL :			\
 	           pfn_to_page((__iter).ix.pfn + ((__iter).curr >> PAGE_SHIFT)));\
 	     (((__iter).curr += PAGE_SIZE) < (__iter).max) ||		\
-		((__iter) = __sgt_iter(sg_next((__iter).sgp), false), 0))
+		((__iter) = __sgt_iter(__sg_next((__iter).sgp), false), 0))
 
 /**
  * Request queue structure.