Re: [PATCH 07/13] kmemcheck: add the kmemcheck core

["Vegard Nossum" <vegard.nossum@gmail.com>]

On Thu, Jul 17, 2008 at 11:49 PM, Randy Dunlap <rdunlap@xenotime.net> wrote:
> On Wed, 16 Jul 2008 02:23:29 +0200 Vegard Nossum wrote:
>
>> >From fcd8f514a8962ea22aafb831b9f22a2ea1a13870 Mon Sep 17 00:00:00 2001
>
> eh?

Uhm, probably relative to my previous patch? About the date, I have no
idea. All my "git format-patch" output looks like this.

>> +++ b/arch/x86/mm/kmemcheck/error.h
>> @@ -0,0 +1,15 @@
>> +#ifndef ARCH__X86__MM__KMEMCHECK__ERROR_H
>> +#define ARCH__X86__MM__KMEMCHECK__ERROR_H
>
> We don't usually use double __ here.

But it's such a nice convention! How can we differentiate between
mm/types.h and mm_types.h!

>> +void *kmemcheck_memset(void *s, int c, size_t n)
>> +{
>> +     unsigned long addr;
>> +     unsigned long start_page, start_offset;
>> +     unsigned long end_page, end_offset;
>> +     unsigned long i;
>> +
>> +     if (!n)
>> +             return s;
>> +
>> +     if (!slab_is_available()) {
>> +             __memset(s, c, n);
>> +             return s;
>> +     }
>> +
>> +     addr = (unsigned long) s;
>> +
>> +     start_page = addr & PAGE_MASK;
>> +     end_page = (addr + n) & PAGE_MASK;
>> +
>> +     if (start_page == end_page) {
>> +             /* The entire area is within the same page. Good, we only
>> +              * need one memset(). */
>> +             memset_one_page(s, c, n);
>> +             return s;
>> +     }
>> +
>> +     start_offset = addr & ~PAGE_MASK;
>> +     end_offset = (addr + n) & ~PAGE_MASK;
>> +
>> +     /* Clear the head, body, and tail of the memory area. */
>> +     if (start_offset < PAGE_SIZE)
>> +             memset_one_page(s, c, PAGE_SIZE - start_offset);
>> +     for (i = start_page + PAGE_SIZE; i < end_page; i += PAGE_SIZE)
>> +             memset_one_page((void *) i, c, PAGE_SIZE);
>> +     if (end_offset > 0)
>> +             memset_one_page((void *) end_page, c, end_offset);
>> +
>> +     return s;
>> +}
>> +
>> +EXPORT_SYMBOL(kmemcheck_memset);
>
> We would prefer to have kernel-doc on exported functions...

Oops. I will not add a kernel-doc, but I will add an explanation.

This function is a drop-in replacement for memset, and the reason for
the EXPORT is that the real memset() in fact is just a macro that
calls this function. And memset is needed in modules. This function
should not actually have any users beside the memset() in x86 headers.

Thanks for mini-review! (Does this mean the rest was okay? :-D)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036