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
