[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Roger Pau Monne]

On FreeBSD PTHREAD_STACK_MIN is 2048 by default, which is obviously
too low. Set the default back to the previous value (16 * 1024), or if
that's too low set it to PTHREAD_STACK_MIN.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@citrix.com>
---
 tools/xenstore/xs.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index dd03a85..968141d 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -724,7 +724,10 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
 	struct iovec iov[2];
 
 #ifdef USE_PTHREAD
-#define READ_THREAD_STACKSIZE PTHREAD_STACK_MIN
+#define DEFAULT_THREAD_STACKSIZE (16 * 1024)
+#define READ_THREAD_STACKSIZE 					\
+	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
+	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
 
 	/* We dynamically create a reader thread on demand. */
 	mutex_lock(&h->request_mutex);
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Jackson]

Roger Pau Monne writes ("[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> On FreeBSD PTHREAD_STACK_MIN is 2048 by default, which is obviously
> too low. Set the default back to the previous value (16 * 1024), or if
> that's too low set it to PTHREAD_STACK_MIN.

The "previous value" is 16K, which was used before
35e874b1d5d56dd2098313364b879c637fa56844.  In that commit, Ian C
wrote:

    Consindered setting a lower bound but the stack requirements of
    the watcher thread are pretty minimal (tens of bytes from the
    looks of it) and unlikely to blow PTHREAD_STACK_MIN on any useful
    platform.

I've just re-reviewed the read_thread code and I concur with Ian
Campbell's assessment.

So I don't understand why PTHREAD_STACK_MIN isn't sufficient.  Is
(nearly) that whole amount used by the C runtime system somehow ?

Thanks,
Ian.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Mon, 2014-03-10 at 17:12 +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> > On FreeBSD PTHREAD_STACK_MIN is 2048 by default, which is obviously
> > too low.

How does this manifest itself? (I suppose this may be answered as part
of answering Ian J)

>  Set the default back to the previous value (16 * 1024), or if
> > that's too low set it to PTHREAD_STACK_MIN.
> 
> The "previous value" is 16K, which was used before
> 35e874b1d5d56dd2098313364b879c637fa56844.  In that commit, Ian C
> wrote:
> 
>     Consindered setting a lower bound but the stack requirements of
>     the watcher thread are pretty minimal (tens of bytes from the
>     looks of it) and unlikely to blow PTHREAD_STACK_MIN on any useful
>     platform.
> 
> I've just re-reviewed the read_thread code and I concur with Ian
> Campbell's assessment.
> 
> So I don't understand why PTHREAD_STACK_MIN isn't sufficient.  Is
> (nearly) that whole amount used by the C runtime system somehow ?
> 
> Thanks,
> Ian.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Roger Pau Monné]

