[2/2] drm/i915: fix the racy object accounting

Commit Message

Daniel Vetter July 24, 2013, 1:49 p.m. UTC

Use an atomic_t for the count (since that is useful for leak detection
in tests) and just rip out the object memory check.

v2: Rebase onto the new object create refcount fix patch.

Cc: Chris Wilson <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  5 ++---
 drivers/gpu/drm/i915/i915_drv.h     |  5 ++---
 drivers/gpu/drm/i915/i915_gem.c     | 21 ++++-----------------
 3 files changed, 8 insertions(+), 23 deletions(-)

Comments

Chris Wilson July 24, 2013, 5:09 p.m. UTC | #1

On Wed, Jul 24, 2013 at 03:49:43PM +0200, Daniel Vetter wrote:
> Use an atomic_t for the count (since that is useful for leak detection
> in tests) and just rip out the object memory check.
> 
> v2: Rebase onto the new object create refcount fix patch.
> 
> Cc: Chris Wilson <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>

I will miss that summary of total allocation bytes, and would actually
like to have the information provided by the patch you reverted earlier.
-Chris

Daniel Vetter July 24, 2013, 9:30 p.m. UTC | #2

On Wed, Jul 24, 2013 at 06:09:14PM +0100, Chris Wilson wrote:
> On Wed, Jul 24, 2013 at 03:49:43PM +0200, Daniel Vetter wrote:
> > Use an atomic_t for the count (since that is useful for leak detection
> > in tests) and just rip out the object memory check.
> > 
> > v2: Rebase onto the new object create refcount fix patch.
> > 
> > Cc: Chris Wilson <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> 
> I will miss that summary of total allocation bytes, and would actually
> like to have the information provided by the patch you reverted earlier.

We already dump both the bound and unbound list, and imo adding those up
and claiming is everything is too misleading. I guess I could do a
spinlock around the object_memory instead, but since I only cared about
the object count I didn't bother.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9d871c7..67d6699 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -228,9 +228,8 @@  static int i915_gem_object_info(struct seq_file *m, void *data)
 	if (ret)
 		return ret;
 
-	seq_printf(m, "%u objects, %zu bytes\n",
-		   dev_priv->mm.object_count,
-		   dev_priv->mm.object_memory);
+	seq_printf(m, "%u objects\n",
+		   atomic_read(&dev_priv->mm.object_count));
 
 	size = count = mappable_size = mappable_count = 0;
 	count_objects(&dev_priv->mm.bound_list, global_list);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e295561..e5cc6ed 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -925,9 +925,8 @@  struct i915_gem_mm {
 	/* storage for physical objects */
 	struct drm_i915_gem_phys_object *phys_objs[I915_MAX_PHYS_OBJECT];
 
-	/* accounting, useful for userland debugging */
-	size_t object_memory;
-	u32 object_count;
+	/* accounting, useful for leak detection in tests */
+	atomic_t object_count;
 };
 
 struct drm_i915_error_state_buf {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dc7e6de..c271c1a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -71,21 +71,6 @@  static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj)
 	obj->fence_reg = I915_FENCE_REG_NONE;
 }
 
-/* some bookkeeping */
-static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
-				  size_t size)
-{
-	dev_priv->mm.object_count++;
-	dev_priv->mm.object_memory += size;
-}
-
-static void i915_gem_info_remove_obj(struct drm_i915_private *dev_priv,
-				     size_t size)
-{
-	dev_priv->mm.object_count--;
-	dev_priv->mm.object_memory -= size;
-}
-
 static int
 i915_gem_wait_for_error(struct i915_gpu_error *error)
 {
@@ -3861,6 +3846,8 @@  unlock:
 void i915_gem_object_init(struct drm_i915_gem_object *obj,
 			  const struct drm_i915_gem_object_ops *ops)
 {
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+
 	INIT_LIST_HEAD(&obj->mm_list);
 	INIT_LIST_HEAD(&obj->global_list);
 	INIT_LIST_HEAD(&obj->ring_list);
@@ -3874,7 +3861,7 @@  void i915_gem_object_init(struct drm_i915_gem_object *obj,
 	/* Avoid an unnecessary call to unbind on the first bind. */
 	obj->map_and_fenceable = true;
 
-	i915_gem_info_add_obj(obj->base.dev->dev_private, obj->base.size);
+	atomic_inc(&dev_priv->mm.object_count);
 }
 
 static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
@@ -3982,7 +3969,7 @@  void i915_gem_free_object(struct drm_gem_object *gem_obj)
 		drm_prime_gem_destroy(&obj->base, NULL);
 
 	drm_gem_object_release(&obj->base);
-	i915_gem_info_remove_obj(dev_priv, obj->base.size);
+	atomic_dec(&dev_priv->mm.object_count);
 
 	kfree(obj->bit_17);
 	i915_gem_object_free(obj);