svn commit: r216016 - head/sys/sparc64/include

[Alan Cox]

Marius Strobl wrote:
> On Mon, Nov 29, 2010 at 08:23:08PM +0100, Marius Strobl wrote:
>   
>> On Tue, Nov 30, 2010 at 12:31:31AM +0600, Max Khon wrote:
>>     
>>> Marius,
>>>
>>> On Mon, Nov 29, 2010 at 1:45 AM, Marius Strobl <marius at alchemy.franken.de>wrote:
>>>
>>> On Sun, Nov 28, 2010 at 07:26:20PM +0000, Max Khon wrote:
>>>       
>>>>> Author: fjoe
>>>>> Date: Sun Nov 28 19:26:20 2010
>>>>> New Revision: 216016
>>>>> URL: http://svn.freebsd.org/changeset/base/216016
>>>>>
>>>>> Log:
>>>>>   Define VM_KMEM_SIZE_MAX on sparc64. Otherwise kernel built with
>>>>>   DEBUG_MEMGUARD panics early in kmeminit() with the message
>>>>>   "kmem_suballoc: bad status return of 1" because of zero "size" argument
>>>>>   passed to kmem_suballoc() due to "vm_kmem_size_max" being zero.
>>>>>
>>>>>   The problem also exists on ia64.
>>>>>
>>>>> Modified:
>>>>>   head/sys/sparc64/include/vmparam.h
>>>>>
>>>>> Modified: head/sys/sparc64/include/vmparam.h
>>>>>
>>>>>           
>>>> ==============================================================================
>>>>         
>>>>> --- head/sys/sparc64/include/vmparam.h        Sun Nov 28 18:59:52 2010
>>>>>           
>>>>      (r216015)
>>>>         
>>>>> +++ head/sys/sparc64/include/vmparam.h        Sun Nov 28 19:26:20 2010
>>>>>           
>>>>      (r216016)
>>>>         
>>>>> @@ -237,6 +237,14 @@
>>>>>  #endif
>>>>>
>>>>>  /*
>>>>> + * Ceiling on amount of kmem_map kva space.
>>>>> + */
>>>>> +#ifndef VM_KMEM_SIZE_MAX
>>>>> +#define      VM_KMEM_SIZE_MAX        ((VM_MAX_KERNEL_ADDRESS - \
>>>>> +    VM_MIN_KERNEL_ADDRESS + 1) * 3 / 5)
>>>>> +#endif
>>>>> +
>>>>> +/*
>>>>>   * Initial pagein size of beginning of executable file.
>>>>>   */
>>>>>  #ifndef      VM_INITIAL_PAGEIN
>>>>>           
>>>> How was that value determined?
>>>>
>>>>         
>>> I've just copied it from amd64 to be non-zero for now. Do you have a better
>>> idea of what it should look like?
>>>
>>>       
>> Well, on sparc64 VM_MAX_KERNEL_ADDRESS already is dynamically adjusted
>> to the maximum appropriate for the specific CPU during the early cycles
>> of the kernel so I'd think one could just use VM_MAX_KERNEL_ADDRESS -
>> VM_MIN_KERNEL_ADDRESS for VM_KMEM_SIZE_MAX there, I'm not sure what
>> the intention of the ceiling provided by that macro actually is though
>> In any case, the commit message of r180210 which changed the amd64
>> version to the current one talks about limiting the kmem map to 3.6GB
>> and while it also fails to explain where that value comes from it
>> looks rather amd64 specific and the formula used by the macro will
>> result in a different ceiling on sparc64 and thus inappropriate. I've
>> CC'ed alc@ who hopefully can shed some light on this.
>> Apart from this the actual bug here seems to be that memguard_fudge()
>> can't deal with a km_max being zero or that zero is passed to it as
>> kmeminit() allows for VM_KMEM_SIZE_MAX not being defined.
>>
>>     
>
> Oops, forgot to actually CC alc at .
>   

There's nothing particularly amd64-specific about the definition.  In 
general, if you allow the kmem_map, which is basically the kernel's 
heap, to consume the entire kernel address space as you propose, then 
you're leaving no room for the buffer cache, thread stacks, pipes, and a 
few other things.  Since the cap on the kmem_map size as defined by 
r180210 is a fraction of the overall kernel address space size, it 
scales automatically with the kernel address space size and should be a 
reasonable cap definition for most architectures.

In the specific case of sparc64, I think it's fair to say that the 
kernel virtual address is sufficiently large and the amount of physical 
memory in any of the supported machines is small enough in comparison 
that it hasn't mattered that a kmem_map cap doesn't exist, because most 
of the aforementioned structures are scaled based on the amount of 
physical memory.  In fact, it probably won't matter any time soon.

All of that said, I would suggest fixing memguard_fudge() and reverting 
r216016 and the follow up change.  All I think that is required to fix 
memguard_fudge() is

Index: vm/memguard.c
===================================================================
--- vm/memguard.c       (revision 216070)
+++ vm/memguard.c       (working copy)
@@ -186,7 +186,7 @@ memguard_fudge(unsigned long km_size, unsigned lon
        memguard_mapsize = round_page(memguard_mapsize);
        if (memguard_mapsize / (2 * PAGE_SIZE) > mem_pgs)
                memguard_mapsize = mem_pgs * 2 * PAGE_SIZE;
-       if (km_size + memguard_mapsize > km_max)
+       if (km_max > 0 && km_size + memguard_mapsize > km_max)
                return (km_max);
        return (km_size + memguard_mapsize);
 }


Regards,
Alan