[Qemu-devel] [PATCH V9 0/7] coroutine: mmap stack memory and stack size

[Qemu-devel] [PATCH V9 0/7] coroutine: mmap stack memory and stack size

[Peter Lieven]

I decided to split this from the rest of the Qemu RSS usage series as
it contains the more or less non contentious patches.

I omitted the MAP_GROWSDOWN flag in mmap as we are not 100% sure which
side effects it has.

I kept the guard page which is now nicely makes the stacks visible in
smaps. The old version of the relevent patch lacked the MAP_FIXED flag
in the second call to mmap.

v8->v9:
 - Patch 1: return size of stack including guard page [Kevin]
 - Patch 2: added to rename coroutine sigaltstack strucht [Kevin]

v7->v8:
 The series failed on platforms with 64kB page size. Thus the following changes
 where made:
 - Patch 1: add the guard page to the stack memory and do not deduct it [Kevin, Stephan]
 - Patch 1: Submit the requested page size as a pointer so that qemu_alloc_stack can
            adjust the size according to system requirements and that the full size is usable
            to the caller.
 - Patch 6: reduced stack size to 60kB so that on systems with 4kB page size we still get
            64kB allocations.

v6->v7:
 - Patch 1: avoid multiple calls to sysconf and getpagesize [Richard]

v5->v6:
 - Patch 1: added info that the guard page is deducted from stack memory to
            commit msg and headers [Stefan]
 - rebased to master

v4->v5:
 - Patch 1: check if _SC_THREAD_STACK_MIN is defined
 - Patch 1: guard against sysconf(_SC_THREAD_STACK_MIN) returning -1 [Eric]

v3->v4:
 - Patch 1: add a static function to adjust the stack size [Richard]
 - Patch 1: round up the stack size to multiple of the pagesize.

v2->v3:
 - Patch 1,6: adjusted commit message to mention the guard page [Markus]

v1->v2:
 - Patch 1: added an architecture dependend guard page [Richard]
 - Patch 1: avoid stacks smaller than _SC_THREAD_STACK_MIN [Richard]
 - Patch 1: use mmap+mprotect instead of mmap+mmap [Richard]
 - Patch 5: u_int32_t -> uint32_t [Richard]
 - Patch 5: only available if stack grows down

Peter Lieven (7):
  oslib-posix: add helpers for stack alloc and free
  coroutine-sigaltstack: rename coroutine struct appropriately
  coroutine: add a macro for the coroutine stack size
  coroutine-ucontext: use helper for allocating stack memory
  coroutine-sigaltstack: use helper for allocating stack memory
  oslib-posix: add a configure switch to debug stack usage
  coroutine: reduce stack size to 60kB

 configure                    | 19 +++++++++++
 include/qemu/coroutine_int.h |  2 ++
 include/sysemu/os-posix.h    | 27 ++++++++++++++++
 util/coroutine-sigaltstack.c | 25 +++++++-------
 util/coroutine-ucontext.c    | 11 ++++---
 util/coroutine-win32.c       |  2 +-
 util/oslib-posix.c           | 77 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 145 insertions(+), 18 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH V9 1/7] oslib-posix: add helpers for stack alloc and free

[Peter Lieven]

the allocated stack will be adjusted to the minimum supported stack size
by the OS and rounded up to be a multiple of the system pagesize.
Additionally an architecture dependent guard page is added to the stack
to catch stack overflows.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 include/sysemu/os-posix.h | 27 +++++++++++++++++++++++++++
 util/oslib-posix.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 9c7dfdf..3cfedbc 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -60,4 +60,31 @@ int qemu_utimens(const char *path, const qemu_timespec *times);
 
 bool is_daemonized(void);
 
