From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0CFD6C4167B for ; Wed, 28 Dec 2022 14:43:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232862AbiL1Ont (ORCPT ); Wed, 28 Dec 2022 09:43:49 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33580 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232023AbiL1Onq (ORCPT ); Wed, 28 Dec 2022 09:43:46 -0500 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 6CAC6F001 for ; Wed, 28 Dec 2022 06:43:45 -0800 (PST) Received: from [192.168.1.139] (unknown [171.76.80.102]) by linux.microsoft.com (Postfix) with ESMTPSA id 676CE20F26B6; Wed, 28 Dec 2022 06:43:42 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 676CE20F26B6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1672238625; bh=eISDS/91oiq1DS7pKCpl1TnGMMOr/ZfUtjW/6tYTuBo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=JA/DFLkJg+lq0r974VDtsSIzXPguPqbLgZMMrdHHFydbLbq6uiflSW7pRUYc7jZ4V JCJbMMLq6SQXaSF44FNDtApMoSBCxsMeTzl/KU2oCe71ev6/CiHYxYpBdettrtcK9H iKndRKC3FCLd8gA6bCKjQE49B/+rLttmkRNOpAnY= Message-ID: Date: Wed, 28 Dec 2022 20:13:39 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH v2 1/2] drm/i915: convert i915_active.count from atomic_t to refcount_t Content-Language: en-US To: Deepak R Varma , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Tvrtko Ursulin , David Airlie , Daniel Vetter , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: Saurabh Singh Sengar References: From: Praveen Kumar In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > 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 > +#include > #include > #include > #include > @@ -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.