RFR (S): 8182703: Correct G1 barrier queue lock orderings

[Erik Österlund]

Hi Thomas,

On 2017-07-04 20:14, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2017-07-04 at 19:43 +0200, Thomas Schatzl wrote:
>> Hi,
>>
>> On Mon, 2017-06-26 at 15:34 +0200, Erik Österlund wrote:
>>> Hi,
>>>
>>> Webrev: http://cr.openjdk.java.net/~eosterlund/8182703/webrev.02/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8182703
>>>
>>    looks good apart from the comment at Monitor::event_types. It now
>> contradicts itself from one sentence to the next ("special must be
>> lowest" and then "oh no, after all access must be lowest"). Please
>> try to find some better wording here :)
> Some more comments about the comment added in this change:
>
>    96   // The rank access is reserved for locks that may be required to
> perform
>    97   // memory accesses that require special GC barriers, such as
> SATB barriers.
>    98   // Since memory accesses should be able to be performed pretty
> much anywhere
>    99   // in the code, that wannts being more special than the
> "special" rank.
>
> - s/wannts/requires in that comment.

Fixed.

> - I do not think the access lock rank is used for special GC barriers,
> at least the "SATB barrier" is a bad example. The SATB barrier is
> commonly the pre-write barrier in generated code, and the locks do not
> have a lot in common with write barriers.
> Maybe the text wanted to give an example for why locks of this rank
> could be called at any time - because the lock might be taken as part
> of some SATB barrier code?

I do not understand why SATB is a bad example. Perhaps you could elaborate?
It is specifically the SATB barriers that are the biggest issue for me 
and what made me want to make this change. It is required for both 
writes but also for all weak loads. And it is specifically the weak 
loads that give me the most headache and serves as the main motivator 
for this. These include resolving jweaks, looking up strings, keeping 
class holders alive on compiler threads when looking up metadata in 
ciEnv, and a whole bunch of other stuff. The current code for handling 
weak loads is full of hacks like these:

{ MutexLockerEx m(...);
   oop obj = load_weak_oop(...);
}

keep_alive(obj);

return obj;

...where the keep alive barrier required by SATB for weak loads has been 
moved way out from the critical section (even multiple levels up in the 
call tree) due to lock ordering problems with G1 SATB barrier code that 
forbids this barrier while holding certain locks.
For the new GC barrier interface that introduces declarative semantics, 
I need the barriers to be tightly bound to the access, and I need 
accesses to not be disallowed due to holding other VM locks. We already 
perform JNIHandles::resolve while holding "special" ranked locks today, 
and hopefully get away with it by making sure these resolutions can 
never be passed a jweak disguised as a jobject. But I do not want to 
require hotspot developers to have to think about whether what they are 
resolving could be weak and then have to consider that SATB barriers 
require locks with high ranks, requiring them to rewrite the code.

Having said that, of course the post-write barriers are problematic as 
well, as I want it to be possible to perform stores without having to 
think about random G1 locks in a similar fashion.

Speaking of which, I am entertaining the idea that perhaps the 
HeapRegionRemSet::_m lock ought to get the new access rank too. It seems 
to me like it could happen that a JavaThread performing a reference 
store decides to join in on concurrent refinement and has to take that 
_m lock when adding a reference to a remembered set. Therefore, this 
current "leaf" ranked lock could be acquired when performing a store on 
JavaThreads. The current leaf rank is not conflicting with my currently 
proposed changes, and I do not require it for refactoring weak loads. 
The reason is that:
1) Only JavaThreads help out with refinement, and only due to their 
local queue being is full (not when e.g. a card could not be parsed and 
the shared queue is grabbed).
2) JavaThreads do not acquire the shared queue lock before calling 
enqueue on the local queue in their barriers as they use their own local 
queue instead.
3) Because of 1 and 2, when collaborative concurrent refinement is 
called from the queue, the shared queue lock is not held.
4) The cbl monitor and free list lock are not held either when 
concurrent refinement is called from the queue.
5) Due to 3 and 4, no access locks from the queues are held when calling 
concurrent refinement helper code.

Having said that, it would still be good for consistency to move that 
lock down to access too, so that JavaThread reference stores can be 
performed more freely in the code. If there are plans of getting rid of 
that lock from refinement, then I think I can live with the current leaf 
rank, but if there are no plans of getting rid of that lock from 
refinement, I think I should probably squeeze that lock order change 
into this change. Perhaps the rank should be changed meanwhile anyway. 
Do you agree about this?

Thanks,
/Erik

> Thanks,
>    Thomas
>