+/**
+ * qemu_alloc_stack:
+ * @sz: pointer to a size_t holding the requested usable stack size
+ *
+ * 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()). This function also inserts an
+ * additional guard page to catch a potential stack overflow.
+ * Note that the memory required for the guard page and alignment
+ * and minimal stack size restrictions will increase the value of sz.
+ *
+ * The allocated stack must 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
+ * @sz: size of stack in bytes
+ *
+ * Free a stack allocated via qemu_alloc_stack(). Note that sz must
+ * be exactly the adjusted stack size returned by qemu_alloc_stack.
+ */
+void qemu_free_stack(void *stack, size_t sz);
+
 #endif
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f2d4e9e..5745229 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -499,3 +499,45 @@ pid_t qemu_fork(Error **errp)
     }
     return pid;
 }
+
+void *qemu_alloc_stack(size_t *sz)
+{
+    void *ptr, *guardpage;
+    size_t pagesz = getpagesize();
+#ifdef _SC_THREAD_STACK_MIN
+    /* avoid stacks smaller than _SC_THREAD_STACK_MIN */
+    long min_stack_sz = sysconf(_SC_THREAD_STACK_MIN);
+    *sz = MAX(MAX(min_stack_sz, 0), *sz);
+#endif
+    /* adjust stack size to a multiple of the page size */
+    *sz = ROUND_UP(*sz, pagesz);
+    /* allocate one extra page for the guard page */
+    *sz += pagesz;
+
+    ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE,
+               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (ptr == MAP_FAILED) {
+        abort();
+    }
+
+#if defined(HOST_IA64)
+    /* separate register stack */
+    guardpage = ptr + (((sz - pagesz) / 2) & ~pagesz);
+#elif defined(HOST_HPPA)
+    /* stack grows up */
+    guardpage = ptr + sz - pagesz;
+#else
+    /* stack grows down */
+    guardpage = ptr;
+#endif
+    if (mprotect(guardpage, pagesz, PROT_NONE) != 0) {
+        abort();
+    }
+
+    return ptr;
+}
+
+void qemu_free_stack(void *stack, size_t sz)
+{
+    munmap(stack, sz);
+}
-- 
1.9.1

Re: [Qemu-devel] [PATCH V9 1/7] oslib-posix: add helpers for stack alloc and free

[Kevin Wolf]

Am 27.09.2016 um 11:58 hat Peter Lieven geschrieben:
> the allocated stack will be adjusted to the minimum supported stack size
> by the OS and rounded up to be a multiple of the system pagesize.
> Additionally an architecture dependent guard page is added to the stack
> to catch stack overflows.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  include/sysemu/os-posix.h | 27 +++++++++++++++++++++++++++
>  util/oslib-posix.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> index 9c7dfdf..3cfedbc 100644
> --- a/include/sysemu/os-posix.h
> +++ b/include/sysemu/os-posix.h
> @@ -60,4 +60,31 @@ int qemu_utimens(const char *path, const qemu_timespec *times);
>  
>  bool is_daemonized(void);
>  
> +/**
> + * qemu_alloc_stack:
> + * @sz: pointer to a size_t holding the requested usable stack size
> + *
> + * 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()). This function also inserts an
> + * additional guard page to catch a potential stack overflow.
> + * Note that the memory required for the guard page and alignment
> + * and minimal stack size restrictions will increase the value of sz.
> + *
> + * The allocated stack must 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
> + * @sz: size of stack in bytes
> + *
> + * Free a stack allocated via qemu_alloc_stack(). Note that sz must
> + * be exactly the adjusted stack size returned by qemu_alloc_stack.
> + */
> +void qemu_free_stack(void *stack, size_t sz);
> +
>  #endif
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index f2d4e9e..5745229 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -499,3 +499,45 @@ pid_t qemu_fork(Error **errp)
>      }
>      return pid;
>  }
> +
> +void *qemu_alloc_stack(size_t *sz)
> +{
> +    void *ptr, *guardpage;
> +    size_t pagesz = getpagesize();
> +#ifdef _SC_THREAD_STACK_MIN
> +    /* avoid stacks smaller than _SC_THREAD_STACK_MIN */
> +    long min_stack_sz = sysconf(_SC_THREAD_STACK_MIN);
> +    *sz = MAX(MAX(min_stack_sz, 0), *sz);
> +#endif
> +    /* adjust stack size to a multiple of the page size */
> +    *sz = ROUND_UP(*sz, pagesz);
> +    /* allocate one extra page for the guard page */
> +    *sz += pagesz;
> +
> +    ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE,
> +               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +    if (ptr == MAP_FAILED) {
> +        abort();
> +    }
> +
> +#if defined(HOST_IA64)
> +    /* separate register stack */
> +    guardpage = ptr + (((sz - pagesz) / 2) & ~pagesz);

s/sz/*sz/

> +#elif defined(HOST_HPPA)
> +    /* stack grows up */
> +    guardpage = ptr + sz - pagesz;

Here too. I can fix both while applying the series.

> +#else
> +    /* stack grows down */
> +    guardpage = ptr;
> +#endif
> +    if (mprotect(guardpage, pagesz, PROT_NONE) != 0) {
> +        abort();
> +    }
> +
> +    return ptr;
> +}