On 11/03/14 14:24, Ian Campbell wrote:
> On Mon, 2014-03-10 at 17:12 +0000, Ian Jackson wrote:
>> Roger Pau Monne writes ("[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
>>> On FreeBSD PTHREAD_STACK_MIN is 2048 by default, which is obviously
>>> too low.
> 
> How does this manifest itself? (I suppose this may be answered as part
> of answering Ian J)

Yes, I'm still looking into it, this gdb output:

Starting program: /usr/local/bin/xenstore-watch /foo
[New LWP 100169]
[New Thread 801406800 (LWP 100182/xenstore-watch)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 801406800 (LWP 100182/xenstore-watch)]
0x0000000800ac1258 in sbrk () from /lib/libc.so.7
(gdb) bt
#0  0x0000000800ac1258 in sbrk () from /lib/libc.so.7
#1  0x0000000800ac110e in sbrk () from /lib/libc.so.7
#2  0x0000000800ac9ee8 in sbrk () from /lib/libc.so.7
#3  0x0000000800ac456b in sbrk () from /lib/libc.so.7
#4  0x0000000800ac447d in sbrk () from /lib/libc.so.7
#5  0x0000000800aaf6ce in syscall () from /lib/libc.so.7
#6  0x0000000800acb37b in malloc () from /lib/libc.so.7
#7  0x00000008008202b9 in read_message (h=0x801417080, nonblocking=0) at xs.c:313
#8  0x0000000800820a06 in read_thread (arg=0x801417080) at xs.c:313
#9  0x0000000800dc64a4 in pthread_create () from /lib/libthr.so.3
#10 0x0000000000000000 in ?? ()

I've also tried to debug it using valgrind, and here's what I got:

[root@loki ~/xen/xen]# valgrind xenstore-watch /foo
==1901== Memcheck, a memory error detector
==1901== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==1901== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==1901== Command: xenstore-watch /foo
==1901==
==1901== Syscall param socketcall.connect(serv_addr..sa_len) points to uninitialised byte(s)
==1901==    at 0x152A14A: connect (in /lib/libc.so.7)
==1901==    by 0x1210B46: get_handle (xs.c:205)
==1901==    by 0x1210CEC: xs_open (xs.c:297)
==1901==    by 0x4027B1: main (xenstore_client.c:635)
==1901==  Address 0x7ff000a70 is on thread 1's stack
==1901==
/foo

Strangely enough, when running under valgrind it doesn't segfault, and 
I'm still trying to figure out why valgrind complains.

Roger.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Tue, 2014-03-11 at 14:52 +0100, Roger Pau Monné wrote:
> On 11/03/14 14:24, Ian Campbell wrote:
> > On Mon, 2014-03-10 at 17:12 +0000, Ian Jackson wrote:
> >> Roger Pau Monne writes ("[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> >>> On FreeBSD PTHREAD_STACK_MIN is 2048 by default, which is obviously
> >>> too low.

It occurs to me that 2048 is < PAGE_SIZE. Which makes this seem like an
interesting choice of stack min, especially combined with the fact that
the failure seems to involve malloc.

Perhaps the stack is malloc'd (rather than coming from brk or an anon
mmap), so overrunning would cause heap corruption which seems to be what
you are seeing.

> > How does this manifest itself? (I suppose this may be answered as part
> > of answering Ian J)
> 
> Yes, I'm still looking into it, this gdb output:
> 
> Starting program: /usr/local/bin/xenstore-watch /foo
> [New LWP 100169]
> [New Thread 801406800 (LWP 100182/xenstore-watch)]
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 801406800 (LWP 100182/xenstore-watch)]
> 0x0000000800ac1258 in sbrk () from /lib/libc.so.7
> (gdb) bt
> #0  0x0000000800ac1258 in sbrk () from /lib/libc.so.7
> #1  0x0000000800ac110e in sbrk () from /lib/libc.so.7
> #2  0x0000000800ac9ee8 in sbrk () from /lib/libc.so.7
> #3  0x0000000800ac456b in sbrk () from /lib/libc.so.7
> #4  0x0000000800ac447d in sbrk () from /lib/libc.so.7
> #5  0x0000000800aaf6ce in syscall () from /lib/libc.so.7
> #6  0x0000000800acb37b in malloc () from /lib/libc.so.7
> #7  0x00000008008202b9 in read_message (h=0x801417080, nonblocking=0) at xs.c:313
> #8  0x0000000800820a06 in read_thread (arg=0x801417080) at xs.c:313
> #9  0x0000000800dc64a4 in pthread_create () from /lib/libthr.so.3
> #10 0x0000000000000000 in ?? ()

Does 
frame 1 ; print $sp
frame 2 ; print $sp
etc
tell you anything useful about the stack usage at each level?

> I've also tried to debug it using valgrind,

Under BSD? Did someone wire up the dom0 OS specific bit? If so: Neat!

