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

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

[Chris Wilson]

For a deferred-free cache of unreferenced bound objects, a simple
reference count is required without the baggage of kref.

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);
 	}
 }
-- 
1.7.2.3

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

[Dave Airlie]

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);
>  	}
>  }

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

[Chris Wilson]

On Fri, 26 Nov 2010 08:38:29 +1000, Dave Airlie <airlied@redhat.com> wrote:
> 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?

The issue with kref is that it does:

void kref_get(struct kref *kref)
{
        WARN_ON(!atomic_read(&kref->refcount));
        atomic_inc(&kref->refcount);
        smp_mb__after_atomic_inc();
}

which causes havoc when you are trying to keep a list of unreferenced
objects. That's all I'm trying to avoid.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Alan Cox]

> void kref_get(struct kref *kref)
> {
>         WARN_ON(!atomic_read(&kref->refcount));
>         atomic_inc(&kref->refcount);
>         smp_mb__after_atomic_inc();
> }
> 
> which causes havoc when you are trying to keep a list of unreferenced
> objects. That's all I'm trying to avoid.

You cannot have a list of unreferenced objects, the list itself is a set
of references.

You might want something to scavenge objects whose refcount is the list
only but thats not quite the same thing, and kref's are quite happy if
used that way.

Alan

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

[Thomas Hellstrom]

On 11/25/2010 11:38 PM, Dave Airlie wrote:
> 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.
>
>    
This last thing is really a generic problem with objects looked up from 
user-space. Consider the lookup path:

read_lock()
lookup_object_name();
kref_get();
read_unlock();

And the destroy path:

vs the desired put path
if (reference_put()) {
write_lock():
remove_name();
write_unlock();
destroy();
}

This is racy, in that the kref_get() can hit a zero refcount. I think an 
ideal thing here would be to add a kref_get_unless_zero() for this 
situation, that returns an error if the refcount was indeed zero. I 
don't think that would violate the kref idea, since we still never 
increase the refcount of a zeroed object, and the user needs to provide 
the necessary synchronization to make sure the object isn't gone while 
still the refcount is zero.

/Thomas






> Dave.
>
>    

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

[Daniel Vetter]

On Sun, Nov 28, 2010 at 01:32:27PM +0100, Thomas Hellstrom wrote:
> This is racy, in that the kref_get() can hit a zero refcount. I
> think an ideal thing here would be to add a kref_get_unless_zero()
> for this situation, that returns an error if the refcount was indeed
> zero. I don't think that would violate the kref idea, since we still
> never increase the refcount of a zeroed object, and the user needs
> to provide the necessary synchronization to make sure the object
> isn't gone while still the refcount is zero.

I've got curious rechecked the code and it all looks sane. The trick is
that all the userspace-visible names hold a ref to the underlying gem
object (including the flink name). If all userspace handles are destroyed
(save the flink name) the flink name gets destroyed, too (see
obj->handle_count). All this is independently protected with spinlocks
and it happens _before_ dropping the ref to the underlying bo (and taking
any necessary locks to do so). I don't see any races nor locking problems,
there.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

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

[Thomas Hellstrom]

On 11/28/2010 02:35 PM, Daniel Vetter wrote:
> On Sun, Nov 28, 2010 at 01:32:27PM +0100, Thomas Hellstrom wrote:
>    
>> This is racy, in that the kref_get() can hit a zero refcount. I
>> think an ideal thing here would be to add a kref_get_unless_zero()
>> for this situation, that returns an error if the refcount was indeed
>> zero. I don't think that would violate the kref idea, since we still
>> never increase the refcount of a zeroed object, and the user needs
>> to provide the necessary synchronization to make sure the object
>> isn't gone while still the refcount is zero.
>>      
> I've got curious rechecked the code and it all looks sane. The trick is
> that all the userspace-visible names hold a ref to the underlying gem
> object (including the flink name). If all userspace handles are destroyed
> (save the flink name) the flink name gets destroyed, too (see
> obj->handle_count). All this is independently protected with spinlocks
> and it happens _before_ dropping the ref to the underlying bo (and taking
> any necessary locks to do so). I don't see any races nor locking problems,
> there.
> -Daniel
>    

