Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: convert i915_active.count from atomic_t to refcount_t

[Praveen Kumar <kumarpraveen@linux.microsoft.com>]

On 25-12-2022 13:17, Deepak R Varma wrote:
> The refcount_* APIs are designed to address known issues with the
> atomic_t APIs for reference counting. They provide following distinct
> advantages:
>    - protect the reference counters from overflow/underflow
>    - avoid use-after-free errors
>    - provide improved memory ordering guarantee schemes
>    - neater and safer.
> Hence, convert the atomic_t count member variable and associated
> atomic_*() API calls to equivalent refcount_t type and refcount_*() API
> calls.
> 
> This patch proposal address the following warnings generated by
> the atomic_as_refcounter.cocci coccinelle script
> 	atomic_add_unless
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> Please note:
>    1. Proposed changes are compile tested only.
>    2. This patch 1/2 is required to be applied before patch 2/2 due to
>       interdependency.
> 
> Changes in v2:
>    1. Patch added to the patch series.
>    2. Handle build issues Reported-by: kernel test robot <lkp@intel.com>
>       Earlier a standalone patch was sent for the i915 base driver only. The
>       Kernel Test Robot reported build failure for additional atomic_*() calls
>       specific to i915 debugging support when enabled. This version now includes
>       those changes as well.
> 
> 
>  drivers/gpu/drm/i915/i915_active.c       | 28 +++++++++++++-----------
>  drivers/gpu/drm/i915/i915_active.h       |  6 ++---
>  drivers/gpu/drm/i915/i915_active_types.h |  4 ++--
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 7412abf166a8..5e58d8b1e947 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -92,14 +92,14 @@ static void debug_active_init(struct i915_active *ref)
>  static void debug_active_activate(struct i915_active *ref)
>  {
>  	lockdep_assert_held(&ref->tree_lock);
> -	if (!atomic_read(&ref->count)) /* before the first inc */
> +	if (!refcount_read(&ref->count)) /* before the first inc */
>  		debug_object_activate(ref, &active_debug_desc);
>  }
> 
>  static void debug_active_deactivate(struct i915_active *ref)
>  {
>  	lockdep_assert_held(&ref->tree_lock);
> -	if (!atomic_read(&ref->count)) /* after the last dec */
> +	if (!refcount_read(&ref->count)) /* after the last dec */
>  		debug_object_deactivate(ref, &active_debug_desc);
>  }
> 
> @@ -133,7 +133,7 @@ __active_retire(struct i915_active *ref)
>  	GEM_BUG_ON(i915_active_is_idle(ref));
> 
>  	/* return the unused nodes to our slabcache -- flushing the allocator */
> -	if (!atomic_dec_and_lock_irqsave(&ref->count, &ref->tree_lock, flags))
> +	if (!refcount_dec_and_lock_irqsave(&ref->count, &ref->tree_lock, &flags))
>  		return;
> 
>  	GEM_BUG_ON(rcu_access_pointer(ref->excl.fence));
> @@ -179,8 +179,8 @@ active_work(struct work_struct *wrk)
>  {
>  	struct i915_active *ref = container_of(wrk, typeof(*ref), work);
> 
> -	GEM_BUG_ON(!atomic_read(&ref->count));
> -	if (atomic_add_unless(&ref->count, -1, 1))
> +	GEM_BUG_ON(!refcount_read(&ref->count));
> +	if (refcount_dec_not_one(&ref->count))

I'm not sure if this is correct here, I assume we should be adding instead here its decrementing ?

>  		return;
> 
>  	__active_retire(ref);
> @@ -189,8 +189,8 @@ active_work(struct work_struct *wrk)
>  static void
>  active_retire(struct i915_active *ref)
>  {
> -	GEM_BUG_ON(!atomic_read(&ref->count));
> -	if (atomic_add_unless(&ref->count, -1, 1))
> +	GEM_BUG_ON(!refcount_read(&ref->count));
> +	if (refcount_dec_not_one(&ref->count))
>  		return;
> 
>  	if (ref->flags & I915_ACTIVE_RETIRE_SLEEPS) {
> @@ -354,7 +354,7 @@ void __i915_active_init(struct i915_active *ref,
>  	ref->cache = NULL;
> 
>  	init_llist_head(&ref->preallocated_barriers);
> -	atomic_set(&ref->count, 0);
> +	refcount_set(&ref->count, 0);
>  	__mutex_init(&ref->mutex, "i915_active", mkey);
>  	__i915_active_fence_init(&ref->excl, NULL, excl_retire);
>  	INIT_WORK(&ref->work, active_work);
> @@ -445,7 +445,7 @@ int i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
> 
>  	if (replace_barrier(ref, active)) {
>  		RCU_INIT_POINTER(active->fence, NULL);
> -		atomic_dec(&ref->count);
> +		refcount_dec(&ref->count);
>  	}
>  	if (!__i915_active_fence_set(active, fence))
>  		__i915_active_acquire(ref);
> @@ -488,14 +488,16 @@ i915_active_set_exclusive(struct i915_active *ref, struct dma_fence *f)
>  bool i915_active_acquire_if_busy(struct i915_active *ref)
>  {
>  	debug_active_assert(ref);
> -	return atomic_add_unless(&ref->count, 1, 0);
> +	return refcount_add_not_zero(1, &ref->count);
>  }
> 
>  static void __i915_active_activate(struct i915_active *ref)
>  {
>  	spin_lock_irq(&ref->tree_lock); /* __active_retire() */
> -	if (!atomic_fetch_inc(&ref->count))
> +	if (!refcount_inc_not_zero(&ref->count)) {
> +		refcount_inc(&ref->count);
>  		debug_active_activate(ref);
> +	}
>  	spin_unlock_irq(&ref->tree_lock);
>  }
> 
> @@ -757,7 +759,7 @@ int i915_sw_fence_await_active(struct i915_sw_fence *fence,
>  void i915_active_fini(struct i915_active *ref)
>  {
>  	debug_active_fini(ref);
> -	GEM_BUG_ON(atomic_read(&ref->count));
> +	GEM_BUG_ON(refcount_read(&ref->count));
>  	GEM_BUG_ON(work_pending(&ref->work));
>  	mutex_destroy(&ref->mutex);
> 
> @@ -927,7 +929,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> 
>  		first = first->next;
> 
> -		atomic_dec(&ref->count);
> +		refcount_dec(&ref->count);
>  		intel_engine_pm_put(barrier_to_engine(node));
> 
>  		kmem_cache_free(slab_cache, node);
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index 7eb44132183a..116c7c28466a 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -193,14 +193,14 @@ void i915_active_release(struct i915_active *ref);
> 
>  static inline void __i915_active_acquire(struct i915_active *ref)
>  {
> -	GEM_BUG_ON(!atomic_read(&ref->count));
> -	atomic_inc(&ref->count);
> +	GEM_BUG_ON(!refcount_read(&ref->count));
> +	refcount_inc(&ref->count);
>  }
> 
>  static inline bool
>  i915_active_is_idle(const struct i915_active *ref)
>  {
> -	return !atomic_read(&ref->count);
> +	return !refcount_read(&ref->count);
>  }
> 
>  void i915_active_fini(struct i915_active *ref);
> diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
> index b02a78ac87db..152a3a25d9f7 100644
> --- a/drivers/gpu/drm/i915/i915_active_types.h
> +++ b/drivers/gpu/drm/i915/i915_active_types.h
> @@ -7,7 +7,7 @@
>  #ifndef _I915_ACTIVE_TYPES_H_
>  #define _I915_ACTIVE_TYPES_H_
> 
> -#include <linux/atomic.h>
> +#include <linux/refcount.h>
>  #include <linux/dma-fence.h>
>  #include <linux/llist.h>
>  #include <linux/mutex.h>
> @@ -23,7 +23,7 @@ struct i915_active_fence {
>  struct active_node;
> 
>  struct i915_active {
> -	atomic_t count;
> +	refcount_t count;
>  	struct mutex mutex;
> 
>  	spinlock_t tree_lock;
> --
> 2.34.1
> 
> 

Regards,

~Praveen.