>  and here's what I got:
> 
> [root@loki ~/xen/xen]# valgrind xenstore-watch /foo
> ==1901== Memcheck, a memory error detector
> ==1901== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
> ==1901== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
> ==1901== Command: xenstore-watch /foo
> ==1901==
> ==1901== Syscall param socketcall.connect(serv_addr..sa_len) points to uninitialised byte(s)
> ==1901==    at 0x152A14A: connect (in /lib/libc.so.7)
> ==1901==    by 0x1210B46: get_handle (xs.c:205)
> ==1901==    by 0x1210CEC: xs_open (xs.c:297)
> ==1901==    by 0x4027B1: main (xenstore_client.c:635)
> ==1901==  Address 0x7ff000a70 is on thread 1's stack
> ==1901==
> /foo
> 
> Strangely enough, when running under valgrind it doesn't segfault,

valgrind interposes it's own malloc and stuff which will change
behaviour, and I wouldn't be all that surprised if it were gettings its
fingers into some of the pthread stuff too.

>  and 
> I'm still trying to figure out why valgrind complains.

It seems to be an unrelated issue though?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Roger Pau Monné]

On 11/03/14 15:12, Ian Campbell wrote:
> On Tue, 2014-03-11 at 14:52 +0100, Roger Pau Monné wrote:
>> On 11/03/14 14:24, Ian Campbell wrote:
>>> On Mon, 2014-03-10 at 17:12 +0000, Ian Jackson wrote:
>>>> Roger Pau Monne writes ("[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
>>>>> On FreeBSD PTHREAD_STACK_MIN is 2048 by default, which is obviously
>>>>> too low.
> 
> It occurs to me that 2048 is < PAGE_SIZE. Which makes this seem like an
> interesting choice of stack min, especially combined with the fact that
> the failure seems to involve malloc.
> 
> Perhaps the stack is malloc'd (rather than coming from brk or an anon
> mmap), so overrunning would cause heap corruption which seems to be what
> you are seeing.
> 
>>> How does this manifest itself? (I suppose this may be answered as part
>>> of answering Ian J)
>>
>> Yes, I'm still looking into it, this gdb output:
>>
>> Starting program: /usr/local/bin/xenstore-watch /foo
>> [New LWP 100169]
>> [New Thread 801406800 (LWP 100182/xenstore-watch)]
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> [Switching to Thread 801406800 (LWP 100182/xenstore-watch)]
>> 0x0000000800ac1258 in sbrk () from /lib/libc.so.7
>> (gdb) bt
>> #0  0x0000000800ac1258 in sbrk () from /lib/libc.so.7
>> #1  0x0000000800ac110e in sbrk () from /lib/libc.so.7
>> #2  0x0000000800ac9ee8 in sbrk () from /lib/libc.so.7
>> #3  0x0000000800ac456b in sbrk () from /lib/libc.so.7
>> #4  0x0000000800ac447d in sbrk () from /lib/libc.so.7
>> #5  0x0000000800aaf6ce in syscall () from /lib/libc.so.7
>> #6  0x0000000800acb37b in malloc () from /lib/libc.so.7
>> #7  0x00000008008202b9 in read_message (h=0x801417080, nonblocking=0) at xs.c:313
>> #8  0x0000000800820a06 in read_thread (arg=0x801417080) at xs.c:313
>> #9  0x0000000800dc64a4 in pthread_create () from /lib/libthr.so.3
>> #10 0x0000000000000000 in ?? ()
> 
> Does 
> frame 1 ; print $sp
> frame 2 ; print $sp
> etc
> tell you anything useful about the stack usage at each level?

Thanks, I've been able to get the stack pointer at each frame, here are
the results (from frame 0 to frame 10):

0x7fffffbfcff0
0x7fffffbfd0a0
0x7fffffbfd0e0
0x7fffffbfd120
0x7fffffbfd160
0x7fffffbfd1a0
0x7fffffbfd1e0
0x7fffffbfd6a0
0x7fffffbfd7a0
0x7fffffbfd7c0
0x7fffffbfd800

Doing:

0x7fffffbfd800 - 0x7fffffbfcff0 = 0x810

Which is 2064 in decimal. The biggest culprit seems to be malloc, which
is using 1216 bytes of the stack.