Hi, Daniel!

I'm not saying that the current gem code is incorrect. In particular, 
the gem name system allows upping the handle_count even if it's zero and 
the name is just about to be destroyed. I see that more as a curiosity, 
and gem would work just as well if it used kref_get_unless_zero on the 
handle_count.

However this is a generic problem.
It appears in the general user-visible object case. For example, 
currently gem needs to take the struct_mutex *before* unreferencing to 
avoid hitting this problem with a racing mmap lookup (At least I think 
that's why the struct_mutex is taken).  If mmap were using 
kref_get_unless_zero, and returning error on failure, the struct_mutex 
would be needed only inside the kref release function, and that is what 
Dave was referring to in his mail.

An identical problem appears in TTM where we need to take the 
bdev::vm_lock around unreffing a ttm bo, and in the ttm_object 
functionality were we also have a general lookup mechanism for stuff 
like contexts, surfaces, user-visible fences or whatever.

In particular this also applies to the rcu case, were we cannot block 
readers. If we were to move over all object lookup to use RCU locks, 
we'd want to be completely 100% sure that once a refcount reaches zero, 
it won't be upped again, and we can safely go ahead and use call_rcu for 
destruction. Let's say this wasn't true, call_rcu could in principle be 
called multiple times for the same object, and that isn't allowed until 
the callback has been run...

And the simple solution to this problem is kref_get_unless_zero().

/Thomas

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

[Daniel Vetter]

Hi Thomas,

On Sun, Nov 28, 2010 at 03:19:50PM +0100, Thomas Hellstrom wrote:
> I'm not saying that the current gem code is incorrect. In
> particular, the gem name system allows upping the handle_count even
> if it's zero and the name is just about to be destroyed. I see that
> more as a curiosity, and gem would work just as well if it used
> kref_get_unless_zero on the handle_count.

Yeah, noticed that, too. But that's just userspace racing against itself,
so no problem.

> However this is a generic problem.
> It appears in the general user-visible object case. For example,
> currently gem needs to take the struct_mutex *before* unreferencing
> to avoid hitting this problem with a racing mmap lookup (At least I
> think that's why the struct_mutex is taken).  If mmap were using
> kref_get_unless_zero, and returning error on failure, the
> struct_mutex would be needed only inside the kref release function,
> and that is what Dave was referring to in his mail.

Indeed, I haven't noticed that. drm/i915 doesn't hold a ref to the
underlying bo for the offset_hash, which requires the dev->struct_mutex
around unref. An actual mmap gets itself a ref (protected by
dev->struct_mutex) so from there on its all fine. I think the offset_hash
should hold a reference too, which gets destroyed when handle_count
reaches 0 (like the flink name). Haven't checked what ttm does for this
case.

I think having a pointer without an accompanying reference is a bug, i.e.
I've jotted this down on my list of things to fix. Then we could move the
mutex taking inside the actual free function.

> In particular this also applies to the rcu case, were we cannot
> block readers. If we were to move over all object lookup to use RCU
> locks, we'd want to be completely 100% sure that once a refcount
> reaches zero, it won't be upped again, and we can safely go ahead
> and use call_rcu for destruction. Let's say this wasn't true,
> call_rcu could in principle be called multiple times for the same
> object, and that isn't allowed until the callback has been run...

Well, if the lookup structure has a reference to the underlying object we
only need to defer the unref till all readers are gone with call_rcu.

> And the simple solution to this problem is kref_get_unless_zero().

IMHO kref is for kernel references. And shouldn't be abused for userspace
lookup with strange semantics. So kref_get_unless_zero sounds like a very
bad idea hiding brittle locking semantics at best, bugs at worst.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

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

[Thomas Hellstrom]

On 11/28/2010 04:13 PM, Daniel Vetter wrote:
> Hi Thomas,
>
> On Sun, Nov 28, 2010 at 03:19:50PM +0100, Thomas Hellstrom wrote:
>    
>> I'm not saying that the current gem code is incorrect. In
>> particular, the gem name system allows upping the handle_count even
>> if it's zero and the name is just about to be destroyed. I see that
>> more as a curiosity, and gem would work just as well if it used
>> kref_get_unless_zero on the handle_count.
>>      
> Yeah, noticed that, too. But that's just userspace racing against itself,
> so no problem.
>
>    
>> However this is a generic problem.
>> It appears in the general user-visible object case. For example,
>> currently gem needs to take the struct_mutex *before* unreferencing
>> to avoid hitting this problem with a racing mmap lookup (At least I
>> think that's why the struct_mutex is taken).  If mmap were using
>> kref_get_unless_zero, and returning error on failure, the
>> struct_mutex would be needed only inside the kref release function,
>> and that is what Dave was referring to in his mail.
>>      
> Indeed, I haven't noticed that. drm/i915 doesn't hold a ref to the
> underlying bo for the offset_hash, which requires the dev->struct_mutex
> around unref. An actual mmap gets itself a ref (protected by
> dev->struct_mutex) so from there on its all fine. I think the offset_hash
> should hold a reference too, which gets destroyed when handle_count
> reaches 0 (like the flink name). Haven't checked what ttm does for this
> case.
>
>    

For TTM the "name" does not hold a reference just like the gem vm 
handle, but we take a lock around unref'ing so that destruction of the 
"name" and unreffing is atomic. That doesn't work with RCU since we 
can't block readers.

> I think having a pointer without an accompanying reference is a bug, i.e.
> I've jotted this down on my list of things to fix. Then we could move the
> mutex taking inside the actual free function.
>
>    
>> In particular this also applies to the rcu case, were we cannot
>> block readers. If we were to move over all object lookup to use RCU
>> locks, we'd want to be completely 100% sure that once a refcount
>> reaches zero, it won't be upped again, and we can safely go ahead
>> and use call_rcu for destruction. Let's say this wasn't true,
>> call_rcu could in principle be called multiple times for the same
>> object, and that isn't allowed until the callback has been run...
>>      
> Well, if the lookup structure has a reference to the underlying object we
> only need to defer the unref till all readers are gone with call_rcu.
>    

So how do you determine when to delete the lookup structure (or in the 
gem case, the name)? Currently gem does this when all handles are gone, 
but then again, how do you block a handle from registering a new 
reference just after you've removed the name and called rcu, but before 
the rcu callback is called? You can't block readers while you remove 
first the name and then the reference.

What you *can* do is to check again rcu in the callback for the refcount 
to be nonzero and abort, but then again you need a protection to avoid 
the rcu callback to be called again when the refcount reaches zero the 
second time, while the rcu head sits on the run list.... This quickly 
gets quite ugly, but if you have a good solution to this problem, I'm 
all ears.

>    
>> And the simple solution to this problem is kref_get_unless_zero().
>>      
>    
> IMHO kref is for kernel references. And shouldn't be abused for userspace
> lookup with strange semantics.
These are perfectly valid kernel references. Typically the pointer in 
the handle's referencing idr entry. If kref_get_unless_zero() returns an 
error, you're not allowed to create a new pointer, and lookup returns an 
error. It's as easy as that.

>   So kref_get_unless_zero sounds like a very
> bad idea hiding brittle locking semantics at best, bugs at worst.
>    

I don't agree. IMHO it's a good thing to do in situations where you 
might be racing with lookup object removal, but the underlying object is 
guaranteed to not have been destroyed. Perfectly suitable for RCU-like 
situations.

/Thomas



> Cheers, Danie
>