[PATCH]use kzalloc in vfs where appropriate

[PATCH]use kzalloc in vfs where appropriate

[Oliver Neukum]

Hi,

this goes through the generic file system code and uses kzalloc where
appropriate.

	Regards
		Oliver

Signed-Off-By: Oliver Neukum <oliver@neukum.org>

--- a/fs/super.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/super.c	2006-03-17 16:27:46.000000000 +0100
@@ -55,11 +55,10 @@
  */
 static struct super_block *alloc_super(void)
 {
-	struct super_block *s = kmalloc(sizeof(struct super_block),  GFP_USER);
+	struct super_block *s = kzalloc(sizeof(struct super_block),  GFP_USER);
 	static struct super_operations default_op;
 
 	if (s) {
-		memset(s, 0, sizeof(struct super_block));
 		if (security_sb_alloc(s)) {
 			kfree(s);
 			s = NULL;
--- a/fs/pipe.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/pipe.c	2006-03-17 16:44:38.000000000 +0100
@@ -662,10 +662,9 @@
 {
 	struct pipe_inode_info *info;
 
-	info = kmalloc(sizeof(struct pipe_inode_info), GFP_KERNEL);
+	info = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL);
 	if (!info)
 		goto fail_page;
-	memset(info, 0, sizeof(*info));
 	inode->i_pipe = info;
 
 	init_waitqueue_head(PIPE_WAIT(*inode));
--- a/fs/file.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/file.c	2006-03-17 16:44:40.000000000 +0100
@@ -237,10 +237,9 @@
   	fd_set *new_openset = NULL, *new_execset = NULL;
 	struct file **new_fds;
 
-	fdt = kmalloc(sizeof(*fdt), GFP_KERNEL);
+	fdt = kzalloc(sizeof(*fdt), GFP_KERNEL);
 	if (!fdt)
   		goto out;
-	memset(fdt, 0, sizeof(*fdt));
 
 	nfds = __FD_SETSIZE;
   	/* Expand to the max in easy steps */
--- a/fs/exec.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/exec.c	2006-03-17 16:44:43.000000000 +0100
@@ -1143,10 +1143,9 @@
 	int i;
 
 	retval = -ENOMEM;
-	bprm = kmalloc(sizeof(*bprm), GFP_KERNEL);
+	bprm = kzalloc(sizeof(*bprm), GFP_KERNEL);
 	if (!bprm)
 		goto out_ret;
-	memset(bprm, 0, sizeof(*bprm));
 
 	file = open_exec(filename);
 	retval = PTR_ERR(file);
--- a/fs/compat.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/compat.c	2006-03-17 16:44:45.000000000 +0100
@@ -1474,10 +1474,9 @@
 	int i;
 
 	retval = -ENOMEM;
-	bprm = kmalloc(sizeof(*bprm), GFP_KERNEL);
+	bprm = kzalloc(sizeof(*bprm), GFP_KERNEL);
 	if (!bprm)
 		goto out_ret;
-	memset(bprm, 0, sizeof(*bprm));
 
 	file = open_exec(filename);
 	retval = PTR_ERR(file);
--- a/fs/char_dev.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/char_dev.c	2006-03-17 16:44:47.000000000 +0100
@@ -145,12 +145,10 @@
 	int ret = 0;
 	int i;
 
-	cd = kmalloc(sizeof(struct char_device_struct), GFP_KERNEL);
+	cd = kzalloc(sizeof(struct char_device_struct), GFP_KERNEL);
 	if (cd == NULL)
 		return ERR_PTR(-ENOMEM);
 
-	memset(cd, 0, sizeof(struct char_device_struct));
-
 	down(&chrdevs_lock);
 
 	/* temporary */
@@ -465,9 +463,8 @@
 
 struct cdev *cdev_alloc(void)
 {
-	struct cdev *p = kmalloc(sizeof(struct cdev), GFP_KERNEL);
+	struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL);
 	if (p) {
-		memset(p, 0, sizeof(struct cdev));
 		p->kobj.ktype = &ktype_cdev_dynamic;
 		INIT_LIST_HEAD(&p->list);
 		kobject_init(&p->kobj);
--- a/fs/bio.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/bio.c	2006-03-17 16:44:49.000000000 +0100
@@ -635,12 +635,10 @@
 		return ERR_PTR(-ENOMEM);
 
 	ret = -ENOMEM;
-	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
+	pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
 	if (!pages)
 		goto out;
 
-	memset(pages, 0, nr_pages * sizeof(struct page *));
-
 	for (i = 0; i < iov_count; i++) {
 		unsigned long uaddr = (unsigned long)iov[i].iov_base;
 		unsigned long len = iov[i].iov_len;
@@ -1182,12 +1180,11 @@
 
 struct bio_set *bioset_create(int bio_pool_size, int bvec_pool_size, int scale)
 {
-	struct bio_set *bs = kmalloc(sizeof(*bs), GFP_KERNEL);
+	struct bio_set *bs = kzalloc(sizeof(*bs), GFP_KERNEL);
 
 	if (!bs)
 		return NULL;
 
-	memset(bs, 0, sizeof(*bs));
 	bs->bio_pool = mempool_create(bio_pool_size, mempool_alloc_slab,
 			mempool_free_slab, bio_slab);
 
--- a/fs/binfmt_elf.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/binfmt_elf.c	2006-03-17 16:44:51.000000000 +0100
@@ -1465,12 +1465,11 @@
 		read_lock(&tasklist_lock);
 		do_each_thread(g,p)
 			if (current->mm == p->mm && current != p) {
-				tmp = kmalloc(sizeof(*tmp), GFP_ATOMIC);
+				tmp = kzalloc(sizeof(*tmp), GFP_ATOMIC);
 				if (!tmp) {
 					read_unlock(&tasklist_lock);
 					goto cleanup;
 				}
-				memset(tmp, 0, sizeof(*tmp));
 				INIT_LIST_HEAD(&tmp->list);
 				tmp->thread = p;
 				list_add(&tmp->list, &thread_list);
--- a/fs/aio.c	2006-03-11 23:12:55.000000000 +0100
+++ b/fs/aio.c	2006-03-17 16:44:53.000000000 +0100
@@ -122,10 +122,9 @@
 	info->nr = 0;
 	info->ring_pages = info->internal_pages;
 	if (nr_pages > AIO_RING_PAGES) {
-		info->ring_pages = kmalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
+		info->ring_pages = kzalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
 		if (!info->ring_pages)
 			return -ENOMEM;
-		memset(info->ring_pages, 0, sizeof(struct page *) * nr_pages);
 	}
 
 	info->mmap_size = nr_pages * PAGE_SIZE;

Re: [PATCH]use kzalloc in vfs where appropriate

[Matthew Wilcox]

On Fri, Mar 17, 2006 at 09:58:14PM +0100, Oliver Neukum wrote:
> --- a/fs/bio.c	2006-03-11 23:12:55.000000000 +0100
> +++ b/fs/bio.c	2006-03-17 16:44:49.000000000 +0100
> @@ -635,12 +635,10 @@
>  		return ERR_PTR(-ENOMEM);
>  
>  	ret = -ENOMEM;
> -	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> +	pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);

Didn't we just discuss this one and conclude it needed to use kcalloc
instead?

Re: [PATCH]use kzalloc in vfs where appropriate

[Oliver Neukum]

Am Freitag, 17. März 2006 22:08 schrieb Matthew Wilcox:
> On Fri, Mar 17, 2006 at 09:58:14PM +0100, Oliver Neukum wrote:
> > --- a/fs/bio.c	2006-03-11 23:12:55.000000000 +0100
> > +++ b/fs/bio.c	2006-03-17 16:44:49.000000000 +0100
> > @@ -635,12 +635,10 @@
> >  		return ERR_PTR(-ENOMEM);
> >  
> >  	ret = -ENOMEM;
> > -	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > +	pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> 
> Didn't we just discuss this one and conclude it needed to use kcalloc
> instead?

I've found some discussion in the archive, but no conclusion. Could you
elaborate?

	Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]use kzalloc in vfs where appropriate

[Arjan van de Ven]

On Sat, 2006-03-18 at 11:44 +0100, Oliver Neukum wrote:
> Am Freitag, 17. März 2006 22:08 schrieb Matthew Wilcox:
> > On Fri, Mar 17, 2006 at 09:58:14PM +0100, Oliver Neukum wrote:
> > > --- a/fs/bio.c	2006-03-11 23:12:55.000000000 +0100
> > > +++ b/fs/bio.c	2006-03-17 16:44:49.000000000 +0100
> > > @@ -635,12 +635,10 @@
> > >  		return ERR_PTR(-ENOMEM);
> > >  
> > >  	ret = -ENOMEM;
> > > -	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > > +	pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > 
> > Didn't we just discuss this one and conclude it needed to use kcalloc
> > instead?
> 
> I've found some discussion in the archive, but no conclusion. Could you
> elaborate?

kcalloc is the array allocator.
Here.. you're allocating an array of nr_pages worth of pointers.
kcalloc does extra checks because it KNOWS it's an array...


-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]use kzalloc in vfs where appropriate

[Oliver Neukum]

Am Samstag, 18. März 2006 11:55 schrieb Arjan van de Ven:
> On Sat, 2006-03-18 at 11:44 +0100, Oliver Neukum wrote:
> > Am Freitag, 17. März 2006 22:08 schrieb Matthew Wilcox:
> > > On Fri, Mar 17, 2006 at 09:58:14PM +0100, Oliver Neukum wrote:
> > > > --- a/fs/bio.c	2006-03-11 23:12:55.000000000 +0100
> > > > +++ b/fs/bio.c	2006-03-17 16:44:49.000000000 +0100
> > > > @@ -635,12 +635,10 @@
> > > >  		return ERR_PTR(-ENOMEM);
> > > >  
> > > >  	ret = -ENOMEM;
> > > > -	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > > > +	pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > > 
> > > Didn't we just discuss this one and conclude it needed to use kcalloc
> > > instead?
> > 
> > I've found some discussion in the archive, but no conclusion. Could you
> > elaborate?
> 
> kcalloc is the array allocator.
> Here.. you're allocating an array of nr_pages worth of pointers.
> kcalloc does extra checks because it KNOWS it's an array...

I see. A patch is coming.
Shouldn't this check from slab.h:

	if (n != 0 && size > INT_MAX / n)
		return NULL;

carry an "unlikely"?

	Regards
		Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]use kzalloc in vfs where appropriate

[Arjan van de Ven]

On Sun, 2006-03-19 at 14:29 +0100, Oliver Neukum wrote:
> Am Samstag, 18. März 2006 11:55 schrieb Arjan van de Ven:
> > On Sat, 2006-03-18 at 11:44 +0100, Oliver Neukum wrote:
> > > Am Freitag, 17. März 2006 22:08 schrieb Matthew Wilcox:
> > > > On Fri, Mar 17, 2006 at 09:58:14PM +0100, Oliver Neukum wrote:
> > > > > --- a/fs/bio.c	2006-03-11 23:12:55.000000000 +0100
> > > > > +++ b/fs/bio.c	2006-03-17 16:44:49.000000000 +0100
> > > > > @@ -635,12 +635,10 @@
> > > > >  		return ERR_PTR(-ENOMEM);
> > > > >  
> > > > >  	ret = -ENOMEM;
> > > > > -	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > > > > +	pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > > > 
> > > > Didn't we just discuss this one and conclude it needed to use kcalloc
> > > > instead?
> > > 
> > > I've found some discussion in the archive, but no conclusion. Could you
> > > elaborate?
> > 
> > kcalloc is the array allocator.
> > Here.. you're allocating an array of nr_pages worth of pointers.
> > kcalloc does extra checks because it KNOWS it's an array...
> 
> I see. A patch is coming.
> Shouldn't this check from slab.h:
> 
> 	if (n != 0 && size > INT_MAX / n)
> 		return NULL;
> 
> carry an "unlikely"?

gcc is most likely smart enough for that already, and most of the time n
is a compile time constant already :)

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]use kzalloc in vfs where appropriate

[Oliver Neukum]

Am Sonntag, 19. März 2006 17:09 schrieb Arjan van de Ven:
> On Sun, 2006-03-19 at 14:29 +0100, Oliver Neukum wrote:
> > Am Samstag, 18. März 2006 11:55 schrieb Arjan van de Ven:
> > > On Sat, 2006-03-18 at 11:44 +0100, Oliver Neukum wrote:
> > > > Am Freitag, 17. März 2006 22:08 schrieb Matthew Wilcox:
> > > > > On Fri, Mar 17, 2006 at 09:58:14PM +0100, Oliver Neukum wrote:
> > > > > > --- a/fs/bio.c	2006-03-11 23:12:55.000000000 +0100
> > > > > > +++ b/fs/bio.c	2006-03-17 16:44:49.000000000 +0100
> > > > > > @@ -635,12 +635,10 @@
> > > > > >  		return ERR_PTR(-ENOMEM);
> > > > > >  
> > > > > >  	ret = -ENOMEM;
> > > > > > -	pages = kmalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > > > > > +	pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
> > > > > 
> > > > > Didn't we just discuss this one and conclude it needed to use kcalloc
> > > > > instead?
> > > > 
> > > > I've found some discussion in the archive, but no conclusion. Could you
> > > > elaborate?
> > > 
> > > kcalloc is the array allocator.
> > > Here.. you're allocating an array of nr_pages worth of pointers.
> > > kcalloc does extra checks because it KNOWS it's an array...
> > 
> > I see. A patch is coming.
> > Shouldn't this check from slab.h:
> > 
> > 	if (n != 0 && size > INT_MAX / n)
> > 		return NULL;
> > 
> > carry an "unlikely"?
> 
> gcc is most likely smart enough for that already, and most of the time n
> is a compile time constant already :)

