[Qemu-devel] [PATCH V8 0/6] coroutine: mmap stack memory and stack size

[Qemu-devel] [PATCH V8 0/6] 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.

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 (6):
  oslib-posix: add helpers for stack alloc and free
  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 |  9 ++---
 util/coroutine-ucontext.c    | 11 +++---
 util/coroutine-win32.c       |  2 +-
 util/oslib-posix.c           | 81 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 141 insertions(+), 10 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH V8 1/6] 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        | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 9c7dfdf..4a0f493 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 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 useable stack memory can be greater than the
+ * requested stack size due to alignment and minimal stack size
+ * restrictions. In this case the value of sz is adjusted.
+ *
+ * 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().
+ */
+void qemu_free_stack(void *stack, size_t sz);
+
 #endif
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f2d4e9e..7d053b8 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -499,3 +499,46 @@ pid_t qemu_fork(Error **errp)
     }
     return pid;
 }
+
+void *qemu_alloc_stack(size_t *sz)
+{
+    void *ptr, *guardpage;
+    size_t pagesz = getpagesize();
+    size_t allocsz;
+#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 */
+    allocsz = *sz + getpagesize();
+
+    ptr = mmap(NULL, allocsz, PROT_READ | PROT_WRITE,
+               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (ptr == MAP_FAILED) {
+        abort();
+    }
+
+#if defined(HOST_IA64)
+    /* separate register stack */
+    guardpage = ptr + (((allocsz - pagesz) / 2) & ~pagesz);
+#elif defined(HOST_HPPA)
+    /* stack grows up */
+    guardpage = ptr + allocsz - 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 + getpagesize());
+}
-- 
1.9.1

[Qemu-devel] [PATCH V8 2/6] 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 a7c3366..9c2854c 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;
     CoroutineUContext *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 V8 3/6] 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

[Qemu-devel] [PATCH V8 4/6] 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 9c2854c..d9c7f66 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;
 } CoroutineUContext;
 
@@ -143,7 +144,6 @@ static void coroutine_trampoline(int signal)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineUContext *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_)
 {
     CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
 
-    g_free(co->stack);
+    qemu_free_stack(co->stack, co->stack_size);
     g_free(co);
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH V8 5/6] 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 | 40 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

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 7d053b8..18940d9 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();
     size_t allocsz;
 #ifdef _SC_THREAD_STACK_MIN
@@ -535,10 +542,41 @@ void *qemu_alloc_stack(size_t *sz)
         abort();
     }
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    for (ptr2 = ptr + pagesz; ptr2 < ptr + allocsz; 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)
 {
-    munmap(stack, sz + getpagesize());
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    unsigned int usage;
+    void *ptr;
+#endif
+    size_t pagesz = getpagesize();
+    size_t allocsz = sz + pagesz;
+
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    for (ptr = stack + pagesz; ptr < stack + allocsz; 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, allocsz);
 }
-- 
1.9.1

[Qemu-devel] [PATCH V8 6/6] 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 V8 1/6] oslib-posix: add helpers for stack alloc and free

[Kevin Wolf]

Am 26.09.2016 um 13:44 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        | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> index 9c7dfdf..4a0f493 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 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 useable stack memory can be greater than the
> + * requested stack size due to alignment and minimal stack size
> + * restrictions. In this case the value of sz is adjusted.
> + *
> + * The allocated stack must be freed with qemu_free_stack().
> + *
> + * Returns: pointer to (the lowest address of) the stack memory.

Not quite. It's the pointer to the lowest address of the guard page,
while the returned stack size doesn't include the guard page. This is an
awkward interface, and consequently patch 3 fails to use it correctly.

So you end up with something like:

    |GGGG|....|....|....|
     **** **** ****

    G = guard page
    . = allocated stack page
    * = stack as used for makecontext()

That is, the guard page is included in the stack used to create the
coroutine context, and the last page stays unused. On systems where we
only allocate a single page for the stack, this obviously means that the
tests still fail.

Kevin

Re: [Qemu-devel] [PATCH V8 4/6] coroutine-sigaltstack: use helper for allocating stack memory

[Kevin Wolf]

Am 26.09.2016 um 13:44 hat Peter Lieven geschrieben:
> 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 9c2854c..d9c7f66 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;
>  } CoroutineUContext;

Not related to your patch, but somehow I feel some renaming would be in
order... (compare the struct name and the source file name)

Kevin

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

[Peter Lieven]

Am 26.09.2016 um 15:44 schrieb Kevin Wolf:
> Am 26.09.2016 um 13:44 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        | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 70 insertions(+)
>>
>> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
>> index 9c7dfdf..4a0f493 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 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 useable stack memory can be greater than the
>> + * requested stack size due to alignment and minimal stack size
>> + * restrictions. In this case the value of sz is adjusted.
>> + *
>> + * The allocated stack must be freed with qemu_free_stack().
>> + *
>> + * Returns: pointer to (the lowest address of) the stack memory.
> Not quite. It's the pointer to the lowest address of the guard page,
> while the returned stack size doesn't include the guard page. This is an
> awkward interface, and consequently patch 3 fails to use it correctly.
>
> So you end up with something like:
>
>      |GGGG|....|....|....|
>       **** **** ****
>
>      G = guard page
>      . = allocated stack page
>      * = stack as used for makecontext()
>
> That is, the guard page is included in the stack used to create the
> coroutine context, and the last page stays unused. On systems where we
> only allocate a single page for the stack, this obviously means that the
> tests still fail.

you are right. so I should adjust the size to allocsz instead?

the other option would be to keep version 7 of this series and
adjust the COROUTINE_SIZE to MAX(2*pagesize(), 1 << 16) to
avoid the problem?

Peter

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

[Kevin Wolf]

Am 26.09.2016 um 16:43 hat Peter Lieven geschrieben:
> Am 26.09.2016 um 15:44 schrieb Kevin Wolf:
> >Am 26.09.2016 um 13:44 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        | 43 +++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 70 insertions(+)
> >>
> >>diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> >>index 9c7dfdf..4a0f493 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 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 useable stack memory can be greater than the
> >>+ * requested stack size due to alignment and minimal stack size
> >>+ * restrictions. In this case the value of sz is adjusted.
> >>+ *
> >>+ * The allocated stack must be freed with qemu_free_stack().
> >>+ *
> >>+ * Returns: pointer to (the lowest address of) the stack memory.
> >Not quite. It's the pointer to the lowest address of the guard page,
> >while the returned stack size doesn't include the guard page. This is an
> >awkward interface, and consequently patch 3 fails to use it correctly.
> >
> >So you end up with something like:
> >
> >     |GGGG|....|....|....|
> >      **** **** ****
> >
> >     G = guard page
> >     . = allocated stack page
> >     * = stack as used for makecontext()
> >
> >That is, the guard page is included in the stack used to create the
> >coroutine context, and the last page stays unused. On systems where we
> >only allocate a single page for the stack, this obviously means that the
> >tests still fail.
> 
> you are right. so I should adjust the size to allocsz instead?

That's probably the easiest fix.

Kevin

> the other option would be to keep version 7 of this series and
> adjust the COROUTINE_SIZE to MAX(2*pagesize(), 1 << 16) to
> avoid the problem?
> 
> Peter