kmalloc to kzalloc patches for drivers/block [sane version]

kmalloc to kzalloc patches for drivers/block [sane version]

[Om Narasimhan]

This patch changes the kmalloc() calls followed by memset(,0,) to kzalloc.

    cciss.c : Changed the kmalloc/memset pair to kzalloc
    cpqarray.c : km2zalloc conversion and code size reduction by
changing multiple cleanup calls and returns of the function
getgeometry() by adding a label.
    loop.c : km2zalloc converion


Signed off by Om Narasimhan <om.turyx@gmail.com>
 drivers/block/cciss.c    |    4 +--
 drivers/block/cpqarray.c |   72 +++++++++++++++-------------------------------
 drivers/block/loop.c     |    4 +--
 3 files changed, 25 insertions(+), 55 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 2cd3391..a800a69 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -900,7 +900,7 @@ #if 0				/* 'buf_size' member is 16-bits
 				return -EINVAL;
 #endif
 			if (iocommand.buf_size > 0) {
-				buff = kmalloc(iocommand.buf_size, GFP_KERNEL);
+				buff = kzalloc(iocommand.buf_size, GFP_KERNEL);
 				if (buff == NULL)
 					return -EFAULT;
 			}
@@ -911,8 +911,6 @@ #endif
 					kfree(buff);
 					return -EFAULT;
 				}
