[patch] kref shrinkage patches -- 1 of 2 -- kref shrinkage

[patch] kref shrinkage patches -- 1 of 2 -- kref shrinkage

[Ravikiran G Thirumalai]

Greg,
Here's the first step towards getting to kref_get_rcu :)
This patch is to shrink the kref object by removing the 
function pointer 'release' from the kref object, and modifying
kref_init and kref_put interfaces so that kref_init will not take
the release pointer anymore and kref_put will.  Patch will probably talk
better.

Just had a question about definition of kref_get though, 
why does it need to return struct kref * ? struct kref * is anywayz 
being passed to it, hence the caller has it anywayz -- so it doesn't 
value add anything afaics (but i might have limited vision so pls correct me)
In fact, I see that the scsi applications  of kref have code like

<quote drivers/scsi/sd.c>

        if (!kref_get(&sdkp->kref))
                goto out_sdkp;

</>

the goto can never happen afaics. IMO,  kref_get must return void to 
avoid such wrong usage -- or atleast add some bold comments . Do let me 
know and I can make a patch to do either.

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

diff -ruN -X dontdiff2 linux-2.6.7/include/linux/kref.h kref-2.6.7/include/linux/kref.h
--- linux-2.6.7/include/linux/kref.h	2004-06-16 10:48:59.000000000 +0530
+++ kref-2.6.7/include/linux/kref.h	2004-07-20 12:53:24.226739304 +0530
@@ -8,6 +8,9 @@
  * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
  * Copyright (C) 2002-2003 Open Source Development Labs
  *
+ * 07/2004 - struct kref shrinkage by Ravikiran Thirumalai <kiran@in.ibm.com>
+ * Copyright (C) 2004 IBM Corp.
+ *
  * This file is released under the GPLv2.
  *
  */
@@ -21,12 +24,11 @@
 
 struct kref {
 	atomic_t refcount;
-	void (*release)(struct kref *kref);
 };
 
-void kref_init(struct kref *kref, void (*release)(struct kref *));
+void kref_init(struct kref *kref);
 struct kref *kref_get(struct kref *kref);
-void kref_put(struct kref *kref);
+void kref_put(struct kref *kref, void (*release) (struct kref *kref));
 
 
 #endif /* _KREF_H_ */
diff -ruN -X dontdiff2 linux-2.6.7/lib/kref.c kref-2.6.7/lib/kref.c
--- linux-2.6.7/lib/kref.c	2004-06-16 10:50:26.000000000 +0530
+++ kref-2.6.7/lib/kref.c	2004-07-20 13:00:36.819975136 +0530
@@ -7,6 +7,9 @@
  * based on lib/kobject.c which was:
  * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
  *
+ * 07/2004 - kref object shrinkage by Ravikiran Thirumalai <kiran@in.ibm.com>
+ * Copyright (C) 2004 IBM Corp.
+ * 
  * This file is released under the GPLv2.
  *
  */
@@ -19,15 +22,10 @@
 /**
  * kref_init - initialize object.
  * @kref: object in question.
- * @release: pointer to a function that will clean up the object
- *	     when the last reference to the object is released.
- *	     This pointer is required.
  */
-void kref_init(struct kref *kref, void (*release)(struct kref *kref))
+void kref_init(struct kref *kref)
 {
-	WARN_ON(release == NULL);
 	atomic_set(&kref->refcount,1);
-	kref->release = release;
 }
 
 /**
@@ -44,14 +42,17 @@
 /**
  * kref_put - decrement refcount for object.
  * @kref: object.
- *
- * Decrement the refcount, and if 0, call kref->release().
+ * @release: pointer to the function that will clean up the object
+ *	     when the last reference to the object is released.
+ *	     This pointer is required.
+ * Decrement the refcount, and if 0, call release().
  */
-void kref_put(struct kref *kref)
+void kref_put(struct kref *kref, void (*release) (struct kref *kref))
 {
+	WARN_ON(release == NULL);
 	if (atomic_dec_and_test(&kref->refcount)) {
 		pr_debug("kref cleaning up\n");
-		kref->release(kref);
+		release(kref);
 	}
 }
 

[patch] kref shrinkage patches -- 2 of 2 -- kref shrinkage

[Ravikiran G Thirumalai]

Here's the patch to change current kref users to use the modified 
kref_init and kref_put api (result of kref object shrinkage).

