[patch] Add kref_read and kref_put_last primitives

[patch] Add kref_read and kref_put_last primitives

[Ravikiran G Thirumalai]

Greg,
Here is a patch to add kref_read and kref_put_last.
These primitives were needed to change files_struct.f_count
refcounter to use kref api.

kref_put_last is needed sometimes when a refcount might
have an unconventional release which needs more than
the refcounted object to process object release-- like in the
files_struct.f_count conversion patch at __aio_put_req().
The following patch depends on kref shrinkage patches.
(which you have already included).

Please include.

Thanks,
Kiran

Signed-off-by: Ravikiran Thirumalai <kiran@in.ibm.com>

diff -ruN -X dontdiff2 linux-kref-2.6.7/include/linux/kref.h files_struct-kref-2.6.7/include/linux/kref.h
--- linux-kref-2.6.7/include/linux/kref.h	2004-07-26 18:53:24.000000000 +0530
+++ files_struct-kref-2.6.7/include/linux/kref.h	2004-07-26 18:48:43.543549752 +0530
@@ -26,6 +26,8 @@
 void kref_init(struct kref *kref);
 struct kref *kref_get(struct kref *kref);
 void kref_put(struct kref *kref, void (*release) (struct kref *kref));
+int kref_read(struct kref *kref);
+int kref_put_last(struct kref *kref);
 
 
 #endif /* _KREF_H_ */
diff -ruN -X dontdiff2 linux-kref-2.6.7/lib/kref.c files_struct-kref-2.6.7/lib/kref.c
--- linux-kref-2.6.7/lib/kref.c	2004-07-26 18:53:24.228879096 +0530
+++ files_struct-kref-2.6.7/lib/kref.c	2004-07-26 19:02:20.782310576 +0530
@@ -54,6 +54,33 @@
 	}
 }
 
+/**
+ * kref_read - Return the refcount value.
+ * @kref: object.
+ */
+int kref_read(struct kref *kref)
+{
+	return atomic_read(&kref->refcount);
+}
+
+/**
+ * kref_put_last - decrement refcount for object.
+ * @kref: object.
+ *
+ * Decrement the refcount, and if 0 return true.
+ * Returns false otherwise.  
+ * Use this only if you cannot use kref_put -- when the 
+ * release function of kref_put needs more than just the 
+ * refcounted object. Use of kref_put_last when kref_put
+ * can do will be frowned upon.
+ */
+int kref_put_last(struct kref *kref)
+{
+	return atomic_dec_and_test(&kref->refcount);
+}
+
 EXPORT_SYMBOL(kref_init);
 EXPORT_SYMBOL(kref_get);
 EXPORT_SYMBOL(kref_put);
+EXPORT_SYMBOL(kref_read);
+EXPORT_SYMBOL(kref_put_last);

Re: [patch] Add kref_read and kref_put_last primitives

[Ravikiran G Thirumalai]

On Mon, Jul 26, 2004 at 08:18:56PM +0530, Ravikiran G Thirumalai wrote:
> ... 
> kref_put_last is needed sometimes when a refcount might
> have an unconventional release which needs more than
> the refcounted object to process object release-- like in the
> files_struct.f_count conversion patch at __aio_put_req().
  ^^^^^^^^^^^^^^^^^^^

Oops struct file.f_count :) sorry.

Re: [patch] Add kref_read and kref_put_last primitives

[Christoph Hellwig]

On Mon, Jul 26, 2004 at 08:18:56PM +0530, Ravikiran G Thirumalai wrote:
> Greg,
> Here is a patch to add kref_read and kref_put_last.
> These primitives were needed to change files_struct.f_count
> refcounter to use kref api.
> 
> kref_put_last is needed sometimes when a refcount might
> have an unconventional release which needs more than
> the refcounted object to process object release-- like in the
> files_struct.f_count conversion patch at __aio_put_req().
> The following patch depends on kref shrinkage patches.
> (which you have already included).

Why don't you simply use an atomic_t if that's what you seem to
want?

Re: [patch] Add kref_read and kref_put_last primitives

[Ravikiran G Thirumalai]

On Mon, Jul 26, 2004 at 05:31:51PM +0100, Christoph Hellwig wrote:
> On Mon, Jul 26, 2004 at 08:18:56PM +0530, Ravikiran G Thirumalai wrote:
> > ...
> 
> Why don't you simply use an atomic_t if that's what you seem to
> want?
> 

struct kref does just that.  The kref api are just abstractions for 
refcounting which i presume is recommended for all refcounters in the
kernel.  I am just converting the struct file.f_count refcounter
to use kref with this patch.  

My actual intentions are to extend the kref api to do lock free refcounting 
and use that for the file.f_count refcounter, and make fd lookup lock 
free using rcu.  My earlier experiments with such patches showed 
performance improvements for threaded workloads which do lot of io.

Thanks,
Kiran

Re: [patch] Add kref_read and kref_put_last primitives

[Christoph Hellwig]

On Tue, Jul 27, 2004 at 12:39:15PM +0530, Ravikiran G Thirumalai wrote:
> struct kref does just that.  The kref api are just abstractions for 
> refcounting which i presume is recommended for all refcounters in the
> kernel.  I am just converting the struct file.f_count refcounter
> to use kref with this patch.  

So what exactly does the API make easier?  APIs for the APIs sense don't
make much sense.