Kevin

Re: [Qemu-devel] [PATCH V9 1/7] oslib-posix: add helpers for stack alloc and free

[Peter Lieven]

Am 27.09.2016 um 13:59 schrieb Kevin Wolf:
> Am 27.09.2016 um 11:58 hat Peter Lieven geschrieben:
>> the allocated stack will be adjusted to the minimum supported stack size
>> by the OS and rounded up to be a multiple of the system pagesize.
>> Additionally an architecture dependent guard page is added to the stack
>> to catch stack overflows.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   include/sysemu/os-posix.h | 27 +++++++++++++++++++++++++++
>>   util/oslib-posix.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 69 insertions(+)
>>
>> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
>> index 9c7dfdf..3cfedbc 100644
>> --- a/include/sysemu/os-posix.h
>> +++ b/include/sysemu/os-posix.h
>> @@ -60,4 +60,31 @@ int qemu_utimens(const char *path, const qemu_timespec *times);
>>   
>>   bool is_daemonized(void);
>>   
>> +/**
>> + * qemu_alloc_stack:
>> + * @sz: pointer to a size_t holding the requested usable stack size
>> + *
>> + * 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()). This function also inserts an
>> + * additional guard page to catch a potential stack overflow.
>> + * Note that the memory required for the guard page and alignment
>> + * and minimal stack size restrictions will increase the value of sz.
>> + *
>> + * The allocated stack must 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
>> + * @sz: size of stack in bytes
>> + *
>> + * Free a stack allocated via qemu_alloc_stack(). Note that sz must
>> + * be exactly the adjusted stack size returned by qemu_alloc_stack.
>> + */
>> +void qemu_free_stack(void *stack, size_t sz);
>> +
>>   #endif
>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>> index f2d4e9e..5745229 100644
>> --- a/util/oslib-posix.c
>> +++ b/util/oslib-posix.c
>> @@ -499,3 +499,45 @@ pid_t qemu_fork(Error **errp)
>>       }
>>       return pid;
>>   }
>> +
>> +void *qemu_alloc_stack(size_t *sz)
>> +{
>> +    void *ptr, *guardpage;
>> +    size_t pagesz = getpagesize();
>> +#ifdef _SC_THREAD_STACK_MIN
>> +    /* avoid stacks smaller than _SC_THREAD_STACK_MIN */
>> +    long min_stack_sz = sysconf(_SC_THREAD_STACK_MIN);
>> +    *sz = MAX(MAX(min_stack_sz, 0), *sz);
>> +#endif
>> +    /* adjust stack size to a multiple of the page size */
>> +    *sz = ROUND_UP(*sz, pagesz);
>> +    /* allocate one extra page for the guard page */
>> +    *sz += pagesz;
>> +
>> +    ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE,
>> +               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +    if (ptr == MAP_FAILED) {
>> +        abort();
>> +    }
>> +
>> +#if defined(HOST_IA64)
>> +    /* separate register stack */
>> +    guardpage = ptr + (((sz - pagesz) / 2) & ~pagesz);
> s/sz/*sz/
>
>> +#elif defined(HOST_HPPA)
>> +    /* stack grows up */
>> +    guardpage = ptr + sz - pagesz;
> Here too. I can fix both while applying the series.

That would be nice, thank you.

Peter

[Qemu-devel] [PATCH V9 2/7] coroutine-sigaltstack: rename coroutine struct appropriately

[Peter Lieven]

The name of the sigaltstack coroutine struct was misleading.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 util/coroutine-sigaltstack.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index a7c3366..171cd44 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -34,7 +34,7 @@ typedef struct {
     Coroutine base;
     void *stack;
     sigjmp_buf env;
-} CoroutineUContext;
+} CoroutineSigAltStack;
 
 /**
  * Per-thread coroutine bookkeeping
@@ -44,7 +44,7 @@ typedef struct {
     Coroutine *current;
 
     /** The default coroutine */
