Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
Wei Liu writes ("Re: [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread"): > On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > > Of course, the alternative is to bump it to another arbitrary value, but the > > requirements may change depending on the application and its libraries that > > are linking against xenstore. Personally I think the libraries and applications that are putting big structures into TLS are mad. But I guess we have to live in the world of madness. > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html > > [2] > > https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00012.html > > [3] > > https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00085.html > > [4] https://bugzilla.redhat.com/show_bug.cgi?id=1350264 > > > > Signed-off-by: Chris Patterson > > I'm tempted to just ack and apply this patch. If I hear no objection by > Friday I will do so. Can we please at least retain the ability for an application that wants to make lots of threads, to set the stack size used by libxenstore ? I guess this would have to be a global function, callable by anyone (even bypassing libxl). Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On 27/09/16 11:06, Wei Liu wrote: On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > From: Chris Patterson > > xs_watch() creates a thread to listen to xenstore events. Currently, the > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. > > There have been several bug reports and workarounds related to the issue > where xs_watch() fails because its attempt to create the reader thread with > pthread_create() fails. This is due to insufficient stack space size > given the requirements for thread-local storage usage in the applications > and libraries that are linked against libxenstore. [1,2,3,4]. > > Specifying the stack size appears to have been added to reduce memory > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). > > This has already been bumped up once to the greater of 16k and > PTHREAD_STACK_MIN (da6a0e86d6a079102abdd0858a19f1e1fae584fc). > > This patch reverts to sticking with the system's default stack size and > removes the code used to set the thread's stack size. > > Of course, the alternative is to bump it to another arbitrary value, but the > requirements may change depending on the application and its libraries that > are linking against xenstore. > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html > [2] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00012.html > [3] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00085.html > [4] https://bugzilla.redhat.com/show_bug.cgi?id=1350264 > > Signed-off-by: Chris Patterson I'm tempted to just ack and apply this patch. If I hear no objection by Friday I will do so. I think the reason we added this patch has gone away, Simon ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > From: Chris Patterson > > xs_watch() creates a thread to listen to xenstore events. Currently, the > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. > > There have been several bug reports and workarounds related to the issue > where xs_watch() fails because its attempt to create the reader thread with > pthread_create() fails. This is due to insufficient stack space size > given the requirements for thread-local storage usage in the applications > and libraries that are linked against libxenstore. [1,2,3,4]. > > Specifying the stack size appears to have been added to reduce memory > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). > > This has already been bumped up once to the greater of 16k and > PTHREAD_STACK_MIN (da6a0e86d6a079102abdd0858a19f1e1fae584fc). > > This patch reverts to sticking with the system's default stack size and > removes the code used to set the thread's stack size. > > Of course, the alternative is to bump it to another arbitrary value, but the > requirements may change depending on the application and its libraries that > are linking against xenstore. > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html > [2] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00012.html > [3] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00085.html > [4] https://bugzilla.redhat.com/show_bug.cgi?id=1350264 > > Signed-off-by: Chris Patterson I'm tempted to just ack and apply this patch. If I hear no objection by Friday I will do so. > --- > tools/xenstore/xs.c | 10 -- > 1 file changed, 10 deletions(-) > > diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c > index d1e01ba..c62b120 100644 > --- a/tools/xenstore/xs.c > +++ b/tools/xenstore/xs.c > @@ -723,11 +723,6 @@ bool xs_watch(struct xs_handle *h, const char *path, > const char *token) > struct iovec iov[2]; > > #ifdef USE_PTHREAD > -#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); > if (!h->read_thr_exists) { > @@ -738,11 +733,6 @@ bool xs_watch(struct xs_handle *h, const char *path, > const char *token) > mutex_unlock(&h->request_mutex); > return false; > } > - if (pthread_attr_setstacksize(&attr, READ_THREAD_STACKSIZE) != > 0) { > - pthread_attr_destroy(&attr); > - mutex_unlock(&h->request_mutex); > - return false; > - } > > sigfillset(&set); > pthread_sigmask(SIG_SETMASK, &set, &old_set); > -- > 2.1.4 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 10:07:10AM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Sep 21, 2016 at 09:50:30AM -0400, Chris Patterson wrote: > > On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu wrote: > > > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: > > >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > > >> > From: Chris Patterson > > >> > > > >> > xs_watch() creates a thread to listen to xenstore events. Currently, > > >> > the > > >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. > > >> > > > >> > There have been several bug reports and workarounds related to the > > >> > issue > > >> > where xs_watch() fails because its attempt to create the reader thread > > >> > with > > >> > pthread_create() fails. This is due to insufficient stack space size > > >> > given the requirements for thread-local storage usage in the > > >> > applications > > >> > and libraries that are linked against libxenstore. [1,2,3,4]. > > >> > > > >> > Specifying the stack size appears to have been added to reduce memory > > >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). > > >> > > >> Ugh. 8MB. > > > > > > OOI isn't that 8MB virtual memory, which means it shouldn't have real > > > impact unless it is used? > > /me nods. That is my recollection too. But it does mean that 'top' > shows the application as bigger (by 8MB). > > > > > > > > >From what I understand, that is correct. At least in the Linux/glibc > > case, I believe the stack is allocated using anonymous mmap() and that > > resident memory usage shouldn't be greater than what you actually end > > up writing. However, I do not know if this holds true universally... > > That should be faily easy to find out. One just needs to check > the RSS size. Not sure how to do that on FreeBSD thought.. In any case, I don't think we should be setting the pthread stack size at all, we don't really know how much stack libc functions will try to use, so setting this to any value is likely wrong IMHO. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 10:23:09AM -0400, Chris Patterson wrote: > On Wed, Sep 21, 2016 at 10:07 AM, Konrad Rzeszutek Wilk > wrote: > > On Wed, Sep 21, 2016 at 09:50:30AM -0400, Chris Patterson wrote: > >> On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu wrote: > >> > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: > >> >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > >> >> > From: Chris Patterson > >> >> > > >> >> > xs_watch() creates a thread to listen to xenstore events. Currently, > >> >> > the > >> >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. > >> >> > > >> >> > There have been several bug reports and workarounds related to the > >> >> > issue > >> >> > where xs_watch() fails because its attempt to create the reader > >> >> > thread with > >> >> > pthread_create() fails. This is due to insufficient stack space size > >> >> > given the requirements for thread-local storage usage in the > >> >> > applications > >> >> > and libraries that are linked against libxenstore. [1,2,3,4]. > >> >> > > >> >> > Specifying the stack size appears to have been added to reduce memory > >> >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). > >> >> > >> >> Ugh. 8MB. > >> > > >> > OOI isn't that 8MB virtual memory, which means it shouldn't have real > >> > impact unless it is used? > > > > /me nods. That is my recollection too. But it does mean that 'top' > > shows the application as bigger (by 8MB). > > > >> > > >> > >> >From what I understand, that is correct. At least in the Linux/glibc > >> case, I believe the stack is allocated using anonymous mmap() and that > >> resident memory usage shouldn't be greater than what you actually end > >> up writing. However, I do not know if this holds true universally... > > > > That should be faily easy to find out. One just needs to check > > the RSS size. Not sure how to do that on FreeBSD thought.. > > I did validate that yesterday (using pmap -x ) and it appeared to > hold true (for that case anyways). Thank you for checking that. > > If using the default stack size (as this patch reverts to), the stack > size can be controlled with ulimit -s That should be part of the patch description and perhaps even in libxs.h header. Lets wait for Simon to chime in. It may be that he had some other reasons to limit the stack size that weren't mentioned in the commit. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 10:07 AM, Konrad Rzeszutek Wilk wrote: > On Wed, Sep 21, 2016 at 09:50:30AM -0400, Chris Patterson wrote: >> On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu wrote: >> > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: >> >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: >> >> > From: Chris Patterson >> >> > >> >> > xs_watch() creates a thread to listen to xenstore events. Currently, >> >> > the >> >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. >> >> > >> >> > There have been several bug reports and workarounds related to the issue >> >> > where xs_watch() fails because its attempt to create the reader thread >> >> > with >> >> > pthread_create() fails. This is due to insufficient stack space size >> >> > given the requirements for thread-local storage usage in the >> >> > applications >> >> > and libraries that are linked against libxenstore. [1,2,3,4]. >> >> > >> >> > Specifying the stack size appears to have been added to reduce memory >> >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). >> >> >> >> Ugh. 8MB. >> > >> > OOI isn't that 8MB virtual memory, which means it shouldn't have real >> > impact unless it is used? > > /me nods. That is my recollection too. But it does mean that 'top' > shows the application as bigger (by 8MB). > >> > >> >> >From what I understand, that is correct. At least in the Linux/glibc >> case, I believe the stack is allocated using anonymous mmap() and that >> resident memory usage shouldn't be greater than what you actually end >> up writing. However, I do not know if this holds true universally... > > That should be faily easy to find out. One just needs to check > the RSS size. Not sure how to do that on FreeBSD thought.. I did validate that yesterday (using pmap -x ) and it appeared to hold true (for that case anyways). If using the default stack size (as this patch reverts to), the stack size can be controlled with ulimit -s ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 09:50:30AM -0400, Chris Patterson wrote: > On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu wrote: > > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: > >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > >> > From: Chris Patterson > >> > > >> > xs_watch() creates a thread to listen to xenstore events. Currently, the > >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. > >> > > >> > There have been several bug reports and workarounds related to the issue > >> > where xs_watch() fails because its attempt to create the reader thread > >> > with > >> > pthread_create() fails. This is due to insufficient stack space size > >> > given the requirements for thread-local storage usage in the applications > >> > and libraries that are linked against libxenstore. [1,2,3,4]. > >> > > >> > Specifying the stack size appears to have been added to reduce memory > >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). > >> > >> Ugh. 8MB. > > > > OOI isn't that 8MB virtual memory, which means it shouldn't have real > > impact unless it is used? /me nods. That is my recollection too. But it does mean that 'top' shows the application as bigger (by 8MB). > > > > >From what I understand, that is correct. At least in the Linux/glibc > case, I believe the stack is allocated using anonymous mmap() and that > resident memory usage shouldn't be greater than what you actually end > up writing. However, I do not know if this holds true universally... That should be faily easy to find out. One just needs to check the RSS size. Not sure how to do that on FreeBSD thought.. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu wrote: > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: >> > From: Chris Patterson >> > >> > xs_watch() creates a thread to listen to xenstore events. Currently, the >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. >> > >> > There have been several bug reports and workarounds related to the issue >> > where xs_watch() fails because its attempt to create the reader thread with >> > pthread_create() fails. This is due to insufficient stack space size >> > given the requirements for thread-local storage usage in the applications >> > and libraries that are linked against libxenstore. [1,2,3,4]. >> > >> > Specifying the stack size appears to have been added to reduce memory >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). >> >> Ugh. 8MB. > > OOI isn't that 8MB virtual memory, which means it shouldn't have real > impact unless it is used? > From what I understand, that is correct. At least in the Linux/glibc case, I believe the stack is allocated using anonymous mmap() and that resident memory usage shouldn't be greater than what you actually end up writing. However, I do not know if this holds true universally... ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: > On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > > From: Chris Patterson > > > > xs_watch() creates a thread to listen to xenstore events. Currently, the > > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. > > > > There have been several bug reports and workarounds related to the issue > > where xs_watch() fails because its attempt to create the reader thread with > > pthread_create() fails. This is due to insufficient stack space size > > given the requirements for thread-local storage usage in the applications > > and libraries that are linked against libxenstore. [1,2,3,4]. > > > > Specifying the stack size appears to have been added to reduce memory > > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). > > Ugh. 8MB. OOI isn't that 8MB virtual memory, which means it shouldn't have real impact unless it is used? Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > From: Chris Patterson > > xs_watch() creates a thread to listen to xenstore events. Currently, the > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. > > There have been several bug reports and workarounds related to the issue > where xs_watch() fails because its attempt to create the reader thread with > pthread_create() fails. This is due to insufficient stack space size > given the requirements for thread-local storage usage in the applications > and libraries that are linked against libxenstore. [1,2,3,4]. > > Specifying the stack size appears to have been added to reduce memory > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). Ugh. 8MB. > > This has already been bumped up once to the greater of 16k and > PTHREAD_STACK_MIN (da6a0e86d6a079102abdd0858a19f1e1fae584fc). And that was too low (2048). CC-ing Roger. > > This patch reverts to sticking with the system's default stack size and > removes the code used to set the thread's stack size. Which brings effectively reverts 1d00c73b983b09fbee4d9dc0f58f6663c361c345 CC-ing Simon to see what was the drive behind the memory consumption. As in whether this was a real problem or they saw an issue with a large stack size. And whether (if this is XenServer product related) they could recompile pthread library to a have smaller pthread default (instead of 8MB say something like 16Kb). > > Of course, the alternative is to bump it to another arbitrary value, but the > requirements may change depending on the application and its libraries that > are linking against xenstore. > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html > [2] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00012.html > [3] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00085.html > [4] https://bugzilla.redhat.com/show_bug.cgi?id=1350264 Thanks for providing the pointers! > > Signed-off-by: Chris Patterson > --- > tools/xenstore/xs.c | 10 -- > 1 file changed, 10 deletions(-) > > diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c > index d1e01ba..c62b120 100644 > --- a/tools/xenstore/xs.c > +++ b/tools/xenstore/xs.c > @@ -723,11 +723,6 @@ bool xs_watch(struct xs_handle *h, const char *path, > const char *token) > struct iovec iov[2]; > > #ifdef USE_PTHREAD > -#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); > if (!h->read_thr_exists) { > @@ -738,11 +733,6 @@ bool xs_watch(struct xs_handle *h, const char *path, > const char *token) > mutex_unlock(&h->request_mutex); > return false; > } > - if (pthread_attr_setstacksize(&attr, READ_THREAD_STACKSIZE) != > 0) { > - pthread_attr_destroy(&attr); > - mutex_unlock(&h->request_mutex); > - return false; > - } > > sigfillset(&set); > pthread_sigmask(SIG_SETMASK, &set, &old_set); > -- > 2.1.4 > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
From: Chris Patterson xs_watch() creates a thread to listen to xenstore events. Currently, the thread is created with the greater of 16K or PTHREAD_MIN_SIZE. There have been several bug reports and workarounds related to the issue where xs_watch() fails because its attempt to create the reader thread with pthread_create() fails. This is due to insufficient stack space size given the requirements for thread-local storage usage in the applications and libraries that are linked against libxenstore. [1,2,3,4]. Specifying the stack size appears to have been added to reduce memory footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). This has already been bumped up once to the greater of 16k and PTHREAD_STACK_MIN (da6a0e86d6a079102abdd0858a19f1e1fae584fc). This patch reverts to sticking with the system's default stack size and removes the code used to set the thread's stack size. Of course, the alternative is to bump it to another arbitrary value, but the requirements may change depending on the application and its libraries that are linking against xenstore. [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html [2] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00012.html [3] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00085.html [4] https://bugzilla.redhat.com/show_bug.cgi?id=1350264 Signed-off-by: Chris Patterson --- tools/xenstore/xs.c | 10 -- 1 file changed, 10 deletions(-) diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c index d1e01ba..c62b120 100644 --- a/tools/xenstore/xs.c +++ b/tools/xenstore/xs.c @@ -723,11 +723,6 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token) struct iovec iov[2]; #ifdef USE_PTHREAD -#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); if (!h->read_thr_exists) { @@ -738,11 +733,6 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token) mutex_unlock(&h->request_mutex); return false; } - if (pthread_attr_setstacksize(&attr, READ_THREAD_STACKSIZE) != 0) { - pthread_attr_destroy(&attr); - mutex_unlock(&h->request_mutex); - return false; - } sigfillset(&set); pthread_sigmask(SIG_SETMASK, &set, &old_set); -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel