* [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-06-30 7:37 [Qemu-devel] [PATCH 0/6] coroutine: mmap stack memory and stack size Peter Lieven
@ 2016-06-30 7:37 ` Peter Lieven
2016-07-01 20:12 ` Richard Henderson
2016-06-30 7:37 ` [Qemu-devel] [PATCH 2/6] coroutine: add a macro for the coroutine stack size Peter Lieven
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-06-30 7:37 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, mreitz, pbonzini, mst, dgilbert, peter.maydell, eblake,
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-06-30 7:37 ` [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free Peter Lieven
@ 2016-07-01 20:12 ` Richard Henderson
2016-07-01 20:49 ` Richard Henderson
2016-07-04 6:16 ` Peter Lieven
0 siblings, 2 replies; 23+ messages in thread
From: Richard Henderson @ 2016-07-01 20:12 UTC (permalink / raw)
To: Peter Lieven, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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~
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-01 20:12 ` Richard Henderson
@ 2016-07-01 20:49 ` Richard Henderson
2016-07-04 6:18 ` Peter Lieven
2016-07-04 6:16 ` Peter Lieven
1 sibling, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2016-07-01 20:49 UTC (permalink / raw)
To: Peter Lieven, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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~
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-01 20:49 ` Richard Henderson
@ 2016-07-04 6:18 ` Peter Lieven
2016-07-04 10:19 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-07-04 6:18 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-04 6:18 ` Peter Lieven
@ 2016-07-04 10:19 ` Paolo Bonzini
2016-07-04 10:25 ` Peter Lieven
0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2016-07-04 10:19 UTC (permalink / raw)
To: Peter Lieven, Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-04 10:19 ` Paolo Bonzini
@ 2016-07-04 10:25 ` Peter Lieven
2016-07-04 10:34 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-07-04 10:25 UTC (permalink / raw)
To: Paolo Bonzini, Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-04 10:25 ` Peter Lieven
@ 2016-07-04 10:34 ` Paolo Bonzini
2016-07-04 10:35 ` Peter Lieven
0 siblings, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2016-07-04 10:34 UTC (permalink / raw)
To: Peter Lieven, Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-04 10:34 ` Paolo Bonzini
@ 2016-07-04 10:35 ` Peter Lieven
2016-07-04 10:48 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-07-04 10:35 UTC (permalink / raw)
To: Paolo Bonzini, Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-04 10:35 ` Peter Lieven
@ 2016-07-04 10:48 ` Paolo Bonzini
0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2016-07-04 10:48 UTC (permalink / raw)
To: Peter Lieven, Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free
2016-07-01 20:12 ` Richard Henderson
2016-07-01 20:49 ` Richard Henderson
@ 2016-07-04 6:16 ` Peter Lieven
1 sibling, 0 replies; 23+ messages in thread
From: Peter Lieven @ 2016-07-04 6:16 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 2/6] coroutine: add a macro for the coroutine stack size
2016-06-30 7:37 [Qemu-devel] [PATCH 0/6] coroutine: mmap stack memory and stack size Peter Lieven
2016-06-30 7:37 ` [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free Peter Lieven
@ 2016-06-30 7:37 ` Peter Lieven
2016-07-01 20:50 ` Richard Henderson
2016-06-30 7:37 ` [Qemu-devel] [PATCH 3/6] coroutine-ucontext: use helper for allocating stack memory Peter Lieven
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-06-30 7:37 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, mreitz, pbonzini, mst, dgilbert, peter.maydell, eblake,
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] coroutine: add a macro for the coroutine stack size
2016-06-30 7:37 ` [Qemu-devel] [PATCH 2/6] coroutine: add a macro for the coroutine stack size Peter Lieven
@ 2016-07-01 20:50 ` Richard Henderson
0 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2016-07-01 20:50 UTC (permalink / raw)
To: Peter Lieven, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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~
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 3/6] coroutine-ucontext: use helper for allocating stack memory
2016-06-30 7:37 [Qemu-devel] [PATCH 0/6] coroutine: mmap stack memory and stack size Peter Lieven
2016-06-30 7:37 ` [Qemu-devel] [PATCH 1/6] oslib-posix: add helpers for stack alloc and free Peter Lieven
2016-06-30 7:37 ` [Qemu-devel] [PATCH 2/6] coroutine: add a macro for the coroutine stack size Peter Lieven
@ 2016-06-30 7:37 ` Peter Lieven
2016-07-01 20:52 ` Richard Henderson
2016-06-30 7:37 ` [Qemu-devel] [PATCH 4/6] coroutine-sigaltstack: " Peter Lieven
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-06-30 7:37 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, mreitz, pbonzini, mst, dgilbert, peter.maydell, eblake,
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] coroutine-ucontext: use helper for allocating stack memory
2016-06-30 7:37 ` [Qemu-devel] [PATCH 3/6] coroutine-ucontext: use helper for allocating stack memory Peter Lieven
@ 2016-07-01 20:52 ` Richard Henderson
0 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2016-07-01 20:52 UTC (permalink / raw)
To: Peter Lieven, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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~
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 4/6] coroutine-sigaltstack: use helper for allocating stack memory
2016-06-30 7:37 [Qemu-devel] [PATCH 0/6] coroutine: mmap stack memory and stack size Peter Lieven
` (2 preceding siblings ...)
2016-06-30 7:37 ` [Qemu-devel] [PATCH 3/6] coroutine-ucontext: use helper for allocating stack memory Peter Lieven
@ 2016-06-30 7:37 ` Peter Lieven
2016-07-01 20:53 ` Richard Henderson
2016-06-30 7:37 ` [Qemu-devel] [PATCH 5/6] oslib-posix: add a configure switch to debug stack usage Peter Lieven
2016-06-30 7:37 ` [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB Peter Lieven
5 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-06-30 7:37 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, mreitz, pbonzini, mst, dgilbert, peter.maydell, eblake,
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] coroutine-sigaltstack: use helper for allocating stack memory
2016-06-30 7:37 ` [Qemu-devel] [PATCH 4/6] coroutine-sigaltstack: " Peter Lieven
@ 2016-07-01 20:53 ` Richard Henderson
0 siblings, 0 replies; 23+ messages in thread
From: Richard Henderson @ 2016-07-01 20:53 UTC (permalink / raw)
To: Peter Lieven, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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~
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 5/6] oslib-posix: add a configure switch to debug stack usage
2016-06-30 7:37 [Qemu-devel] [PATCH 0/6] coroutine: mmap stack memory and stack size Peter Lieven
` (3 preceding siblings ...)
2016-06-30 7:37 ` [Qemu-devel] [PATCH 4/6] coroutine-sigaltstack: " Peter Lieven
@ 2016-06-30 7:37 ` Peter Lieven
2016-07-01 20:55 ` Richard Henderson
2016-06-30 7:37 ` [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB Peter Lieven
5 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-06-30 7:37 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, mreitz, pbonzini, mst, dgilbert, peter.maydell, eblake,
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB
2016-06-30 7:37 [Qemu-devel] [PATCH 0/6] coroutine: mmap stack memory and stack size Peter Lieven
` (4 preceding siblings ...)
2016-06-30 7:37 ` [Qemu-devel] [PATCH 5/6] oslib-posix: add a configure switch to debug stack usage Peter Lieven
@ 2016-06-30 7:37 ` Peter Lieven
2016-07-01 21:13 ` Richard Henderson
5 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-06-30 7:37 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, mreitz, pbonzini, mst, dgilbert, peter.maydell, eblake,
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB
2016-06-30 7:37 ` [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB Peter Lieven
@ 2016-07-01 21:13 ` Richard Henderson
2016-07-04 6:22 ` Peter Lieven
0 siblings, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2016-07-01 21:13 UTC (permalink / raw)
To: Peter Lieven, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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~
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB
2016-07-01 21:13 ` Richard Henderson
@ 2016-07-04 6:22 ` Peter Lieven
2016-07-04 10:20 ` Paolo Bonzini
0 siblings, 1 reply; 23+ messages in thread
From: Peter Lieven @ 2016-07-04 6:22 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz, pbonzini
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] coroutine: reduce stack size to 64kB
2016-07-04 6:22 ` Peter Lieven
@ 2016-07-04 10:20 ` Paolo Bonzini
0 siblings, 0 replies; 23+ messages in thread
From: Paolo Bonzini @ 2016-07-04 10:20 UTC (permalink / raw)
To: Peter Lieven, Richard Henderson, qemu-devel
Cc: kwolf, peter.maydell, mst, dgilbert, mreitz
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
^ permalink raw reply [flat|nested] 23+ messages in thread