[PATCH] Block: Check major number before allocate the buffer in register_blkdev()

[PATCH] Block: Check major number before allocate the buffer in register_blkdev()

[wzt.wzt]

Check major number before allocate the buffer, if the major number is not exist,
and the register_blkdev() called many times, kmalloc()/kfree() will be no need 
to invoked many times. So check the major number before use kmalloc() to allocate
the buffer will be better.

Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>

---
 block/genhd.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index d13ba76..15360cc 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -300,30 +300,29 @@ int register_blkdev(unsigned int major, const char *name)
 		ret = major;
 	}
 
-	p = kmalloc(sizeof(struct blk_major_name), GFP_KERNEL);
-	if (p == NULL) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	p->major = major;
-	strlcpy(p->name, name, sizeof(p->name));
-	p->next = NULL;
 	index = major_to_index(major);
 
 	for (n = &major_names[index]; *n; n = &(*n)->next) {
 		if ((*n)->major == major)
 			break;
 	}
-	if (!*n)
+	if (!*n) {
+		p = kmalloc(sizeof(struct blk_major_name), GFP_KERNEL);
+		if (p == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		p->major = major;
+		strlcpy(p->name, name, sizeof(p->name));
+		p->next = NULL;
 		*n = p;
-	else
+	}
+	else {
 		ret = -EBUSY;
 
-	if (ret < 0) {
 		printk("register_blkdev: cannot get major %d for %s\n",
 		       major, name);
-		kfree(p);
 	}
 out:
 	mutex_unlock(&block_class_lock);
-- 
1.6.5.3

Re: [PATCH] Block: Check major number before allocate the buffer in register_blkdev()

[Jens Axboe]

On Mon, May 10 2010, wzt.wzt@gmail.com wrote:
> Check major number before allocate the buffer, if the major number is not exist,
> and the register_blkdev() called many times, kmalloc()/kfree() will be no need 
> to invoked many times. So check the major number before use kmalloc() to allocate
> the buffer will be better.

This would generally be sound advice for performance oriented code, but
I can't see it making any difference here.

-- 
Jens Axboe

Re: [PATCH] Block: Check major number before allocate the buffer in register_blkdev()

[wzt wzt]

the original code use kmalloc() to allocate struct blk_major_name
buffer first,  then find the major number in major_names array. if
found it,  it will kfree() the unused struct blk_major_name buffer,
if register_blkdev() called many times like:
register_blkdev(22, "aa");
....
register_blkdev(22, "aa");
kmalloc()/kfree() will be no need to invoked many times, my point is
that find the major number first, then allocate the buffer will be
better. this patch can handle the special case.

On Mon, May 10, 2010 at 7:56 PM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Mon, May 10 2010, wzt.wzt@gmail.com wrote:
>> Check major number before allocate the buffer, if the major number is not exist,
>> and the register_blkdev() called many times, kmalloc()/kfree() will be no need
>> to invoked many times. So check the major number before use kmalloc() to allocate
>> the buffer will be better.
>
> This would generally be sound advice for performance oriented code, but
> I can't see it making any difference here.
>
> --
> Jens Axboe
>
>

Re: [PATCH] Block: Check major number before allocate the buffer in register_blkdev()

[Jens Axboe]

Please don't top post when replying to emails, I fixed this one up for
you.

On Mon, May 10 2010, wzt wzt wrote:
> On Mon, May 10, 2010 at 7:56 PM, Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Mon, May 10 2010, wzt.wzt@gmail.com wrote:
> >> Check major number before allocate the buffer, if the major number is not exist,
> >> and the register_blkdev() called many times, kmalloc()/kfree() will be no need
> >> to invoked many times. So check the major number before use kmalloc() to allocate
> >> the buffer will be better.
> >
> > This would generally be sound advice for performance oriented code, but
> > I can't see it making any difference here.
>
> the original code use kmalloc() to allocate struct blk_major_name
> buffer first,  then find the major number in major_names array. if
> found it,  it will kfree() the unused struct blk_major_name buffer,
> if register_blkdev() called many times like:
> register_blkdev(22, "aa");
> ....
> register_blkdev(22, "aa");
> kmalloc()/kfree() will be no need to invoked many times, my point is
> that find the major number first, then allocate the buffer will be
> better. this patch can handle the special case.

Yes I realize how it works and what your patch does, my point is that it
seems pointless to change code like that. Your 'test case' above isn't a
valid one. If register_blkdev() was called tons of times per second and
hence would be a hot code path, and it additionally most of the time
ended up freeing the buffer, then there would be a case for changing it.

-- 
Jens Axboe