RFR: 8212748: ZGC: Add reentrant locking functionality

[Per Liden]

Hi Kim,

On 11/20/18 9:37 PM, Kim Barrett wrote:
>> On Nov 20, 2018, at 2:45 AM, Per Liden <per.liden at oracle.com> wrote:
>>
>> Hi,
>>
>> On 10/26/18 2:00 AM, Kim Barrett wrote:
>>>> On Oct 25, 2018, at 12:20 PM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>>>
>>>> Hi Per,
>>>>
>>>> Thanks for having a look at this.
>>>>
>>>> On 2018-10-25 17:27, Per Liden wrote:
>>>>> Hi Erik,
>>>>> On 2018-10-25 15:44, Erik Österlund wrote:
>>>>>> Hi,
>>>>>>
>>>>>> ZGC needs a per-nmethod lock to be used for concurrent IC cleaning, protecting misaligned oops from concurrently being patched by nmethod entry barriers, and read using CompiledMethod::is_unloading(), with interactions crossing JavaThreads and non-Java threads. This patch adds that utility.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~eosterlund/8212748/webrev.00/
>>>>> I'd like to suggest that ZReentrantLock doens't inherit from ZLock, but that it instead has a ZLock. And that ZLocker is adjusted to take a T* instead of a ZLock*.
>>>>
>>>> Wouldn't it be annoying that you can't use a plain ZLocker then, but instead would have to use ZLocker<ZLock>? I'm not sure I see the advantage of that approach, only a usability disadvantage. I'm not opposed either though if you prefer it to be done in that way.
>>> For what its worth, that’s exactly how the similar C++11 facility works, e.g. one says
>>>      std::lock_guard<std::mutex> lk(m);
>>> or
>>>      std::lock_guard<std::recursive_mutex> lk(rm);
>>
>> Erik and I discussed this some more and we ended up with the following patch:
>>
>> http://cr.openjdk.java.net/~pliden/8212748/webrev.0/
>>
>> cheers,
>> Per
> 
> ------------------------------------------------------------------------------
> 
> Is there a reason why this was not based on a pthread_mutex_t
> configured with the PTHREAD_MUTEX_RECURSIVE attribute?  (Rather than
> this "hand-rolled" recursion.)
> 
> ------------------------------------------------------------------------------
> 
> What is RecursiveZLock::is_owned for?  That might be why the RECURSIVE
> attribute wasn't used, but since I don't see the point, except perhaps
> for assertion checks.  Part of my question is, if is_owned is
> desirable for recursive case, why not for ordinary?  A similar
> implementation would seem to suffice.

Yes, is_owned() is used in some asserts, but more importantly it's used 
in few other cases too. The locking around concurrent class unloading is 
a bit messy, but in some contexts (which has multiple entry points) 
is_owned() is used to figure out if it's safe to do some operations 
because the thread has the lock, or if that should be postponed.

cheers,
Per

> 
> ------------------------------------------------------------------------------
>