Re: synchronization of JVMTI phase notifications [Fwd: Data visibility between threads in Hotspot]

[Daniel D. Daugherty]

Hiroshi,

I'm leaning toward making the two fields volatile and adding the
appropriate OrderAccess::XXX calls. I vaguely remember an existing
bug for events posting late. I'm going to see if I can find it.

Dan


Hiroshi Yamauchi wrote:

Hi Dan,

On Wed, Feb 11, 2009 at 12:43 PM, Daniel D. Daugherty <daniel.daughe...@sun.com <mailto:daniel.daughe...@sun.com>> wrote:

    I'll chime in on parts of this thread below.




    Tim Bell wrote:

        I don't know the precise answer to this question, so I am
        forwarding it to the Serviceability list to get a wider audience.

        -------- Original Message --------
        Subject:     Data visibility between threads in Hotspot
        Date:     Tue, 10 Feb 2009 11:14:24 -0800

        Hi Tim,

        I have a Hotspot question. Chuck Rasbold pointed me to you.
        Feel free to
        forward this message to other experts at Sun, if needed.

        If one Hotspot engineer wants to guarantee the immediate
        visibility of a
        write to a static variable to reads in other threads (assuming
        the reads
        and the writes are properly synchronized via a mutex), what
        does s/he do?

        Does the use of MutexLocker guarantee the visibility? It
        probably does not.


    I believe that MutexLocker does guarantee the visibility:

    src/share/vm/runtime/mutexLocker.hpp:

     133  // See orderAccess.hpp.  We assume throughout the VM that
    MutexLocker's
     134  // and friends constructors do a fence, a lock and an
    acquire *in that
     135  // order*.  And that their destructors do a release and
    unlock, in *that*
     136  // order.  If their implementations change such that these
    assumptions
     137  // are violated, a whole lot of code will break.


OK. That's handy.

    See src/share/vm/runtime/orderAccess.hpp for the gory details.




        There are several volatile variables in Hotpot, but it may not
        really
        work because C++ compilers do not guarantee the visibility or
        limit the
        instruction reordering.

    See the "C++ Volatility" section in
    src/share/vm/runtime/orderAccess.hpp.


It's an interesting header file to read.

        For example, there is the static variable JvmtiEnvBase:_phase
        which
        indicates the JVMTI phase in which the JVM is currently in.
        But AFAICT,
        there is no synchronization used by the callers of get_phase() and
        set_phase() and apparently they can be called by multiple
        threads. Also,
        the JvmtiEventEnabled::_enabled_bits which is a jlong variable
        that
        indicates which events are enabled for a single jvmtiEnv. The
        writes and
        reads to it are not synchronized, either. These are race
        conditions,
        which implies that some JVMTI event can go off in the dead
        phase when no
        events are supposed to go off. I'm actually looking into a bug
        in a
        JVMTI agent that I suspect is due to this.


    It certainly looks like JvmtiEnvBase::_phase should minimally be a
    volatile; it may also require some sort of synchronization to force
    the updated values to be visible...

    JvmtiEventEnabled::_enabled_bits also looks problematic. I'm going
    to have to mull on both of these and talk to some other folks.

The bug that I ran across was a memory corruption crash because a CompiledMethodLoad event happened to be sent (due to this race condition) after Agent_OnUnload call where I had already deallocated the agent's data objects. This happens very rarely (once in every more than 100k runs of a more-or-less javac invocation) but often enough to fix.

I have a patch attached, which I used to suppress the bug by avoiding the events to be sent during the dead phase by adding synchronization. Basically I make the _phase variable volatile and I use a mutex to guard event callbacks in the all the post_xxx functions and the phase transition to the dead phase in post_vm_death(). Probably we don't need the volatile as long as the mutex guarantees the visibility. The other phase transitions (onload->primodial->start->live) may not need the same degree of synchronization because events that can be sent in a earlier phase can be sent in the next phase as well.

The _enabled_bits and the callback function table will probably need a similar synchronization as well. But the attached patch does not implement it and only ensures that no events are sent during the dead phase. I'd like to get the fix into openjdk in one form or another.

Thanks,
Hiroshi

    Dan



        Thanks,
        Hiroshi