-			} else {
-				memset(buff, 0, iocommand.buf_size);
 			}
 			if ((c = cmd_alloc(host, 0)) == NULL) {
 				kfree(buff);
diff --git a/drivers/block/cpqarray.c b/drivers/block/cpqarray.c
index 78082ed..8a697c7 100644
--- a/drivers/block/cpqarray.c
+++ b/drivers/block/cpqarray.c
@@ -424,7 +424,7 @@ static int __init cpqarray_register_ctlr
 	hba[i]->cmd_pool = (cmdlist_t *)pci_alloc_consistent(
 		hba[i]->pci_dev, NR_CMDS * sizeof(cmdlist_t),
 		&(hba[i]->cmd_pool_dhandle));
-	hba[i]->cmd_pool_bits = kmalloc(
+	hba[i]->cmd_pool_bits = kzalloc(
 		((NR_CMDS+BITS_PER_LONG-1)/BITS_PER_LONG)*sizeof(unsigned long),
 		GFP_KERNEL);

@@ -432,7 +432,6 @@ static int __init cpqarray_register_ctlr
 			goto Enomem1;

 	memset(hba[i]->cmd_pool, 0, NR_CMDS * sizeof(cmdlist_t));
-	memset(hba[i]->cmd_pool_bits, 0,
((NR_CMDS+BITS_PER_LONG-1)/BITS_PER_LONG)*sizeof(unsigned long));
 	printk(KERN_INFO "cpqarray: Finding drives on %s",
 		hba[i]->devname);

@@ -523,7 +522,6 @@ static int __init cpqarray_init_one( str
 	i = alloc_cpqarray_hba();
 	if( i < 0 )
 		return (-1);
-	memset(hba[i], 0, sizeof(ctlr_info_t));
 	sprintf(hba[i]->devname, "ida%d", i);
 	hba[i]->ctlr = i;
 	/* Initialize the pdev driver private data */
@@ -580,7 +578,7 @@ static int alloc_cpqarray_hba(void)

 	for(i=0; i< MAX_CTLR; i++) {
 		if (hba[i] == NULL) {
-			hba[i] = kmalloc(sizeof(ctlr_info_t), GFP_KERNEL);
+			hba[i] = kzalloc(sizeof(ctlr_info_t), GFP_KERNEL);
 			if(hba[i]==NULL) {
 				printk(KERN_ERR "cpqarray: out of memory.\n");
 				return (-1);
@@ -765,7 +763,6 @@ static int __init cpqarray_eisa_detect(v
 			continue;
 		}

-		memset(hba[ctlr], 0, sizeof(ctlr_info_t));
 		hba[ctlr]->io_mem_addr = eisa[i];
 		hba[ctlr]->io_mem_length = 0x7FF;
 		if(!request_region(hba[ctlr]->io_mem_addr,
@@ -1642,58 +1639,46 @@ static void start_fwbk(int ctlr)
     It is used only at init time.
 *****************************************************************/
 static void getgeometry(int ctlr)
-{				
-	id_log_drv_t *id_ldrive;
-	id_ctlr_t *id_ctlr_buf;
-	sense_log_drv_stat_t *id_lstatus_buf;
-	config_t *sense_config_buf;
+{
+	id_log_drv_t *id_ldrive = NULL;
+	id_ctlr_t *id_ctlr_buf = NULL;
+	sense_log_drv_stat_t *id_lstatus_buf = NULL;
+	config_t *sense_config_buf = NULL;
 	unsigned int log_unit, log_index;
 	int ret_code, size;
-	drv_info_t *drv;
+	drv_info_t *drv = NULL;
 	ctlr_info_t *info_p = hba[ctlr];
 	int i;

-	info_p->log_drv_map = 0;	
-	
-	id_ldrive = (id_log_drv_t *)kmalloc(sizeof(id_log_drv_t), GFP_KERNEL);
+	info_p->log_drv_map = 0;
+	id_ldrive = (id_log_drv_t *)kzalloc(sizeof(id_log_drv_t), GFP_KERNEL);
 	if(id_ldrive == NULL)
 	{
 		printk( KERN_ERR "cpqarray:  out of memory.\n");
-		return;
+		goto end;
 	}

-	id_ctlr_buf = (id_ctlr_t *)kmalloc(sizeof(id_ctlr_t), GFP_KERNEL);
+	id_ctlr_buf = (id_ctlr_t *)kzalloc(sizeof(id_ctlr_t), GFP_KERNEL);
 	if(id_ctlr_buf == NULL)
 	{
-		kfree(id_ldrive);
 		printk( KERN_ERR "cpqarray:  out of memory.\n");
-		return;
+		goto end;
 	}

-	id_lstatus_buf = (sense_log_drv_stat_t
*)kmalloc(sizeof(sense_log_drv_stat_t), GFP_KERNEL);
+	id_lstatus_buf = (sense_log_drv_stat_t
*)kzalloc(sizeof(sense_log_drv_stat_t), GFP_KERNEL);
 	if(id_lstatus_buf == NULL)
 	{
-		kfree(id_ctlr_buf);
-		kfree(id_ldrive);
 		printk( KERN_ERR "cpqarray:  out of memory.\n");
-		return;
+		goto end;
 	}

-	sense_config_buf = (config_t *)kmalloc(sizeof(config_t), GFP_KERNEL);
+	sense_config_buf = (config_t *)kzalloc(sizeof(config_t), GFP_KERNEL);
 	if(sense_config_buf == NULL)
 	{
-		kfree(id_lstatus_buf);
-		kfree(id_ctlr_buf);
-		kfree(id_ldrive);
 		printk( KERN_ERR "cpqarray:  out of memory.\n");
-		return;
+		goto end;
 	}

-	memset(id_ldrive, 0, sizeof(id_log_drv_t));
-	memset(id_ctlr_buf, 0, sizeof(id_ctlr_t));
-	memset(id_lstatus_buf, 0, sizeof(sense_log_drv_stat_t));
-	memset(sense_config_buf, 0, sizeof(config_t));
-
 	info_p->phys_drives = 0;
 	info_p->log_drv_map = 0;
 	info_p->drv_assign_map = 0;
@@ -1709,11 +1694,7 @@ static void getgeometry(int ctlr)
 		 */
 		 /* Free all the buffers and return */
 		printk(KERN_ERR "cpqarray: error sending ID controller\n");
-		kfree(sense_config_buf);
-                kfree(id_lstatus_buf);
-                kfree(id_ctlr_buf);
-                kfree(id_ldrive);
-                return;
+		goto end;
         }

 	info_p->log_drives = id_ctlr_buf->nr_drvs;
@@ -1760,11 +1741,7 @@ static void getgeometry(int ctlr)
 			 "Access to this controller has been disabled\n",
 				ctlr, log_unit);
 			/* Free all the buffers and return */
-                	kfree(sense_config_buf);
-                	kfree(id_lstatus_buf);
-                	kfree(id_ctlr_buf);
-                	kfree(id_ldrive);
-                	return;
+			goto end;
 		}
 		/*
 		   Make sure the logical drive is configured
@@ -1795,11 +1772,7 @@ static void getgeometry(int ctlr)
 					info_p->log_drv_map = 0;
 					/* Free all the buffers and return */
                 			printk(KERN_ERR "cpqarray: error sending sense config\n");
-                			kfree(sense_config_buf);
-                			kfree(id_lstatus_buf);
-                			kfree(id_ctlr_buf);
-                			kfree(id_ldrive);
-                			return;
+					goto end;

 				}

@@ -1815,9 +1788,10 @@ static void getgeometry(int ctlr)
 			log_index = log_index + 1;
 		}		/* end of if logical drive configured */
 	}			/* end of for log_unit */
+end:
 	kfree(sense_config_buf);
-  	kfree(id_ldrive);
-  	kfree(id_lstatus_buf);
+	kfree(id_ldrive);
+	kfree(id_lstatus_buf);
 	kfree(id_ctlr_buf);
 	return;

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 7b3b94d..cc4785f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1260,11 +1260,9 @@ static int __init loop_init(void)
 	if (register_blkdev(LOOP_MAJOR, "loop"))
 		return -EIO;

-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
+	loop_dev = kzalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
 	if (!loop_dev)
 		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));
-
 	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
 	if (!disks)
 		goto out_mem2;

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Nishanth Aravamudan]

On 20.09.2006 [23:11:25 -0700], Om Narasimhan wrote:
> This patch changes the kmalloc() calls followed by memset(,0,) to kzalloc.
> 
>     cciss.c : Changed the kmalloc/memset pair to kzalloc
>     cpqarray.c : km2zalloc conversion and code size reduction by
> changing multiple cleanup calls and returns of the function
> getgeometry() by adding a label.
>     loop.c : km2zalloc converion
> 
> 
> Signed off by Om Narasimhan <om.turyx@gmail.com>

This is not the canonical format, per SubmittingPatches. It should be:

Signed-off-by: Random J Developer <random@developer.example.org>

>  drivers/block/cciss.c    |    4 +--
>  drivers/block/cpqarray.c |   72 +++++++++++++++-------------------------------
>  drivers/block/loop.c     |    4 +--
>  3 files changed, 25 insertions(+), 55 deletions(-)

Your diffstat should have indicated to you that this should be split up
better. Please (re-)read SubmittingPatches. *One* logical change per
patch, most importantly.

> 
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index 2cd3391..a800a69 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -900,7 +900,7 @@ #if 0				/* 'buf_size' member is 16-bits
>  				return -EINVAL;
>  #endif
>  			if (iocommand.buf_size > 0) {
> -				buff = kmalloc(iocommand.buf_size, GFP_KERNEL);
> +				buff = kzalloc(iocommand.buf_size, GFP_KERNEL);
>  				if (buff == NULL)
>  					return -EFAULT;
>  			}
> @@ -911,8 +911,6 @@ #endif
>  					kfree(buff);
>  					return -EFAULT;
>  				}
> -			} else {
> -				memset(buff, 0, iocommand.buf_size);
>  			}
>  			if ((c = cmd_alloc(host, 0)) == NULL) {
>  				kfree(buff);

This changes performance potentially, no? The memset before was
conditional upon (iocommand.Request.Type.Direction == XFER_WRITE) and
now the memory will always be zero'd.

> diff --git a/drivers/block/cpqarray.c b/drivers/block/cpqarray.c
> index 78082ed..8a697c7 100644
> --- a/drivers/block/cpqarray.c
> +++ b/drivers/block/cpqarray.c
> @@ -1642,58 +1639,46 @@ static void start_fwbk(int ctlr)
>      It is used only at init time.
>  *****************************************************************/
>  static void getgeometry(int ctlr)
> -{				
> -	id_log_drv_t *id_ldrive;
> -	id_ctlr_t *id_ctlr_buf;
> -	sense_log_drv_stat_t *id_lstatus_buf;
> -	config_t *sense_config_buf;
> +{

Unrelated whitespace change.

> +	id_log_drv_t *id_ldrive = NULL;
> +	id_ctlr_t *id_ctlr_buf = NULL;
> +	sense_log_drv_stat_t *id_lstatus_buf = NULL;
> +	config_t *sense_config_buf = NULL;

Why initialize if you're going to immediately assign the return of
kzalloc()?

>  	unsigned int log_unit, log_index;
>  	int ret_code, size;
> -	drv_info_t *drv;
> +	drv_info_t *drv = NULL;

What does this do? Seems unnecessary and unrelated.

<snip>

> -		kfree(id_ctlr_buf);
> -		kfree(id_ldrive);
>  		printk( KERN_ERR "cpqarray:  out of memory.\n");
> -		return;
> +		goto end;

All of this rearrangement needs to be a separate patch.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Om Narasimhan]

Thanks for the comments.
> >
> > Signed off by Om Narasimhan <om.turyx@gmail.com>
>
> This is not the canonical format, per SubmittingPatches. It should be:
>
> Signed-off-by: Random J Developer <random@developer.example.org>
OK. I would take care of it.
>
> >  drivers/block/cciss.c    |    4 +--
> >  drivers/block/cpqarray.c |   72 +++++++++++++++-------------------------------
> >  drivers/block/loop.c     |    4 +--
> >  3 files changed, 25 insertions(+), 55 deletions(-)
>
> Your diffstat should have indicated to you that this should be split up
> better. Please (re-)read SubmittingPatches. *One* logical change per
> patch, most importantly.
OK. I would resubmit.
> >
> > diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> > index 2cd3391..a800a69 100644
> > --- a/drivers/block/cciss.c
> > +++ b/drivers/block/cciss.c
> > @@ -900,7 +900,7 @@ #if 0                             /* 'buf_size' member is 16-bits
> >                               return -EINVAL;
> >  #endif
> >                       if (iocommand.buf_size > 0) {
> > -                             buff = kmalloc(iocommand.buf_size, GFP_KERNEL);
> > +                             buff = kzalloc(iocommand.buf_size, GFP_KERNEL);
> >                               if (buff == NULL)
> >                                       return -EFAULT;
> >                       }
> > @@ -911,8 +911,6 @@ #endif
> >                                       kfree(buff);
> >                                       return -EFAULT;
> >                               }
> > -                     } else {
> > -                             memset(buff, 0, iocommand.buf_size);
> >                       }
> >                       if ((c = cmd_alloc(host, 0)) == NULL) {
> >                               kfree(buff);
>
> This changes performance potentially, no? The memset before was
> conditional upon (iocommand.Request.Type.Direction == XFER_WRITE) and
> now the memory will always be zero'd.
Yes, but not the functionality.
if (iocommand.buf_size > 0), code allocates using kmalloc. if
direction is XFER_WRITE, it does a copy_from_user(), and free()s the
allocated buffer, not really caring what data came in from userspace.
Else, it does memset(). So I could safely replace the kmalloc() with
kzalloc() without compromising functionality.

Thanks,
Om.

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Om Narasimhan]

