[patch 0/2] kref debugging (retry)

[patch 0/2] kref debugging (retry)

[Akinobu Mita]

This patch series enable to detect kref_put() with unreferenced object,
and split all kref debugging checks into new config debug option.

I can find many places where I can replace refcounter with kref by doing
"grep -r atomic_dec_and_test linux/".

If we have this detection of kref_put() with unreferenced object,
The work of kref convertions would be more than code cleanup and
consolidation.
--

[patch 1/2] kref: detect kref_put() with unreferenced object

[Akinobu Mita]

[-- Attachment #1: kref-more-warn.patch --]
[-- Type: text/plain, Size: 1711 bytes --]

This patch adds warning to detect kref_put() with unreferenced object.

The idea of detection kref_put() with unreferenced object was stolen
from BUG_ON()es in blocks/ll_rw_blk.c and fs/bio.c

ll_rw_blk.c:    BUG_ON(atomic_read(&ioc->refcount) == 0);

bio.c:          BIO_BUG_ON(!atomic_read(&bio->bi_cnt));

But the kref counter usually does not fall to zero. Because kref is
trying to reduce the number of atomic_dec_and_test()

So this patch also set kref counter to zero here:

+	if (atomic_read(&kref->refcount) == 1)
+		atomic_set(&kref->refcount, 0);

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
CC: Greg KH <greg@kroah.com>
CC: Patrick Mochel <mochel@osdl.org>

 lib/kref.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

Index: 2.6-git/lib/kref.c
===================================================================
--- 2.6-git.orig/lib/kref.c
+++ 2.6-git/lib/kref.c
@@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
  */
 int kref_put(struct kref *kref, void (*release)(struct kref *kref))
 {
+	WARN_ON(atomic_read(&kref->refcount) < 1);
 	WARN_ON(release == NULL);
 	WARN_ON(release == (void (*)(struct kref *))kfree);
 
@@ -56,12 +57,13 @@ int kref_put(struct kref *kref, void (*r
 	 * if current count is one, we are the last user and can release object
 	 * right now, avoiding an atomic operation on 'refcount'
 	 */
-	if ((atomic_read(&kref->refcount) == 1) ||
-	    (atomic_dec_and_test(&kref->refcount))) {
-		release(kref);
-		return 1;
-	}
-	return 0;
+	if (atomic_read(&kref->refcount) == 1)
+		atomic_set(&kref->refcount, 0);
+	else if (!atomic_dec_and_test(&kref->refcount))
+		return 0;
+
+	release(kref);
+	return 1;
 }
 
 EXPORT_SYMBOL(kref_init);

--

Re: [patch 1/2] kref: detect kref_put() with unreferenced object

[Greg KH]

On Tue, Apr 25, 2006 at 04:21:38PM +0800, Akinobu Mita wrote:
> This patch adds warning to detect kref_put() with unreferenced object.
> 
> The idea of detection kref_put() with unreferenced object was stolen
> from BUG_ON()es in blocks/ll_rw_blk.c and fs/bio.c
> 
> ll_rw_blk.c:    BUG_ON(atomic_read(&ioc->refcount) == 0);
> 
> bio.c:          BIO_BUG_ON(!atomic_read(&bio->bi_cnt));
> 
> But the kref counter usually does not fall to zero. Because kref is
> trying to reduce the number of atomic_dec_and_test()
> 
> So this patch also set kref counter to zero here:
> 
> +	if (atomic_read(&kref->refcount) == 1)
> +		atomic_set(&kref->refcount, 0);

I really don't see how this is going to make it easier to debug
anything.  Remember, when a kref goes to 0, it is automatically freed.
So any code that tries to use it afterward just dies a horrible death,
if CONFIG_DEBUG_SLAB is enabled.

And please be careful, you can't replace all usages of atomic_t counters
in the kernel with krefs...

thanks,

greg k-h

Re: [patch 1/2] kref: detect kref_put() with unreferenced object

[Akinobu Mita]

> I really don't see how this is going to make it easier to debug
> anything.  Remember, when a kref goes to 0, it is automatically freed.
> So any code that tries to use it afterward just dies a horrible death,
> if CONFIG_DEBUG_SLAB is enabled.

Thanks, I finally understood it is not needed as you said.
So I can just remove that BUG_ON()es for detecting zero atomic_t counter
while I replace atomic_t counters with kref.

[patch 2/2] kref: kref debugging config option

[Akinobu Mita]

[-- Attachment #1: kref-debug.patch --]
[-- Type: text/plain, Size: 2317 bytes --]

make all kref debugging checks new config option.

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
CC: Greg KH <greg@kroah.com>
CC: Patrick Mochel <mochel@osdl.org>

 lib/Kconfig.debug |    7 +++++++
 lib/kref.c        |   30 +++++++++++++++++++++++++-----
 2 files changed, 32 insertions(+), 5 deletions(-)

Index: 2.6-git/lib/Kconfig.debug
===================================================================
--- 2.6-git.orig/lib/Kconfig.debug
+++ 2.6-git/lib/Kconfig.debug
@@ -130,6 +130,13 @@ config DEBUG_KOBJECT
 	  If you say Y here, some extra kobject debugging messages will be sent
 	  to the syslog. 
 
+config DEBUG_KREF
+	bool "kref debugging"
+	depends on DEBUG_KERNEL
+	help
+	  This option enables addition error checking for kref,
+	  library routines for handling generic reference counted objects.
+
 config DEBUG_HIGHMEM
 	bool "Highmem debugging"
 	depends on DEBUG_KERNEL && HIGHMEM
Index: 2.6-git/lib/kref.c
===================================================================
--- 2.6-git.orig/lib/kref.c
+++ 2.6-git/lib/kref.c
@@ -20,16 +20,38 @@
  */
 void kref_init(struct kref *kref)
 {
-	atomic_set(&kref->refcount,1);
+	atomic_set(&kref->refcount, 1);
 }
 
+#ifdef CONFIG_DEBUG_KREF
+static void kref_get_debug_check(struct kref *kref)
+{
+	WARN_ON(!atomic_read(&kref->refcount));
+}
+static void kref_put_debug_check(struct kref *kref,
+				void (*release)(struct kref *kref))
+{
+	WARN_ON(atomic_read(&kref->refcount) < 1);
+	WARN_ON(release == NULL);
+	WARN_ON(release == (void (*)(struct kref *))kfree);
+}
+#else
+static void kref_get_debug_check(struct kref *kref)
+{
+}
+static void kref_put_debug_check(struct kref *kref,
+				void (*release)(struct kref *kref))
+{
+}
+#endif
+
 /**
  * kref_get - increment refcount for object.
  * @kref: object.
  */
 void kref_get(struct kref *kref)
 {
-	WARN_ON(!atomic_read(&kref->refcount));
+	kref_get_debug_check(kref);
 	atomic_inc(&kref->refcount);
 }
 
@@ -49,9 +71,7 @@ void kref_get(struct kref *kref)
  */
 int kref_put(struct kref *kref, void (*release)(struct kref *kref))
 {
-	WARN_ON(atomic_read(&kref->refcount) < 1);
-	WARN_ON(release == NULL);
-	WARN_ON(release == (void (*)(struct kref *))kfree);
+	kref_put_debug_check(kref, release);
 
 	/*
 	 * if current count is one, we are the last user and can release object

--

Re: [patch 2/2] kref: kref debugging config option

[Greg KH]

On Tue, Apr 25, 2006 at 04:21:39PM +0800, Akinobu Mita wrote:
> -	WARN_ON(atomic_read(&kref->refcount) < 1);
> -	WARN_ON(release == NULL);
> -	WARN_ON(release == (void (*)(struct kref *))kfree);

No, those two last WARN_ON() are not "debugging" checks, they are
"checks for people trying to do bad things with kref code".  I always
want them "on", as there should not be any excuse for anyone to do this.

So I don't think this patch is needed at all.

thanks,

greg k-h