Skip to contentSkip to conversation

remove explicit memset to memory allocated with k[zc]alloc

PATCH In review Christophe Jaillet 3 participants 4 messages Dec 18, 2007 – Dec 31, 2007 Kernel janitors

lore
Share & tools
lore
#0 lore raw

PATCH 3/3 remove explicit memset to memory allocated with k[zc]alloc

1) Remove an explicit memset(.., 0, ...) to a varialable allocated with kzalloc.

2) Allocate 'src' with kmalloc instead of kzalloc as all elements of the 'src' buffer are initialized in a 'for(...)' loop

---

--- linux-2.6.24-rc5/drivers/dma/iop-adma.c 2007-12-16 11:15:52.000000000 +0100
+++ linux-2.6.24-rc5/drivers/dma/iop-adma.c.cj 2007-12-17 22:05:36.000000000 +0100
@@ -858,7 +858,7 @@ static int __devinit iop_adma_memcpy_sel

  dev_dbg(device->common.dev, "%s\n", __FUNCTION__);

- src = kzalloc(sizeof(u8) * IOP_ADMA_TEST_SIZE, GFP_KERNEL);
+ src = kmalloc(sizeof(u8) * IOP_ADMA_TEST_SIZE, GFP_KERNEL); if (!src)
return -ENOMEM;
dest = kzalloc(sizeof(u8) * IOP_ADMA_TEST_SIZE, GFP_KERNEL);
@@ -871,8 +871,6 @@ static int __devinit iop_adma_memcpy_sel
for (i = 0; i < IOP_ADMA_TEST_SIZE; i++)
((u8 *) src)[i] = (u8)i;

- memset(dest, 0, IOP_ADMA_TEST_SIZE);
-
/* Start copy, using first DMA channel */ dma_chan = container_of(device->common.channels.next,
struct dma_chan,

#1 lore raw

Hi!

Quoted 5 lines

1) Remove an explicit memset(.., 0, ...) to a varialable allocated with kzalloc.

2) Allocate 'src' with kmalloc instead of kzalloc as all elements of the 'src' buffer are initialized in a 'for(...)' loop

The changes look good to me, but there's another thing that could be done in that file:
There's no need for the expression "sizeof(u8)" since it always returns 1. Could you fix that too and submit a new patch?

There are quite a lot of statements like that in the kernel...

$ grep -r "sizeof(u8)" . | wc -l
53
Quoted 14 lines

--- linux-2.6.24-rc5/drivers/dma/iop-adma.c 2007-12-16 11:15:52.000000000 +0100
+++ linux-2.6.24-rc5/drivers/dma/iop-adma.c.cj 2007-12-17 22:05:36.000000000 +0100

(quoted diff) +1 -1
@@ -858,7 +858,7 @@ static int __devinit iop_adma_memcpy_sel
 
  dev_dbg(device->common.dev, "%s\n", __FUNCTION__);
 
- src = kzalloc(sizeof(u8) * IOP_ADMA_TEST_SIZE, GFP_KERNEL);
+ src = kmalloc(sizeof(u8) * IOP_ADMA_TEST_SIZE, GFP_KERNEL);
  if (!src)
   return -ENOMEM;
  dest = kzalloc(sizeof(u8) * IOP_ADMA_TEST_SIZE, GFP_KERNEL);

Review static int __devinit iop_adma_memcpy_sel · line 876

Quoted 8 lines
(quoted diff) +0 -2
@@ -871,8 +871,6 @@ static int __devinit iop_adma_memcpy_sel
  for (i = 0; i < IOP_ADMA_TEST_SIZE; i++)
   ((u8 *) src)[i] = (u8)i;
 
- memset(dest, 0, IOP_ADMA_TEST_SIZE);
-
  /* Start copy, using first DMA channel */
  dma_chan = container_of(device->common.channels.next,
     struct dma_chan,

Regards,
Andi

#2 lore raw

Re: [PATCH 3/3] remove explicit memset to memory allocated with

In reply to Christophe Jaillet
Quoted 7 lines · Andi Drebes

Hi!

Quoted 5 lines

1) Remove an explicit memset(.., 0, ...) to a varialable allocated with kzalloc.

2) Allocate 'src' with kmalloc instead of kzalloc as all elements of the 'src' buffer are initialized in a 'for(...)' loop

Quoted 8 lines

The changes look good to me, but there's another thing that could be done in that file:
There's no need for the expression "sizeof(u8)" since it always returns 1. Could you fix that too and submit a new patch?

There are quite a lot of statements like that in the kernel...

$ grep -r "sizeof(u8)" . | wc -l
53

What is the problem you are trying to solve?

It might in some places be an improvement to replace a sizeof(u8) with a sizeof() on the actual variable, but blindly replacing sizeof(u8) with 1 sounds like an attempt to make the code harder to read without bringing any advantage...

Quoted 2 lines

Regards,
Andi

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#3 lore raw
In reply to Christophe Jaillet
Quoted 10 lines
Quoted 4 lines

There are quite a lot of statements like that in the kernel...

$ grep -r "sizeof(u8)" . | wc -l
53

What is the problem you are trying to solve?
It might in some places be an improvement to replace a sizeof(u8) with a sizeof() on the actual variable, but blindly replacing sizeof(u8) with 1 sounds like an attempt to make the code harder to read without bringing any advantage...

Perhaps my comments on the particular patch from Christophe sounded as if they applied to all sizeof(u8) statements. I was not talking about *blindly* replacing them. I didn't even say that they *all* should be replaced. What I wanted to say was: maybe one should have a look at other places in the kernel aswell. The above grep was just a quick grep through the sources.

Of course, there's no functional problem with sizeof(u8). It should be kept in places where it makes things clear. But in my eyes, sizeof(u8) in something like this (as in the lines affected by the patch):

kmalloc(sizeof(u8) * IOP_ADMA_TEST_SIZE, GFP_KERNEL);

isn't very useful. I think it's quite clear that kmalloc takes a number of bytes as the first argument. So replacing the above code with the following

kmalloc(IOP_ADMA_TEST_SIZE, GFP_KERNEL);

would save some bytes and make it compile faster. And it isn't harder to read. However, I don't insist on changes like that. I just thought that for drivers/dma/iop-adma.c it may be included in Christophe's patch.

Andi