On Wed, 4 Sep 2024 15:23:51 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
> Scoped methods are critical methods in the FFM API where memory is accessed > in a potentially unsafe way. When closing shared arenas, we look at threads > in the middle of a scoped operation involving that arena, and if we find one, > we make it fail (by installing an async handshake on that thread). > > To find whether a thread is in a scoped method or not, we need a stack walk. > For performance reasons, it is preferrable to have the stack walk to be > bounded in size. > > A test started picking up a JVM assertion where the stack of a scoped method > (namely `ScopedMemoryAccess::isLoaded`) is too big. This is caused by the > scoped method stack walk finding the thread using the scoped method in the > middle of some JNI lookup (which is required as `isLoaded` eventually ends up > in a native method). This condition seems to have been made easier by the > fact that these [changes](https://git.openjdk.org/jdk/pull/19213). > > This PR reverts the stack trace associated with JNI lookup to what it was > before, by eliminating the extra frame with a bit of refactoring/cleanup. But > this is not enough: the stress test introduced in this PR still fails, even > when the stack associated with `ClassLoader::findNative` is restored. > > To address this problem in full, I have resorted to `registerNatives` - that > is, the native `isLoaded0`, `load0`, `unload0` and `force0` are > pre-registered, when the static initializer of `MappedMemoryUtils` is ran. > This means that we no longer need to run a JNI lookup in the middle of a > scoped method call. This brings the stack back under control, and passes the > stress test. > > Of course there's more to do in this area - we should have a more direct test > to check the stack associated with scoped methods (for instance, vector > load/store operations are also potential suspects), in order to catch > "suspicious refactoring" earlier in the process. For this reason I also filed > a follow up i[ssue](https://bugs.openjdk.org/browse/JDK-8339551). src/java.base/aix/native/libnio/MappedMemoryUtils.c line 229: > 227: {"unload0", "(JJ)V", (void *)&MMU_unload0}, > 228: {"force0", "(" FD "JJ)V", (void *)&MMU_force0}, > 229: }; I did a pass over this and I think it looks okay. Do the MMU_* functions still need to be jni-exported? In any case, probably better to rename them to MemoryMemoryUtils_* to avoid anything looking at symbols and thinking they are something to do with the memory management unit :-) It looks most of the signatures for this functions are misaligned with edits. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20854#discussion_r1745168389
