|
Hi Yasumasa,
On 10/19/17 06:43, Yasumasa Suenaga wrote:
Sorry, I
forgot the fix to use OrderAccess::load_acquire() in
PerfMemory::is_initialized().
I fixed it in new webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.09/
It looks good to me.
Thank you for your patience.
Yasumasa
On 2017/10/19 21:24, David Holmes wrote:
On 19/10/2017 9:44 PM, Yasumasa Suenaga
wrote:
Hi,
I suggest we leave the volatile off
for now and file a RFE to add volatile_static_field support
to VMStructs and update later.
Okay. David or Serguei, could you file it?
I've filed:
https://bugs.openjdk.java.net/browse/JDK-8189685
need PerfMemory class update and a volatile_static_field support
in VMStructs
Feel free to update it if necessary.
Thanks,
Serguei
I'd suggest to fall back to your
previous approach as synchronization was not there
in the first place, and it is not a part of the original
issue you are trying to fix
(if David or anyone else does not a simple solution).
I don't think trying to introduce
locking would be a good idea as it would likely lead to
deadlocks when a crash occurs. This could also be
investigated as a future RFE if desired.
Sorry, I have mistake the spell of "usable".
I've fixed it in new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.08/
Can I list David and Serguei as Reviewer?
I will send a changeset to Serguei if it can.
Yes.
Thanks,
David
Thanks,
Yasumasa
On 2017/10/19 19:41, David Holmes wrote:
Hi Serguei, Yasumasa,
I suggest we leave the volatile off for now and file a RFE
to add volatile_static_field support to VMStructs and update
later.
I don't think trying to introduce locking would be a good
idea as it would likely lead to deadlocks when a crash
occurs. This could also be investigated as a future RFE if
desired.
Thanks,
David
On 19/10/2017 7:37 PM, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
I see the problem.
As it occurred making these variables volatile is
non-trivial.
But thank you a lot for trying!
I'd suggest to fall back to your previous approach as
synchronization was not there
in the first place, and it is not a part of the original
issue you are trying to fix
(if David or anyone else does not a simple solution).
But let's check if David does not object against it.
I will sponsor your fix after you send me a patch.
Thanks,
Serguei
On 10/18/17 20:21, Yasumasa Suenaga wrote:
Sorry, I have mistake.
But I cannot compile yet:
diff -r 3e7702cd3f19
src/hotspot/share/runtime/vmStructs.cpp
--- a/src/hotspot/share/runtime/vmStructs.cpp Thu Sep
07 15:40:20 2017 +0200
+++ b/src/hotspot/share/runtime/vmStructs.cpp Thu Oct
19 12:21:11 2017 +0900
@@ -578,7 +578,7 @@
static_field(PerfMemory, _top,
char*) \
static_field(PerfMemory, _capacity,
size_t) \
static_field(PerfMemory, _prologue,
PerfDataPrologue*) \
- static_field(PerfMemory, _initialized,
jint) \
+ static_field(PerfMemory,
_initialized, volatile
jint) \
--------------
In file included from
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:104:0:
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:58:
error: invalid conversion from 'volatile void*' to
'void*' [-fpermissive]
{ QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1,
0, &typeName::fieldName },
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:6:
note: in expansion of macro
'GENERATE_STATIC_VM_STRUCT_ENTRY'
static_field(PerfMemory,
_initialized, volatile
jint) \
^~~~~~~~~~~~
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3:
note: in expansion of macro 'VM_STRUCTS'
VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
^
gmake[3]: *** [lib/CompileJvm.gmk:210:
/home/ysuenaga/OpenJDK/jdk10-hs/build/linux-x86_64-normal-server-fastdebug/hotspot/variant-server/libjvm/objs/vmStructs.o]
Error 1
gmake[2]: *** [make/Main.gmk:266: hotspot-server-libs]
Error 2
ERROR: Build failed for target 'images' in configuration
'linux-x86_64-normal-server-fastdebug' (exit code 2)
--------------
On 2017/10/19 12:18, Yasumasa Suenaga wrote:
Hi Serguei,
Would the below work? :
578 static_field(PerfMemory, _initialized,
volatile jint) \
It'd be similar to this non-static case:
362 nonstatic_field(ConstantPoolCacheEntry,
_f1, volatile
Metadata*) \
I got error messages as below:
---------------
In file included from
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:104:0:
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:39:
error: expected unqualified-id before 'volatile'
static_field(PerfMemory, volatile
_initialized, jint) \
^
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:69:
note: in definition of macro
'GENERATE_STATIC_VM_STRUCT_ENTRY'
{ QUOTE(typeName), QUOTE(fieldName), QUOTE(type),
1, 0, &typeName::fieldName },
^~~~~~~~~
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3:
note: in expansion of macro 'VM_STRUCTS'
VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
^
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:39:
error: expected '}' before 'volatile'
static_field(PerfMemory, volatile
_initialized, jint) \
^
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:69:
note: in definition of macro
'GENERATE_STATIC_VM_STRUCT_ENTRY'
{ QUOTE(typeName), QUOTE(fieldName), QUOTE(type),
1, 0, &typeName::fieldName },
^~~~~~~~~
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3:
note: in expansion of macro 'VM_STRUCTS'
VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
^
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:39:
error: expected '}' before 'volatile'
static_field(PerfMemory, volatile
_initialized, jint) \
^
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:69:
note: in definition of macro
'GENERATE_STATIC_VM_STRUCT_ENTRY'
{ QUOTE(typeName), QUOTE(fieldName), QUOTE(type),
1, 0, &typeName::fieldName },
^~~~~~~~~
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3:
note: in expansion of macro 'VM_STRUCTS'
VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
^
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:79:
error: expected declaration before '}' token
{ QUOTE(typeName), QUOTE(fieldName), QUOTE(type),
1, 0, &typeName::fieldName },
^
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:6:
note: in expansion of macro
'GENERATE_STATIC_VM_STRUCT_ENTRY'
static_field(PerfMemory, volatile
_initialized, jint) \
^~~~~~~~~~~~
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3:
note: in expansion of macro 'VM_STRUCTS'
VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
^
gmake[3]: *** [lib/CompileJvm.gmk:210:
/home/ysuenaga/OpenJDK/jdk10-hs/build/linux-x86_64-normal-server-fastdebug/hotspot/variant-server/libjvm/objs/vmStructs.o]
Error 1
gmake[2]: *** [make/Main.gmk:266: hotspot-server-libs]
Error 2
ERROR: Build failed for target 'images' in
configuration 'linux-x86_64-normal-server-fastdebug'
(exit code 2)
---------------
I changed as below:
---------------
diff -r 3e7702cd3f19
src/hotspot/share/runtime/perfMemory.cpp
--- a/src/hotspot/share/runtime/perfMemory.cpp Thu
Sep 07 15:40:20 2017 +0200
+++ b/src/hotspot/share/runtime/perfMemory.cpp Thu
Oct 19 12:15:30 2017 +0900
@@ -51,8 +51,9 @@
char* PerfMemory::_end = NULL;
char* PerfMemory::_top = NULL;
size_t PerfMemory::_capacity = 0;
-jint PerfMemory::_initialized =
false;
+volatile jint PerfMemory::_initialized =
0;
PerfDataPrologue* PerfMemory::_prologue =
NULL;
+volatile bool PerfMemory::_destroyed =
false;
--- a/src/hotspot/share/runtime/perfMemory.hpp Thu
Sep 07 15:40:20 2017 +0200
+++ b/src/hotspot/share/runtime/perfMemory.hpp Thu
Oct 19 12:15:30 2017 +0900
@@ -113,13 +113,15 @@
*/
class PerfMemory : AllStatic {
friend class VMStructs;
+ friend class PerfMemoryTest;
private:
static char* _start;
static char* _end;
static char* _top;
static size_t _capacity;
static PerfDataPrologue* _prologue;
- static jint _initialized;
+ static volatile jint _initialized;
+ static volatile bool _destroyed;
diff -r 3e7702cd3f19
src/hotspot/share/runtime/vmStructs.cpp
--- a/src/hotspot/share/runtime/vmStructs.cpp Thu
Sep 07 15:40:20 2017 +0200
+++ b/src/hotspot/share/runtime/vmStructs.cpp Thu
Oct 19 12:15:30 2017 +0900
@@ -578,7 +578,7 @@
static_field(PerfMemory, _top,
char*) \
static_field(PerfMemory, _capacity,
size_t) \
static_field(PerfMemory, _prologue,
PerfDataPrologue*) \
- static_field(PerfMemory, _initialized,
jint) \
+ static_field(PerfMemory, volatile
_initialized, jint) \
---------------
Thanks,
Yasumasa
On 2017/10/19 6:18, serguei.spit...@oracle.com wrote:
On 10/18/17 06:51, Yasumasa
Suenaga wrote:
Hi David, Serguei,
because as soon as we have
checked is_usable() and abort happening in
another thread may have changed that by calling
destroy.
This code is basically broken if we hit an abort
path instead of a normal VM shutdown.
Can we use MutexLocker for initialize() and
destroy() ?
I've tried to fix about your comments, but I have
an issue about volatile.
PerfMemory.java depends on
PerfMemory::_initialized. However VMStructs cannot
handle static volatile variables.
I think two approaches as below:
1. Remove _initialized check from
PerfMemory.java
SA will throw UnmappedAddressException if
JSnap try to access invalid address including
uninitialized memory.
2. Add static volatile support to VMStructs
Which should we do?
1. is easy to fix. But 2. might be right way...
Would the below work? :
578 static_field(PerfMemory, _initialized,
volatile jint) \
It'd be similar to this non-static case:
362 nonstatic_field(ConstantPoolCacheEntry,
_f1, volatile
Metadata*) \
Thanks,
Serguei
Thanks,
Yasumasa
On 2017/10/18 21:34, David Holmes wrote:
Just to clarify ...
On 18/10/2017 10:28 PM, David Holmes wrote:
On 18/10/2017 8:26 PM,
serguei.spit...@oracle.com wrote:
Hi David,
Thank you for jumping to this review and
helping Yasumasa to sort it out!
I've just discovered that this issue was
already on the table for several months
without a significant progress.
On 10/18/17 02:48, David Holmes wrote:
Hi Serguei
On 18/10/2017 7:25 PM,
serguei.spit...@oracle.com wrote:
Hi Yasumasa,
Sorry for a quite late participation.
I looked at the previous webrevs and
think that this one is much better.
Some concern is if we need any kind of
synchronization here, e.g. CAS.
But it depends on the PerfMemory class
usage.
Should we make the static variables
'_initialized' and '_destroyed'
volatile?
For good measure - yes.
Also, the
'_initialized' is set to 1 with:
159
OrderAccess::release_store(&_initialized,
1);
Should we do the same to set the
'_destroyed'?:
200 _destroyed = true;
There is a benign initialization race but
we need the release_store to ensure all
the data fields can be read if
_initialized is seen as true. But what is
missing is a load_acquire() in
is_initialized() to ensure we synchronize
with that store!
Yes, I noticed that the load_acquire() is
missed. :|
There is also a potential for a
destruction race (if multiple aborts
happens concurrently in different threads)
but that also seems benign. In this case
there is no data being set so the store to
_destroyed does not need to be a
release_store.
I'm not convinced yet this is benign as the
PerfMemory::destroy() has this call:
197 delete_memory_region();
Yes though most of its work ends up being
no-ops.
Now, I started thinking about the asserts
that call the is_useable().
Should they be returns instead?
I think this is a somewhat confused chunk of
code. It's only fractionally thread-safe yet
once in use could be in use concurrently with
an aborting thread that calls destroy(). I
don't think there is any simple fix for this.
If we're in the process of crashing does it
really matter if we trigger a secondary crash
due to this?
It doesn't matter if we do:
assert(is_usable(),...);
// continue
or
if (!is_usable()) return;
// continue
because as soon as we have checked is_usable()
and abort happening in another thread may have
changed that by calling destroy.
This code is basically broken if we hit an abort
path instead of a normal VM shutdown.
David
-----
The problems with this
code go way beyond what Yasumasa is trying to
address with the JSnap problem and I would not
want to put it back on him to try and come up
with an overall solution.
Then the
is_destroyed() would better to have the
load_acquire().
You could add a load_acquire and do the
store_release. It certainly would not hurt,
but I don't think it would actually benefit
anything either.
Cheers,
David
Just interested to
know what do you think on this.
Thanks,
Serguei
Cheers,
David
Thanks,
Serguei
On 10/18/17 00:39, Yasumasa Suenaga
wrote:
Hi David,
Thank you for your comment.
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.07/
Serguei, please comment about this :-)
Yasumasa
2017-10-18 16:09 GMT+09:00 David
Holmes<david.hol...@oracle.com>:
Hi Yasumasa,
On 18/10/2017 4:34 PM, Yasumasa
Suenaga wrote:
Hi David,
I don't
think we need the extra fields,
just ensure the existing ones
can't
be accessed (other than by the
tools) after destroy is called.
I've added
PerfMemory::is_useable() to check
whether we can access to
PerfMemory.
I think this webrev prevent to
access to PerfMemory after
destroy() call.
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.06/
This:
90 void PerfMemory::initialize()
{
91
92 if (_prologue != NULL)
93 // initialization already
performed
94 return;
shouldn't check _prologue, but
is_initialized().
213 assert(is_useable(), "called
before initialization");
-> "called before init or after
destroy"
Could add a similar assert in
PerfMemory::mark_updated().
Let's see what Serguei thinks. :)
Thanks,
David
Thanks,
Yasumasa
2017-10-18 13:44 GMT+09:00 David
Holmes<david.hol...@oracle.com>:
On
18/10/2017 2:27 PM, Yasumasa
Suenaga wrote:
Hi David,
2017-10-18 12:55 GMT+09:00
David
Holmes<david.hol...@oracle.com>:
On 18/10/2017 12:37 PM,
Yasumasa Suenaga wrote:
Hi David,
With
your changes you no
longer null out
_prologue so the
assertion
would
now not fail and we'd
proceed to access the
deleted memory region!
On Linux,
PerfMemory::delete_memory_region()
does not call munmap()
for PerfMemory.
Perhaps not but there are
still other actions that
happen and the point
is
we should not be able to
continue to use PerfMemory
once it has been
destroyed (even if the
destruction is only
logical).
I received same comment from
Dmitry in the past, but we
couldn't
decide how should we do.
http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-May/019728.html
In that discussion, I uploaded
another webrev which adds
other fields
for
JSnap.
Is it suitable?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
I don't think we need the extra
fields, just ensure the existing
ones
can't
be accessed (other than by the
tools) after destroy is called.
I'm
unclear why you no
longer clear all the
fields set during
initialization?
PerfMemory.java in
jdk.hotspot.agent needs
these field values.
`jhsdb jsnap --core` is
failed if they are
cleared.
I'm not familiar with these
tools. When do we produce a
core file after
calling PerfMemory::destroy
?
PerfMemory::destroy() is
called before aborting.
Ah - right. I assume we need to
close off the perfdata file
before we
abort.
Thanks,
David
-----------------------
#0 perfMemory_exit ()
at
/usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/share/vm/runtime/perfMemory.cpp:80
#1 0x00007f99b091c949 in
os::shutdown ()
at
/usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/os/linux/vm/os_linux.cpp:1483
#2 0x00007f99b091c980 in
os::abort
(dump_core=<optimized
out>)
at
/usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/os/linux/vm/os_linux.cpp:1503
#3 0x00007f99b0b689c3 in
VMError::report_and_die (
this=this@entry=0x7ffcacf40b50)
at
/usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/share/vm/utilities/vmError.cpp:1060
#4 0x00007f99b0926f04 in
JVM_handle_linux_signal
(sig=sig@entry=11,
info=info@entry=0x7ffcacf40df0,
ucVoid=ucVoid@entry=0x7ffcacf40cc0,
abort_if_unrecognized=abort_if_unrecognized@entry=1)
at
/usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/os_cpu/linux_x86/vm/os_linux_x86.cpp:541
-----------------------
Thanks,
Yasumasa
But
it seems to me that
there are various checks
of
_prologue that should
really be checking
is_initialized() and/or
is_destroyed() as a
guard.
Should I change all
assertions for _prologue?
Assertions and direct
guards. Checking _prologue
is a placeholder for
the
real check.
Thanks,
David
Thanks,
Yasumasa
2017-10-18 10:53 GMT+09:00
David
Holmes<david.hol...@oracle.com>:
Hi Yasumasa,
By chance we ran into
this bug which I
analysed yesterday:
https://bugs.openjdk.java.net/browse/JDK-8189390
We hit the assertion:
# Internal Error
(/open/src/hotspot/share/runtime/perfMemory.cpp:216),
pid=17874, tid=17875
# assert(_prologue !=
__null) failed: called
before initialization
#
which is misleading
because it can fail if
called before
initialization,
or
after
PerfMemory::destroy has
been called.
With your changes you no
longer null out
_prologue so the
assertion
would
now not fail and we'd
proceed to access the
deleted memory region!
I'm unclear why you no
longer clear all the
fields set during
initialization? But it
seems to me that there
are various checks of
_prologue that should
really be checking
is_initialized() and/or
is_destroyed() as a
guard.
Thanks,
David
On 16/10/2017 11:25 PM,
Yasumasa Suenaga wrote:
PING:
Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.05/
Thanks,
Yasumasa
On 2017/10/03 13:18,
Yasumasa Suenaga
wrote:
Hi all,
I added gtest unit
test case for this
change in new
webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.05/
Could you review it?
Thanks,
Yasumasa
2017-09-27 0:01
GMT+09:00 Yasumasa
Suenaga<yasue...@gmail.com>:
Hi all,
I uploaded new
webrev to be
adapted to
jdk10/hs:
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.04/
Thanks,
Yasumasa
On 2017/09/21
7:45, Yasumasa
Suenaga wrote:
PING:
Have you checked
this issue?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/
Yasumasa
On 2017/07/01
23:43, Yasumasa
Suenaga wrote:
PING:
Have you
checked this
issue?
Yasumasa
On 2017/06/13
14:10,
Yasumasa
Suenaga wrote:
Hi all,
I want to
discuss about
JDK-8151815:
Could not
parse core
image
with
JSnap.
In last year,
I found JSnap
cannot parse
coredump and
I've sent
review
request for it
as
JDK-8151815.
However it has
not been
reviewed
yet
[1].
We've
discussed
about safety
implementation,
but we could
not
get
consensus.
IMHO all SA
tools should
be handled
java processes
and core
images,
and
PerfCounter
value is
useful. So I
fix this
issue.
I uploaded new
webrev for
this issue. I
think this
patch is
safety
because new
flag
PerfMemory::_destroyed
guards double
free, and
all
members in
PerfMemory is
accessible
(they are not
munmap'ed)
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/
Can you
cooperate?
Thanks,
Yasumasa
[1]
http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-April/019480.html
|