-    CoroutineUContext leader;
+    CoroutineSigAltStack leader;
 
     /** Information for the signal handler (trampoline) */
     sigjmp_buf tr_reenter;
@@ -89,7 +89,7 @@ static void __attribute__((constructor)) coroutine_init(void)
  * (from the signal handler when it is not signal handling, read ahead
  * for more information).
  */
-static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
+static void coroutine_bootstrap(CoroutineSigAltStack *self, Coroutine *co)
 {
     /* Initialize longjmp environment and switch back the caller */
     if (!sigsetjmp(self->env, 0)) {
@@ -109,7 +109,7 @@ static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
  */
 static void coroutine_trampoline(int signal)
 {
-    CoroutineUContext *self;
+    CoroutineSigAltStack *self;
     Coroutine *co;
     CoroutineThreadState *coTS;
 
@@ -144,7 +144,7 @@ static void coroutine_trampoline(int signal)
 Coroutine *qemu_coroutine_new(void)
 {
     const size_t stack_size = 1 << 20;
-    CoroutineUContext *co;
+    CoroutineSigAltStack *co;
     CoroutineThreadState *coTS;
     struct sigaction sa;
     struct sigaction osa;
@@ -251,7 +251,7 @@ Coroutine *qemu_coroutine_new(void)
 
 void qemu_coroutine_delete(Coroutine *co_)
 {
-    CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
+    CoroutineSigAltStack *co = DO_UPCAST(CoroutineSigAltStack, base, co_);
 
     g_free(co->stack);
     g_free(co);
@@ -260,8 +260,8 @@ void qemu_coroutine_delete(Coroutine *co_)
 CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
                                       CoroutineAction action)
 {
-    CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
-    CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
+    CoroutineSigAltStack *from = DO_UPCAST(CoroutineSigAltStack, base, from_);
+    CoroutineSigAltStack *to = DO_UPCAST(CoroutineSigAltStack, base, to_);
     CoroutineThreadState *s = coroutine_get_thread_state();
     int ret;
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH V9 2/7] coroutine-sigaltstack: rename coroutine struct appropriately

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

On 09/27/2016 04:58 AM, Peter Lieven wrote:
> The name of the sigaltstack coroutine struct was misleading.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  util/coroutine-sigaltstack.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH V9 3/7] coroutine: add a macro for the coroutine stack size

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 include/qemu/coroutine_int.h | 2 ++
 util/coroutine-sigaltstack.c | 2 +-
 util/coroutine-ucontext.c    | 2 +-
 util/coroutine-win32.c       | 2 +-
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index 6df9d33..14d4f1d 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,6 +28,8 @@
 #include "qemu/queue.h"
 #include "qemu/coroutine.h"
 
+#define COROUTINE_STACK_SIZE (1 << 20)
+
 typedef enum {
     COROUTINE_YIELD = 1,
     COROUTINE_TERMINATE = 2,
diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index 171cd44..a5bcb7e 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -143,7 +143,7 @@ static void coroutine_trampoline(int signal)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = 1 << 20;
+    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineSigAltStack *co;
     CoroutineThreadState *coTS;
     struct sigaction sa;
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 2bb7e10..31254ab 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -82,7 +82,7 @@ static void coroutine_trampoline(int i0, int i1)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = 1 << 20;
+    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineUContext *co;
     ucontext_t old_uc, uc;
     sigjmp_buf old_env;
diff --git a/util/coroutine-win32.c b/util/coroutine-win32.c
index 02e28e8..de6bd4f 100644
--- a/util/coroutine-win32.c
+++ b/util/coroutine-win32.c
@@ -71,7 +71,7 @@ static void CALLBACK coroutine_trampoline(void *co_)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = 1 << 20;
+    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineWin32 *co;
 
     co = g_malloc0(sizeof(*co));
-- 
1.9.1

[Qemu-devel] [PATCH V9 4/7] coroutine-ucontext: use helper for allocating stack memory

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 util/coroutine-ucontext.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 31254ab..6621f3f 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -34,6 +34,7 @@
 typedef struct {
     Coroutine base;
     void *stack;
+    size_t stack_size;
     sigjmp_buf env;
 
 #ifdef CONFIG_VALGRIND_H
@@ -82,7 +83,6 @@ static void coroutine_trampoline(int i0, int i1)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineUContext *co;
     ucontext_t old_uc, uc;
     sigjmp_buf old_env;
@@ -101,17 +101,18 @@ Coroutine *qemu_coroutine_new(void)
     }
 
     co = g_malloc0(sizeof(*co));
-    co->stack = g_malloc(stack_size);
+    co->stack_size = COROUTINE_STACK_SIZE;
+    co->stack = qemu_alloc_stack(&co->stack_size);
     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 = co->stack_size;
     uc.uc_stack.ss_flags = 0;
 
 #ifdef CONFIG_VALGRIND_H
     co->valgrind_stack_id =
-        VALGRIND_STACK_REGISTER(co->stack, co->stack + stack_size);
+        VALGRIND_STACK_REGISTER(co->stack, co->stack + co->stack_size);
 #endif
 
     arg.p = co;
@@ -149,7 +150,7 @@ void qemu_coroutine_delete(Coroutine *co_)
     valgrind_stack_deregister(co);
 #endif
 
-    g_free(co->stack);
+    qemu_free_stack(co->stack, co->stack_size);
     g_free(co);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH V9 4/7] coroutine-ucontext: use helper for allocating stack memory

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On 09/27/2016 04:58 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  util/coroutine-ucontext.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH V9 5/7] coroutine-sigaltstack: use helper for allocating stack memory

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 util/coroutine-sigaltstack.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index a5bcb7e..f6fc49a 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -33,6 +33,7 @@
 typedef struct {
     Coroutine base;
     void *stack;
+    size_t stack_size;
     sigjmp_buf env;
 } CoroutineSigAltStack;
 
@@ -143,7 +144,6 @@ static void coroutine_trampoline(int signal)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineSigAltStack *co;
     CoroutineThreadState *coTS;
     struct sigaction sa;
@@ -164,7 +164,8 @@ Coroutine *qemu_coroutine_new(void)
      */
 
     co = g_malloc0(sizeof(*co));
-    co->stack = g_malloc(stack_size);
+    co->stack_size = COROUTINE_STACK_SIZE;
+    co->stack = qemu_alloc_stack(&co->stack_size);
     co->base.entry_arg = &old_env; /* stash away our jmp_buf */
 
     coTS = coroutine_get_thread_state();
@@ -189,7 +190,7 @@ Coroutine *qemu_coroutine_new(void)
      * Set the new stack.
      */
     ss.ss_sp = co->stack;
-    ss.ss_size = stack_size;
+    ss.ss_size = co->stack_size;
     ss.ss_flags = 0;
     if (sigaltstack(&ss, &oss) < 0) {
         abort();
@@ -253,7 +254,7 @@ void qemu_coroutine_delete(Coroutine *co_)
 {
     CoroutineSigAltStack *co = DO_UPCAST(CoroutineSigAltStack, base, co_);
 
-    g_free(co->stack);
+    qemu_free_stack(co->stack, co->stack_size);
     g_free(co);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH V9 5/7] coroutine-sigaltstack: use helper for allocating stack memory

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On 09/27/2016 04:58 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  util/coroutine-sigaltstack.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH V9 6/7] oslib-posix: add a configure switch to debug stack usage

[Peter Lieven]

this adds a knob to track the maximum stack usage of stacks
created by qemu_alloc_stack.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 configure          | 19 +++++++++++++++++++
 util/oslib-posix.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/configure b/configure
index 8fa62ad..93ef00a 100755
--- a/configure
+++ b/configure
@@ -296,6 +296,7 @@ libiscsi=""
 libnfs=""
 coroutine=""
 coroutine_pool=""
+debug_stack_usage="no"
 seccomp=""
 glusterfs=""
 glusterfs_xlator_opt="no"
@@ -1004,6 +1005,8 @@ for opt do
   ;;
   --enable-coroutine-pool) coroutine_pool="yes"
   ;;
+  --enable-debug-stack-usage) debug_stack_usage="yes"
+  ;;
   --disable-docs) docs="no"
   ;;
   --enable-docs) docs="yes"