Yes it is. The generated code is identical. But on second thought this is still
not optimal. A full division is generated:

	xorl	%edx, %edx
	movl	$2147483647, %eax
	movq	$0, 40(%rsp)
	divq	%rcx
	movl	$8, %edx
	cmpq	%rax, %rdx
	ja	.L313

Rewriting the test as:
n!=0 && n > INT_MAX / size
saves the division because size is much likelier to be a constant, and indeed
the code is better:

	cmpq	$268435455, %rax
	movq	$0, 40(%rsp)
	ja	.L313

Is there anything I am missing?

	Regards
		Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]use kzalloc in vfs where appropriate

[Pekka Enberg]

Hi Oliver,

On 3/19/06, Oliver Neukum <oliver@neukum.org> wrote:
> Yes it is. The generated code is identical. But on second thought this is still
> not optimal. A full division is generated:
>
>         xorl    %edx, %edx
>         movl    $2147483647, %eax
>         movq    $0, 40(%rsp)
>         divq    %rcx
>         movl    $8, %edx
>         cmpq    %rax, %rdx
>         ja      .L313
>
> Rewriting the test as:
> n!=0 && n > INT_MAX / size
> saves the division because size is much likelier to be a constant, and indeed
> the code is better:
>
>         cmpq    $268435455, %rax
>         movq    $0, 40(%rsp)
>         ja      .L313
>
> Is there anything I am missing?