Re: [patch] Add kref_read and kref_put_last primitives

[Greg KH]

On Mon, Jul 26, 2004 at 05:31:51PM +0100, Christoph Hellwig wrote:
> On Mon, Jul 26, 2004 at 08:18:56PM +0530, Ravikiran G Thirumalai wrote:
> > Greg,
> > Here is a patch to add kref_read and kref_put_last.
> > These primitives were needed to change files_struct.f_count
> > refcounter to use kref api.
> > 
> > kref_put_last is needed sometimes when a refcount might
> > have an unconventional release which needs more than
> > the refcounted object to process object release-- like in the
> > files_struct.f_count conversion patch at __aio_put_req().
> > The following patch depends on kref shrinkage patches.
> > (which you have already included).
> 
> Why don't you simply use an atomic_t if that's what you seem to
> want?

Exactly.  In cases like this, where the user, for some reason, wants to
know the state of the reference count, they should not use a struct
kref.  I'm not going to add these functions to the kref api, sorry.

thanks,

greg k-h

Re: [patch] Add kref_read and kref_put_last primitives

[Dipankar Sarma]

On Mon, Aug 02, 2004 at 01:08:49PM -0700, Greg KH wrote:
> On Mon, Jul 26, 2004 at 05:31:51PM +0100, Christoph Hellwig wrote:
> > Why don't you simply use an atomic_t if that's what you seem to
> > want?
> 
> Exactly.  In cases like this, where the user, for some reason, wants to
> know the state of the reference count, they should not use a struct
> kref.  I'm not going to add these functions to the kref api, sorry.
> 

Really, there is no need to use kref in where Ravikiran is using
them (VFS) under the current circumstances. However, if lock-free 
lookup using RCU is to be used, Ravikiran needs to use an abstracted reference 
counter API. This is to optimally support platforms with and without 
cmpxchg. kref is the standard reference counter API at the moment and 
that is where it made sense to add the abstracted lockfree reference counter
support.

So, kref_read() as it is would look weird. But if we consider merging
the rest of the kref APIs (lock-free extensions) in future, then the
entire set including kref_read() would make sense.

Thanks
Dipankar

Re: [patch] Add kref_read and kref_put_last primitives

[Greg KH]

On Tue, Aug 03, 2004 at 11:12:18AM +0530, Dipankar Sarma wrote:
> On Mon, Aug 02, 2004 at 01:08:49PM -0700, Greg KH wrote:
> > On Mon, Jul 26, 2004 at 05:31:51PM +0100, Christoph Hellwig wrote:
> > > Why don't you simply use an atomic_t if that's what you seem to
> > > want?
> > 
> > Exactly.  In cases like this, where the user, for some reason, wants to
> > know the state of the reference count, they should not use a struct
> > kref.  I'm not going to add these functions to the kref api, sorry.
> > 
> 
> Really, there is no need to use kref in where Ravikiran is using
> them (VFS) under the current circumstances. However, if lock-free 
> lookup using RCU is to be used, Ravikiran needs to use an abstracted reference 
> counter API. This is to optimally support platforms with and without 
> cmpxchg. kref is the standard reference counter API at the moment and 
> that is where it made sense to add the abstracted lockfree reference counter
> support.

Hm, I still don't agree :)

> So, kref_read() as it is would look weird. But if we consider merging
> the rest of the kref APIs (lock-free extensions) in future, then the
> entire set including kref_read() would make sense.

No, even with rcu versions, I don't see the need for this in the api.

Sure, for this specific implementation of a atomic_t, it is useful, as
the value is checked.  But that means that you might just want to use an
atomic_t, as it doesn't fit the model of a struct kref at all (something
where you don't touch the reference count directly at all.)

Becides, I don't think that people are convinced that this code needs to
be changed anyway :)

thanks,

greg k-h

Re: [patch] Add kref_read and kref_put_last primitives

[Dipankar Sarma]

On Mon, Aug 02, 2004 at 11:51:30PM -0700, Greg KH wrote:
> On Tue, Aug 03, 2004 at 11:12:18AM +0530, Dipankar Sarma wrote:
> 
> > So, kref_read() as it is would look weird. But if we consider merging
> > the rest of the kref APIs (lock-free extensions) in future, then the
> > entire set including kref_read() would make sense.
> 
> No, even with rcu versions, I don't see the need for this in the api.

I agree that RCU versions really doesn't need it. However, there
is code in many places in the kernel where we actually read
the actual reference count value and even compare it with
constants. Those things are problematic because you can't
use kref there without a kref_read_count() type API.
In typical driver object maintenance, this is not an issue
and rightly not exported.

> Sure, for this specific implementation of a atomic_t, it is useful, as
> the value is checked.  But that means that you might just want to use an
> atomic_t, as it doesn't fit the model of a struct kref at all (something
> where you don't touch the reference count directly at all.)

Which prevents it from being used in many objects where we touch
the reference count directly. If we use atomic_t there, then we
need to abstract out inc/dec for RCU, which results in another
refcounter which you don't like (for good reasons, btw) ;-) 

> Becides, I don't think that people are convinced that this code needs to
> be changed anyway :)

Which code ? If you are talking about the lock-free fd lookup
code, think POSIX threaded apps doing lots of I/O. tiobench
results show how useful it is.

Thanks
Dipankar