Skip to contentSkip to conversation

kref shrinkage patches -- 1 of 2 -- kref shrinkage

PATCH In review Ravikiran G Thirumalai 2 participants 5 messages Jul 20, 2004 – Jul 21, 2004

Subsystems (5)
SCSI USB Library code USB serial +1 more subsystems
lore
Share & tools
lore

Ravikiran G Thirumalai

author
#0 lore raw

PATCH 1/1 kref shrinkage patches -- 1 of 2 -- kref shrinkage

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 <[email protected]>
include/linux/kref.h +5 -3
diff -ruN -X dontdiff2 linux-2.6.7/include/linux/kref.h kref-2.6.7/include/linux/kref.h
@@ -8,6 +8,9 @@
  * Copyright (C) 2002-2003 Patrick Mochel <[email protected]>
  * Copyright (C) 2002-2003 Open Source Development Labs
  *
+ * 07/2004 - struct kref shrinkage by Ravikiran Thirumalai <[email protected]>
+ * 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_ */
lib/kref.c +11 -10
diff -ruN -X dontdiff2 linux-2.6.7/lib/kref.c kref-2.6.7/lib/kref.c
@@ -7,6 +7,9 @@
  * based on lib/kobject.c which was:
  * Copyright (C) 2002-2003 Patrick Mochel <[email protected]>
  *
+ * 07/2004 - kref object shrinkage by Ravikiran Thirumalai <[email protected]>
+ * 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);
 	}
 }
author
#1 lore raw

PATCH 1/1 kref shrinkage patches -- 2 of 2 -- kref shrinkage

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 <[email protected]>
drivers/scsi/sd.c +4 -4
diff -ruN -X dontdiff2 linux-2.6.7/drivers/scsi/sd.c kref-2.6.7/drivers/scsi/sd.c
@@ -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;
drivers/scsi/sr.c +4 -4
diff -ruN -X dontdiff2 linux-2.6.7/drivers/scsi/sr.c kref-2.6.7/drivers/scsi/sr.c
@@ -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;
drivers/usb/core/config.c +3 -2
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/core/config.c kref-2.6.7/drivers/usb/core/config.c
@@ -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);
drivers/usb/core/message.c +2 -1
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/core/message.c kref-2.6.7/drivers/usb/core/message.c
@@ -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);
 }
drivers/usb/core/urb.c +2 -2
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/core/urb.c kref-2.6.7/drivers/usb/core/urb.c
@@ -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);
 }
 
 /**
drivers/usb/host/ehci-mem.c +2 -2
diff -ruN -X dontdiff2 linux-2.6.7/drivers/usb/host/ehci-mem.c kref-2.6.7/drivers/usb/host/ehci-mem.c
@@ -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);
 }
 
 /*-------------------------------------------------------------------------*/
drivers/usb/serial/usb-serial.c +68 -68

184 lines not on this page · raw message

Applied
#2 lore raw

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

Quoted 2 lines · 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).

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

thanks,

greg k-h

#3 lore raw
In reply to Ravikiran G Thirumalai
Quoted 7 lines · 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.

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

Quoted 4 lines

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);
Quoted 8 lines

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 :)

include/linux/kref.h line 16

Quoted 13 lines

diff -ruN -X dontdiff2 linux-2.6.7/include/linux/kref.h kref-2.6.7/include/linux/kref.h

include/linux/kref.h +3 -0
@@ -8,6 +8,9 @@
  * Copyright (C) 2002-2003 Patrick Mochel <[email protected]>
  * Copyright (C) 2002-2003 Open Source Development Labs
  *
+ * 07/2004 - struct kref shrinkage by Ravikiran Thirumalai <[email protected]>
+ * 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.)

include/linux/kref.h line 49

Quoted 11 lines
include/linux/kref.h +4 -2
@@ -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.

include/linux/kref.h line 52

Quoted 4 lines
include/linux/kref.h +2 -1
-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

author
#4 lore raw
Quoted 6 lines · Greg KH

...

Quoted 4 lines

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)

Quoted 7 lines

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