> 
>> I've also tried to debug it using valgrind,
> 
> Under BSD? Did someone wire up the dom0 OS specific bit? If so: Neat!

No, I don't think anyone has wired the Dom0 specific bits, maybe they
don't show up because this is just the xenstore client, which is not
using any ioctls?

> 
>>  and here's what I got:
>>
>> [root@loki ~/xen/xen]# valgrind xenstore-watch /foo
>> ==1901== Memcheck, a memory error detector
>> ==1901== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
>> ==1901== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
>> ==1901== Command: xenstore-watch /foo
>> ==1901==
>> ==1901== Syscall param socketcall.connect(serv_addr..sa_len) points to uninitialised byte(s)
>> ==1901==    at 0x152A14A: connect (in /lib/libc.so.7)
>> ==1901==    by 0x1210B46: get_handle (xs.c:205)
>> ==1901==    by 0x1210CEC: xs_open (xs.c:297)
>> ==1901==    by 0x4027B1: main (xenstore_client.c:635)
>> ==1901==  Address 0x7ff000a70 is on thread 1's stack
>> ==1901==
>> /foo
>>
>> Strangely enough, when running under valgrind it doesn't segfault,
> 
> valgrind interposes it's own malloc and stuff which will change
> behaviour, and I wouldn't be all that surprised if it were gettings its
> fingers into some of the pthread stuff too.
> 
>>  and 
>> I'm still trying to figure out why valgrind complains.
> 
> It seems to be an unrelated issue though?

I think so, it seems like valgrind doesn't really like the cast done in
connect from sockaddr_un to sockaddr.

Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Tue, 2014-03-11 at 16:55 +0100, Roger Pau Monné wrote:
> On 11/03/14 15:12, Ian Campbell wrote:
> > On Tue, 2014-03-11 at 14:52 +0100, Roger Pau Monné wrote:
> >> On 11/03/14 14:24, Ian Campbell wrote:
> >>> On Mon, 2014-03-10 at 17:12 +0000, Ian Jackson wrote:
> >>>> Roger Pau Monne writes ("[PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> >>>>> On FreeBSD PTHREAD_STACK_MIN is 2048 by default, which is obviously
> >>>>> too low.
> > 
> > It occurs to me that 2048 is < PAGE_SIZE. Which makes this seem like an
> > interesting choice of stack min, especially combined with the fact that
> > the failure seems to involve malloc.
> > 
> > Perhaps the stack is malloc'd (rather than coming from brk or an anon
> > mmap), so overrunning would cause heap corruption which seems to be what
> > you are seeing.
> > 
> >>> How does this manifest itself? (I suppose this may be answered as part
> >>> of answering Ian J)
> >>
> >> Yes, I'm still looking into it, this gdb output:
> >>
> >> Starting program: /usr/local/bin/xenstore-watch /foo
> >> [New LWP 100169]
> >> [New Thread 801406800 (LWP 100182/xenstore-watch)]
> >>
> >> Program received signal SIGSEGV, Segmentation fault.
> >> [Switching to Thread 801406800 (LWP 100182/xenstore-watch)]
> >> 0x0000000800ac1258 in sbrk () from /lib/libc.so.7
> >> (gdb) bt
> >> #0  0x0000000800ac1258 in sbrk () from /lib/libc.so.7
> >> #1  0x0000000800ac110e in sbrk () from /lib/libc.so.7
> >> #2  0x0000000800ac9ee8 in sbrk () from /lib/libc.so.7
> >> #3  0x0000000800ac456b in sbrk () from /lib/libc.so.7
> >> #4  0x0000000800ac447d in sbrk () from /lib/libc.so.7
> >> #5  0x0000000800aaf6ce in syscall () from /lib/libc.so.7
> >> #6  0x0000000800acb37b in malloc () from /lib/libc.so.7
> >> #7  0x00000008008202b9 in read_message (h=0x801417080, nonblocking=0) at xs.c:313
> >> #8  0x0000000800820a06 in read_thread (arg=0x801417080) at xs.c:313
> >> #9  0x0000000800dc64a4 in pthread_create () from /lib/libthr.so.3
> >> #10 0x0000000000000000 in ?? ()
> > 
> > Does 
> > frame 1 ; print $sp
> > frame 2 ; print $sp
> > etc
> > tell you anything useful about the stack usage at each level?
> 
> Thanks, I've been able to get the stack pointer at each frame, here are
> the results (from frame 0 to frame 10):
> 
> 0x7fffffbfcff0

