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

[Qemu-devel] [PATCH 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.

The last patch which reduces the stack size of coroutines to 64kB
may be omitted if its found to risky.

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 64kB

 configure                    | 15 ++++++++++++
 include/qemu/coroutine_int.h |  2 ++
 include/sysemu/os-posix.h    | 24 +++++++++++++++++++
 util/coroutine-sigaltstack.c |  7 +++---
 util/coroutine-ucontext.c    |  9 ++++----
 util/coroutine-win32.c       |  2 +-
 util/oslib-posix.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 104 insertions(+), 10 deletions(-)

-- 
1.9.1

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

[Peter Lieven]

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 include/sysemu/os-posix.h | 24 ++++++++++++++++++++++++
 util/oslib-posix.c        | 22 ++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 9c7dfdf..34d102c 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -60,4 +60,28 @@ int qemu_utimens(const char *path, const qemu_timespec *times);
 
 bool is_daemonized(void);
 
+/**
+ * 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 function will register a
+ * guard page right below the stack memory to catch stack overflows.
+ *
+ * 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 e2e1d4d..a567a7d 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -497,3 +497,25 @@ pid_t qemu_fork(Error **errp)
     }
     return pid;
 }
+
+void *qemu_alloc_stack(size_t sz)
+{
+    /* allocate sz bytes plus one extra page for a guard
+     * page at the bottom of the stack */
+    void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
+                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (ptr == MAP_FAILED) {
+        abort();
+    }
+    if (mmap(ptr + getpagesize(), sz, PROT_READ | PROT_WRITE,
+        MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
+        abort();
+    }
+    return ptr + getpagesize();
+}
+
+void qemu_free_stack(void *stack, size_t sz)
+{
+    /* unmap the stack plus the extra guard page */
+    munmap(stack - getpagesize(), sz + getpagesize());
+}
-- 
1.9.1

[Qemu-devel] [PATCH 2/6] coroutine: add a macro for the coroutine stack size

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 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 42d6838..eac323a 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 3/6] coroutine-ucontext: use helper for allocating stack memory

[Peter Lieven]

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

diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 31254ab..b7dea8c 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -82,7 +82,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 +100,17 @@ Coroutine *qemu_coroutine_new(void)
     }
 
     co = g_malloc0(sizeof(*co));
-    co->stack = g_malloc(stack_size);
+    co->stack = qemu_alloc_stack(COROUTINE_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 = COROUTINE_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 + COROUTINE_STACK_SIZE);
 #endif
 
     arg.p = co;
@@ -149,7 +148,7 @@ void qemu_coroutine_delete(Coroutine *co_)
     valgrind_stack_deregister(co);
 #endif
 
-    g_free(co->stack);
+    qemu_free_stack(co->stack, COROUTINE_STACK_SIZE);
     g_free(co);
 }
 
-- 
1.9.1

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

[Peter Lieven]

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

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index 9c2854c..ccf4861 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -143,7 +143,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 +163,7 @@ Coroutine *qemu_coroutine_new(void)
      */
 
     co = g_malloc0(sizeof(*co));
-    co->stack = g_malloc(stack_size);
+    co->stack = qemu_alloc_stack(COROUTINE_STACK_SIZE);
     co->base.entry_arg = &old_env; /* stash away our jmp_buf */
 
     coTS = coroutine_get_thread_state();
@@ -189,7 +188,7 @@ Coroutine *qemu_coroutine_new(void)
      * Set the new stack.
      */
     ss.ss_sp = co->stack;
