Date: Mon, 6 Dec 2010 23:07:33 +0100 From: Marius Strobl <marius@alchemy.franken.de> To: Alan Cox <alc@rice.edu> Cc: svn-src-head@freebsd.org, alc@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Max Khon <fjoe@samodelkin.net> Subject: Re: svn commit: r216016 - head/sys/sparc64/include Message-ID: <20101206220733.GG38282@alchemy.franken.de> In-Reply-To: <4CF7D711.9040505@rice.edu> References: <201011281926.oASJQKiE040689@svn.freebsd.org> <20101128194542.GF9966@alchemy.franken.de> <AANLkTinwkVO%2BRkr7coYGhuMdZ_UQQa3_18c3D6fBLucA@mail.gmail.com> <20101129192308.GX80343@alchemy.franken.de> <20101129192417.GA18893@alchemy.franken.de> <4CF691A5.8070608@rice.edu> <20101202164727.GB38282@alchemy.franken.de> <4CF7D711.9040505@rice.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Dec 02, 2010 at 11:27:45AM -0600, Alan Cox wrote: > Marius Strobl wrote: > >On Wed, Dec 01, 2010 at 12:19:17PM -0600, Alan Cox wrote: > > > >>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@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@. > >>> > >>> > >>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); > >>} > >> > >> > > > >Thanks but unfortunately this variant then still panics in kmem_suballoc() > >when called by memguard_init(): > >KDB: debugger backends: ddb > >KDB: current backend: ddb > >Copyright (c) 1992-2010 The FreeBSD Project. > >Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 > > The Regents of the University of California. All rights reserved. > >FreeBSD is a registered trademark of The FreeBSD Foundation. > >FreeBSD 9.0-CURRENT #17 r215249:216120M: Thu Dec 2 15:17:35 CET 2010 > > marius@v20z.zeist.de:/home/marius/co/build/head2/sparc64.sparc64/usr/home/m4 > >WARNING: WITNESS option enabled, expect reduced performance. > >panic: kmem_suballoc: bad status return of 1 > >cpuid = 0 > >KDB: enter: panic > >[ thread pid 0 tid 0 ] > >Stopped at 0xc03b04c0: ta %xcc, 1 > >db> bt > >Tracing pid 0 tid 0 td 0xc089ca10 > >(null)() at 0xc0371bac > >(null)() at 0xc054d2dc > >(null)() at 0xc0547dc8 > >(null)() at 0xc0359a78 > >(null)() at 0xc031e930 > >(null)() at 0xc0070028 > > > > > > Sorry, I overlooked another use of km_max in memguard_fudge(). Please > try this instead: > > Index: vm/memguard.c > =================================================================== > --- vm/memguard.c (revision 216070) > +++ vm/memguard.c (working copy) > @@ -184,9 +184,10 @@ memguard_fudge(unsigned long km_size, unsigned lon > memguard_mapsize = km_max / vm_memguard_divisor; > /* size must be multiple of PAGE_SIZE */ > memguard_mapsize = round_page(memguard_mapsize); > - if (memguard_mapsize / (2 * PAGE_SIZE) > mem_pgs) > + if (memguard_mapsize == 0 || > + 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); > } With that one the kernel now survies memguard_init() but then panics right afterwards when kmeminit() calls kmem_suballoc(): KDB: debugger backends: ddb KDB: current backend: ddb Copyright (c) 1992-2010 The FreeBSD Project. Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 The Regents of the University of California. All rights reserved. FreeBSD is a registered trademark of The FreeBSD Foundation. FreeBSD 9.0-CURRENT #18 r215249:216120M: Mon Dec 6 13:27:57 CET 2010 marius@v20z.zeist.de:/home/marius/co/build/head2/sparc64.sparc64/usr/home/m4 WARNING: WITNESS option enabled, expect reduced performance. panic: kmem_suballoc: bad status return of 3 cpuid = 0 KDB: enter: panic [ thread pid 0 tid 0 ] Stopped at 0xc03b04c0: ta %xcc, 1 db> bt Tracing pid 0 tid 0 td 0xc089ca10 (null)() at 0xc0371bac (null)() at 0xc054d2fc (null)() at 0xc0359a50 (null)() at 0xc031e930 (null)() at 0xc0070028 Note that the status is different now though. Marius
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101206220733.GG38282>