RFR(S) 8187040: ThreadCritical crashes on Solaris if used between os::init and os::init_2

[Thomas Stüfe]

Hi Mikael,

On Mon, Sep 4, 2017 at 9:26 AM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:

> Hi Thomas,
>
> On 2017-09-01 19:31, Thomas Stüfe wrote:
>
>> Hi Mikael,
>>
>> (I never understood why we cannot just use pthread mutexes on Solaris.
>> Why all this dynamic loading magic, are pthread functions not available on
>> all Solaris versions?)
>>
>
> Beats me, but I suspect that David is right. At some point someone thought
> one was better and now nobody remembers how and nobody wants to put in the
> performance work to determine if it actually makes a difference.
>
>
>> Small nit, instead of adding a new variable _synchronization_initialized,
>> how about _mutex_lock != NULL (in ThreadCritical()) and _mutex_unlock !=
>> NULL (in ~ThreadCritical())?
>>
>
> I didn't want to expose the function pointers through accessors in
> os::Solaris and I'm worried that if we check a different thing in the lock
> versus unlock paths we can end up with a ThreadCritical which tries to
> unlock a lock which was never locked (because the TC was created before
> _mutex_lock was set).
> Also, I think it's clearer to the reader of os::Solaris::synchronization_init
> that the "initialization completed" state is exposed to an external caller.
>
>
That sounds reasonable.


>
>
>
>> I am okay with the removal of ::release(). Even if it were used, it is
>> really safer to let the mutex live until process end.
>>
>
> I agree.
>
>
>> I leave it up to you if you take my suggestion above. The patch is fine
>> for me in the current form.
>>
>
> Thanks for the review, Thomas.
> /Mikael
>
>
Sure!

..Thomas


>
>> Kind Regards, Thomas
>>
>>
>> On Fri, Sep 1, 2017 at 5:23 PM, Mikael Gerdin <mikael.gerdin at oracle.com
>> <mailto:mikael.gerdin at oracle.com>> wrote:
>>
>>     Hi,
>>
>>     Please review this small fix to ThreadCritical.
>>     When working on a piece of code which allocates memory early on I
>>     noticed that it crashed if I enabled NMT.
>>     The reason is that NMT uses ThreadCritical and os::Solaris sets the
>>     ThreadCritical::_initialized flag before it actually sets up the
>>     function pointers the flag is supposed to guard.
>>     os::Solaris::_mutex_lock is not initialized until the init_2 phase
>>     (after command line flag parsing).
>>
>>     My suggested fix is to replace the current short-circuit of
>>     ThreadCritical with a flag set when the Solaris mutex code is
>>     initialized and thereby getting rid of the initialize function on
>>     all platforms.
>>     Additionally, ThreadCritical::release is unreachable code and from
>>     my research has never actually been called, we might as well get rid
>>     of it.
>>
>>     Webrev: http://cr.openjdk.java.net/~mgerdin/8187040/webrev.0/
>>     <http://cr.openjdk.java.net/~mgerdin/8187040/webrev.0/>
>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8187040
>>     <https://bugs.openjdk.java.net/browse/JDK-8187040>
>>     Testing: JPRT
>>
>>     Thanks
>>     /Mikael
>>
>>
>>