-    ss.ss_size = stack_size;
+    ss.ss_size = COROUTINE_STACK_SIZE;
     ss.ss_flags = 0;
     if (sigaltstack(&ss, &oss) < 0) {
         abort();
@@ -253,7 +252,7 @@ void qemu_coroutine_delete(Coroutine *co_)
 {
     CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
 
-    g_free(co->stack);
+    qemu_free_stack(co->stack, COROUTINE_STACK_SIZE);
     g_free(co);
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 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          | 15 +++++++++++++++
 util/oslib-posix.c | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 5929aba..ba66916 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"
@@ -4298,6 +4301,13 @@ fi
 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 "$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
@@ -4866,6 +4876,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"
@@ -5336,6 +5347,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 a567a7d..0a9f2b2 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__)
@@ -500,6 +504,9 @@ pid_t qemu_fork(Error **errp)
 
 void *qemu_alloc_stack(size_t sz)
 {
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    void *ptr2;
+#endif
     /* allocate sz bytes plus one extra page for a guard
      * page at the bottom of the stack */
     void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
@@ -511,11 +518,37 @@ void *qemu_alloc_stack(size_t sz)
         MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
         abort();
     }
-    return ptr + getpagesize();
+    ptr += getpagesize();
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    for (ptr2 = ptr; ptr2 < ptr + sz; ptr2 += sizeof(u_int32_t)) {
+        *(u_int32_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
+    void *ptr;
+    unsigned int usage;
+    for (ptr = stack; ptr < stack + sz; ptr += sizeof(u_int32_t)) {
+        if (*(u_int32_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
+
     /* unmap the stack plus the extra guard page */
     munmap(stack - getpagesize(), sz + getpagesize());
 }
-- 
1.9.1

[Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB

[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
64kB which should still give enough head room.

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 eac323a..f84d777 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 (1 << 16)
 
 typedef enum {
     COROUTINE_YIELD = 1,
-- 
1.9.1

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

[Richard Henderson]

On 06/30/2016 12:37 AM, Peter Lieven wrote:
> +void *qemu_alloc_stack(size_t sz)
> +{
> +    /* allocate sz bytes plus one extra page for a guard
> +     * page at the bottom of the stack */
> +    void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
> +                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +    if (ptr == MAP_FAILED) {
> +        abort();
> +    }
> +    if (mmap(ptr + getpagesize(), sz, PROT_READ | PROT_WRITE,
> +        MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
> +        abort();
> +    }

Why two mmap instead of mmap + mprotect?


r~

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

[Richard Henderson]

On 07/01/2016 01:12 PM, Richard Henderson wrote:
> On 06/30/2016 12:37 AM, Peter Lieven wrote:
>> +void *qemu_alloc_stack(size_t sz)
>> +{
>> +    /* allocate sz bytes plus one extra page for a guard
>> +     * page at the bottom of the stack */
>> +    void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
>> +                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +    if (ptr == MAP_FAILED) {
>> +        abort();
>> +    }
>> +    if (mmap(ptr + getpagesize(), sz, PROT_READ | PROT_WRITE,
>> +        MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
>> +        abort();
>> +    }

Rare platforms now, but fwiw, this is incorrect for hppa and ia64.

For hppa, stack grows up, so the guard page needs to be at the top.

For ia64, there are two stacks, the "normal" program stack (grows down) and the 
register window stack (grows up).  The guard page goes in between.

See e.g. glibc/nptl/allocatestack.c

#ifdef NEED_SEPARATE_REGISTER_STACK
           char *guard = mem + (((size - guardsize) / 2) & ~pagesize_m1);
#elif _STACK_GROWS_DOWN
           char *guard = mem;
#elif _STACK_GROWS_UP
           char *guard = (char *) (((uintptr_t) pd - guardsize) & ~pagesize_m1);
#endif
           if (mprotect (guard, guardsize, PROT_NONE) != 0)



r~

Re: [Qemu-devel] [PATCH 2/6] coroutine: add a macro for the coroutine stack size

[Richard Henderson]

On 06/30/2016 12:37 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  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(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH 3/6] coroutine-ucontext: use helper for allocating stack memory

[Richard Henderson]

On 06/30/2016 12:37 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  util/coroutine-ucontext.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

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

[Richard Henderson]

On 06/30/2016 12:37 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  util/coroutine-sigaltstack.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

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

[Richard Henderson]

On 06/30/2016 12:37 AM, Peter Lieven wrote:
> +    for (ptr2 = ptr; ptr2 < ptr + sz; ptr2 += sizeof(u_int32_t)) {
> +        *(u_int32_t *)ptr2 = 0xdeadbeaf;
> +    }

s/u_int32_t/uint32_t/g


r~

Re: [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB

[Richard Henderson]

On 06/30/2016 12:37 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
> 64kB which should still give enough head room.
>
> 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 eac323a..f84d777 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 (1 << 16)
>
>  typedef enum {
>      COROUTINE_YIELD = 1,
>

Ought we check that this is not smaller than

     sysconf(_SC_THREAD_STACK_MIN)

which (for glibc at least), is 192k for ia64, 128k for aarch64, mips and tile 
(though why it is quite so high in those later cases I don't know).


r~

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

[Peter Lieven]

Am 01.07.2016 um 22:12 schrieb Richard Henderson:
> On 06/30/2016 12:37 AM, Peter Lieven wrote:
>> +void *qemu_alloc_stack(size_t sz)
>> +{
>> +    /* allocate sz bytes plus one extra page for a guard
>> +     * page at the bottom of the stack */
>> +    void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
>> +                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +    if (ptr == MAP_FAILED) {
>> +        abort();
>> +    }
>> +    if (mmap(ptr + getpagesize(), sz, PROT_READ | PROT_WRITE,
>> +        MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
>> +        abort();
>> +    }
>
> Why two mmap instead of mmap + mprotect?
>
>
> r~

I was looking at qemu_ram_mmap too much. mprotect seems the cleaner solution.

Thanks,
Peter

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

[Peter Lieven]

Am 01.07.2016 um 22:49 schrieb Richard Henderson:
> On 07/01/2016 01:12 PM, Richard Henderson wrote:
>> On 06/30/2016 12:37 AM, Peter Lieven wrote:
>>> +void *qemu_alloc_stack(size_t sz)
>>> +{
>>> +    /* allocate sz bytes plus one extra page for a guard
>>> +     * page at the bottom of the stack */
>>> +    void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
>>> +                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>> +    if (ptr == MAP_FAILED) {
>>> +        abort();
>>> +    }
>>> +    if (mmap(ptr + getpagesize(), sz, PROT_READ | PROT_WRITE,
>>> +        MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) == MAP_FAILED) {
>>> +        abort();
>>> +    }
>
> Rare platforms now, but fwiw, this is incorrect for hppa and ia64.
>
> For hppa, stack grows up, so the guard page needs to be at the top.
>
> For ia64, there are two stacks, the "normal" program stack (grows down) and the register window stack (grows up).  The guard page goes in between.
>
> See e.g. glibc/nptl/allocatestack.c
>
> #ifdef NEED_SEPARATE_REGISTER_STACK
>           char *guard = mem + (((size - guardsize) / 2) & ~pagesize_m1);
> #elif _STACK_GROWS_DOWN
>           char *guard = mem;
> #elif _STACK_GROWS_UP
>           char *guard = (char *) (((uintptr_t) pd - guardsize) & ~pagesize_m1);
> #endif
>           if (mprotect (guard, guardsize, PROT_NONE) != 0)

It seems that ia64 needs even more care when allocating a stack, right?
Would you think it is ok to only handle _STACK_GROWS_DOWN and _STACK_GROWS_UP ?

Peter

Re: [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB

[Peter Lieven]

Am 01.07.2016 um 23:13 schrieb Richard Henderson:
> On 06/30/2016 12:37 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
>> 64kB which should still give enough head room.
>>
>> 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 eac323a..f84d777 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 (1 << 16)
>>
>>  typedef enum {
>>      COROUTINE_YIELD = 1,
>>
>
> Ought we check that this is not smaller than
>
>     sysconf(_SC_THREAD_STACK_MIN)
>
> which (for glibc at least), is 192k for ia64, 128k for aarch64, mips and tile (though why it is quite so high in those later cases I don't know).

for x86_64 it seems to be 16k. I would not mind to adjust the stack size either in qemu_alloc_stack or change the macro for the coroutine stack
size into a function returning MAX(1 << 16, sysconf(_SC_THREAD_STACK_MIN)).

Peter

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

[Paolo Bonzini]

On 04/07/2016 08:18, Peter Lieven wrote:
> Am 01.07.2016 um 22:49 schrieb Richard Henderson:
>> On 07/01/2016 01:12 PM, Richard Henderson wrote:
>>> On 06/30/2016 12:37 AM, Peter Lieven wrote:
>>>> +void *qemu_alloc_stack(size_t sz)
>>>> +{
>>>> +    /* allocate sz bytes plus one extra page for a guard
>>>> +     * page at the bottom of the stack */
>>>> +    void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
>>>> +                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>>> +    if (ptr == MAP_FAILED) {
>>>> +        abort();
>>>> +    }
>>>> +    if (mmap(ptr + getpagesize(), sz, PROT_READ | PROT_WRITE,
>>>> +        MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) ==
>>>> MAP_FAILED) {
>>>> +        abort();
>>>> +    }
>>
>> Rare platforms now, but fwiw, this is incorrect for hppa and ia64.
>>
>> For hppa, stack grows up, so the guard page needs to be at the top.
>>
>> For ia64, there are two stacks, the "normal" program stack (grows
>> down) and the register window stack (grows up).  The guard page goes
>> in between.
>>
>> See e.g. glibc/nptl/allocatestack.c
>>
>> #ifdef NEED_SEPARATE_REGISTER_STACK
>>           char *guard = mem + (((size - guardsize) / 2) & ~pagesize_m1);
>> #elif _STACK_GROWS_DOWN
>>           char *guard = mem;
>> #elif _STACK_GROWS_UP
>>           char *guard = (char *) (((uintptr_t) pd - guardsize) &
>> ~pagesize_m1);
>> #endif
>>           if (mprotect (guard, guardsize, PROT_NONE) != 0)
> 
> It seems that ia64 needs even more care when allocating a stack, right?

No, you just pass the stack and the runtime takes care of initializing
the two stack pointers:

    uc.uc_link = &old_uc;
    uc.uc_stack.ss_sp = co->stack;
    uc.uc_stack.ss_size = stack_size;
    uc.uc_stack.ss_flags = 0;

FWIW, I've heard about some experiments with "split" stacks on x86 too.
In this case variables that escaped went on the "grows up" part, while
everything else (parameters, spills and call return addresses) stayed on
the hardware "grows down" part.  The result is more secure and actually
even faster because of better TLB locality.

Paolo

> Would you think it is ok to only handle _STACK_GROWS_DOWN and
> _STACK_GROWS_UP ?
> 
> Peter

Re: [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB

[Paolo Bonzini]

On 04/07/2016 08:22, Peter Lieven wrote:
>>>
>>
>> Ought we check that this is not smaller than
>>
>>     sysconf(_SC_THREAD_STACK_MIN)
>>
>> which (for glibc at least), is 192k for ia64, 128k for aarch64, mips
>> and tile (though why it is quite so high in those later cases I don't
>> know).
> 
> for x86_64 it seems to be 16k. I would not mind to adjust the stack size
> either in qemu_alloc_stack or change the macro for the coroutine stack
> size into a function returning MAX(1 << 16, sysconf(_SC_THREAD_STACK_MIN)).

Yes, either of this is a good idea.

Paolo

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

[Peter Lieven]

Am 04.07.2016 um 12:19 schrieb Paolo Bonzini:
>
> On 04/07/2016 08:18, Peter Lieven wrote:
>> Am 01.07.2016 um 22:49 schrieb Richard Henderson:
>>> On 07/01/2016 01:12 PM, Richard Henderson wrote:
>>>> On 06/30/2016 12:37 AM, Peter Lieven wrote:
>>>>> +void *qemu_alloc_stack(size_t sz)
>>>>> +{
>>>>> +    /* allocate sz bytes plus one extra page for a guard
>>>>> +     * page at the bottom of the stack */
>>>>> +    void *ptr = mmap(NULL, sz + getpagesize(), PROT_NONE,
>>>>> +                     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>>>> +    if (ptr == MAP_FAILED) {
>>>>> +        abort();
>>>>> +    }
>>>>> +    if (mmap(ptr + getpagesize(), sz, PROT_READ | PROT_WRITE,
>>>>> +        MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0) ==
>>>>> MAP_FAILED) {
>>>>> +        abort();
>>>>> +    }
>>> Rare platforms now, but fwiw, this is incorrect for hppa and ia64.
>>>
>>> For hppa, stack grows up, so the guard page needs to be at the top.
>>>
>>> For ia64, there are two stacks, the "normal" program stack (grows
>>> down) and the register window stack (grows up).  The guard page goes
>>> in between.
>>>
>>> See e.g. glibc/nptl/allocatestack.c
>>>
>>> #ifdef NEED_SEPARATE_REGISTER_STACK
>>>            char *guard = mem + (((size - guardsize) / 2) & ~pagesize_m1);
>>> #elif _STACK_GROWS_DOWN
>>>            char *guard = mem;
>>> #elif _STACK_GROWS_UP
>>>            char *guard = (char *) (((uintptr_t) pd - guardsize) &
>>> ~pagesize_m1);
>>> #endif
>>>            if (mprotect (guard, guardsize, PROT_NONE) != 0)
>> It seems that ia64 needs even more care when allocating a stack, right?
> No, you just pass the stack and the runtime takes care of initializing
> the two stack pointers:
>
>      uc.uc_link = &old_uc;
>      uc.uc_stack.ss_sp = co->stack;
>      uc.uc_stack.ss_size = stack_size;
>      uc.uc_stack.ss_flags = 0;
>
> FWIW, I've heard about some experiments with "split" stacks on x86 too.
> In this case variables that escaped went on the "grows up" part, while
> everything else (parameters, spills and call return addresses) stayed on
> the hardware "grows down" part.  The result is more secure and actually
> even faster because of better TLB locality.

So, you would basically copy the if/elif part from allocatestack.c ?

In this case I would also count the guardpage as part of the stack, so
that the usable stack size is actually reduced by one page?

Peter

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

[Paolo Bonzini]

On 04/07/2016 12:25, Peter Lieven wrote:
>>>
>> No, you just pass the stack and the runtime takes care of initializing
>> the two stack pointers:
>>
>>      uc.uc_link = &old_uc;
>>      uc.uc_stack.ss_sp = co->stack;
>>      uc.uc_stack.ss_size = stack_size;
>>      uc.uc_stack.ss_flags = 0;
> 
> So, you would basically copy the if/elif part from allocatestack.c ?

Yes, but note that _STACK_GROWS_{DOWN,UP} and
NEED_SEPARATE_REGISTER_STACK are glibc-specific.  You need to use
HOST_IA64 and HOST_HPPA.

> In this case I would also count the guardpage as part of the stack, so
> that the usable stack size is actually reduced by one page?

Yes.

Paolo

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

[Peter Lieven]

Am 04.07.2016 um 12:34 schrieb Paolo Bonzini:
>
> On 04/07/2016 12:25, Peter Lieven wrote:
>>> No, you just pass the stack and the runtime takes care of initializing
>>> the two stack pointers:
>>>
>>>       uc.uc_link = &old_uc;
>>>       uc.uc_stack.ss_sp = co->stack;
>>>       uc.uc_stack.ss_size = stack_size;
>>>       uc.uc_stack.ss_flags = 0;
>> So, you would basically copy the if/elif part from allocatestack.c ?
> Yes, but note that _STACK_GROWS_{DOWN,UP} and
> NEED_SEPARATE_REGISTER_STACK are glibc-specific.  You need to use
> HOST_IA64 and HOST_HPPA.

Is HOST_HPPA the only supported target where the stack grows up?

Peter

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

[Paolo Bonzini]

On 04/07/2016 12:35, Peter Lieven wrote:
> Am 04.07.2016 um 12:34 schrieb Paolo Bonzini:
>>
>> On 04/07/2016 12:25, Peter Lieven wrote:
>>>> No, you just pass the stack and the runtime takes care of initializing
>>>> the two stack pointers:
>>>>
>>>>       uc.uc_link = &old_uc;
>>>>       uc.uc_stack.ss_sp = co->stack;
>>>>       uc.uc_stack.ss_size = stack_size;
>>>>       uc.uc_stack.ss_flags = 0;
>>> So, you would basically copy the if/elif part from allocatestack.c ?
>> Yes, but note that _STACK_GROWS_{DOWN,UP} and
>> NEED_SEPARATE_REGISTER_STACK are glibc-specific.  You need to use
>> HOST_IA64 and HOST_HPPA.
> 
> Is HOST_HPPA the only supported target where the stack grows up?

Yes, I think so.

Paolo