Re: [PATCH 3/4] sparse: using kzalloc to clean up code

[Bob Liu <lliubbo@gmail.com>]

On Thu, Aug 4, 2011 at 2:10 PM, Pekka Enberg <penberg@kernel.org> wrote:
> On Thu, Aug 4, 2011 at 6:09 AM, Bob Liu <lliubbo@gmail.com> wrote:
>> This patch using kzalloc to clean up sparse_index_alloc() and
>> __GFP_ZERO to clean up __kmalloc_section_memmap().
>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> ---
>>  mm/sparse.c |   24 +++++++-----------------
>>  1 files changed, 7 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index 858e1df..9596635 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -65,15 +65,12 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
>>
>>        if (slab_is_available()) {
>>                if (node_state(nid, N_HIGH_MEMORY))
>> -                       section = kmalloc_node(array_size, GFP_KERNEL, nid);
>> +                       section = kzalloc_node(array_size, GFP_KERNEL, nid);
>>                else
>> -                       section = kmalloc(array_size, GFP_KERNEL);
>> +                       section = kzalloc(array_size, GFP_KERNEL);
>>        } else
>>                section = alloc_bootmem_node(NODE_DATA(nid), array_size);
>>
>> -       if (section)
>> -               memset(section, 0, array_size);
>> -
>
> You now broke the alloc_bootmem_node() path.
>

Yes.
But In my opinion, the alloc_bootmem_node() will also return zeroed memory.
I saw it has used kzalloc or memset() but i'm not pretty sure.
CC'd yinghai@kernel.org,hpa@zytor.com

Thanks for your review.

>>        return section;
>>  }
>>
>> @@ -636,19 +633,12 @@ static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
>>        struct page *page, *ret;
>>        unsigned long memmap_size = sizeof(struct page) * nr_pages;
>>
>> -       page = alloc_pages(GFP_KERNEL|__GFP_NOWARN, get_order(memmap_size));
>> +       page = alloc_pages(GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO,
>> +                                       get_order(memmap_size));
>>        if (page)
>> -               goto got_map_page;
>> -
>> -       ret = vmalloc(memmap_size);
>> -       if (ret)
>> -               goto got_map_ptr;
>> -
>> -       return NULL;
>> -got_map_page:
>> -       ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
>> -got_map_ptr:
>> -       memset(ret, 0, memmap_size);
>> +               ret = (struct page *)pfn_to_kaddr(page_to_pfn(page));
>> +       else
>> +               ret = vzalloc(memmap_size);
>>
>>        return ret;
>>  }
>> --
>> 1.6.3.3
>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
>

-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>