Did you check allyesconfig vmlinux size before and after? If it helps,
like it probably does, post a patch!

                                   Pekka

Re: [PATCH]use kzalloc in vfs where appropriate

[Oliver Neukum]

> > Rewriting the test as:
> > n!=0 && n > INT_MAX / size
> > saves the division because size is much likelier to be a constant, and indeed
> > the code is better:
> >
> >         cmpq    $268435455, %rax
> >         movq    $0, 40(%rsp)
> >         ja      .L313
> >
> > Is there anything I am missing?
> 
> Did you check allyesconfig vmlinux size before and after? If it helps,
> like it probably does, post a patch!

Hi Pekka,

it saves 18K of 232M. I am making a patch.

	Regards
		Oliver

Re: [PATCH]use kzalloc in vfs where appropriate

[Denis Vlasenko]

On Monday 20 March 2006 09:25, Pekka Enberg wrote:
> > Rewriting the test as:
> > n!=0 && n > INT_MAX / size
> > saves the division because size is much likelier to be a constant, and indeed
> > the code is better:
> >
> >         cmpq    $268435455, %rax
> >         movq    $0, 40(%rsp)
> >         ja      .L313
> >
> > Is there anything I am missing?

You may drop "n!=0" part, but you must check size!=0.
Since if size is 0, kcalloc returns NULL, then

        if (!size || n > INT_MAX / size)
                return NULL;