<-PAGE BOUNDARY HERE

Hence the segfault I expct...

> 0x7fffffbfd0a0
> 0x7fffffbfd0e0
> 0x7fffffbfd120
> 0x7fffffbfd160
> 0x7fffffbfd1a0
> 0x7fffffbfd1e0
> 0x7fffffbfd6a0
> 0x7fffffbfd7a0
> 0x7fffffbfd7c0
> 0x7fffffbfd800
> 
> Doing:
> 
> 0x7fffffbfd800 - 0x7fffffbfcff0 = 0x810
> 
> Which is 2064 in decimal. The biggest culprit seems to be malloc, which
> is using 1216 bytes of the stack.

Wow!

http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/stdlib/malloc.c?rev=1.54.10.1&content-type=text/x-cvsweb-markup I suppose? malloc itself looks fairly small, but there's a lot of inlining in that function... I don't see any large on stack allocations (e.g. arrays) but I suppose it all adds up.

> >> I've also tried to debug it using valgrind,
> > 
> > Under BSD? Did someone wire up the dom0 OS specific bit? If so: Neat!
> 
> No, I don't think anyone has wired the Dom0 specific bits, maybe they
> don't show up because this is just the xenstore client, which is not
> using any ioctls?

Oh yes, that makes sense, you'd be using the Unix domain socket.

> >>  and here's what I got:
> >>
> >> [root@loki ~/xen/xen]# valgrind xenstore-watch /foo
> >> ==1901== Memcheck, a memory error detector
> >> ==1901== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
> >> ==1901== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
> >> ==1901== Command: xenstore-watch /foo
> >> ==1901==
> >> ==1901== Syscall param socketcall.connect(serv_addr..sa_len) points to uninitialised byte(s)
> >> ==1901==    at 0x152A14A: connect (in /lib/libc.so.7)
> >> ==1901==    by 0x1210B46: get_handle (xs.c:205)
> >> ==1901==    by 0x1210CEC: xs_open (xs.c:297)
> >> ==1901==    by 0x4027B1: main (xenstore_client.c:635)
> >> ==1901==  Address 0x7ff000a70 is on thread 1's stack
> >> ==1901==
> >> /foo
> >>
> >> Strangely enough, when running under valgrind it doesn't segfault,
> > 
> > valgrind interposes it's own malloc and stuff which will change
> > behaviour, and I wouldn't be all that surprised if it were gettings its
> > fingers into some of the pthread stuff too.
> > 
> >>  and 
> >> I'm still trying to figure out why valgrind complains.
> > 
> > It seems to be an unrelated issue though?
> 
> I think so, it seems like valgrind doesn't really like the cast done in
> connect from sockaddr_un to sockaddr.

Not all that surprising I guess, it's a bit of an odd interface!

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Tue, 2014-03-11 at 16:03 +0000, Ian Campbell wrote:
> http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/stdlib/malloc.c?rev=1.54.10.1&content-type=text/x-cvsweb-markup I suppose?

Oops, the first hit on my freebsd search was for netbsd and I didn't
notice...

Ian.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Roger Pau Monné]

