Re: [PATCH] drm: Replace kref with a simple atomic reference count

[Dave Airlie <airlied@redhat.com>]

On Thu, 2010-11-25 at 21:40 +0000, Chris Wilson wrote:
> For a deferred-free cache of unreferenced bound objects, a simple
> reference count is required without the baggage of kref.

eh?

you've just out of lined kref for no real gain.

the whole point of kref is that its standard and doesn't require
auditing for the people not inlining it.

The only place I can see the advantage is not taking the struct mutex in
the free path and really we should just fix the free function to take
the struct mutex if required and wrap the real free.

Dave.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_gem.c  |   12 +++---------
>  drivers/gpu/drm/drm_info.c |    2 +-
>  include/drm/drmP.h         |   16 +++++++++-------
>  3 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ea1c4b0..874e776 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -141,7 +141,7 @@ int drm_gem_object_init(struct drm_device *dev,
>  	if (IS_ERR(obj->filp))
>  		return -ENOMEM;
>  
> -	kref_init(&obj->refcount);
> +	atomic_set(&obj->refcount, 1);
>  	atomic_set(&obj->handle_count, 0);
>  	obj->size = size;
>  
> @@ -436,9 +436,8 @@ EXPORT_SYMBOL(drm_gem_object_release);
>   * Frees the object
>   */
>  void
> -drm_gem_object_free(struct kref *kref)
> +drm_gem_object_free(struct drm_gem_object *obj)
>  {
> -	struct drm_gem_object *obj = (struct drm_gem_object *) kref;
>  	struct drm_device *dev = obj->dev;
>  
>  	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> @@ -448,11 +447,6 @@ drm_gem_object_free(struct kref *kref)
>  }
>  EXPORT_SYMBOL(drm_gem_object_free);
>  
> -static void drm_gem_object_ref_bug(struct kref *list_kref)
> -{
> -	BUG();
> -}
> -
>  /**
>   * Called after the last handle to the object has been closed
>   *
> @@ -476,7 +470,7 @@ void drm_gem_object_handle_free(struct drm_gem_object *obj)
>  		*
>  		* This cannot be the last reference, since the handle holds one too.
>  		 */
> -		kref_put(&obj->refcount, drm_gem_object_ref_bug);
> +		atomic_dec(&obj->refcount);
>  	} else
>  		spin_unlock(&dev->object_name_lock);
>  
> diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
> index 3cdbaf3..4a80bd8 100644
> --- a/drivers/gpu/drm/drm_info.c
> +++ b/drivers/gpu/drm/drm_info.c
> @@ -256,7 +256,7 @@ int drm_gem_one_name_info(int id, void *ptr, void *data)
>  	seq_printf(m, "%6d %8zd %7d %8d\n",
>  		   obj->name, obj->size,
>  		   atomic_read(&obj->handle_count),
> -		   atomic_read(&obj->refcount.refcount));
> +		   atomic_read(&obj->refcount));
>  	return 0;
>  }
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 2b33980..f332c56 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -609,7 +609,7 @@ struct drm_gem_mm {
>   */
>  struct drm_gem_object {
>  	/** Reference count of this object */
> -	struct kref refcount;
> +	atomic_t refcount;
>  
>  	/** Handle count of this object. Each handle also holds a reference */
>  	atomic_t handle_count; /* number of handles on this object */
> @@ -1504,7 +1504,7 @@ extern void drm_sysfs_connector_remove(struct drm_connector *connector);
>  int drm_gem_init(struct drm_device *dev);
>  void drm_gem_destroy(struct drm_device *dev);
>  void drm_gem_object_release(struct drm_gem_object *obj);
> -void drm_gem_object_free(struct kref *kref);
> +void drm_gem_object_free(struct drm_gem_object *obj);
>  struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev,
>  					    size_t size);
>  int drm_gem_object_init(struct drm_device *dev,
> @@ -1519,23 +1519,25 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>  static inline void
>  drm_gem_object_reference(struct drm_gem_object *obj)
>  {
> -	kref_get(&obj->refcount);
> +	atomic_inc(&obj->refcount);
> +	smp_mb__after_atomic_inc();
>  }
>  
>  static inline void
>  drm_gem_object_unreference(struct drm_gem_object *obj)
>  {
> -	if (obj != NULL)
> -		kref_put(&obj->refcount, drm_gem_object_free);
> +	if (obj != NULL && atomic_dec_and_test(&obj->refcount))
> +		drm_gem_object_free(obj);
>  }
>  
>  static inline void
>  drm_gem_object_unreference_unlocked(struct drm_gem_object *obj)
>  {
> -	if (obj != NULL) {
> +	if (obj != NULL && atomic_dec_and_test(&obj->refcount)) {
>  		struct drm_device *dev = obj->dev;
>  		mutex_lock(&dev->struct_mutex);
> -		kref_put(&obj->refcount, drm_gem_object_free);
> +		if (atomic_read(&obj->refcount) == 0)
> +			drm_gem_object_free(obj);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
>  }