Thanks,
Kiran


 scsi/sd.c               |    8 +-
 scsi/sr.c               |    8 +-
 usb/core/config.c       |    5 +
 usb/core/message.c      |    3 -
 usb/core/urb.c          |    4 -
 usb/host/ehci-mem.c     |    4 -
 usb/serial/usb-serial.c |  136 ++++++++++++++++++++++++------------------------
 7 files changed, 85 insertions(+), 83 deletions(-)

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

diff -ruN -X dontdiff2 linux-2.6.7/drivers/scsi/sd.c kref-2.6.7/drivers/scsi/sd.c
--- linux-2.6.7/drivers/scsi/sd.c	2004-06-16 10:49:44.000000000 +0530
+++ kref-2.6.7/drivers/scsi/sd.c	2004-07-20 13:03:49.000000000 +0530
@@ -186,7 +186,7 @@
 	return sdkp;
 
  out_put:
-	kref_put(&sdkp->kref);
+	kref_put(&sdkp->kref, scsi_disk_release);
  out_sdkp:
 	sdkp = NULL;
  out:
@@ -198,7 +198,7 @@
 {
 	down(&sd_ref_sem);
 	scsi_device_put(sdkp->device);
-	kref_put(&sdkp->kref);
+	kref_put(&sdkp->kref, scsi_disk_release);
 	up(&sd_ref_sem);
 }
 
@@ -1360,7 +1360,7 @@
 		goto out;
 
 	memset (sdkp, 0, sizeof(*sdkp));
-	kref_init(&sdkp->kref, scsi_disk_release);
+	kref_init(&sdkp->kref);
 
 	/* Note: We can accomodate 64 partitions, but the genhd code
 	 * assumes partitions allocate consecutive minors, which they don't.
@@ -1462,7 +1462,7 @@
 	del_gendisk(sdkp->disk);
 	sd_shutdown(dev);
 	down(&sd_ref_sem);
-	kref_put(&sdkp->kref);
+	kref_put(&sdkp->kref, scsi_disk_release);
 	up(&sd_ref_sem);
 
 	return 0;
diff -ruN -X dontdiff2 linux-2.6.7/drivers/scsi/sr.c kref-2.6.7/drivers/scsi/sr.c
--- linux-2.6.7/drivers/scsi/sr.c	2004-06-16 10:50:27.000000000 +0530
+++ kref-2.6.7/drivers/scsi/sr.c	2004-07-20 13:03:49.000000000 +0530
@@ -144,7 +144,7 @@
 	goto out;
 
  out_put:
-	kref_put(&cd->kref);
+	kref_put(&cd->kref, sr_kref_release);
  out_null:
 	cd = NULL;
  out:
@@ -156,7 +156,7 @@
 {
 	down(&sr_ref_sem);
 	scsi_device_put(cd->device);
-	kref_put(&cd->kref);
+	kref_put(&cd->kref, sr_kref_release);
 	up(&sr_ref_sem);
 }
 
@@ -573,7 +573,7 @@
 		goto fail;
 	memset(cd, 0, sizeof(*cd));
 
-	kref_init(&cd->kref, sr_kref_release);
+	kref_init(&cd->kref);
 
 	disk = alloc_disk(1);
 	if (!disk)
@@ -952,7 +952,7 @@
 	del_gendisk(cd->disk);
 
 	down(&sr_ref_sem);
-	kref_put(&cd->kref);
+	kref_put(&cd->kref, sr_kref_release);
 	up(&sr_ref_sem);
 
 	return 0;
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/core/config.c kref-2.6.7/drivers/usb/core/config.c
--- linux-2.6.7/drivers/usb/core/config.c	2004-06-16 10:48:52.000000000 +0530
+++ kref-2.6.7/drivers/usb/core/config.c	2004-07-20 13:03:49.000000000 +0530
@@ -356,7 +356,7 @@
 		if (!intfc)
 			return -ENOMEM;
 		memset(intfc, 0, len);
-		kref_init(&intfc->ref, usb_release_interface_cache);
+		kref_init(&intfc->ref);
 	}
 
 	/* Skip over any Class Specific or Vendor Specific descriptors;
@@ -422,7 +422,8 @@
 
 		for (i = 0; i < cf->desc.bNumInterfaces; i++) {
 			if (cf->intf_cache[i])
-				kref_put(&cf->intf_cache[i]->ref);
+				kref_put(&cf->intf_cache[i]->ref, 
+					  usb_release_interface_cache);
 		}
 	}
 	kfree(dev->config);
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/core/message.c kref-2.6.7/drivers/usb/core/message.c
--- linux-2.6.7/drivers/usb/core/message.c	2004-06-16 10:49:02.000000000 +0530
+++ kref-2.6.7/drivers/usb/core/message.c	2004-07-20 15:07:24.000000000 +0530
@@ -1077,11 +1077,12 @@
 
 static void release_interface(struct device *dev)
 {
+	extern void destroy_serial(struct kref *kref);
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_interface_cache *intfc =
 			altsetting_to_usb_interface_cache(intf->altsetting);
 
-	kref_put(&intfc->ref);
+	kref_put(&intfc->ref, destroy_serial);
 	kfree(intf);
 }
 
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/core/urb.c kref-2.6.7/drivers/usb/core/urb.c
--- linux-2.6.7/drivers/usb/core/urb.c	2004-06-16 10:49:36.000000000 +0530
+++ kref-2.6.7/drivers/usb/core/urb.c	2004-07-20 13:03:49.000000000 +0530
@@ -39,7 +39,7 @@
 {
 	if (urb) {
 		memset(urb, 0, sizeof(*urb));
-		kref_init(&urb->kref, urb_destroy);
+		kref_init(&urb->kref);
 		spin_lock_init(&urb->lock);
 	}
 }
@@ -88,7 +88,7 @@
 void usb_free_urb(struct urb *urb)
 {
 	if (urb)
-		kref_put(&urb->kref);
+		kref_put(&urb->kref, urb_destroy);
 }
 
 /**
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/host/ehci-mem.c kref-2.6.7/drivers/usb/host/ehci-mem.c
--- linux-2.6.7/drivers/usb/host/ehci-mem.c	2004-06-16 10:50:03.000000000 +0530
+++ kref-2.6.7/drivers/usb/host/ehci-mem.c	2004-07-20 13:03:49.000000000 +0530
@@ -114,7 +114,7 @@
 		return qh;
 
 	memset (qh, 0, sizeof *qh);
-	kref_init(&qh->kref, qh_destroy);
+	kref_init(&qh->kref);
 	qh->ehci = ehci;
 	qh->qh_dma = dma;
 	// INIT_LIST_HEAD (&qh->qh_list);
@@ -139,7 +139,7 @@
 
 static inline void qh_put (struct ehci_qh *qh)
 {
-	kref_put(&qh->kref);
+	kref_put(&qh->kref, qh_destroy);
 }
 
 /*-------------------------------------------------------------------------*/
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/serial/usb-serial.c kref-2.6.7/drivers/usb/serial/usb-serial.c
--- linux-2.6.7/drivers/usb/serial/usb-serial.c	2004-06-16 10:49:17.000000000 +0530
+++ kref-2.6.7/drivers/usb/serial/usb-serial.c	2004-07-20 15:07:52.000000000 +0530
@@ -443,6 +443,69 @@
 	return;
 }
 
