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>
---
Changed since v1:
* remove test for _POSIX_THREAD_ATTR_STACKSIZE
* use define for constant
diff -r 52ffce7a036e -r 2d0a29ab3f91 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c Tue May 29 11:36:36 2012 +0100
+++ b/tools/xenstore/xs.c Wed May 30 13:41:07 2012 +0100
@@ -702,14 +702,29 @@ bool xs_watch(struct xs_handle *h, const
struct iovec iov[2];
#ifdef USE_PTHREAD
+#define READ_THREAD_STACKSIZE (16 * 1024)
+
/* We dynamically create a reader thread on demand. */
mutex_lock(&h->request_mutex);
if (!h->read_thr_exists) {
- if (pthread_create(&h->read_thr, NULL, read_thread, h) != 0) {
+ pthread_attr_t attr;
+
+ if (pthread_attr_init(&attr) != 0) {
+ 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;
+ }
+ if (pthread_create(&h->read_thr, &attr, read_thread, h) != 0) {
+ pthread_attr_destroy(&attr);
mutex_unlock(&h->request_mutex);
return false;
}
h->read_thr_exists = 1;
+ pthread_attr_destroy(&attr);
}
mutex_unlock(&h->request_mutex);
#endif
Simon Rowe writes ("[Xen-devel] [PATCH V2] xs: set read_thread
stacksize"):> 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.
Thanks. This seems like a genuine bug but I have one comments about
your fix. The effect of your patch is to make the stacksize of the
libxenstore private thread be small.
However, we do not take any care to avoid signals being delivered to
this thread. This is bad enough at the moment, but with your patch
these signals may now be delivered to a thread with a small stack.
Can I request that you provide another patch too, which would probably
come before this one, which uses pthread_sigprocmask to block all
signals on the private thread ? This is done by changing the signal
mask to all signals blocked and then restoring it afterwards.
Thanks,
Ian.
On Fri, 2012-06-01 at 19:05 +0100, Ian Jackson wrote:> Simon Rowe writes ("[Xen-devel] [PATCH V2] xs: set read_thread stacksize"): > > 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. > > Thanks. This seems like a genuine bug but I have one comments about > your fix. The effect of your patch is to make the stacksize of the > libxenstore private thread be small. > > However, we do not take any care to avoid signals being delivered to > this thread. This is bad enough at the moment, but with your patch > these signals may now be delivered to a thread with a small stack. > > Can I request that you provide another patch too, which would probably > come before this one, which uses pthread_sigprocmask to block all > signals on the private thread ? This is done by changing the signal > mask to all signals blocked and then restoring it afterwards.Do you mean pthread_sigmask or sigprocmask rather than the hybrid pthread_sigprocmask (which appears not to exist). Ian.
Ian Campbell writes ("Re: [Xen-devel] [PATCH V2] xs: set read_thread
stacksize"):> On Fri, 2012-06-01 at 19:05 +0100, Ian Jackson wrote:
> > Can I request that you provide another patch too, which would probably
> > come before this one, which uses pthread_sigprocmask to block all
> > signals on the private thread ? This is done by changing the signal
> > mask to all signals blocked and then restoring it afterwards.
>
> Do you mean pthread_sigmask or sigprocmask rather than the hybrid
> pthread_sigprocmask (which appears not to exist).
I mean pthread_sigmask, of course.
I remembered that they had pointlessly invented a new function name
but not that they''d pointlessly varied the spelling...
Ian.