| Message ID | 1474406979-8909-1-git-send-email-cjp256@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > From: Chris Patterson <pattersonc@ainfosec.com> > > 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 <pattersonc@ainfosec.com> > --- > 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
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 <pattersonc@ainfosec.com> > > > > 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.
On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu <wei.liu2@citrix.com> 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 <pattersonc@ainfosec.com> >> > >> > 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...
On Wed, Sep 21, 2016 at 09:50:30AM -0400, Chris Patterson wrote: > On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu <wei.liu2@citrix.com> 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 <pattersonc@ainfosec.com> > >> > > >> > 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..
On Wed, Sep 21, 2016 at 10:07 AM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 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 <wei.liu2@citrix.com> 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 <pattersonc@ainfosec.com> >> >> > >> >> > 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 <pid>) 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 <num_kb>
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 > <konrad.wilk@oracle.com> 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 <wei.liu2@citrix.com> 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 <pattersonc@ainfosec.com> > >> >> > > >> >> > 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 <pid>) 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 <num_kb> 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.
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 <wei.liu2@citrix.com> 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 <pattersonc@ainfosec.com> > > >> > > > >> > 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.
On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: > From: Chris Patterson <pattersonc@ainfosec.com> > > 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 <pattersonc@ainfosec.com> 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 >
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 <pattersonc@ainfosec.com> >> > >> > 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 <pattersonc@ainfosec.com> > 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
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 <pattersonc@ainfosec.com>
>
> 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.
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);