[PATCH] xs: set read_thread stacksize

[PATCH] xs: set read_thread stacksize

[Simon Rowe]

xs_watch() creates a thread to wake watchers using default attributes. The
stacksize can be quite large (8 MB on Linux), applications that link against
xenstore end up having a larger memory footprint than necessary.

Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>

diff -r 53e0571f94e4 -r 0cf61ed6ce86 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Fri May 25 08:21:25 2012 +0100
+++ b/tools/xenstore/xs.c	Tue May 29 16:45:03 2012 +0100
@@ -705,11 +705,31 @@ bool xs_watch(struct xs_handle *h, const
 	/* We dynamically create a reader thread on demand. */
 	mutex_lock(&h->request_mutex);
 	if (!h->read_thr_exists) {
+#if _POSIX_THREAD_ATTR_STACKSIZE > 0
+		pthread_attr_t attr;
+
+		if (pthread_attr_init(&attr) != 0) {
+			mutex_unlock(&h->request_mutex);
+			return false;
+		}
+		if (pthread_attr_setstacksize(&attr, 16 * 1024) != 0) {
+			pthread_attr_destroy(&attr);
+			mutex_unlock(&h->request_mutex);
+			return false;
+		}
+
+		if (pthread_create(&h->read_thr, &attr, read_thread, h) != 0) {
+			pthread_attr_destroy(&attr);
+#else
 		if (pthread_create(&h->read_thr, NULL, read_thread, h) != 0) {
+#endif
 			mutex_unlock(&h->request_mutex);
 			return false;
 		}
 		h->read_thr_exists = 1;
+#if _POSIX_THREAD_ATTR_STACKSIZE > 0
+		pthread_attr_destroy(&attr);
+#endif
 	}
 	mutex_unlock(&h->request_mutex);
 #endif

Re: [PATCH] xs: set read_thread stacksize

[David Vrabel]

On 29/05/12 16:56, Simon Rowe wrote:
> xs_watch() creates a thread to wake watchers using default attributes. The
> stacksize can be quite large (8 MB on Linux), applications that link against
> xenstore end up having a larger memory footprint than necessary.
> 
> Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>
> 
> diff -r 53e0571f94e4 -r 0cf61ed6ce86 tools/xenstore/xs.c
> --- a/tools/xenstore/xs.c	Fri May 25 08:21:25 2012 +0100
> +++ b/tools/xenstore/xs.c	Tue May 29 16:45:03 2012 +0100
> @@ -705,11 +705,31 @@ bool xs_watch(struct xs_handle *h, const
>  	/* We dynamically create a reader thread on demand. */
>  	mutex_lock(&h->request_mutex);
>  	if (!h->read_thr_exists) {
> +#if _POSIX_THREAD_ATTR_STACKSIZE > 0

This #if seems unnecessary.  pthread_attr_setsetstacksize() doesn't
appear to be an optional.

> +		pthread_attr_t attr;
> +
> +		if (pthread_attr_init(&attr) != 0) {
> +			mutex_unlock(&h->request_mutex);
> +			return false;
> +		}
> +		if (pthread_attr_setstacksize(&attr, 16 * 1024) != 0) {

#define for this value?

David

Re: [PATCH] xs: set read_thread stacksize

[Ian Campbell]

On Tue, 2012-05-29 at 17:39 +0100, David Vrabel wrote:
> On 29/05/12 16:56, Simon Rowe wrote:
> > xs_watch() creates a thread to wake watchers using default attributes. The
> > stacksize can be quite large (8 MB on Linux), applications that link against
> > xenstore end up having a larger memory footprint than necessary.
> > 
> > Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>
> > 
> > diff -r 53e0571f94e4 -r 0cf61ed6ce86 tools/xenstore/xs.c
> > --- a/tools/xenstore/xs.c	Fri May 25 08:21:25 2012 +0100
> > +++ b/tools/xenstore/xs.c	Tue May 29 16:45:03 2012 +0100
> > @@ -705,11 +705,31 @@ bool xs_watch(struct xs_handle *h, const
> >  	/* We dynamically create a reader thread on demand. */
> >  	mutex_lock(&h->request_mutex);
> >  	if (!h->read_thr_exists) {
> > +#if _POSIX_THREAD_ATTR_STACKSIZE > 0
> 
> This #if seems unnecessary.  pthread_attr_setsetstacksize() doesn't
> appear to be an optional.

...and if it were then autoconf is the way to figure that out now,
unless _POSIX_THREAD_ATTR_STACKSIZE is specified somewhere (which I
doubt).

Also if it is only pthread_attr_setstacksize which is optional, rather
than pthread_attr_* generally, then the #if could be pulled into just
surround that call, presuming there is no harm in a "NULL" attr.

> > +		pthread_attr_t attr;
> > +
> > +		if (pthread_attr_init(&attr) != 0) {
> > +			mutex_unlock(&h->request_mutex);
> > +			return false;
> > +		}
> > +		if (pthread_attr_setstacksize(&attr, 16 * 1024) != 0) {
> 
> #define for this value?

Yes, please.

Ian.

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

Re: [PATCH] xs: set read_thread stacksize

[Simon Rowe]

On Tuesday 29 May 2012 20:39:33 Ian Campbell wrote:

> ...and if it were then autoconf is the way to figure that out now,
> unless _POSIX_THREAD_ATTR_STACKSIZE is specified somewhere (which I
> doubt).

I was following the recommendation of the POSIX Threads: Semi-FAQ which states


5.2 How can I determine if a system supports the Stack Attribute(s)?

If the header file unistd.h defines the symbolic constant  
_POSIX_THREAD_ATTR_STACKSIZE to a value greater than 0, the implementation 
should support the getting and setting of the Stack Size Attribute. If it 
defined to a value of 200112L then the current specification is supported.


If this needs to be done via autoconf let me know.

> Also if it is only pthread_attr_setstacksize which is optional, rather
> than pthread_attr_* generally, then the #if could be pulled into just
> surround that call, presuming there is no harm in a "NULL" attr.

I don't quite get you, do you mean only protect the actual 
pthread_attr_setstacksize() call with #ifdef and therefore always call 
pthread_attr_init()?
 
> > > +		pthread_attr_t attr;
> > > +
> > > +		if (pthread_attr_init(&attr) != 0) {
> > > +			mutex_unlock(&h->request_mutex);
> > > +			return false;
> > > +		}
> > > +		if (pthread_attr_setstacksize(&attr, 16 * 1024) != 0) {
> > 
> > #define for this value?
> 
> Yes, please.

Will do,

	Simon

--
[1] http://www.cognitus.net/html/howto/pthreadSemiFAQ_5.html#s5_1

Re: [PATCH] xs: set read_thread stacksize

[Ian Campbell]

On Wed, 2012-05-30 at 08:56 +0100, Simon Rowe wrote:
> On Tuesday 29 May 2012 20:39:33 Ian Campbell wrote:
> 
> > ...and if it were then autoconf is the way to figure that out now,
> > unless _POSIX_THREAD_ATTR_STACKSIZE is specified somewhere (which I
> > doubt).
> 
> I was following the recommendation of the POSIX Threads: Semi-FAQ which states
> 
> 
> 5.2 How can I determine if a system supports the Stack Attribute(s)?
> 
> If the header file unistd.h defines the symbolic constant  
> _POSIX_THREAD_ATTR_STACKSIZE to a value greater than 0, the implementation 
> should support the getting and setting of the Stack Size Attribute. If it 
> defined to a value of 200112L then the current specification is supported.
> 
> 
> If this needs to be done via autoconf let me know.

If this little trick applies to both NetBSD and uclibc too then I guess
it would be OK, otherwise I think autoconf is necessary.

> > Also if it is only pthread_attr_setstacksize which is optional, rather
> > than pthread_attr_* generally, then the #if could be pulled into just
> > surround that call, presuming there is no harm in a "NULL" attr.
> 
> I don't quite get you, do you mean only protect the actual 
> pthread_attr_setstacksize() call with #ifdef and therefore always call 
> pthread_attr_init()?

Yes, that'll reduce the ifdeffery and inparticular removes the double
	if (pthread_create(&h->read_thr, NULL, read_thread, h) != 0) {
in either half, which is bit tricky to follow and is going to be prone
to drift across the two sides.

Re: [PATCH] xs: set read_thread stacksize

[Simon Rowe]

On Wednesday 30 May 2012 10:40:15 Ian Campbell wrote:

> If this little trick applies to both NetBSD and uclibc too then I guess
> it would be OK, otherwise I think autoconf is necessary.

It doesn't look to my untrained eye that xenstore is autoconf-aware. The
makefile unilaterally sets USE_PTHREAD for example.

Shall I just drop this test for now and if/when xenstore is updated to use
autoconf it can be addressed then?

		Simon

Re: [PATCH] xs: set read_thread stacksize

[Ian Campbell]

On Wed, 2012-05-30 at 13:10 +0100, Simon Rowe wrote:
> On Wednesday 30 May 2012 10:40:15 Ian Campbell wrote:
> 
> > If this little trick applies to both NetBSD and uclibc too then I guess
> > it would be OK, otherwise I think autoconf is necessary.
> 
> It doesn't look to my untrained eye that xenstore is autoconf-aware. The
> makefile unilaterally sets USE_PTHREAD for example.

It has autoconf stuff available to it, I think, it just hasn't had cause
to use it yet...

(USE_PTHREAD is a bit of an odd one anyway, it refers only to the client
library/cmdline tools and is for building against libc's which don't
have pthreads)

> Shall I just drop this test for now and if/when xenstore is updated to use
> autoconf it can be addressed then?

I'd like to here from Roger about what this means for NetBSD and uclibc,
if it works on those then I think it is fine to do this.

Ian.

Re: [PATCH] xs: set read_thread stacksize

[Roger Pau Monne]

Ian Campbell wrote:
> On Wed, 2012-05-30 at 13:10 +0100, Simon Rowe wrote:
>> On Wednesday 30 May 2012 10:40:15 Ian Campbell wrote:
>>
>>> If this little trick applies to both NetBSD and uclibc too then I guess
>>> it would be OK, otherwise I think autoconf is necessary.
>> It doesn't look to my untrained eye that xenstore is autoconf-aware. The
>> makefile unilaterally sets USE_PTHREAD for example.
>
> It has autoconf stuff available to it, I think, it just hasn't had cause
> to use it yet...
>
> (USE_PTHREAD is a bit of an odd one anyway, it refers only to the client
> library/cmdline tools and is for building against libc's which don't
> have pthreads)
>
>> Shall I just drop this test for now and if/when xenstore is updated to use
>> autoconf it can be addressed then?
>
> I'd like to here from Roger about what this means for NetBSD and uclibc,
> if it works on those then I think it is fine to do this.

Sorry for the delay, I just received this today (don't know why). I've 
been looking at NetBSD and uClibc, and both have pthread_attr_setstacksize.

What I don't really like is the hardcoded (16 * 1024) value, how do you 
know this is greater than PTHREAD_STACK_MIN?

Frankly I don't understand why do we have to touch this, even if you 
requested 256MB of stack it won't we allocated until you get a page 
fault, so you are only using the physical memory you need.

Re: [PATCH] xs: set read_thread stacksize

[Ian Campbell]

On Thu, 2012-06-21 at 10:09 +0100, Roger Pau Monne wrote:
> Ian Campbell wrote:
> > On Wed, 2012-05-30 at 13:10 +0100, Simon Rowe wrote:
> >> On Wednesday 30 May 2012 10:40:15 Ian Campbell wrote:
> >>
> >>> If this little trick applies to both NetBSD and uclibc too then I guess
> >>> it would be OK, otherwise I think autoconf is necessary.
> >> It doesn't look to my untrained eye that xenstore is autoconf-aware. The
> >> makefile unilaterally sets USE_PTHREAD for example.
> >
> > It has autoconf stuff available to it, I think, it just hasn't had cause
> > to use it yet...
> >
> > (USE_PTHREAD is a bit of an odd one anyway, it refers only to the client
> > library/cmdline tools and is for building against libc's which don't
> > have pthreads)
> >
> >> Shall I just drop this test for now and if/when xenstore is updated to use
> >> autoconf it can be addressed then?
> >
> > I'd like to here from Roger about what this means for NetBSD and uclibc,
> > if it works on those then I think it is fine to do this.
> 
> Sorry for the delay, I just received this today (don't know why).

It seemed to have been stuck in my outbox, I thought it was another
unrelated mail (which I sent this morning) so I hit go.

BTW, I think this patch went in already...

>  I've 
> been looking at NetBSD and uClibc, and both have pthread_attr_setstacksize.
> 
> What I don't really like is the hardcoded (16 * 1024) value, how do you 
> know this is greater than PTHREAD_STACK_MIN?

pthread_attr_setstacksize(1) specifically says that PTHREAD_STACK_MIN is
16K, but I don't know if that is a real specified thing or just Linux
bias in the manpage.

http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_attr_setstacksize.html doesn't seem to say anything about the value of PTHREAD_STACK_MIN.

How about we fix this when we come across a system which has a smaller
minimum stack?

> Frankly I don't understand why do we have to touch this, even if you 
> requested 256MB of stack it won't we allocated until you get a page 
> fault, so you are only using the physical memory you need.

With 256M stack 4 threads would take up 1G of your virtual address space
(regardless of whether it is populated or not), and 12 threads would
take up 3G -- which is the whole virtual address space of a 32 bit
process, which is rather limiting.

Ian.

Re: [PATCH] xs: set read_thread stacksize

[Roger Pau Monne]

Ian Campbell wrote:
> On Thu, 2012-06-21 at 10:09 +0100, Roger Pau Monne wrote:
>> Ian Campbell wrote:
>>> On Wed, 2012-05-30 at 13:10 +0100, Simon Rowe wrote:
>>>> On Wednesday 30 May 2012 10:40:15 Ian Campbell wrote:
>>>>
>>>>> If this little trick applies to both NetBSD and uclibc too then I guess
>>>>> it would be OK, otherwise I think autoconf is necessary.
>>>> It doesn't look to my untrained eye that xenstore is autoconf-aware. The
>>>> makefile unilaterally sets USE_PTHREAD for example.
>>> It has autoconf stuff available to it, I think, it just hasn't had cause
>>> to use it yet...
>>>
>>> (USE_PTHREAD is a bit of an odd one anyway, it refers only to the client
>>> library/cmdline tools and is for building against libc's which don't
>>> have pthreads)
>>>
>>>> Shall I just drop this test for now and if/when xenstore is updated to use
>>>> autoconf it can be addressed then?
>>> I'd like to here from Roger about what this means for NetBSD and uclibc,
>>> if it works on those then I think it is fine to do this.
>> Sorry for the delay, I just received this today (don't know why).
>
> It seemed to have been stuck in my outbox, I thought it was another
> unrelated mail (which I sent this morning) so I hit go.
>
> BTW, I think this patch went in already...
>
>>   I've
>> been looking at NetBSD and uClibc, and both have pthread_attr_setstacksize.
>>
>> What I don't really like is the hardcoded (16 * 1024) value, how do you
>> know this is greater than PTHREAD_STACK_MIN?
>
> pthread_attr_setstacksize(1) specifically says that PTHREAD_STACK_MIN is
> 16K, but I don't know if that is a real specified thing or just Linux
> bias in the manpage.

PTHREAD_STACK_MIN it's also present on NetBSD and uClibc, so I guess we 
should use PTHREAD_STACK_MIN (or PTHREAD_STACK_MIN * 2) if the default 
stack size has to be changed to some sensible value (which I still don't 
think it has to). Can we guarantee PTHREAD_STACK_MIN is not going to 
change to something greater than 16k thus breaking this patch?

> http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_attr_setstacksize.html doesn't seem to say anything about the value of PTHREAD_STACK_MIN.
>
> How about we fix this when we come across a system which has a smaller
> minimum stack?
>
>> Frankly I don't understand why do we have to touch this, even if you
>> requested 256MB of stack it won't we allocated until you get a page
>> fault, so you are only using the physical memory you need.
>
> With 256M stack 4 threads would take up 1G of your virtual address space
> (regardless of whether it is populated or not), and 12 threads would
> take up 3G -- which is the whole virtual address space of a 32 bit
> process, which is rather limiting.

This should be set by the OS to a sensible value that allows creating a 
reasonable number of threads, given that the default in Linux is 8MB, it 
will allow you to create 375 threads on a 32bit system, which looks like 
a pretty high number to me. Anyway limiting the stack size of a single 
thread won't make much of difference regarding this, because all the 
other threads will still take the default value.

Re: [PATCH] xs: set read_thread stacksize

[Ian Campbell]

On Thu, 2012-06-21 at 10:39 +0100, Roger Pau Monne wrote:
> Ian Campbell wrote:
> > On Thu, 2012-06-21 at 10:09 +0100, Roger Pau Monne wrote:
> >> Ian Campbell wrote:
> >>> On Wed, 2012-05-30 at 13:10 +0100, Simon Rowe wrote:
> >>>> On Wednesday 30 May 2012 10:40:15 Ian Campbell wrote:
> >>>>
> >>>>> If this little trick applies to both NetBSD and uclibc too then I guess
> >>>>> it would be OK, otherwise I think autoconf is necessary.
> >>>> It doesn't look to my untrained eye that xenstore is autoconf-aware. The
> >>>> makefile unilaterally sets USE_PTHREAD for example.
> >>> It has autoconf stuff available to it, I think, it just hasn't had cause
> >>> to use it yet...
> >>>
> >>> (USE_PTHREAD is a bit of an odd one anyway, it refers only to the client
> >>> library/cmdline tools and is for building against libc's which don't
> >>> have pthreads)
> >>>
> >>>> Shall I just drop this test for now and if/when xenstore is updated to use
> >>>> autoconf it can be addressed then?
> >>> I'd like to here from Roger about what this means for NetBSD and uclibc,
> >>> if it works on those then I think it is fine to do this.
> >> Sorry for the delay, I just received this today (don't know why).
> >
> > It seemed to have been stuck in my outbox, I thought it was another
> > unrelated mail (which I sent this morning) so I hit go.
> >
> > BTW, I think this patch went in already...
> >
> >>   I've
> >> been looking at NetBSD and uClibc, and both have pthread_attr_setstacksize.
> >>
> >> What I don't really like is the hardcoded (16 * 1024) value, how do you
> >> know this is greater than PTHREAD_STACK_MIN?
> >
> > pthread_attr_setstacksize(1) specifically says that PTHREAD_STACK_MIN is
> > 16K, but I don't know if that is a real specified thing or just Linux
> > bias in the manpage.
> 
> PTHREAD_STACK_MIN it's also present on NetBSD and uClibc, so I guess we 
> should use PTHREAD_STACK_MIN (or PTHREAD_STACK_MIN * 2) if the default 
> stack size has to be changed to some sensible value (which I still don't 
> think it has to). Can we guarantee PTHREAD_STACK_MIN is not going to 
> change to something greater than 16k thus breaking this patch?

Using PTHREAD_STACK_MIN sounds like a good idea, since this patch is in
can you send an incremental update? You could do max(PTHREAD_STACK_MIN,
16*1204) perhaps?

> > http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_attr_setstacksize.html doesn't seem to say anything about the value of PTHREAD_STACK_MIN.
> >
> > How about we fix this when we come across a system which has a smaller
> > minimum stack?
> >
> >> Frankly I don't understand why do we have to touch this, even if you
> >> requested 256MB of stack it won't we allocated until you get a page
> >> fault, so you are only using the physical memory you need.
> >
> > With 256M stack 4 threads would take up 1G of your virtual address space
> > (regardless of whether it is populated or not), and 12 threads would
> > take up 3G -- which is the whole virtual address space of a 32 bit
> > process, which is rather limiting.
> 
> This should be set by the OS to a sensible value that allows creating a 
> reasonable number of threads, given that the default in Linux is 8MB, it 
> will allow you to create 375 threads on a 32bit system, which looks like 
> a pretty high number to me.

It won't be that high in practice, due to all the other users of virtual
memory as well as fragmentation (i.e. libraries get loaded all over the
place and perhaps don't leave large contiguous chunks etc).

>  Anyway limiting the stack size of a single 
> thread won't make much of difference regarding this, because all the 
> other threads will still take the default value.

I don't recall if I was the original author of this patch or if I was
just involved at the time but this limit really did get hit on
XenServer. IIRC care is taken in whichever process it was to reduce all
the threads to reasonable stack sizes. Its only polite that libxenstore
as a library minimises it's stack usage in order that users of the
library can in turn minimise it's own footprint.

Ian.

Re: [PATCH] xs: set read_thread stacksize

[Roger Pau Monne]

Ian Campbell wrote:
> Using PTHREAD_STACK_MIN sounds like a good idea, since this patch is in
> can you send an incremental update? You could do max(PTHREAD_STACK_MIN,
> 16*1204) perhaps?

If we are trying to reduce stack as much as possible shouldn't we use 
PTHREAD_STACK_MIN directly?

Re: [PATCH] xs: set read_thread stacksize

[Ian Campbell]

On Thu, 2012-06-21 at 11:27 +0100, Roger Pau Monne wrote:
> Ian Campbell wrote:
> > Using PTHREAD_STACK_MIN sounds like a good idea, since this patch is in
> > can you send an incremental update? You could do max(PTHREAD_STACK_MIN,
> > 16*1204) perhaps?
> 
> If we are trying to reduce stack as much as possible shouldn't we use 
> PTHREAD_STACK_MIN directly?

My thinking was that 16K has been validated as a minimum. I/we don't
know what would happen e.g. if PTHREAD_STACK_MIN was 8K without doing
further investigative work.

Ian.

Re: [PATCH] xs: set read_thread stacksize

[Ian Jackson]

Roger Pau Monne writes ("Re: [Xen-devel] [PATCH] xs: set read_thread stacksize"):
> Frankly I don't understand why do we have to touch this, even if you 
> requested 256MB of stack it won't we allocated until you get a page 
> fault, so you are only using the physical memory you need.

It uses up a lot of address space on 32-bit if nothing else.

Ian.