On 11/03/14 17:03, Ian Campbell wrote:
> On Tue, 2014-03-11 at 16:55 +0100, Roger Pau Monné wrote:
>>
>> Thanks, I've been able to get the stack pointer at each frame, here are
>> the results (from frame 0 to frame 10):
>>
>> 0x7fffffbfcff0
> 
> <-PAGE BOUNDARY HERE
> 
> Hence the segfault I expct...
> 
>> 0x7fffffbfd0a0
>> 0x7fffffbfd0e0
>> 0x7fffffbfd120
>> 0x7fffffbfd160
>> 0x7fffffbfd1a0
>> 0x7fffffbfd1e0
>> 0x7fffffbfd6a0
>> 0x7fffffbfd7a0
>> 0x7fffffbfd7c0
>> 0x7fffffbfd800
>>
>> Doing:
>>
>> 0x7fffffbfd800 - 0x7fffffbfcff0 = 0x810
>>
>> Which is 2064 in decimal. The biggest culprit seems to be malloc, which
>> is using 1216 bytes of the stack.
> 
> Wow!
> 
> http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/stdlib/malloc.c?rev=1.54.10.1&content-type=text/x-cvsweb-markup I suppose? malloc itself looks fairly small, but there's a lot of inlining in that function... I don't see any large on stack allocations (e.g. arrays) but I suppose it all adds up.

This is FreeBSD, which AFAIK is using jemalloc [0], the source inside of
the FreeBSD tree seems to be:

http://svnweb.freebsd.org/base/head/contrib/jemalloc/src/

And it looks like there's quite a lot of inlining there also...

Another question to ask would be why FreeBSD sets PTHREAD_STACK_MIN to
2048 when even a simple malloc call is going to blow that up, but I
guess you can run a thread with 2048 bytes of stack being really careful.

Roger.

[0] http://www.canonware.com/jemalloc/


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Tue, 2014-03-11 at 17:18 +0100, Roger Pau Monné wrote:
> On 11/03/14 17:03, Ian Campbell wrote:
> > On Tue, 2014-03-11 at 16:55 +0100, Roger Pau Monné wrote:
> >>
> >> Thanks, I've been able to get the stack pointer at each frame, here are
> >> the results (from frame 0 to frame 10):
> >>
> >> 0x7fffffbfcff0
> > 
> > <-PAGE BOUNDARY HERE
> > 
> > Hence the segfault I expct...
> > 
> >> 0x7fffffbfd0a0
> >> 0x7fffffbfd0e0
> >> 0x7fffffbfd120
> >> 0x7fffffbfd160
> >> 0x7fffffbfd1a0
> >> 0x7fffffbfd1e0
> >> 0x7fffffbfd6a0
> >> 0x7fffffbfd7a0
> >> 0x7fffffbfd7c0
> >> 0x7fffffbfd800
> >>
> >> Doing:
> >>
> >> 0x7fffffbfd800 - 0x7fffffbfcff0 = 0x810
> >>
> >> Which is 2064 in decimal. The biggest culprit seems to be malloc, which
> >> is using 1216 bytes of the stack.
> > 
> > Wow!
> > 
> > http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/stdlib/malloc.c?rev=1.54.10.1&content-type=text/x-cvsweb-markup I suppose? malloc itself looks fairly small, but there's a lot of inlining in that function... I don't see any large on stack allocations (e.g. arrays) but I suppose it all adds up.
> 
> This is FreeBSD, which AFAIK is using jemalloc [0], the source inside of
> the FreeBSD tree seems to be:
> 
> http://svnweb.freebsd.org/base/head/contrib/jemalloc/src/

Right, I think.

> And it looks like there's quite a lot of inlining there also...
> 
> Another question to ask would be why FreeBSD sets PTHREAD_STACK_MIN to
> 2048 when even a simple malloc call is going to blow that up,

It does seem rather aggressive, but I suppose it is "min" not
"sensible_min".

>  but I
> guess you can run a thread with 2048 bytes of stack being really careful.

In theory I suppose a thread can use no stack at all if it is careful...



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> On Tue, 2014-03-11 at 17:18 +0100, Roger Pau Monné wrote:
> > Another question to ask would be why FreeBSD sets PTHREAD_STACK_MIN to
> > 2048 when even a simple malloc call is going to blow that up,
> 
> It does seem rather aggressive, but I suppose it is "min" not
> "sensible_min".