+static void serial_shutdown (struct usb_serial *serial)
+{
+	dbg ("%s", __FUNCTION__);
+
+	serial->type->shutdown(serial);
+}
+
+void destroy_serial(struct kref *kref)
+{
+	struct usb_serial *serial;
+	struct usb_serial_port *port;
+	int i;
+
+	serial = to_usb_serial(kref);
+
+	dbg ("%s - %s", __FUNCTION__, serial->type->name);
+	serial_shutdown (serial);
+
+	/* return the minor range that this device had */
+	return_serial(serial);
+
+	for (i = 0; i < serial->num_ports; ++i)
+		serial->port[i]->open_count = 0;
+
+	/* the ports are cleaned up and released in port_release() */
+	for (i = 0; i < serial->num_ports; ++i)
+		if (serial->port[i]->dev.parent != NULL) {
+			device_unregister(&serial->port[i]->dev);
+			serial->port[i] = NULL;
+		}
+
+	/* If this is a "fake" port, we have to clean it up here, as it will
+	 * not get cleaned up in port_release() as it was never registered with
+	 * the driver core */
+	if (serial->num_ports < serial->num_port_pointers) {
+		for (i = serial->num_ports; i < serial->num_port_pointers; ++i) {
+			port = serial->port[i];
+			if (!port)
+				continue;
+			if (port->read_urb) {
+				usb_unlink_urb(port->read_urb);
+				usb_free_urb(port->read_urb);
+			}
+			if (port->write_urb) {
+				usb_unlink_urb(port->write_urb);
+				usb_free_urb(port->write_urb);
+			}
+			if (port->interrupt_in_urb) {
+				usb_unlink_urb(port->interrupt_in_urb);
+				usb_free_urb(port->interrupt_in_urb);
+			}
+			kfree(port->bulk_in_buffer);
+			kfree(port->bulk_out_buffer);
+			kfree(port->interrupt_in_buffer);
+		}
+	}
+
+	usb_put_dev(serial->dev);
+
+	/* free up any memory that we allocated */
+	kfree (serial);
+}
+
 /*****************************************************************************
  * Driver tty interface functions
  *****************************************************************************/
