Date: Wed, 12 Apr 2017 13:17:19 -0400 From: Jung-uk Kim <jkim@FreeBSD.org> To: Daniel Eischen <deischen@freebsd.org>, Eric van Gyzen <eric@vangyzen.net> Cc: redraiment@gmail.com, Konstantin Belousov <kostikbel@gmail.com>, freebsd-java@freebsd.org Subject: Re: OpenJDK8 Thread.sleep will deadlock while turn down system date time. Message-ID: <2349193f-568b-37f1-dcc5-7e0002046325@FreeBSD.org> In-Reply-To: <Pine.GSO.4.64.1704111923550.21979@sea.ntplx.net> References: <CAPRzLQSOfySJWqN8CoLNchRs_JgHkeQz57ZNB9E__Meip3zmOQ@mail.gmail.com> <20170408070340.GD1788@kib.kiev.ua> <a48038a5-3f12-f7ef-9e94-6c33b4672c02@vangyzen.net> <Pine.GSO.4.64.1704111923550.21979@sea.ntplx.net>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --cV6uTUg3GVSRShqXNSgmnjFCclB7rnd38 Content-Type: multipart/mixed; boundary="JqwANXKv1kEpbKlP8nB59T9AmqT6F2xU5"; protected-headers="v1" From: Jung-uk Kim <jkim@FreeBSD.org> To: Daniel Eischen <deischen@freebsd.org>, Eric van Gyzen <eric@vangyzen.net> Cc: redraiment@gmail.com, Konstantin Belousov <kostikbel@gmail.com>, freebsd-java@freebsd.org Message-ID: <2349193f-568b-37f1-dcc5-7e0002046325@FreeBSD.org> Subject: Re: OpenJDK8 Thread.sleep will deadlock while turn down system date time. References: <CAPRzLQSOfySJWqN8CoLNchRs_JgHkeQz57ZNB9E__Meip3zmOQ@mail.gmail.com> <20170408070340.GD1788@kib.kiev.ua> <a48038a5-3f12-f7ef-9e94-6c33b4672c02@vangyzen.net> <Pine.GSO.4.64.1704111923550.21979@sea.ntplx.net> In-Reply-To: <Pine.GSO.4.64.1704111923550.21979@sea.ntplx.net> --JqwANXKv1kEpbKlP8nB59T9AmqT6F2xU5 Content-Type: multipart/mixed; boundary="------------3CF86F02B7ADBD7F1FAAA726" Content-Language: en-GB This is a multi-part message in MIME format. --------------3CF86F02B7ADBD7F1FAAA726 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/11/2017 19:55, Daniel Eischen wrote: > On Tue, 11 Apr 2017, Eric van Gyzen wrote: >=20 >> On 4/8/17 2:03 AM, Konstantin Belousov wrote: > [ ... ] >>> >>> If JVM sets timeouts using absolute times than it might be fixed in >>> latest >>> HEAD and stable/11. >> >> The JVM uses pthread_cond_timedwait() to implement Thread.sleep(), so >> it always uses absolute times. Furthermore, it uses the default >> clock, CLOCK_REALTIME. My recent change (r315280) does not fix this >> behavior; in fact, I believe it will make the problem worse, since >> moving the clock forward will wake the thread prematurely. >> >> I think the JVM should be fixed to use CLOCK_MONOTONIC. Would someone= >> like to do the research to determine the correct behavior of >> Thread.sleep()? Specifically, should the duration of the sleep be >> affected by adjustments to the real-time clock? I expect that it >> should /not/ be affected, especially since the API takes a >> relative/interval time. Ideally, we would find the answer in the >> formal language specification; failing that, I would be satisfied with= >> empirical data from testing on Windows, Linux, MacOS, and Solaris.=20 >> I'll be happy to write a patch once we know what it should do. >> >> Please keep me CC'd, since I'm not on freebsd-java@. (Thanks for the >> CC, Kostik.) >=20 > It seems CLOCK_REALTIME behavior has bugged Linux in the past, too. I > think using CLOCK_MONOTONIC, perhaps via pthread_condattr_setclock() in= > our JVM, would be the better approach. http://bugs.java.com/view_bug.do?bug_id=3D6900441 http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/rev/2e6938dd68f2 Basically, it was fixed in Linux source but it wasn't merged to ours somehow. Please try the attached patch. > You'd probably also want to check that whatever java.util.concurrent > relies on, if different, also behaves similarly (monotonically). Runtime behaviour depends on HotSpot implementation. Jung-uk Kim --------------3CF86F02B7ADBD7F1FAAA726 Content-Type: text/x-patch; name="openjdk8.diff" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="openjdk8.diff" Index: java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__bsd.cpp =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__bsd.cpp (nonexist= ent) +++ java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__bsd.cpp (working = copy) @@ -0,0 +1,245 @@ +--- hotspot/src/os/bsd/vm/os_bsd.cpp.orig 2017-04-12 17:03:59 UTC ++++ hotspot/src/os/bsd/vm/os_bsd.cpp +@@ -155,6 +155,7 @@ int (*os::Bsd::_getcpuclockid)(pthread_t + #endif + pthread_t os::Bsd::_main_thread; + int os::Bsd::_page_size =3D -1; ++pthread_condattr_t os::Bsd::_condattr[1]; +=20 + static jlong initial_time_count=3D0; +=20 +@@ -1076,12 +1077,15 @@ void os::Bsd::clock_init() { + void os::Bsd::clock_init() { + struct timespec res; + struct timespec tp; ++ _getcpuclockid =3D (int (*)(pthread_t, clockid_t *))dlsym(RTLD_DEFAUL= T, "pthread_getcpuclockid"); + if (::clock_getres(CLOCK_MONOTONIC, &res) =3D=3D 0 && + ::clock_gettime(CLOCK_MONOTONIC, &tp) =3D=3D 0) { + // yes, monotonic clock is supported + _clock_gettime =3D ::clock_gettime; ++ return; + } +- _getcpuclockid =3D (int (*)(pthread_t, clockid_t *))dlsym(RTLD_DEFAUL= T, "pthread_getcpuclockid"); ++ warning("No monotonic clock was available - timed services may " \ ++ "be adversely affected if the time-of-day clock changes"); + } + #endif +=20 +@@ -1117,7 +1121,7 @@ jlong os::javaTimeNanos() { + jlong os::javaTimeNanos() { + if (Bsd::supports_monotonic_clock()) { + struct timespec tp; +- int status =3D Bsd::_clock_gettime(CLOCK_MONOTONIC, &tp); ++ int status =3D ::clock_gettime(CLOCK_MONOTONIC, &tp); + assert(status =3D=3D 0, "gettime error"); + jlong result =3D jlong(tp.tv_sec) * (1000 * 1000 * 1000) + jlong(tp= =2Etv_nsec); + return result; +@@ -3721,6 +3725,25 @@ void os::init(void) { + Bsd::clock_init(); + initial_time_count =3D javaTimeNanos(); +=20 ++ // pthread_condattr initialization for monotonic clock ++ int status; ++ pthread_condattr_t* _condattr =3D os::Bsd::condAttr(); ++ if ((status =3D pthread_condattr_init(_condattr)) !=3D 0) { ++ fatal(err_msg("pthread_condattr_init: %s", strerror(status))); ++ } ++ // Only set the clock if CLOCK_MONOTONIC is available ++ if (Bsd::supports_monotonic_clock()) { ++ if ((status =3D pthread_condattr_setclock(_condattr, CLOCK_MONOTONI= C)) !=3D 0) { ++ if (status =3D=3D EINVAL) { ++ warning("Unable to use monotonic clock with relative timed-wait= s" \ ++ " - changes to the time-of-day clock may have adverse a= ffects"); ++ } else { ++ fatal(err_msg("pthread_condattr_setclock: %s", strerror(status)= )); ++ } ++ } ++ } ++ // else it defaults to CLOCK_REALTIME ++ + #ifdef __APPLE__ + // XXXDARWIN + // Work around the unaligned VM callbacks in hotspot's +@@ -4483,21 +4506,36 @@ void os::pause() { +=20 + static struct timespec* compute_abstime(struct timespec* abstime, jlong= millis) { + if (millis < 0) millis =3D 0; +- struct timeval now; +- int status =3D gettimeofday(&now, NULL); +- assert(status =3D=3D 0, "gettimeofday"); ++ + jlong seconds =3D millis / 1000; + millis %=3D 1000; + if (seconds > 50000000) { // see man cond_timedwait(3T) + seconds =3D 50000000; + } +- abstime->tv_sec =3D now.tv_sec + seconds; +- long usec =3D now.tv_usec + millis * 1000; +- if (usec >=3D 1000000) { +- abstime->tv_sec +=3D 1; +- usec -=3D 1000000; ++ ++ if (os::Bsd::supports_monotonic_clock()) { ++ struct timespec now; ++ int status =3D ::clock_gettime(CLOCK_MONOTONIC, &now); ++ assert_status(status =3D=3D 0, status, "clock_gettime"); ++ abstime->tv_sec =3D now.tv_sec + seconds; ++ long nanos =3D now.tv_nsec + millis * NANOSECS_PER_MILLISEC; ++ if (nanos >=3D NANOSECS_PER_SEC) { ++ abstime->tv_sec +=3D 1; ++ nanos -=3D NANOSECS_PER_SEC; ++ } ++ abstime->tv_nsec =3D nanos; ++ } else { ++ struct timeval now; ++ int status =3D gettimeofday(&now, NULL); ++ assert(status =3D=3D 0, "gettimeofday"); ++ abstime->tv_sec =3D now.tv_sec + seconds; ++ long usec =3D now.tv_usec + millis * 1000; ++ if (usec >=3D 1000000) { ++ abstime->tv_sec +=3D 1; ++ usec -=3D 1000000; ++ } ++ abstime->tv_nsec =3D usec * 1000; + } +- abstime->tv_nsec =3D usec * 1000; + return abstime; + } +=20 +@@ -4589,7 +4627,7 @@ int os::PlatformEvent::park(jlong millis + status =3D os::Bsd::safe_cond_timedwait(_cond, _mutex, &abst); + if (status !=3D 0 && WorkAroundNPTLTimedWaitHang) { + pthread_cond_destroy (_cond); +- pthread_cond_init (_cond, NULL) ; ++ pthread_cond_init (_cond, os::Bsd::condAttr()) ; + } + assert_status(status =3D=3D 0 || status =3D=3D EINTR || + status =3D=3D ETIMEDOUT, +@@ -4690,32 +4728,50 @@ void os::PlatformEvent::unpark() { +=20 + static void unpackTime(struct timespec* absTime, bool isAbsolute, jlong= time) { + assert (time > 0, "convertTime"); ++ time_t max_secs =3D 0; +=20 +- struct timeval now; +- int status =3D gettimeofday(&now, NULL); +- assert(status =3D=3D 0, "gettimeofday"); ++ if (!os::Bsd::supports_monotonic_clock() || isAbsolute) { ++ struct timeval now; ++ int status =3D gettimeofday(&now, NULL); ++ assert(status =3D=3D 0, "gettimeofday"); +=20 +- time_t max_secs =3D now.tv_sec + MAX_SECS; ++ max_secs =3D now.tv_sec + MAX_SECS; +=20 +- if (isAbsolute) { +- jlong secs =3D time / 1000; +- if (secs > max_secs) { +- absTime->tv_sec =3D max_secs; +- } +- else { +- absTime->tv_sec =3D secs; ++ if (isAbsolute) { ++ jlong secs =3D time / 1000; ++ if (secs > max_secs) { ++ absTime->tv_sec =3D max_secs; ++ } else { ++ absTime->tv_sec =3D secs; ++ } ++ absTime->tv_nsec =3D (time % 1000) * NANOSECS_PER_MILLISEC; ++ } else { ++ jlong secs =3D time / NANOSECS_PER_SEC; ++ if (secs >=3D MAX_SECS) { ++ absTime->tv_sec =3D max_secs; ++ absTime->tv_nsec =3D 0; ++ } else { ++ absTime->tv_sec =3D now.tv_sec + secs; ++ absTime->tv_nsec =3D (time % NANOSECS_PER_SEC) + now.tv_usec*10= 00; ++ if (absTime->tv_nsec >=3D NANOSECS_PER_SEC) { ++ absTime->tv_nsec -=3D NANOSECS_PER_SEC; ++ ++absTime->tv_sec; // note: this must be <=3D max_secs ++ } ++ } + } +- absTime->tv_nsec =3D (time % 1000) * NANOSECS_PER_MILLISEC; +- } +- else { ++ } else { ++ // must be relative using monotonic clock ++ struct timespec now; ++ int status =3D ::clock_gettime(CLOCK_MONOTONIC, &now); ++ assert_status(status =3D=3D 0, status, "clock_gettime"); ++ max_secs =3D now.tv_sec + MAX_SECS; + jlong secs =3D time / NANOSECS_PER_SEC; + if (secs >=3D MAX_SECS) { + absTime->tv_sec =3D max_secs; + absTime->tv_nsec =3D 0; +- } +- else { ++ } else { + absTime->tv_sec =3D now.tv_sec + secs; +- absTime->tv_nsec =3D (time % NANOSECS_PER_SEC) + now.tv_usec*1000= ; ++ absTime->tv_nsec =3D (time % NANOSECS_PER_SEC) + now.tv_nsec; + if (absTime->tv_nsec >=3D NANOSECS_PER_SEC) { + absTime->tv_nsec -=3D NANOSECS_PER_SEC; + ++absTime->tv_sec; // note: this must be <=3D max_secs +@@ -4795,15 +4851,19 @@ void Parker::park(bool isAbsolute, jlong + jt->set_suspend_equivalent(); + // cleared by handle_special_suspend_equivalent_condition() or java_s= uspend_self() +=20 ++ assert(_cur_index =3D=3D -1, "invariant"); + if (time =3D=3D 0) { +- status =3D pthread_cond_wait (_cond, _mutex) ; ++ _cur_index =3D REL_INDEX; // arbitrary choice when not timed ++ status =3D pthread_cond_wait (&_cond[_cur_index], _mutex) ; + } else { +- status =3D os::Bsd::safe_cond_timedwait (_cond, _mutex, &absTime) ;= ++ _cur_index =3D isAbsolute ? ABS_INDEX : REL_INDEX; ++ status =3D os::Bsd::safe_cond_timedwait (&_cond[_cur_index], _mutex= , &absTime) ; + if (status !=3D 0 && WorkAroundNPTLTimedWaitHang) { +- pthread_cond_destroy (_cond) ; +- pthread_cond_init (_cond, NULL); ++ pthread_cond_destroy (&_cond[_cur_index]) ; ++ pthread_cond_init (&_cond[_cur_index], isAbsolute ? NULL : os:= :Bsd::condAttr()); + } + } ++ _cur_index =3D -1; + assert_status(status =3D=3D 0 || status =3D=3D EINTR || + status =3D=3D ETIMEDOUT, + status, "cond_timedwait"); +@@ -4832,17 +4892,26 @@ void Parker::unpark() { + s =3D _counter; + _counter =3D 1; + if (s < 1) { +- if (WorkAroundNPTLTimedWaitHang) { +- status =3D pthread_cond_signal (_cond) ; +- assert (status =3D=3D 0, "invariant") ; ++ // thread might be parked ++ if (_cur_index !=3D -1) { ++ // thread is definitely parked ++ if (WorkAroundNPTLTimedWaitHang) { ++ status =3D pthread_cond_signal (&_cond[_cur_index]); ++ assert (status =3D=3D 0, "invariant"); + status =3D pthread_mutex_unlock(_mutex); +- assert (status =3D=3D 0, "invariant") ; +- } else { ++ assert (status =3D=3D 0, "invariant"); ++ } else { ++ // must capture correct index before unlocking ++ int index =3D _cur_index; + status =3D pthread_mutex_unlock(_mutex); +- assert (status =3D=3D 0, "invariant") ; +- status =3D pthread_cond_signal (_cond) ; +- assert (status =3D=3D 0, "invariant") ; +- } ++ assert (status =3D=3D 0, "invariant"); ++ status =3D pthread_cond_signal (&_cond[index]); ++ assert (status =3D=3D 0, "invariant"); ++ } ++ } else { ++ pthread_mutex_unlock(_mutex); ++ assert (status =3D=3D 0, "invariant") ; ++ } + } else { + pthread_mutex_unlock(_mutex); + assert (status =3D=3D 0, "invariant") ; Property changes on: java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__= bsd.cpp ___________________________________________________________________ Added: fbsd:nokeywords ## -0,0 +1 ## +yes \ No newline at end of property Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Added: svn:mime-type ## -0,0 +1 ## +text/plain \ No newline at end of property Index: java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__bsd.hpp =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__bsd.hpp (nonexist= ent) +++ java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__bsd.hpp (working = copy) @@ -0,0 +1,63 @@ +--- hotspot/src/os/bsd/vm/os_bsd.hpp.orig 2017-04-12 15:33:50 UTC ++++ hotspot/src/os/bsd/vm/os_bsd.hpp +@@ -155,6 +155,13 @@ class Bsd { + #endif + } +=20 ++ // pthread_cond clock suppport ++ private: ++ static pthread_condattr_t _condattr[1]; ++ ++ public: ++ static pthread_condattr_t* condAttr() { return _condattr; } ++ + // Stack repair handling +=20 + // none present +@@ -220,7 +227,7 @@ class PlatformEvent : public CHeapObj<mt + public: + PlatformEvent() { + int status; +- status =3D pthread_cond_init (_cond, NULL); ++ status =3D pthread_cond_init (_cond, os::Bsd::condAttr()); + assert_status(status =3D=3D 0, status, "cond_init"); + status =3D pthread_mutex_init (_mutex, NULL); + assert_status(status =3D=3D 0, status, "mutex_init"); +@@ -235,14 +242,19 @@ class PlatformEvent : public CHeapObj<mt + void park () ; + void unpark () ; + int TryPark () ; +- int park (jlong millis) ; ++ int park (jlong millis) ; // relative timed-wait only + void SetAssociation (Thread * a) { _Assoc =3D a ; } + }; +=20 + class PlatformParker : public CHeapObj<mtInternal> { + protected: ++ enum { ++ REL_INDEX =3D 0, ++ ABS_INDEX =3D 1 ++ }; ++ int _cur_index; // which cond is in use: -1, 0, 1 + pthread_mutex_t _mutex [1] ; +- pthread_cond_t _cond [1] ; ++ pthread_cond_t _cond [2] ; // one for relative times and one for = abs. +=20 + public: // TODO-FIXME: make dtor private + ~PlatformParker() { guarantee (0, "invariant") ; } +@@ -250,10 +262,13 @@ class PlatformParker : public CHeapObj<m + public: + PlatformParker() { + int status; +- status =3D pthread_cond_init (_cond, NULL); +- assert_status(status =3D=3D 0, status, "cond_init"); ++ status =3D pthread_cond_init (&_cond[REL_INDEX], os::Bsd::condAtt= r()); ++ assert_status(status =3D=3D 0, status, "cond_init rel"); ++ status =3D pthread_cond_init (&_cond[ABS_INDEX], NULL); ++ assert_status(status =3D=3D 0, status, "cond_init abs"); + status =3D pthread_mutex_init (_mutex, NULL); + assert_status(status =3D=3D 0, status, "mutex_init"); ++ _cur_index =3D -1; // mark as unused + } + }; +=20 Property changes on: java/openjdk8/files/patch-hotspot_src_os_bsd_vm_os__= bsd.hpp ___________________________________________________________________ Added: fbsd:nokeywords ## -0,0 +1 ## +yes \ No newline at end of property Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Added: svn:mime-type ## -0,0 +1 ## +text/plain \ No newline at end of property --------------3CF86F02B7ADBD7F1FAAA726-- --JqwANXKv1kEpbKlP8nB59T9AmqT6F2xU5-- --cV6uTUg3GVSRShqXNSgmnjFCclB7rnd38 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEl1bqgKaRyqfWXu/CfJ+WJvzb8UYFAljuYS0ACgkQfJ+WJvzb 8UaBoAgAnJQA16ACwCj6UsBiXMJBbyUkuKK/bBULiSkQTJInC0stZCbo3PEXBBMB DKbqWlvmXmJMSRK07pqjjPd61SR/dxCrcjFjioykQHinRdgwiYBLb5b6I+2wtCcn MVFfuOWfCpYlTQ8w/hn8L0KB6bJCh4pqqbmYrwrHGJMfPsXoItQ6n+/JuT2rd6Gu KBpo5obBp/pU5c5g3AcCukQr8alt/TAuihjwz23OtaaTHLDMFavnCwuF4myyRh2G idy7CTDvg8hGe4gvIPg/HWutvfh+sl5o1nxIiWO+UPijpNw9H9ifoSwBEnh0dlic 2pXCvuOasYDZE4fWQtXDnUaYnxtytw== =Pl7V -----END PGP SIGNATURE----- --cV6uTUg3GVSRShqXNSgmnjFCclB7rnd38--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2349193f-568b-37f1-dcc5-7e0002046325>