From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52574) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHq9N-0002JY-AJ for qemu-devel@nongnu.org; Tue, 28 Jun 2016 06:21:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHq9H-0004iX-UX for qemu-devel@nongnu.org; Tue, 28 Jun 2016 06:21:48 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:39271 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHq9H-0004iP-KW for qemu-devel@nongnu.org; Tue, 28 Jun 2016 06:21:43 -0400 References: <1467104499-27517-1-git-send-email-pl@kamp.de> <1467104499-27517-2-git-send-email-pl@kamp.de> From: Peter Lieven Message-ID: <57724FB3.3020008@kamp.de> Date: Tue, 28 Jun 2016 12:21:39 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/15] coroutine-ucontext: mmap stack memory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , Kevin Wolf , Max Reitz , Paolo Bonzini , "Michael S. Tsirkin" , "Dr. David Alan Gilbert" , Gerd Hoffmann Am 28.06.2016 um 12:02 schrieb Peter Maydell: > On 28 June 2016 at 10:01, Peter Lieven wrote: >> coroutine-ucontext currently allocates stack memory from heap as on most systems the >> stack size lays below the threshold for mmapping memory. This patch forces mmaping >> of stacks to avoid large holes on the heap when a coroutine is deleted. It additionally >> allows us for adding a guard page at the bottom of the stack to avoid overflows. >> >> Suggested-by: Peter Maydell >> Signed-off-by: Peter Lieven >> --- >> util/coroutine-ucontext.c | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c >> index 2bb7e10..841e7db 100644 >> --- a/util/coroutine-ucontext.c >> +++ b/util/coroutine-ucontext.c >> @@ -80,9 +80,10 @@ static void coroutine_trampoline(int i0, int i1) >> } >> } >> >> +#define COROUTINE_STACK_SIZE (1 << 20) >> + >> Coroutine *qemu_coroutine_new(void) >> { >> - const size_t stack_size = 1 << 20; >> CoroutineUContext *co; >> ucontext_t old_uc, uc; >> sigjmp_buf old_env; >> @@ -101,17 +102,32 @@ Coroutine *qemu_coroutine_new(void) >> } >> >> co = g_malloc0(sizeof(*co)); >> + >> +#ifdef MAP_GROWSDOWN >> + co->stack = mmap(NULL, COROUTINE_STACK_SIZE, PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANONYMOUS | MAP_GROWSDOWN, -1, 0); >> + if (co->stack == MAP_FAILED) { >> + abort(); >> + } >> + /* add a guard page at bottom of the stack */ >> + if (mmap(co->stack, getpagesize(), PROT_NONE, >> + MAP_PRIVATE | MAP_ANONYMOUS | MAP_GROWSDOWN, -1, 0) == MAP_FAILED) { >> + abort(); >> + } >> +#else >> co->stack = g_malloc(stack_size); > I would just mmap() always; then we get the benefit of the > guard page even if there's no MAP_GROWSDOWN. > > Also, does MAP_GROWSDOWN help with the RSS issues? I > noticed that glibc itself doesn't use it for pthread > stacks as far as I can tell, so maybe it's obsolete? > (Ulrich Drepper apparently thought so in 2008: > https://lwn.net/Articles/294001/ ) I have seen this thread. The MAP_GROWSDOWN does not help at all as far as it seems. Only reducing the stack size does. > >> +#endif > Can we abstract this out into an alloc/dealloc function, please? > > /** > * qemu_alloc_stack: > * @sz: size of required stack in bytes > * > * Allocate memory that can be used as a stack, for instance for > * coroutines. If the memory cannot be allocated, this function > * will abort (like g_malloc()). The allocated stack should be > * freed with qemu_free_stack(). > * > * Returns: pointer to (the lowest address of) the stack memory. > */ > void *qemu_alloc_stack(size_t sz); > > /** > * qemu_free_stack: > * @stack: stack to free > * > * Free a stack allocated via qemu_alloc_stack(). > */ > void qemu_free_stack(void *stack); we need to pass the size also for munmap. > > util/coroutine-sigaltstack.c can then use the same function > for stack allocation. > > I would put the implementation in util/oslib-posix.c and the > header in include/sysemu/os-posix.h, unless somebody has a > better idea. > >> + >> co->base.entry_arg = &old_env; /* stash away our jmp_buf */ >> >> uc.uc_link = &old_uc; >> uc.uc_stack.ss_sp = co->stack; >> - uc.uc_stack.ss_size = stack_size; >> + uc.uc_stack.ss_size = COROUTINE_STACK_SIZE; > Because of the guard page, your code above isn't actually > allocating this much stack. oh yes, you are right. Peter