Skip to contentSkip to conversation

kref, a tiny, sane, reference count object

RFC In review Greg KH 3 participants 7 messages Mar 13, 2004 – Mar 14, 2004

Subsystems (3)
USB Library code USB serial
lore
Share & tools
lore

Greg KH

author
#0 lore raw

RFC 1/1 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

include/linux/kref.h new file +32 -0
diff -Nru a/include/linux/kref.h b/include/linux/kref.h
@@ -0,0 +1,32 @@
+/*
+ * kref.c - library routines for handling generic reference counted objects
+ *
+ * Copyright (C) 2004 Greg Kroah-Hartman <[email protected]>
+ * Copyright (C) 2004 IBM Corp.
+ *
+ * based on kobject.h which was:
+ * Copyright (C) 2002-2003 Patrick Mochel <[email protected]>
+ * 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_ */
lib/Makefile +3 -0
diff -Nru a/lib/Makefile b/lib/Makefile
@@ -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
lib/kref.c new file +76 -0

76 lines not on this page · raw message

author
#1 lore raw

PATCH 1/1 convert usb-serial core to use kref instead of kobject

Quoted 2 lines

I'll follow up with a patch that converts the usb-serial core from using kobjects to using krefs instead.

And here is that patch.

thanks,

greg k-h

drivers/usb/serial/usb-serial.c +9 -16
diff -Nru a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
@@ -391,7 +391,7 @@
 	struct usb_serial *serial = serial_table[index];
 
 	if (serial)
-		kobject_get (&serial->kobj);
+		kref_get(&serial->kref);
 	return serial;
 }
 

@@ -486,7 +486,7 @@
 		if (retval) {
 			port->open_count = 0;
 			module_put(serial->type->owner);
-			kobject_put(&serial->kobj);
+			kref_put(&serial->kref);
 		}
 	}
 bailout:

@@ -518,7 +518,7 @@
 	}
 
 	module_put(port->serial->type->owner);
-	kobject_put(&port->serial->kobj);
+	kref_put(&port->serial->kref);
 }
 
 static int serial_write (struct tty_struct * tty, int from_user, const unsigned char *buf, int count)

@@ -748,7 +748,7 @@
 			begin += length;
 			length = 0;
 		}
-		kobject_put(&serial->kobj);
+		kref_put(&serial->kref);
 	}
 	*eof = 1;
 done:

@@ -830,15 +830,15 @@
 	wake_up_interruptible(&tty->write_wait);
 }
 
-static void destroy_serial (struct kobject *kobj)
+static void destroy_serial(struct kref *kref)
 {
 	struct usb_serial *serial;
 	struct usb_serial_port *port;
 	int i;
 
-	dbg ("%s - %s", __FUNCTION__, kobj->name);
+	serial = to_usb_serial(kref);
 
-	serial = to_usb_serial(kobj);
+	dbg ("%s - %s", __FUNCTION__, serial->type->name);
 	serial_shutdown (serial);
 
 	/* return the minor range that this device had */

@@ -886,10 +886,6 @@
 	kfree (serial);
 }
 
-static struct kobj_type usb_serial_kobj_type = {
-	.release = destroy_serial,
-};
-
 static void port_release(struct device *dev)
 {
 	struct usb_serial_port *port = to_usb_serial_port(dev);

@@ -930,10 +926,7 @@
 	serial->interface = interface;
 	serial->vendor = dev->descriptor.idVendor;
 	serial->product = dev->descriptor.idProduct;
-
-	/* initialize the kobject portion of the usb_device */
-	kobject_init(&serial->kobj);
-	serial->kobj.ktype = &usb_serial_kobj_type;
+	kref_init(&serial->kref, destroy_serial);
 
 	return serial;
 }

@@ -1284,7 +1277,7 @@
 	if (serial) {
 		/* let the last holder of this object 
 		 * cause it to be cleaned up */
-		kobject_put (&serial->kobj);
+		kref_put(&serial->kref);
 	}
 	dev_info(dev, "device disconnected\n");
 }
drivers/usb/serial/usb-serial.h +3 -2
diff -Nru a/drivers/usb/serial/usb-serial.h b/drivers/usb/serial/usb-serial.h
@@ -55,6 +55,7 @@
 #define __LINUX_USB_SERIAL_H
 
 #include <linux/config.h>
+#include <linux/kref.h>
 
 #define SERIAL_TTY_MAJOR	188	/* Nice legal number now */
 #define SERIAL_TTY_MINORS	255	/* loads of devices :) */

@@ -163,10 +164,10 @@
 	__u16				vendor;
 	__u16				product;
 	struct usb_serial_port *	port[MAX_NUM_PORTS];
-	struct kobject			kobj;
+	struct kref			kref;
 	void *				private;
 };
-#define to_usb_serial(d) container_of(d, struct usb_serial, kobj)
+#define to_usb_serial(d) container_of(d, struct usb_serial, kref)
 
 #define NUM_DONT_CARE	(-1)
#2 lore raw
In reply to Greg KH
Quoted 2 lines · Greg KH

For all of those people, this patch is for you.

It does rather neatly capture a common idiom.

lib/kref.c line 41

Quoted 8 lines
lib/kref.c new file +8 -0
+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?

lib/kref.c line 50

Quoted 4 lines
lib/kref.c new file +4 -0
+void kref_cleanup(struct kref *kref)
+{
+	if (!kref)
+		return;

and here?

#3 lore raw
Quoted 7 lines · Andrew Morton
Quoted 2 lines · Greg KH

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.

#4 lore raw
Quoted 13 lines · Nick Piggin
Quoted 7 lines · Andrew Morton
Quoted 2 lines · Greg KH

For all of those people, this patch is for you.

It does rather neatly capture a common idiom.

Quoted 6 lines

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.

Quoted 2 lines

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".

#5 lore raw
Quoted 32 lines · Andrew Morton
Quoted 14 lines · Nick Piggin
Quoted 9 lines · Andrew Morton
Quoted 3 lines · Greg KH

For all of those people, this patch is for you.

It does rather neatly capture a common idiom.

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 7 lines

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.

author
#6 lore raw
In reply to Andrew Morton
Quoted 10 lines · Andrew Morton

lib/kref.c line 41

Quoted 8 lines
lib/kref.c new file +8 -0
+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