Re: [PATCH] xs: set read_thread stacksize

[Ian Campbell <Ian.Campbell@citrix.com>]

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.