Comments incorporated
Changes kmalloc() calls succeeded by memset(,0,) to kzalloc()

Signed off by : Om Narasimhan <om.turyx@gmail.com>
 drivers/block/cciss.c    |    4 +---
 drivers/block/cpqarray.c |    7 ++-----
 drivers/block/loop.c     |    3 +--
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 2cd3391..a800a69 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -900,7 +900,7 @@ #if 0				/* 'buf_size' member is 16-bits
 				return -EINVAL;
 #endif
 			if (iocommand.buf_size > 0) {
-				buff = kmalloc(iocommand.buf_size, GFP_KERNEL);
+				buff = kzalloc(iocommand.buf_size, GFP_KERNEL);
 				if (buff == NULL)
 					return -EFAULT;
 			}
@@ -911,8 +911,6 @@ #endif
 					kfree(buff);
 					return -EFAULT;
 				}
-			} else {
-				memset(buff, 0, iocommand.buf_size);
 			}
 			if ((c = cmd_alloc(host, 0)) == NULL) {
 				kfree(buff);
diff --git a/drivers/block/cpqarray.c b/drivers/block/cpqarray.c
index 78082ed..34f8e96 100644
--- a/drivers/block/cpqarray.c
+++ b/drivers/block/cpqarray.c
@@ -424,7 +424,7 @@ static int __init cpqarray_register_ctlr
 	hba[i]->cmd_pool = (cmdlist_t *)pci_alloc_consistent(
 		hba[i]->pci_dev, NR_CMDS * sizeof(cmdlist_t),
 		&(hba[i]->cmd_pool_dhandle));
-	hba[i]->cmd_pool_bits = kmalloc(
+	hba[i]->cmd_pool_bits = kzalloc(
 		((NR_CMDS+BITS_PER_LONG-1)/BITS_PER_LONG)*sizeof(unsigned long),
 		GFP_KERNEL);

@@ -432,7 +432,6 @@ static int __init cpqarray_register_ctlr
 			goto Enomem1;

 	memset(hba[i]->cmd_pool, 0, NR_CMDS * sizeof(cmdlist_t));
-	memset(hba[i]->cmd_pool_bits, 0,
((NR_CMDS+BITS_PER_LONG-1)/BITS_PER_LONG)*sizeof(unsigned long));
 	printk(KERN_INFO "cpqarray: Finding drives on %s",
 		hba[i]->devname);

@@ -523,7 +522,6 @@ static int __init cpqarray_init_one( str
 	i = alloc_cpqarray_hba();
 	if( i < 0 )
 		return (-1);
-	memset(hba[i], 0, sizeof(ctlr_info_t));
 	sprintf(hba[i]->devname, "ida%d", i);
 	hba[i]->ctlr = i;
 	/* Initialize the pdev driver private data */
@@ -580,7 +578,7 @@ static int alloc_cpqarray_hba(void)

 	for(i=0; i< MAX_CTLR; i++) {
 		if (hba[i] == NULL) {
-			hba[i] = kmalloc(sizeof(ctlr_info_t), GFP_KERNEL);
+			hba[i] = kzalloc(sizeof(ctlr_info_t), GFP_KERNEL);
 			if(hba[i]==NULL) {
 				printk(KERN_ERR "cpqarray: out of memory.\n");
 				return (-1);
@@ -765,7 +763,6 @@ static int __init cpqarray_eisa_detect(v
 			continue;
 		}

-		memset(hba[ctlr], 0, sizeof(ctlr_info_t));
 		hba[ctlr]->io_mem_addr = eisa[i];
 		hba[ctlr]->io_mem_length = 0x7FF;
 		if(!request_region(hba[ctlr]->io_mem_addr,
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 7b3b94d..91b48ef 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1260,10 +1260,9 @@ static int __init loop_init(void)
 	if (register_blkdev(LOOP_MAJOR, "loop"))
 		return -EIO;

-	loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
+	loop_dev = kzalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
 	if (!loop_dev)
 		goto out_mem1;
-	memset(loop_dev, 0, max_loop * sizeof(struct loop_device));

 	disks = kmalloc(max_loop * sizeof(struct gendisk *), GFP_KERNEL);
 	if (!disks)

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Jiri Slaby]

Om Narasimhan wrote:
> Thanks for the comments.
>> >
>> > Signed off by Om Narasimhan <om.turyx@gmail.com>
>>
>> This is not the canonical format, per SubmittingPatches. It should be:
>>
>> Signed-off-by: Random J Developer <random@developer.example.org>
> OK. I would take care of it.
>>
>> >  drivers/block/cciss.c    |    4 +--
>> >  drivers/block/cpqarray.c |   72 
>> +++++++++++++++-------------------------------
>> >  drivers/block/loop.c     |    4 +--
>> >  3 files changed, 25 insertions(+), 55 deletions(-)
>>
>> Your diffstat should have indicated to you that this should be split up
>> better. Please (re-)read SubmittingPatches. *One* logical change per
>> patch, most importantly.
> OK. I would resubmit.
>> >
>> > diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
>> > index 2cd3391..a800a69 100644
>> > --- a/drivers/block/cciss.c
>> > +++ b/drivers/block/cciss.c
>> > @@ -900,7 +900,7 @@ #if 0                             /* 'buf_size' 
>> member is 16-bits
>> >                               return -EINVAL;
>> >  #endif
>> >                       if (iocommand.buf_size > 0) {
>> > -                             buff = kmalloc(iocommand.buf_size, 
>> GFP_KERNEL);
>> > +                             buff = kzalloc(iocommand.buf_size, 
>> GFP_KERNEL);
>> >                               if (buff == NULL)
>> >                                       return -EFAULT;
>> >                       }
>> > @@ -911,8 +911,6 @@ #endif
>> >                                       kfree(buff);
>> >                                       return -EFAULT;
>> >                               }
>> > -                     } else {
>> > -                             memset(buff, 0, iocommand.buf_size);
>> >                       }
>> >                       if ((c = cmd_alloc(host, 0)) == NULL) {
>> >                               kfree(buff);
>>
>> This changes performance potentially, no? The memset before was
>> conditional upon (iocommand.Request.Type.Direction == XFER_WRITE) and
>> now the memory will always be zero'd.
> Yes, but not the functionality.
> if (iocommand.buf_size > 0), code allocates using kmalloc. if
> direction is XFER_WRITE, it does a copy_from_user(), and free()s the
> allocated buffer, not really caring what data came in from userspace.
> Else, it does memset(). So I could safely replace the kmalloc() with
> kzalloc() without compromising functionality.

Ok, this is something like I need 10 bytes of memory, so I request two memory 
pages for reserved use. It works, but it kills performance.

Why you zero memory that is not needed to be zeroed?

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Jiri Slaby]

Om Narasimhan wrote:
> Comments incorporated
> Changes kmalloc() calls succeeded by memset(,0,) to kzalloc()
> 
> Signed off by : Om Narasimhan <om.turyx@gmail.com>
> drivers/block/cciss.c    |    4 +---
> drivers/block/cpqarray.c |    7 ++-----
> drivers/block/loop.c     |    3 +--
> 3 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index 2cd3391..a800a69 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -900,7 +900,7 @@ #if 0                /* 'buf_size' member is 16-bits
>                 return -EINVAL;
> #endif
>             if (iocommand.buf_size > 0) {
> -                buff = kmalloc(iocommand.buf_size, GFP_KERNEL);
> +                buff = kzalloc(iocommand.buf_size, GFP_KERNEL);
>                 if (buff == NULL)
>                     return -EFAULT;
>             }
> @@ -911,8 +911,6 @@ #endif
>                     kfree(buff);
>                     return -EFAULT;
>                 }
> -            } else {
> -                memset(buff, 0, iocommand.buf_size);

No.

>             }
>             if ((c = cmd_alloc(host, 0)) == NULL) {
>                 kfree(buff);
> diff --git a/drivers/block/cpqarray.c b/drivers/block/cpqarray.c
> index 78082ed..34f8e96 100644
> --- a/drivers/block/cpqarray.c
> +++ b/drivers/block/cpqarray.c
> @@ -424,7 +424,7 @@ static int __init cpqarray_register_ctlr
>     hba[i]->cmd_pool = (cmdlist_t *)pci_alloc_consistent(
>         hba[i]->pci_dev, NR_CMDS * sizeof(cmdlist_t),
>         &(hba[i]->cmd_pool_dhandle));
> -    hba[i]->cmd_pool_bits = kmalloc(
> +    hba[i]->cmd_pool_bits = kzalloc(
>         ((NR_CMDS+BITS_PER_LONG-1)/BITS_PER_LONG)*sizeof(unsigned long),
>         GFP_KERNEL);

kcalloc?

> @@ -432,7 +432,6 @@ static int __init cpqarray_register_ctlr
>             goto Enomem1;
> 
>     memset(hba[i]->cmd_pool, 0, NR_CMDS * sizeof(cmdlist_t));
> -    memset(hba[i]->cmd_pool_bits, 0,
> ((NR_CMDS+BITS_PER_LONG-1)/BITS_PER_LONG)*sizeof(unsigned long));

What's this? Wrapped? kcalloc?

>     printk(KERN_INFO "cpqarray: Finding drives on %s",
>         hba[i]->devname);
> 
> @@ -523,7 +522,6 @@ static int __init cpqarray_init_one( str
>     i = alloc_cpqarray_hba();
>     if( i < 0 )
>         return (-1);
> -    memset(hba[i], 0, sizeof(ctlr_info_t));
>     sprintf(hba[i]->devname, "ida%d", i);
>     hba[i]->ctlr = i;
>     /* Initialize the pdev driver private data */
> @@ -580,7 +578,7 @@ static int alloc_cpqarray_hba(void)
> 
>     for(i=0; i< MAX_CTLR; i++) {
>         if (hba[i] == NULL) {
> -            hba[i] = kmalloc(sizeof(ctlr_info_t), GFP_KERNEL);
> +            hba[i] = kzalloc(sizeof(ctlr_info_t), GFP_KERNEL);
>             if(hba[i]==NULL) {
>                 printk(KERN_ERR "cpqarray: out of memory.\n");
>                 return (-1);
> @@ -765,7 +763,6 @@ static int __init cpqarray_eisa_detect(v
>             continue;
>         }
> 
> -        memset(hba[ctlr], 0, sizeof(ctlr_info_t));
>         hba[ctlr]->io_mem_addr = eisa[i];
>         hba[ctlr]->io_mem_length = 0x7FF;
>         if(!request_region(hba[ctlr]->io_mem_addr,
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 7b3b94d..91b48ef 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1260,10 +1260,9 @@ static int __init loop_init(void)
>     if (register_blkdev(LOOP_MAJOR, "loop"))
>         return -EIO;
> 
> -    loop_dev = kmalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);
> +    loop_dev = kzalloc(max_loop * sizeof(struct loop_device), GFP_KERNEL);

kcalloc?

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Paulo Marques]

Jiri Slaby wrote:
> Om Narasimhan wrote:
>> Thanks for the comments.
>>> >
>>> > Signed off by Om Narasimhan <om.turyx@gmail.com>
>>>
>>> This is not the canonical format, per SubmittingPatches. It should be:
>>>
>>> Signed-off-by: Random J Developer <random@developer.example.org>
>> OK. I would take care of it.
>>>
>>> >  drivers/block/cciss.c    |    4 +--
>>> >  drivers/block/cpqarray.c |   72 
>>> +++++++++++++++-------------------------------
>>> >  drivers/block/loop.c     |    4 +--
>>> >  3 files changed, 25 insertions(+), 55 deletions(-)
>>>
>>> Your diffstat should have indicated to you that this should be split up
>>> better. Please (re-)read SubmittingPatches. *One* logical change per
>>> patch, most importantly.
>> OK. I would resubmit.
>>> >
>>> > diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
>>> > index 2cd3391..a800a69 100644
>>> > --- a/drivers/block/cciss.c
>>> > +++ b/drivers/block/cciss.c
>>> > @@ -900,7 +900,7 @@ #if 0                             /* 'buf_size' 
>>> member is 16-bits
>>> >                               return -EINVAL;
>>> >  #endif
>>> >                       if (iocommand.buf_size > 0) {
>>> > -                             buff = kmalloc(iocommand.buf_size, 
>>> GFP_KERNEL);
>>> > +                             buff = kzalloc(iocommand.buf_size, 
>>> GFP_KERNEL);
>>> >                               if (buff == NULL)
>>> >                                       return -EFAULT;
>>> >                       }
>>> > @@ -911,8 +911,6 @@ #endif
>>> >                                       kfree(buff);
>>> >                                       return -EFAULT;
>>> >                               }
>>> > -                     } else {
>>> > -                             memset(buff, 0, iocommand.buf_size);
>>> >                       }
>>> >                       if ((c = cmd_alloc(host, 0)) == NULL) {
>>> >                               kfree(buff);
>>>
>>> This changes performance potentially, no? The memset before was
>>> conditional upon (iocommand.Request.Type.Direction == XFER_WRITE) and
>>> now the memory will always be zero'd.
>> Yes, but not the functionality.
>> if (iocommand.buf_size > 0), code allocates using kmalloc. if
>> direction is XFER_WRITE, it does a copy_from_user(), and free()s the
>> allocated buffer, not really caring what data came in from userspace.

You really misread that code. It frees the buffer and returns -EFAULT if 
the copy_from_user _failed_. This is standard procedure and that code 
doesn't need to be changed to kzalloc.

Please only do kmalloc to k[zc]alloc changes that are really trivial. 
There is no point in risking inserting new bugs (or performance 
regressions) for some micro-space-optimization such as this.

-- 
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Paulo Marques]

Jiri Slaby wrote:
> Om Narasimhan wrote:
>> [...]
>> --- a/drivers/block/cpqarray.c
>> +++ b/drivers/block/cpqarray.c
>> @@ -424,7 +424,7 @@ static int __init cpqarray_register_ctlr
>>     hba[i]->cmd_pool = (cmdlist_t *)pci_alloc_consistent(
>>         hba[i]->pci_dev, NR_CMDS * sizeof(cmdlist_t),
>>         &(hba[i]->cmd_pool_dhandle));
>> -    hba[i]->cmd_pool_bits = kmalloc(
>> +    hba[i]->cmd_pool_bits = kzalloc(
>>         ((NR_CMDS+BITS_PER_LONG-1)/BITS_PER_LONG)*sizeof(unsigned long),
>>         GFP_KERNEL);
> 
> kcalloc?

Agreed on every comment except this one. That complex expression is 
really just a constant in the end, so there is no point in using kcalloc.

-- 
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Pekka Enberg]

On 9/22/06, Paulo Marques <pmarques@grupopie.com> wrote:
> Agreed on every comment except this one. That complex expression is
> really just a constant in the end, so there is no point in using kcalloc.

The code is arguably easier to read with kcalloc.

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Paulo Marques]

Pekka Enberg wrote:
> On 9/22/06, Paulo Marques <pmarques@grupopie.com> wrote:
>> Agreed on every comment except this one. That complex expression is
>> really just a constant in the end, so there is no point in using kcalloc.
> 
> The code is arguably easier to read with kcalloc.

I was afraid the kcalloc call would have the added overhead of an extra 
parameter and a multiplication, but since it is actually declared as a 
static inline, gcc should optimize everything away (because both 
parameters are constants) and give the same result in the end.

So, its fine by me either way.

-- 
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Dmitry Torokhov]

On 9/22/06, Paulo Marques <pmarques@grupopie.com> wrote:
> Pekka Enberg wrote:
> > On 9/22/06, Paulo Marques <pmarques@grupopie.com> wrote:
> >> Agreed on every comment except this one. That complex expression is
> >> really just a constant in the end, so there is no point in using kcalloc.
> >
> > The code is arguably easier to read with kcalloc.
>
> I was afraid the kcalloc call would have the added overhead of an extra
> parameter and a multiplication, but since it is actually declared as a
> static inline, gcc should optimize everything away (because both
> parameters are constants) and give the same result in the end.
>

I think the we should follow a rule like this: when allocating several
separate objects of the same type at the same time (like 10 "card"
structures) kcalloc should be used. When allocating one object (even
consisting of "several ints") kmalloc/kzalloc should be used. As far
as I can see the code just tries to allocate longish bitmap and so
kzalloc is better.

Better yet, why don't you DECLARE_BITMAP(cmd_pool_bits) and embed it
right into struct ctrl_info instead of dynamically allocating it?

-- 
Dmitry

Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

[Om Narasimhan]

> I think the we should follow a rule like this: when allocating several
> separate objects of the same type at the same time (like 10 "card"
> structures) kcalloc should be used. When allocating one object (even
> consisting of "several ints") kmalloc/kzalloc should be used. As far
> as I can see the code just tries to allocate longish bitmap and so
> kzalloc is better.
>
> Better yet, why don't you DECLARE_BITMAP(cmd_pool_bits) and embed it
> right into struct ctrl_info instead of dynamically allocating it?
hba is decalred as
static ctlr_info_t *hba[MAX_CTLR]
So if I change the cmd_pool_bits to embed the DECLARE_BITMAP
statement, while compiling this file as a module, is there not a
chance of cmd_pool_bits cross a page boundary and allocated in two non
contiguous (physical) pages? Would it cause a problem with
__find_fist_zero_bit() and friends?
Regards,
Om.