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

[Erik Österlund]

Hi Kim,

Thank you for the review.

On 2017-07-05 04:00, Kim Barrett wrote:
>> On Jun 26, 2017, at 9:34 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi,
>>
>> Webrev: http://cr.openjdk.java.net/~eosterlund/8182703/webrev.02/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8182703
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/ptrQueue.cpp
> Removing unlock / relock around
>    78   qset()->enqueue_complete_buffer(node);
>
> I would prefer that this part of this changeset not be made at this
> time.
>
> This part isn't necessary for the main point of this changeset.  It's
> a cleanup that is enabled by the lock rank changes, where the rank
> changes are required for other reasons.

Okay.

> It also at least conflicts with, and probably breaks, a pending change
> of mine.  (I have a largish stack of patches in this area that didn't
> quite make it into JDK 9 before the original FC date, and which I've
> been (all too slowly) trying to work my way through and bring into JDK
> 10.)

I agree that it would be possible to just correct the ranks while 
allowing the spaghetti synchronization code to remain in the code base. 
Here are some comments about that to me not so attractive idea:
1) I would really like to get rid of that code, because I think it is 
poor synchronization practice and its stated reason for existence is 
gone now.
2) I have to do *something* about that part in the current change, 
otherwise the comment motivating its existence will be incorrect after 
my lock rank change. There is no longer a legitimate motivation for 
doing that unlock and re-lock. So we have the choice of making a new 
made up motivation why we do this anyway, or to remove it. For me the 
choice is easily to remove it.
3) If some new actual motivation for dropping that lock arises later on 
down the road (which I am dubious about), then I do not see an issue 
with simply re-adding it then, when/if that becomes necessary again, 
with a new corresponding motivation added in appropriately.

As far as your new changes go, I am curious what they do to motivate 
unlocking/re-locking this shared queue lock again. As outlined in my 
recent email to Thomas, we do not hold either of these queue locks when 
concurrent refinement helper code is called from GC barriers invoked 
from JavaThreads, even with my new changes. If it is in this code path 
that you will perform more work (just speculating), then that should be 
invariant of this cleanup.

Therefore I would like to know, since I am asked to withdraw the code 
that cleans up the hacky spaghetti synchronization code, with the 
motivation that there might be a new reason for doing this later on, 
that we are at least certain that this unlock/re-lock will for sure be 
needed then.

> The RFR says:
>
>> 2) There is an issue that the shared queue locks have a "special"
>> rank, which is below the lock ranks used by the cbl monitor and free
>> list monitor. This leads to an issue when these locks have to be taken
>> while holding the shared queue locks. The current solution is to drop
>> the shared queue locks temporarily, introducing nasty data
>> races. These races are guarded, but the whole race seems very
>> unnecessary.
> This isn't entirely accurate, as the shared queue locks are not
> "special" rank; the current lock ranks are described correctly later
> in the RFR.

Yes you are right.

> It's true there is an "interesting" bit of code there to temporarily
> drop the shared queue lock.  I don't think it's harmful to do so, and
> could have some small benefit now.  More importantly, one of the
> changes in that afore mentioned stack of patches puts more (possibly
> significantly more in some cases) work into that dropped-lock region.
> And if that idea ultimately doesn't pan out, simply removing the
> unlock/relock pair is not, IMO, the right way to clean things up;
> there is some additional refactoring that ought to be done.

Could you please elaborate why you do not consider removing the 
unlock/lock due to incorrect lock ranks being the right cleanup after 
that very incorrect lock rank issue has been resolved?

> ------------------------------------------------------------------------------
>
> The lock ranking changes look good.

Thanks, I am glad we agree about this.

/Erik