RE: [RFC] kref, a tiny, sane, reference count object

RE: [RFC] kref, a tiny, sane, reference count object

[Peter Kjellerstedt]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org 
> [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Greg KH
> Sent: Saturday, March 13, 2004 09:20
> To: linux-kernel@vger.kernel.org
> Subject: [RFC] kref, a tiny, sane, reference count object
> 
> In thinking about people's complaints about the current 
> kobject interface, a lot of people don't like the 
> "complexity" of what is necessary to use a kobject.
> If all you want is something to handle reference 
> counting properly, a kobject can seem a bit "large".
> 
> For all of those people, this patch is for you.  
> Introducing struct kref.  A tiny (only 8 bytes on a 
> 32bit platform) that will properly handle reference 
> counting any structure you want to use it for.  Note 
> that you will have to be careful around the cleanup 
> period (but that can be easily handled by the user 
> with regards to not trying to grab a "new" reference
> if you don't already have one, once the object is 
> gone, just like kobjects and sysfs today work.)
> 
> I've implemented kobjects using a kref to handle the 
> reference counting portion, but will leave that patch
> and change for 2.7, as it will add 4 more bytes (on a 
> 32bit platform) to every kobject, and that wouldn't 
> be nice this early in the 2.6 series.  For now, krefs 
> can stand on their own.
> 
> I've already found loads of places in the kernel that 
> can use this structure to clean up their logic, and 
> will probably be converting a number of them over time 
> to use them.  But no, Al, I will not say this can be 
> used to replace the atomic_t count you have in inodes, 
> as that count is horribly abused in ways I never really 
> wanted to know about (negative counts mean something 
> "special"?  eeeeeek....)
> 
> Anyway, here's a patch against 2.6.4 that adds krefs
> to the kernel.  I'll follow up with a patch that 
> converts the usb-serial core from using kobjects to 
> using krefs instead.
> 
> Comments are appreciated and welcomed.
> 
> thanks,
> 
> greg k-h

Looks simple enough.  But I have a small question. 
In kref_get() and kref_cleanup(), kref is verified 
not to be NULL before being used.  However, this is 
not done in kref_put().  An oversight, or as intended?

//Peter

Re: [RFC] kref, a tiny, sane, reference count object

[Greg KH]

On Sat, Mar 13, 2004 at 10:10:36AM +0100, Peter Kjellerstedt wrote:
> Looks simple enough.  But I have a small question. 
> In kref_get() and kref_cleanup(), kref is verified 
> not to be NULL before being used.  However, this is 
> not done in kref_put().  An oversight, or as intended?

An oversight, thanks for pointing it out.  I'll go fix it up.

thanks again,

greg k-h

Re: [RFC] kref, a tiny, sane, reference count object

[Nick Piggin]

Andrew Morton wrote:

>Nick Piggin <piggin@cyberone.com.au> wrote:
>
>>
>>
>>Andrew Morton wrote:
>>
>>
>>>Greg KH <greg@kroah.com> wrote:
>>>
>>>
>>>>For all of those people, this patch is for you.
>>>>
>>>>
>>>It does rather neatly capture a common idiom.
>>>
>>>
>>But as Andi said - look at all the crap involved when:
>>
>>atomic_inc();
>>if (atomic_dec_and_test())
>>    release();
>>Also neatly captures that idiom.
>>
>
>Well it does more than that, such as trapping the hard-to-diagnose bug
>of grabbing a refcount against a zero-ref object.
>
>
>>And you get more flexibility by being able to use atomic_set
>>directly too.
>>
>
>Do I care about that?  I care more about being able to say "ah, it uses
>kref.  I understand that refcounting idiom, I know it's well debugged and I
>know that it traps common errors".  That's better than "oh crap, this thing
>implements its own refcounting - I need to review it for the usual
>errors".
>
>

OK good point.

Re: [RFC] kref, a tiny, sane, reference count object

[Andrew Morton]

Nick Piggin <piggin@cyberone.com.au> wrote:
>
> 
> 
> Andrew Morton wrote:
> 
> >Greg KH <greg@kroah.com> wrote:
> >
> >>For all of those people, this patch is for you.
> >>
> >
> >It does rather neatly capture a common idiom.
> >
> 
> But as Andi said - look at all the crap involved when:
> 
> atomic_inc();
> if (atomic_dec_and_test())
>     release();
> Also neatly captures that idiom.

Well it does more than that, such as trapping the hard-to-diagnose bug
of grabbing a refcount against a zero-ref object.

> And you get more flexibility by being able to use atomic_set
> directly too.

Do I care about that?  I care more about being able to say "ah, it uses
kref.  I understand that refcounting idiom, I know it's well debugged and I
know that it traps common errors".  That's better than "oh crap, this thing
implements its own refcounting - I need to review it for the usual
errors".

Re: [RFC] kref, a tiny, sane, reference count object

[Nick Piggin]

Andrew Morton wrote:

>Greg KH <greg@kroah.com> wrote:
>
>>For all of those people, this patch is for you.
>>
>
>It does rather neatly capture a common idiom.
>

But as Andi said - look at all the crap involved when:

atomic_inc();
if (atomic_dec_and_test())
    release();
Also neatly captures that idiom.

And you get more flexibility by being able to use atomic_set
directly too.


But if you really like it, I agree it shouldn't allow NULL
pointers and probably get, put and cleanup should be inline.

Re: [RFC] kref, a tiny, sane, reference count object

[Greg KH]

On Sat, Mar 13, 2004 at 04:34:51PM -0800, Andrew Morton wrote:
> > +struct kref * kref_get(struct kref *kref)
> > +{
> > +	if (kref) {
> > +		WARN_ON(!atomic_read(&kref->refcount));
> > +		atomic_inc(&kref->refcount);
> > +	}
> > +	return kref;
> > +}
> 
> Why is a NULL arg permitted here?

Because kobjects permitted it?  :)

