RFR(S): 8050972: Concurrency problem in PcDesc cache

[David Holmes]

On 18/07/2014 6:27 PM, Doerr, Martin wrote:
> Hi David,
>
> yes, the compiler is allowed to generate 2 loads in your example.
> And yes, declaring the field volatile suffices to prevent this.

Surprising - but thank goodness it does the right thing for volatiles!

Thanks for clarifying the ordering non-issue.

David

> Best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of David Holmes
> Sent: Freitag, 18. Juli 2014 07:28
> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
> Subject: Re: RFR(S): 8050972: Concurrency problem in PcDesc cache
>
> On 17/07/2014 6:54 PM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> This webrev fixes an important concurrency issue in nmethod.
>> Please review and test this change.  I please need a sponsor.
>> http://cr.openjdk.java.net/~goetz/webrevs/8050972-pcDescConc/webrev-01/
>>
>> This should be fixed into 8u20, too.
>>
>> The entries of the PcDesc cache in nmethods are not declared as volatile, but they are accessed and modified by several threads concurrently. Some compilers (namely xlC 12 on AIX) duplicate some memory accesses to non-volatile fields. In this case, this has led to the situation that a thread had successfully matched a pc in the cache, but returned the reloaded value which was already overwritten by another thread.
>
> Do you mean that code like:
>
> int val = ptr->field;
> int val2 = 2*val;
>
> might result in the compiler generating two loads for ptr->field?
>
> That would surely break a lot of lock-free algorithms! Or are you saying
> that would only happen if field (ptr?) is not volatile?
>
> Otherwise this change seems insufficient to ensure general MT
> correctness as Vitaly suggested.
>
> David
>
>> Best regards,
>>     Martin and Goetz.
>>