@@ -4276,6 +4279,17 @@ if test "$coroutine" = "gthread" -a "$coroutine_pool" = "yes"; then
   error_exit "'gthread' coroutine backend does not support pool (use --disable-coroutine-pool)"
 fi
 
+if test "$debug_stack_usage" = "yes"; then
+  if test "$cpu" = "ia64" -o "$cpu" = "hppa"; then
+    error_exit "stack usage debugging is not supported for $cpu"
+  fi
+  if test "$coroutine_pool" = "yes"; then
+    echo "WARN: disabling coroutine pool for stack usage debugging"
+    coroutine_pool=no
+  fi
+fi
+
+
 ##########################################
 # check if we have open_by_handle_at
 
@@ -4861,6 +4875,7 @@ echo "QGA MSI support   $guest_agent_msi"
 echo "seccomp support   $seccomp"
 echo "coroutine backend $coroutine"
 echo "coroutine pool    $coroutine_pool"
+echo "debug stack usage $debug_stack_usage"
 echo "GlusterFS support $glusterfs"
 echo "Archipelago support $archipelago"
 echo "gcov              $gcov_tool"
@@ -5330,6 +5345,10 @@ else
   echo "CONFIG_COROUTINE_POOL=0" >> $config_host_mak
 fi
 
+if test "$debug_stack_usage" = "yes" ; then
+  echo "CONFIG_DEBUG_STACK_USAGE=y" >> $config_host_mak
+fi
+
 if test "$open_by_handle_at" = "yes" ; then
   echo "CONFIG_OPEN_BY_HANDLE=y" >> $config_host_mak
 fi
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 5745229..21a5637 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -50,6 +50,10 @@
 
 #include "qemu/mmap-alloc.h"
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+#include "qemu/error-report.h"
+#endif
+
 int qemu_get_thread_id(void)
 {
 #if defined(__linux__)
@@ -503,6 +507,9 @@ pid_t qemu_fork(Error **errp)
 void *qemu_alloc_stack(size_t *sz)
 {
     void *ptr, *guardpage;
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    void *ptr2;
+#endif
     size_t pagesz = getpagesize();
 #ifdef _SC_THREAD_STACK_MIN
     /* avoid stacks smaller than _SC_THREAD_STACK_MIN */
@@ -534,10 +541,38 @@ void *qemu_alloc_stack(size_t *sz)
         abort();
     }
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    for (ptr2 = ptr + pagesz; ptr2 < ptr + *sz; ptr2 += sizeof(uint32_t)) {
+        *(uint32_t *)ptr2 = 0xdeadbeaf;
+    }
+#endif
+
     return ptr;
 }
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+static __thread unsigned int max_stack_usage;
+#endif
+
 void qemu_free_stack(void *stack, size_t sz)
 {
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    unsigned int usage;
+    void *ptr;
+
+    for (ptr = stack + getpagesize(); ptr < stack + sz;
+         ptr += sizeof(uint32_t)) {
+        if (*(uint32_t *)ptr != 0xdeadbeaf) {
+            break;
+        }
+    }
+    usage = sz - (uintptr_t) (ptr - stack);
+    if (usage > max_stack_usage) {
+        error_report("thread %d max stack usage increased from %u to %u",
+                     qemu_get_thread_id(), max_stack_usage, usage);
+        max_stack_usage = usage;
+    }
+#endif
+
     munmap(stack, sz);
 }