@@ -487,7 +550,7 @@
 		if (retval) {
 			port->open_count = 0;
 			module_put(serial->type->owner);
-			kref_put(&serial->kref);
+			kref_put(&serial->kref, destroy_serial);
 		}
 	}
 bailout:
@@ -518,7 +581,7 @@
 	}
 
 	module_put(port->serial->type->owner);
-	kref_put(&port->serial->kref);
+	kref_put(&port->serial->kref, destroy_serial);
 }
 
 static int serial_write (struct tty_struct * tty, int from_user, const unsigned char *buf, int count)
@@ -676,13 +739,6 @@
 	;
 }
 
-static void serial_shutdown (struct usb_serial *serial)
-{
-	dbg ("%s", __FUNCTION__);
-
-	serial->type->shutdown(serial);
-}
-
 static int serial_read_proc (char *page, char **start, off_t off, int count, int *eof, void *data)
 {
 	struct usb_serial *serial;
@@ -716,7 +772,7 @@
 			begin += length;
 			length = 0;
 		}
-		kref_put(&serial->kref);
+		kref_put(&serial->kref, destroy_serial);
 	}
 	*eof = 1;
 done:
@@ -785,62 +841,6 @@
 	wake_up_interruptible(&tty->write_wait);
 }
 
-static void destroy_serial(struct kref *kref)
-{
-	struct usb_serial *serial;
-	struct usb_serial_port *port;
-	int i;
-
-	serial = to_usb_serial(kref);
-
-	dbg ("%s - %s", __FUNCTION__, serial->type->name);
-	serial_shutdown (serial);
-
-	/* return the minor range that this device had */
-	return_serial(serial);
-
-	for (i = 0; i < serial->num_ports; ++i)
-		serial->port[i]->open_count = 0;
-
-	/* the ports are cleaned up and released in port_release() */
-	for (i = 0; i < serial->num_ports; ++i)
-		if (serial->port[i]->dev.parent != NULL) {
-			device_unregister(&serial->port[i]->dev);
-			serial->port[i] = NULL;
-		}
-
-	/* If this is a "fake" port, we have to clean it up here, as it will
-	 * not get cleaned up in port_release() as it was never registered with
-	 * the driver core */
-	if (serial->num_ports < serial->num_port_pointers) {
-		for (i = serial->num_ports; i < serial->num_port_pointers; ++i) {
-			port = serial->port[i];
-			if (!port)
-				continue;
-			if (port->read_urb) {
-				usb_unlink_urb(port->read_urb);
-				usb_free_urb(port->read_urb);
-			}
-			if (port->write_urb) {
-				usb_unlink_urb(port->write_urb);
-				usb_free_urb(port->write_urb);
-			}
-			if (port->interrupt_in_urb) {
-				usb_unlink_urb(port->interrupt_in_urb);
-				usb_free_urb(port->interrupt_in_urb);
-			}
-			kfree(port->bulk_in_buffer);
-			kfree(port->bulk_out_buffer);
-			kfree(port->interrupt_in_buffer);
-		}
-	}
-
-	usb_put_dev(serial->dev);
-
-	/* free up any memory that we allocated */
-	kfree (serial);
-}
-
 static void port_release(struct device *dev)
 {
 	struct usb_serial_port *port = to_usb_serial_port(dev);
@@ -881,7 +881,7 @@
 	serial->interface = interface;
 	serial->vendor = dev->descriptor.idVendor;
 	serial->product = dev->descriptor.idProduct;
-	kref_init(&serial->kref, destroy_serial);
+	kref_init(&serial->kref);
 
 	return serial;
 }
@@ -1231,7 +1231,7 @@
 	if (serial) {
 		/* let the last holder of this object 
 		 * cause it to be cleaned up */
-		kref_put(&serial->kref);
+		kref_put(&serial->kref, destroy_serial);
 	}
 	dev_info(dev, "device disconnected\n");
 }

Re: [patch] kref shrinkage patches -- 1 of 2 -- kref shrinkage

[Greg KH]

On Tue, Jul 20, 2004 at 05:53:11PM +0530, Ravikiran G Thirumalai wrote:
> Greg,
> Here's the first step towards getting to kref_get_rcu :)
> This patch is to shrink the kref object by removing the 
> function pointer 'release' from the kref object, and modifying
> kref_init and kref_put interfaces so that kref_init will not take
> the release pointer anymore and kref_put will.  Patch will probably talk
> better.

