[patch 0/3] use kref

[patch 0/3] use kref

[Akinobu Mita]

Use kref instead of atomic reference counter at:

- blk_queue_tag
- io_context
- bio

--

[patch 1/3] use kref for blk_queue_tag

[Akinobu Mita]

[-- Attachment #1: blk-queue-tag-use-kref.patch --]
[-- Type: text/plain, Size: 2369 bytes --]

Use kref for reference counter of blk_queue_tag.

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
CC: Jens Axboe <axboe@suse.de>
CC: Greg KH <greg@kroah.com>

 block/ll_rw_blk.c      |   35 ++++++++++++++++++++---------------
 include/linux/blkdev.h |    2 +-
 2 files changed, 21 insertions(+), 16 deletions(-)

Index: 2.6-git/block/ll_rw_blk.c
===================================================================
--- 2.6-git.orig/block/ll_rw_blk.c
+++ 2.6-git/block/ll_rw_blk.c
@@ -848,6 +848,23 @@ struct request *blk_queue_find_tag(reque
 
 EXPORT_SYMBOL(blk_queue_find_tag);
 
+static void release_blk_queue_tag(struct kref *kref)
+{
+	struct blk_queue_tag *bqt = container_of(kref, struct blk_queue_tag,
+			kref);
+
+	BUG_ON(bqt->busy);
+	BUG_ON(!list_empty(&bqt->busy_list));
+
+	kfree(bqt->tag_index);
+	bqt->tag_index = NULL;
+
+	kfree(bqt->tag_map);
+	bqt->tag_map = NULL;
+
+	kfree(bqt);
+}
+
 /**
  * __blk_queue_free_tags - release tag maintenance info
  * @q:  the request queue for the device
@@ -863,19 +880,7 @@ static void __blk_queue_free_tags(reques
 	if (!bqt)
 		return;
 
-	if (atomic_dec_and_test(&bqt->refcnt)) {
-		BUG_ON(bqt->busy);
-		BUG_ON(!list_empty(&bqt->busy_list));
-
-		kfree(bqt->tag_index);
-		bqt->tag_index = NULL;
-
-		kfree(bqt->tag_map);
-		bqt->tag_map = NULL;
-
-		kfree(bqt);
-	}
-
+	kref_put(&bqt->kref, release_blk_queue_tag);
 	q->queue_tags = NULL;
 	q->queue_flags &= ~(1 << QUEUE_FLAG_QUEUED);
 }
@@ -951,14 +956,14 @@ int blk_queue_init_tags(request_queue_t 
 
 		INIT_LIST_HEAD(&tags->busy_list);
 		tags->busy = 0;
-		atomic_set(&tags->refcnt, 1);
+		kref_init(&tags->kref);
 	} else if (q->queue_tags) {
 		if ((rc = blk_queue_resize_tags(q, depth)))
 			return rc;
 		set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
 		return 0;
 	} else
-		atomic_inc(&tags->refcnt);
+		kref_get(&tags->kref);
 
 	/*
 	 * assign it, all done
Index: 2.6-git/include/linux/blkdev.h
===================================================================
--- 2.6-git.orig/include/linux/blkdev.h
+++ 2.6-git/include/linux/blkdev.h
@@ -315,7 +315,7 @@ struct blk_queue_tag {
 	int busy;			/* current depth */
 	int max_depth;			/* what we will send to device */
 	int real_max_depth;		/* what the array can hold */
-	atomic_t refcnt;		/* map can be shared */
+	struct kref kref;		/* map can be shared */
 };
 
 struct request_queue

--

Re: [patch 1/3] use kref for blk_queue_tag

[Nick Piggin]

Akinobu Mita wrote:

>Use kref for reference counter of blk_queue_tag.
>
>

Indirect function calls can be pretty expensive. I'd rather not
convert performance critical code 'just because'.

--

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [patch 1/3] use kref for blk_queue_tag

[Greg KH]

On Wed, Apr 26, 2006 at 10:11:00AM +0800, Akinobu Mita wrote:
> Use kref for reference counter of blk_queue_tag.

Why?

Doesn't this, and your other patches here affect performance?

thanks,

greg k-h

[patch 2/3] use kref for io_context

[Akinobu Mita]

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

Use kref for reference counter of io_context.
This patch also removes BUG_ON() for freeing unreferenced io_context.
But kref can detect it if CONFIG_DEBUG_SLAB is enabled.

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
CC: Jens Axboe <axboe@suse.de>
CC: Greg KH <greg@kroah.com>

 block/cfq-iosched.c    |    2 +-
 block/ll_rw_blk.c      |   43 +++++++++++++++++++++----------------------
 include/linux/blkdev.h |    2 +-
 3 files changed, 23 insertions(+), 24 deletions(-)

Index: 2.6-git/block/ll_rw_blk.c
===================================================================
--- 2.6-git.orig/block/ll_rw_blk.c
+++ 2.6-git/block/ll_rw_blk.c
@@ -3536,29 +3536,29 @@ int __init blk_dev_init(void)
 /*
  * IO Context helper functions
  */
+static void release_io_context(struct kref *kref)
+{
+	struct io_context *ioc = container_of(kref, struct io_context, kref);
+	struct cfq_io_context *cic;
+
+	rcu_read_lock();
+	if (ioc->aic && ioc->aic->dtor)
+		ioc->aic->dtor(ioc->aic);
+	if (ioc->cic_root.rb_node != NULL) {
+		struct rb_node *n = rb_first(&ioc->cic_root);
+		cic = rb_entry(n, struct cfq_io_context, rb_node);
+		cic->dtor(ioc);
+	}
+	rcu_read_unlock();
+	kmem_cache_free(iocontext_cachep, ioc);
+}
+
 void put_io_context(struct io_context *ioc)
 {
 	if (ioc == NULL)
 		return;
 
-	BUG_ON(atomic_read(&ioc->refcount) == 0);
-
-	if (atomic_dec_and_test(&ioc->refcount)) {
-		struct cfq_io_context *cic;
-
-		rcu_read_lock();
-		if (ioc->aic && ioc->aic->dtor)
-			ioc->aic->dtor(ioc->aic);
-		if (ioc->cic_root.rb_node != NULL) {
-			struct rb_node *n = rb_first(&ioc->cic_root);
-
-			cic = rb_entry(n, struct cfq_io_context, rb_node);
-			cic->dtor(ioc);
-		}
-		rcu_read_unlock();
-
-		kmem_cache_free(iocontext_cachep, ioc);
-	}
+	kref_put(&ioc->kref, release_io_context);
 }
 EXPORT_SYMBOL(put_io_context);
 
@@ -3606,7 +3606,7 @@ struct io_context *current_io_context(gf
 
 	ret = kmem_cache_alloc(iocontext_cachep, gfp_flags);
 	if (ret) {
-		atomic_set(&ret->refcount, 1);
+		kref_init(&ret->kref);
 		ret->task = current;
 		ret->set_ioprio = NULL;
 		ret->last_waited = jiffies; /* doesn't matter... */
@@ -3631,7 +3631,7 @@ struct io_context *get_io_context(gfp_t 
 	struct io_context *ret;
 	ret = current_io_context(gfp_flags);
 	if (likely(ret))
-		atomic_inc(&ret->refcount);
+		kref_get(&ret->kref);
 	return ret;
 }
 EXPORT_SYMBOL(get_io_context);
@@ -3642,8 +3642,7 @@ void copy_io_context(struct io_context *
 	struct io_context *dst = *pdst;
 
 	if (src) {
-		BUG_ON(atomic_read(&src->refcount) == 0);
-		atomic_inc(&src->refcount);
+		kref_get(&src->kref);
 		put_io_context(dst);
 		*pdst = src;
 	}
Index: 2.6-git/include/linux/blkdev.h
===================================================================
--- 2.6-git.orig/include/linux/blkdev.h
+++ 2.6-git/include/linux/blkdev.h
@@ -88,7 +88,7 @@ struct cfq_io_context {
  * (apart from the atomic refcount), so require no locking.
  */
 struct io_context {
-	atomic_t refcount;
+	struct kref kref;
 	struct task_struct *task;
 
 	int (*set_ioprio)(struct io_context *, unsigned int);
Index: 2.6-git/block/cfq-iosched.c
===================================================================
--- 2.6-git.orig/block/cfq-iosched.c
+++ 2.6-git/block/cfq-iosched.c
@@ -1068,7 +1068,7 @@ __cfq_dispatch_requests(struct cfq_data 
 		dispatched++;
 
 		if (!cfqd->active_cic) {
-			atomic_inc(&crq->io_context->ioc->refcount);
+			kref_get(&crq->io_context->ioc->kref);
 			cfqd->active_cic = crq->io_context;
 		}
 

--

[patch 3/3] use kref for bio

[Akinobu Mita]

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

Use kref for reference counter of bio.
This patch also removes BUG_ON() for freeing unreferenced bio.
But kref can detect it if CONFIG_DEBUG_SLAB is enabled.

Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
CC: Jens Axboe <axboe@suse.de>
CC: Greg KH <greg@kroah.com>

 Documentation/block/biodoc.txt |    2 +-
 fs/bio.c                       |   17 ++++++++++-------
 fs/xfs/linux-2.6/xfs_aops.c    |    1 -
 include/linux/bio.h            |    4 ++--
 4 files changed, 13 insertions(+), 11 deletions(-)

Index: 2.6-git/Documentation/block/biodoc.txt
===================================================================
--- 2.6-git.orig/Documentation/block/biodoc.txt
+++ 2.6-git/Documentation/block/biodoc.txt
@@ -462,7 +462,7 @@ struct bio {
                                         used as index into pool */
        struct bio_vec   *bi_io_vec;  /* the actual vec list */
        bio_end_io_t	*bi_end_io;  /* bi_end_io (bio) */
-       atomic_t		bi_cnt;	     /* pin count: free when it hits zero */
+       struct kref	bi_kref;     /* pin count */
        void             *bi_private;
        bio_destructor_t *bi_destructor; /* bi_destructor (bio) */
 };
Index: 2.6-git/fs/bio.c
===================================================================
--- 2.6-git.orig/fs/bio.c
+++ 2.6-git/fs/bio.c
@@ -139,7 +139,7 @@ void bio_init(struct bio *bio)
 	bio->bi_size = 0;
 	bio->bi_max_vecs = 0;
 	bio->bi_end_io = NULL;
-	atomic_set(&bio->bi_cnt, 1);
+	kref_init(&bio->bi_kref);
 	bio->bi_private = NULL;
 }
 
@@ -208,6 +208,14 @@ void zero_fill_bio(struct bio *bio)
 }
 EXPORT_SYMBOL(zero_fill_bio);
 
+static void release_bio(struct kref *kref)
+{
+	struct bio *bio = container_of(kref, struct bio, bi_kref);
+
+	bio->bi_next = NULL;
+	bio->bi_destructor(bio);
+}
+
 /**
  * bio_put - release a reference to a bio
  * @bio:   bio to release reference to
@@ -218,15 +226,10 @@ EXPORT_SYMBOL(zero_fill_bio);
  **/
 void bio_put(struct bio *bio)
 {
-	BIO_BUG_ON(!atomic_read(&bio->bi_cnt));
-
 	/*
 	 * last put frees it
 	 */
-	if (atomic_dec_and_test(&bio->bi_cnt)) {
-		bio->bi_next = NULL;
-		bio->bi_destructor(bio);
-	}
+	kref_put(&bio->bi_kref, release_bio);
 }
 
 inline int bio_phys_segments(request_queue_t *q, struct bio *bio)
Index: 2.6-git/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- 2.6-git.orig/fs/xfs/linux-2.6/xfs_aops.c
+++ 2.6-git/fs/xfs/linux-2.6/xfs_aops.c
@@ -272,7 +272,6 @@ xfs_end_bio(
 		return 1;
 
 	ASSERT(ioend);
-	ASSERT(atomic_read(&bio->bi_cnt) >= 1);
 
 	/* Toss bio and pass work off to an xfsdatad thread */
 	if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
Index: 2.6-git/include/linux/bio.h
===================================================================
--- 2.6-git.orig/include/linux/bio.h
+++ 2.6-git/include/linux/bio.h
@@ -106,7 +106,7 @@ struct bio {
 	struct bio_vec		*bi_io_vec;	/* the actual vec list */
 
 	bio_end_io_t		*bi_end_io;
-	atomic_t		bi_cnt;		/* pin count */
+	struct kref		bi_kref;	/* pin count */
 
 	void			*bi_private;
 
@@ -249,7 +249,7 @@ struct bio {
  * returns. and then bio would be freed memory when if (bio->bi_flags ...)
  * runs
  */
-#define bio_get(bio)	atomic_inc(&(bio)->bi_cnt)
+#define bio_get(bio)	kref_get(&(bio)->bi_kref)
 
 
 /*

--

Re: [patch 3/3] use kref for bio

[Al Viro]

On Wed, Apr 26, 2006 at 10:11:02AM +0800, Akinobu Mita wrote:
> Use kref for reference counter of bio.
> This patch also removes BUG_ON() for freeing unreferenced bio.
> But kref can detect it if CONFIG_DEBUG_SLAB is enabled.
 
*Ugh*

Let's _not_.  It's extra overhead for no good reason.

Just in case: any kref conversion for
	* super_block
	* inode
	* dentry
	* file
is NAKed while we are at it.

Re: [patch 3/3] use kref for bio

[Jens Axboe]

On Wed, Apr 26 2006, Al Viro wrote:
> On Wed, Apr 26, 2006 at 10:11:02AM +0800, Akinobu Mita wrote:
> > Use kref for reference counter of bio.
> > This patch also removes BUG_ON() for freeing unreferenced bio.
> > But kref can detect it if CONFIG_DEBUG_SLAB is enabled.
>  
> *Ugh*
> 
> Let's _not_.  It's extra overhead for no good reason.

Completely agree. That goes for the other block layer kref patches as
well.

-- 
Jens Axboe

Re: [patch 3/3] use kref for bio

[Greg KH]

On Wed, Apr 26, 2006 at 07:13:44AM +0200, Jens Axboe wrote:
> On Wed, Apr 26 2006, Al Viro wrote:
> > On Wed, Apr 26, 2006 at 10:11:02AM +0800, Akinobu Mita wrote:
> > > Use kref for reference counter of bio.
> > > This patch also removes BUG_ON() for freeing unreferenced bio.
> > > But kref can detect it if CONFIG_DEBUG_SLAB is enabled.
> >  
> > *Ugh*
> > 
> > Let's _not_.  It's extra overhead for no good reason.
> 
> Completely agree. That goes for the other block layer kref patches as
> well.

I also agree, there's a reason I never tried to convert them in the past
:)

thanks,

greg k-h

Re: [patch 3/3] use kref for bio

[Akinobu Mita]

On Tue, Apr 25, 2006 at 10:18:13PM -0700, Greg KH wrote:
> > > Let's _not_.  It's extra overhead for no good reason.
> > 
> > Completely agree. That goes for the other block layer kref patches as
> > well.
> 
> I also agree, there's a reason I never tried to convert them in the past
> :)

kref has faster function for decrement refcount.

kref_put()
{
...

	/*
	 * 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;
	}

If this is good one and the places where Al Viro pointed out really affect
performance, should we propagate this faster one by introducing helper
function like:

static inline int refcount_test(atomic_t *refcount)
{
	return (atomic_read(refcount) == 1) || (atomic_dec_and_test(refcount));
}

and replace atomic_dec_and_test with it?

Re: [patch 3/3] use kref for bio

[Al Viro]

On Wed, Apr 26, 2006 at 03:20:30PM +0800, Akinobu Mita wrote:
> If this is good one and the places where Al Viro pointed out really affect
> performance, should we propagate this faster one by introducing helper
> function like:
> 
> static inline int refcount_test(atomic_t *refcount)
> {
> 	return (atomic_read(refcount) == 1) || (atomic_dec_and_test(refcount));
> }
> 
> and replace atomic_dec_and_test with it?

No.  It's obviously slower than atomic_dec_and_test() if refcount is
greater than 1.  And I'm less than sure that you can show that benefits
in case when it is 1 outweight that.  Moreover, for dentries, inodes,
superblocks and vfsmounts you'd have to pull spin_lock() in front of
it, which would _definitely_ hurt (these are atomic_dec_and_lock()).

Re: [patch 3/3] use kref for bio

[Nick Piggin]

Al Viro wrote:
> On Wed, Apr 26, 2006 at 03:20:30PM +0800, Akinobu Mita wrote:
> 
>>If this is good one and the places where Al Viro pointed out really affect
>>performance, should we propagate this faster one by introducing helper
>>function like:
>>
>>static inline int refcount_test(atomic_t *refcount)
>>{
>>	return (atomic_read(refcount) == 1) || (atomic_dec_and_test(refcount));
>>}
>>
>>and replace atomic_dec_and_test with it?
> 
> 
> No.  It's obviously slower than atomic_dec_and_test() if refcount is
> greater than 1.  And I'm less than sure that you can show that benefits
> in case when it is 1 outweight that.

Especially with the indirect function call. Modern CPUs often won't load
the destructor pointer quickly enough to avoid the pipeline bubble.

>  Moreover, for dentries, inodes,
> superblocks and vfsmounts you'd have to pull spin_lock() in front of
> it, which would _definitely_ hurt (these are atomic_dec_and_lock()).

Also, it results in altered memory barrier semantics. Whether or not
this is actually an issue anywhere, any conversion would have to be
careful. If a memory barrier is required _anywhere_, it is likely to
be required on the final put.

With all those arguments against it, you need to demonstrate
improvements before it can be considered.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com