I think you are correct, if we are passing a NULL pointer to these
functions, we deserve the oops we get, as other, much worse things could
happen (as a kref lives inside another structure.)

I'll go take those checks out.

thanks,

greg k-h

Re: [RFC] kref, a tiny, sane, reference count object

[Andrew Morton]

Greg KH <greg@kroah.com> wrote:
>
> For all of those people, this patch is for you.

It does rather neatly capture a common idiom.

> +struct kref * kref_get(struct kref *kref)
> +{
> +	if (kref) {
> +		WARN_ON(!atomic_read(&kref->refcount));
> +		atomic_inc(&kref->refcount);
> +	}
> +	return kref;
> +}

Why is a NULL arg permitted here?

> +void kref_cleanup(struct kref *kref)
> +{
> +	if (!kref)
> +		return;

and here?

Re: [RFC] kref, a tiny, sane, reference count object

[Andi Kleen]

Greg KH <greg@kroah.com> writes:

> But people do get it wrong (I've seen it happen).  Using this keeps you

If they cannot get a simple reference counter right it is doubtful
the rest of their code will do any good.

> from having to write your own get, and put functions.  Multiply that by
> every usb driver that wants to (and needs to) use this kind of logic,
> and you have a lot of duplicated code that is unnecessary.

Lots of duplicated one liners. Sounds like a big issue.
For me this thing smells more like overabstraction.

> So we write it once, get it correct, and let everyone use it.  Isn't
> that what the code in /lib is for?  :)

I would agree if it is significant code. But all you're replacing is a
few straight forward one/two liners, and that at the cost of less
efficient space usage (12 byte overhead on 64bit)

-Andi

Re: [RFC] kref, a tiny, sane, reference count object

[Greg KH]

On Sat, Mar 13, 2004 at 02:06:56PM +0100, Andi Kleen wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > Comments are appreciated and welcomed.
> 
> I really don't see the advantage of this. Writing the same using
> atomic_inc/ atomic_dec_and_test() implicitely is basically as clean,
> and you will save the overhead of carrying a release() pointer
> around. And the "patterns" for that are easy enough that I doubt
> people will get it wrong.

But people do get it wrong (I've seen it happen).  Using this keeps you
from having to write your own get, and put functions.  Multiply that by
every usb driver that wants to (and needs to) use this kind of logic,
and you have a lot of duplicated code that is unnecessary.

So we write it once, get it correct, and let everyone use it.  Isn't
that what the code in /lib is for?  :)

thanks,

greg k-h

Re: [RFC] kref, a tiny, sane, reference count object

[Andi Kleen]

Greg KH <greg@kroah.com> writes:

> Comments are appreciated and welcomed.

I really don't see the advantage of this. Writing the same using
atomic_inc/ atomic_dec_and_test() implicitely is basically as clean,
and you will save the overhead of carrying a release() pointer
around. And the "patterns" for that are easy enough that I doubt
people will get it wrong.

-Andi

[RFC] kref, a tiny, sane, reference count object

[Greg KH]

In thinking about people's complaints about the current kobject
interface, a lot of people don't like the "complexity" of what is
necessary to use a kobject.  If all you want is something to handle
reference counting properly, a kobject can seem a bit "large".

For all of those people, this patch is for you.  Introducing struct
kref.  A tiny (only 8 bytes on a 32bit platform) that will properly
handle reference counting any structure you want to use it for.  Note
that you will have to be careful around the cleanup period (but that can
be easily handled by the user with regards to not trying to grab a "new"
reference if you don't already have one, once the object is gone, just
like kobjects and sysfs today work.)

I've implemented kobjects using a kref to handle the reference counting
portion, but will leave that patch and change for 2.7, as it will add 4
more bytes (on a 32bit platform) to every kobject, and that wouldn't be
nice this early in the 2.6 series.  For now, krefs can stand on their
own.

I've already found loads of places in the kernel that can use this
structure to clean up their logic, and will probably be converting a
number of them over time to use them.  But no, Al, I will not say this
can be used to replace the atomic_t count you have in inodes, as that
count is horribly abused in ways I never really wanted to know about
(negative counts mean something "special"?  eeeeeek....)

Anyway, here's a patch against 2.6.4 that adds krefs to the kernel.
I'll follow up with a patch that converts the usb-serial core from using
kobjects to using krefs instead.

Comments are appreciated and welcomed.

thanks,

greg k-h


diff -Nru a/include/linux/kref.h b/include/linux/kref.h
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/include/linux/kref.h	Sat Mar 13 00:04:46 2004
@@ -0,0 +1,32 @@
+/*
+ * kref.c - library routines for handling generic reference counted objects
+ *
+ * Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
+ * Copyright (C) 2004 IBM Corp.
+ *
+ * based on kobject.h which was:
+ * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
+ * Copyright (C) 2002-2003 Open Source Development Labs
+ *
+ * This file is released under the GPLv2.
+ *
+ */
+
+#if defined(__KERNEL__) && !defined(_KREF_H_)
+#define _KREF_H_
+
+#include <linux/types.h>
+#include <asm/atomic.h>
+
+
+struct kref {
+	atomic_t refcount;
+	void (*release)(struct kref *kref);
+};
+
+void kref_init(struct kref *kref, void (*release)(struct kref *));
+struct kref *kref_get(struct kref *kref);
+void kref_put(struct kref *kref);
+
+
+#endif /* _KREF_H_ */
diff -Nru a/lib/Makefile b/lib/Makefile
--- a/lib/Makefile	Sat Mar 13 00:04:42 2004
+++ b/lib/Makefile	Sat Mar 13 00:04:42 2004
@@ -8,6 +8,9 @@
 	 kobject.o idr.o div64.o parser.o int_sqrt.o \
 	 bitmap.o extable.o
 
+# hack for now till some static code uses krefs, then it can move up above...
+obj-y += kref.o
+
 lib-$(CONFIG_RWSEM_GENERIC_SPINLOCK) += rwsem-spinlock.o
 lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
 
diff -Nru a/lib/kref.c b/lib/kref.c
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/lib/kref.c	Sat Mar 13 00:04:46 2004
@@ -0,0 +1,76 @@
+/*
+ * kref.c - library routines for handling generic reference counted objects
+ *
+ * Copyright (C) 2004 Greg Kroah-Hartman <greg@kroah.com>
+ * Copyright (C) 2004 IBM Corp.
+ *
+ * based on lib/kobject.c which was:
+ * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
+ *
+ * This file is released under the GPLv2.
+ *
+ */
+
+/* #define DEBUG */
+
+#include <linux/kref.h>
+#include <linux/module.h>
+
+
+/**
+ * kref_init - initialize object.
+ * @kref: object in question.
+ */
+void kref_init(struct kref *kref, void (*release)(struct kref *kref))
+{
+	atomic_set(&kref->refcount,1);
+	kref->release = release;
+}
+
+/**
+ * kref_get - increment refcount for object.
+ * @kref: object.
+ */
+struct kref * kref_get(struct kref *kref)
+{
+	if (kref) {
+		WARN_ON(!atomic_read(&kref->refcount));
+		atomic_inc(&kref->refcount);
+	}
+	return kref;
+}
+
+/**
+ * kref_cleanup - free kref resources. 
+ * @kref: object.
+ */
+void kref_cleanup(struct kref *kref)
+{
+	if (!kref)
+		return;
+
+	pr_debug("kref cleaning up\n");
+	if (kref->release)
+		kref->release(kref);
+	else {
+		printk(KERN_ERR "kref does not have a release() function, "
+			"it is broken and must be fixed.\n");
+		WARN_ON(1);
+	}
+}
+
+/**
+ * kref_put - decrement refcount for object.
+ * @kref: object.
+ *
+ * Decrement the refcount, and if 0, call kref_cleanup().
+ */
+void kref_put(struct kref *kref)
+{
+	if (atomic_dec_and_test(&kref->refcount))
+		kref_cleanup(kref);
+}
+
+EXPORT_SYMBOL(kref_init);
+EXPORT_SYMBOL(kref_get);
+EXPORT_SYMBOL(kref_put);