--
vda

Re: [PATCH]use kzalloc in vfs where appropriate

[Oliver Neukum]

Am Montag, 20. März 2006 14:08 schrieb Denis Vlasenko:
> On Monday 20 March 2006 09:25, Pekka Enberg wrote:
> > > Rewriting the test as:
> > > n!=0 && n > INT_MAX / size
> > > saves the division because size is much likelier to be a constant, and indeed
> > > the code is better:
> > >
> > >         cmpq    $268435455, %rax
> > >         movq    $0, 40(%rsp)
> > >         ja      .L313
> > >
> > > Is there anything I am missing?
> 
> You may drop "n!=0" part, but you must check size!=0.
> Since if size is 0, kcalloc returns NULL, then

Why? size == 0 is a bug. We want to oops here.

	Regards
		Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]use kzalloc in vfs where appropriate

[Pekka J Enberg]

On Mon, 20 Mar 2006, Oliver Neukum wrote:
> Why? size == 0 is a bug. We want to oops here.

Why do you want to oops? The standard calloc() deals with zero so 
kcalloc() should probably do that as well. In any case, if you want to 
change the behavior, please audit kcalloc() callers before optimizing.

			Pekka

Re: [PATCH]use kzalloc in vfs where appropriate

[Pekka J Enberg]

On Monday 20 March 2006 09:25, Pekka Enberg wrote:
> > > Rewriting the test as:
> > > n!=0 && n > INT_MAX / size
> > > saves the division because size is much likelier to be a constant, and indeed
> > > the code is better:
> > >
> > >         cmpq    $268435455, %rax
> > >         movq    $0, 40(%rsp)
> > >         ja      .L313
> > >
> > > Is there anything I am missing?

On Mon, 20 Mar 2006, Denis Vlasenko wrote:
> You may drop "n!=0" part, but you must check size!=0.
> Since if size is 0, kcalloc returns NULL, then
> 
>         if (!size || n > INT_MAX / size)
>                 return NULL;

Uh, oh, I must be getting blind to have missed that...

				Pekka