Nice patches, I've applied them with a few tweaks, as noted below.

> Just had a question about definition of kref_get though, 
> why does it need to return struct kref * ? struct kref * is anywayz 
> being passed to it, hence the caller has it anywayz -- so it doesn't 
> value add anything afaics (but i might have limited vision so pls correct me)

Consistancy with other "get" and "put" functions.  It's quite common to
do:
	have_this_foo(foo_get(foo));

or:
	foo_to_send_off = foo_get(foo);

> In fact, I see that the scsi applications  of kref have code like
> 
> <quote drivers/scsi/sd.c>
> 
>         if (!kref_get(&sdkp->kref))
>                 goto out_sdkp;
> 
> </>

Heh, yeah, that's just dumb, as that failure will never happen :)


> diff -ruN -X dontdiff2 linux-2.6.7/include/linux/kref.h kref-2.6.7/include/linux/kref.h
> --- linux-2.6.7/include/linux/kref.h	2004-06-16 10:48:59.000000000 +0530
> +++ kref-2.6.7/include/linux/kref.h	2004-07-20 12:53:24.226739304 +0530
> @@ -8,6 +8,9 @@
>   * Copyright (C) 2002-2003 Patrick Mochel <mochel@osdl.org>
>   * Copyright (C) 2002-2003 Open Source Development Labs
>   *
> + * 07/2004 - struct kref shrinkage by Ravikiran Thirumalai <kiran@in.ibm.com>
> + * Copyright (C) 2004 IBM Corp.
> + *
>   * This file is released under the GPLv2.
>   *
>   */

This is not needed.  First off, the Copyright statement for IBM is
already included in this file, for this same year, above these lines.
Also, we are not including file history in the files themselves, that's
what Changelog comments are for (which scale much better over time.)

> @@ -44,14 +42,17 @@
>  /**
>   * kref_put - decrement refcount for object.
>   * @kref: object.
> - *
> - * Decrement the refcount, and if 0, call kref->release().
> + * @release: pointer to the function that will clean up the object
> + *	     when the last reference to the object is released.
> + *	     This pointer is required.
> + * Decrement the refcount, and if 0, call release().
>   */

The extra blank line in tha comment was needed for docbook to work
properly, last time I checked.

> -void kref_put(struct kref *kref)
> +void kref_put(struct kref *kref, void (*release) (struct kref *kref))
>  {
> +	WARN_ON(release == NULL);

I also added a check here to test if release == kfree to catch people
like the s390 developers who want to do nasty hacks :)

thanks,

greg k-h

Re: [patch] kref shrinkage patches -- 2 of 2 -- kref shrinkage

[Greg KH]

On Tue, Jul 20, 2004 at 05:57:01PM +0530, Ravikiran G Thirumalai wrote:
> Here's the patch to change current kref users to use the modified 
> kref_init and kref_put api (result of kref object shrinkage).

I applied this too, with some tweaks to the usb-serial code.

thanks,

greg k-h

Re: [patch] kref shrinkage patches -- 1 of 2 -- kref shrinkage

[Ravikiran G Thirumalai]

On Wed, Jul 21, 2004 at 02:14:18AM -0400, Greg KH wrote:
> ... 
> > Just had a question about definition of kref_get though, 
> > why does it need to return struct kref * ? struct kref * is anywayz 
> > being passed to it, hence the caller has it anywayz -- so it doesn't 
> > value add anything afaics (but i might have limited vision so pls correct me)
> 
> Consistancy with other "get" and "put" functions.  It's quite common to
> do:
> 	have_this_foo(foo_get(foo));
> 
> or:
> 	foo_to_send_off = foo_get(foo);
> 

kref_get is invoked from inside foo_get in such cases no? so foo_get
can do kref_get and return struct *foo...like in usb_serial_get_by_index?

Even assuming foo_get is altogether replaced with kref_get, which might 
not be a very common case (looking at the current usage, and future usage in
fget_light etc)
we will still have to do
	foo_to_send_off = KREF_TO_FOO(kref_get(&foo->kref));
instead :
	kref_get(&foo->kref);
	foo_to_send_off = foo;
is better right?  Why use a macro for just one extra line of code.... 
Looking at the current usage and looking at refcount 'getters'
like fget, mntget etc., 'doesn't look like people will use the return
value from kref_get much anywayz.  Since the api is young is it better to
kill the return value now than let people misuse the return type like in the
scsi subsystem? 

Thanks,
Kiran