RFR: 8225035: Thread stack size issue caused by large TLS size

[Jiangli Zhou]

Since the approach of using a simple on/off switch to control the
stack size adjustment seems to be the cleanest, I've updated the CSR
and webrev to go with that. Please help review both of the following:

CSR: https://bugs.openjdk.java.net/browse/JDK-8225498
webrev: http://cr.openjdk.java.net/~jiangli/8225035/webrev.01/

I've also summarized all approaches brought up in the mailing list
discussion and added to the comment section in
https://bugs.openjdk.java.net/browse/JDK-8225035.

A note on passing a dummy 'attr' to __pthread_get_minstack (regarding
Thomas' comment): I've rechecked with that, and did get the correct
minstack size. I think it's safer to pass a valid 'attr' however.


Best regards,
Jiangli

On Wed, Jun 19, 2019 at 4:50 PM Jiangli Zhou <jianglizhou at google.com> wrote:
>
> Hi Thomas,
>
> Thank you for taking the time from your vacation to give the feedback! :)
>
> On Wed, Jun 19, 2019 at 12:03 AM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> >
> > Hi Jiangli and everyone,
> >
> > On Tue, Jun 18, 2019 at 12:41 AM Jiangli Zhou <jianglizhou at google.com> wrote:
> >>
> >> Hi Andrew and Florian,
> >>
> >> With the stack size needs to be aligned at page boundary, it seems that the effect of using large page size is the same for the following two approaches:
> >>
> >> Using a JVM knob which specifies a minimum stack size for stack size adjustment (suggested by Florian)
> >> Using the value obtained from __pthread_get_minstack for stack size adjustment
> >
> > Are you sure? The snippet Florian posted seemed to indicate that the page size factors into the result of __pthread_minstack. Something along the line of "tls_size + x", where x depends on page size? If this is true, for larger page sizes x would dwarf tls_size, which was my original fear - that the size __pthread_get_minstack returns would be unnecessarily large on those platforms.
>
>
> The user driven option (option 1) may cause more memory waste if an
> improper size (too large) is set, and it's not a no-op task for user
> to obtain the correct size information (in some cases it might be
> difficult). The use of __pthread_get_minstack adds extra page(s)
> (depending on the version of the implementation) in addition to the
> '__static_tls_size' (that's why the original patch went with
> _dl_get_tls_static_info()). However, it doesn't seem to make the
> solution worse than the first option, given that the stack size
> adjustment is only enabled with a command-line option for cases where
> the actual stack space usage is already large.
>
> When factoring in the consideration for the large page sizes, option 1
> may complicate things even more as it relies on user to figure out the
> size for different environments.
>
> >
> >
> > Note that I am currently in vacation and have no means to check this.
> >
> >> The difference is that the second approach gets the adjustment value without guessing, which seems to be more user friendly. What are your thoughts on that? If there is anything that I'm missing, please correct.
> >>
> >> After thinking more, it seems that the simple approach with an on/off switch (suggested by David initially) that adjusts all threads' stack size (for the on-stack TLS blocks) when the switch is enabled probably is better. Any objections?
> >>
> >> Thanks a lot for all the feedback and suggestions!!
> >>
> >> Best regards,
> >>
> >> Jiangli
> >
> >
> > I can see the merits of both solutions but lean toward (1), for similar reasons Florian and others gave:
> >
> > + it would work for hypothetical non-TLS related cases
> > + it is simple(r) to understand
> > + it does not rely on the workings of an undocumented glibc function. I still have not understood what __pthread_get_minstack really returns - some sort of minimum stack size, yes, but what does that guarantee, exactly?
> >
> > Open Question: would that a Linux only switch? If we move away from the its-for-TLS reasoning, maybe this should be made platform independent?
> >
> > Disadvantages of (1): we now have a second thread size switch beside Xss, which can be confusing and needs explaining.
> >
> >  -Xss defines thread stack size unless it gets overruled by an explicit size passed to the j.l.Thread constructor or unless it is an internal JVM thread. The new  -XX:MinimumStackSize switch would work for all threads. If both are specified and -Xss < -XX:MinimumStackSize, latter overrides former which may be surprising.
>
> Thanks for the summarize!
>
> In addition to the disadvantages listed above, here are my main
> concerns (I've raised some of them previously) with a user provided
> size adjustment solution:
>
> 1) It shifts burdens to the users, examples below
>     - users need to figure out the different sizes for different environments
>     - need to readjust the size when migrating to a newer glibc or
> kernel version?
> 2) What is the proper size we can recommend/advise the Java users to use?
> 3) Without knowing all the specific details for the other non-TLS
> related cases, I'm lacking sufficient confidence to enlarge the
> current scope. There are too many unknowns to go with a general
> solution.
>
> I'm a huge fan of providing user-friend solutions (ease of use). :)
>
> Best regards,
> Jiangli
>
> >
> >  Cheers, Thomas