Well, actually, a malloc works, doesn't it ?  The problem is that we
have about another 2-3 stack frames with a few more words, on top of
that.

How about we set it to PTHREAD_STACK_MIN+1024 or something ?

Ian.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Tue, 2014-03-11 at 16:25 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> > On Tue, 2014-03-11 at 17:18 +0100, Roger Pau Monné wrote:
> > > Another question to ask would be why FreeBSD sets PTHREAD_STACK_MIN to
> > > 2048 when even a simple malloc call is going to blow that up,
> > 
> > It does seem rather aggressive, but I suppose it is "min" not
> > "sensible_min".
> 
> Well, actually, a malloc works, doesn't it ?  The problem is that we
> have about another 2-3 stack frames with a few more words, on top of
> that.

Yes.

> How about we set it to PTHREAD_STACK_MIN+1024 or something ?

I was thinking of suggesting clamping to a minimum of 4K (a page), but
this seems better.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Roger Pau Monné]

On 11/03/14 17:25, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
>> On Tue, 2014-03-11 at 17:18 +0100, Roger Pau Monné wrote:
>>> Another question to ask would be why FreeBSD sets PTHREAD_STACK_MIN to
>>> 2048 when even a simple malloc call is going to blow that up,
>>
>> It does seem rather aggressive, but I suppose it is "min" not
>> "sensible_min".
> 
> Well, actually, a malloc works, doesn't it ?

No, actually a malloc with PTHREAD_STACK_MIN doesn't work, this sample 
example program fails in the same way:

---
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <assert.h>

#define MALLOC_SIZE 1024

void *
foo(void *arg)
{
	void *bar;

	bar = malloc(MALLOC_SIZE);
	assert(bar != NULL);

	return (NULL);
}

int main(void)
{
	pthread_t thread;
	pthread_attr_t attr;
	int rc, i;

	rc = pthread_attr_init(&attr);
	assert(rc == 0);

	rc = pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN);
	assert(rc == 0);

	rc = pthread_create(&thread, &attr, foo, NULL);
	assert(0 == rc);

	rc = pthread_join(thread, NULL);
	assert(0 == rc);

	exit(EXIT_SUCCESS);
}

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> On 11/03/14 17:25, Ian Jackson wrote:
> > Well, actually, a malloc works, doesn't it ?
> 
> No, actually a malloc with PTHREAD_STACK_MIN doesn't work, this sample 
> example program fails in the same way:

Wow.  I'm sure that can't be intentional.

Ian.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Roger Pau Monné]

On 11/03/14 17:52, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
>> On 11/03/14 17:25, Ian Jackson wrote:
>>> Well, actually, a malloc works, doesn't it ?
>>
>> No, actually a malloc with PTHREAD_STACK_MIN doesn't work, this sample 
>> example program fails in the same way:
> 
> Wow.  I'm sure that can't be intentional.

According to
http://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html (or at
least that's how I read it), it shouldn't be assumed that
PTHREAD_STACK_MIN will be set to a value that allows using libc calls,
the standard even says it's valid to set it to 0.

So I think the proposed patch (or a variation of it), is the right
solution, we shouldn't rely on PTHREAD_STACK_MIN being set to a sane
value. IMHO the only thing we should use PTHREAD_STACK_MIN for is to
check that the value we are passing to pthread_attr_setstacksize is valid.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Wed, 2014-03-12 at 11:27 +0100, Roger Pau Monné wrote:
> On 11/03/14 17:52, Ian Jackson wrote:
> > Roger Pau Monné writes ("Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> >> On 11/03/14 17:25, Ian Jackson wrote:
> >>> Well, actually, a malloc works, doesn't it ?
> >>
> >> No, actually a malloc with PTHREAD_STACK_MIN doesn't work, this sample 
> >> example program fails in the same way:
> > 
> > Wow.  I'm sure that can't be intentional.
> 
> According to
> http://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html (or at
> least that's how I read it), it shouldn't be assumed that
> PTHREAD_STACK_MIN will be set to a value that allows using libc calls,
> the standard even says it's valid to set it to 0.

That's not a terribly helpful definition!

I don't remember seeing that when I wrote that older version.
http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_attr_setstacksize.html doesn't feel the need to talk about such things, which is rather unhelpful of it...

> So I think the proposed patch (or a variation of it), is the right
> solution, we shouldn't rely on PTHREAD_STACK_MIN being set to a sane
> value. IMHO the only thing we should use PTHREAD_STACK_MIN for is to
> check that the value we are passing to pthread_attr_setstacksize is valid.

Yeah, it does seem that way.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Roger Pau Monné]

