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
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 > 0This #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
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
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
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.
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
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.
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.
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.
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.
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.
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?
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.
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.