-- 
1.9.1

Re: [Qemu-devel] [PATCH V9 6/7] oslib-posix: add a configure switch to debug stack usage

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On 09/27/2016 04:58 AM, Peter Lieven wrote:
> this adds a knob to track the maximum stack usage of stacks
> created by qemu_alloc_stack.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  configure          | 19 +++++++++++++++++++
>  util/oslib-posix.c | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)

> @@ -534,10 +541,38 @@ void *qemu_alloc_stack(size_t *sz)
>          abort();
>      }
>  
> +#ifdef CONFIG_DEBUG_STACK_USAGE
> +    for (ptr2 = ptr + pagesz; ptr2 < ptr + *sz; ptr2 += sizeof(uint32_t)) {
> +        *(uint32_t *)ptr2 = 0xdeadbeaf;

It's usually spelled 0xdeadbeef, if you want to tweak it.

> +    }
> +#endif
> +
>      return ptr;
>  }
>  
> +#ifdef CONFIG_DEBUG_STACK_USAGE
> +static __thread unsigned int max_stack_usage;
> +#endif
> +
>  void qemu_free_stack(void *stack, size_t sz)
>  {
> +#ifdef CONFIG_DEBUG_STACK_USAGE
> +    unsigned int usage;
> +    void *ptr;
> +
> +    for (ptr = stack + getpagesize(); ptr < stack + sz;
> +         ptr += sizeof(uint32_t)) {
> +        if (*(uint32_t *)ptr != 0xdeadbeaf) {

Of course, you'd have to do it here, too.

Either way,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH V9 7/7] coroutine: reduce stack size to 60kB

[Peter Lieven]

evaluation with the recently introduced maximum stack usage monitoring revealed
that the actual used stack size was never above 4kB so allocating 1MB stack
for each coroutine is a lot of wasted memory. So reduce the stack size to
60kB which should still give enough head room. The guard page added
in qemu_alloc_stack will catch a potential stack overflow introduced
by this commit. The 60kB + guard page will result in an allocation of
64kB per coroutine on systems where a page is 4kB.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 include/qemu/coroutine_int.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index 14d4f1d..be14260 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,7 +28,7 @@
 #include "qemu/queue.h"
 #include "qemu/coroutine.h"
 
-#define COROUTINE_STACK_SIZE (1 << 20)
+#define COROUTINE_STACK_SIZE 61440
 
 typedef enum {
     COROUTINE_YIELD = 1,
-- 
1.9.1

Re: [Qemu-devel] [PATCH V9 7/7] coroutine: reduce stack size to 60kB

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

On 09/27/2016 04:58 AM, Peter Lieven wrote:
> evaluation with the recently introduced maximum stack usage monitoring revealed
> that the actual used stack size was never above 4kB so allocating 1MB stack
> for each coroutine is a lot of wasted memory. So reduce the stack size to
> 60kB which should still give enough head room. The guard page added
> in qemu_alloc_stack will catch a potential stack overflow introduced
> by this commit. The 60kB + guard page will result in an allocation of
> 64kB per coroutine on systems where a page is 4kB.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  include/qemu/coroutine_int.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
> index 14d4f1d..be14260 100644
> --- a/include/qemu/coroutine_int.h
> +++ b/include/qemu/coroutine_int.h
> @@ -28,7 +28,7 @@
>  #include "qemu/queue.h"
>  #include "qemu/coroutine.h"
>  
> -#define COROUTINE_STACK_SIZE (1 << 20)
> +#define COROUTINE_STACK_SIZE 61440

Should work fine (I might have done 64k + guard page, but that's not
quite as nice of a round number where the guard page is only 4k).

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]