On 12/03/14 11:30, Ian Campbell wrote:
> On Wed, 2014-03-12 at 11:27 +0100, Roger Pau Monné wrote:
>> On 11/03/14 17:52, Ian Jackson wrote:
>>> Roger Pau Monné writes ("Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
>>>> On 11/03/14 17:25, Ian Jackson wrote:
>>>>> Well, actually, a malloc works, doesn't it ?
>>>>
>>>> No, actually a malloc with PTHREAD_STACK_MIN doesn't work, this sample 
>>>> example program fails in the same way:
>>>
>>> Wow.  I'm sure that can't be intentional.
>>
>> According to
>> http://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html (or at
>> least that's how I read it), it shouldn't be assumed that
>> PTHREAD_STACK_MIN will be set to a value that allows using libc calls,
>> the standard even says it's valid to set it to 0.
> 
> That's not a terribly helpful definition!
> 
> I don't remember seeing that when I wrote that older version.
> http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_attr_setstacksize.html doesn't feel the need to talk about such things, which is rather unhelpful of it...
> 
>> So I think the proposed patch (or a variation of it), is the right
>> solution, we shouldn't rely on PTHREAD_STACK_MIN being set to a sane
>> value. IMHO the only thing we should use PTHREAD_STACK_MIN for is to
>> check that the value we are passing to pthread_attr_setstacksize is valid.
> 
> Yeah, it does seem that way.

For reference, here is the original reply that I've received when asking
about PTHREAD_STACK_MIN on freebsd-current:

http://lists.freebsd.org/pipermail/freebsd-current/2014-March/048885.html


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Wed, 2014-03-12 at 10:30 +0000, Ian Campbell wrote:
> > So I think the proposed patch (or a variation of it), is the right
> > solution, we shouldn't rely on PTHREAD_STACK_MIN being set to a sane
> > value. IMHO the only thing we should use PTHREAD_STACK_MIN for is to
> > check that the value we are passing to pthread_attr_setstacksize is valid.
> 
> Yeah, it does seem that way.

Ian, do you agree?

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> On Wed, 2014-03-12 at 10:30 +0000, Ian Campbell wrote:
> > [someone:]
> > > So I think the proposed patch (or a variation of it), is the
> > > right solution, we shouldn't rely on PTHREAD_STACK_MIN being set
> > > to a sane value. IMHO the only thing we should use
> > > PTHREAD_STACK_MIN for is to check that the value we are passing
> > > to pthread_attr_setstacksize is valid.
> > 
> > Yeah, it does seem that way.
> 
> Ian, do you agree?

Yes.

Ian.

Re: [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value

[Ian Campbell]

On Tue, 2014-03-18 at 17:20 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xenstore: set READ_THREAD_STACKSIZE to a sane value"):
> > On Wed, 2014-03-12 at 10:30 +0000, Ian Campbell wrote:
> > > [someone:]
> > > > So I think the proposed patch (or a variation of it), is the
> > > > right solution, we shouldn't rely on PTHREAD_STACK_MIN being set
> > > > to a sane value. IMHO the only thing we should use
> > > > PTHREAD_STACK_MIN for is to check that the value we are passing
> > > > to pthread_attr_setstacksize is valid.
> > > 
> > > Yeah, it does seem that way.
> > 
> > Ian, do you agree?
> 
> Yes.

I've acked + applied the original patch. Thanks Roger.

Ian.