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

[Doerr, Martin]

Hi Vitaly,

the PcDescs are already there before the cache gets used (nmethod creation). The cache is just an array of pointers. It is true that the pointers need to get written atomically.
However, this is also ensured by making them volatile. So this is a second reason why we need this change.

Best regards,
Martin


From: Vitaly Davidovich [mailto:vitalyd at gmail.com]
Sent: Freitag, 18. Juli 2014 13:53
To: Doerr, Martin
Cc: Vladimir Kozlov; hotspot-dev developers; Lindenmaier, Goetz
Subject: RE: RFR(S): 8050972: Concurrency problem in PcDesc cache


Hi Martin,

What if the write of a new entry results in address being set for the entry before the writes into the entry's fields? Is that still ok? Reader may see non-NULL partially written entry, it seems like ... I was thinking you need store-store barrier for that when writing the entry into the array.

Sent from my phone
On Jul 18, 2014 4:35 AM, "Doerr, Martin" <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi Vitaly,

yes, volatile is enough. There’s no requirement on the ordering.
The requirement for the writers is that they must only write valid entries or NULL.
The requirement for the readers is that they must not read several times for the matching and the returning of the result.
If a reader doesn’t find a value in the cache, the slow path will be used.

Best regards,
Martin


From: Vitaly Davidovich [mailto:vitalyd at gmail.com<mailto:vitalyd at gmail.com>]
Sent: Donnerstag, 17. Juli 2014 18:39
To: Doerr, Martin
Cc: hotspot-dev developers; Vladimir Kozlov; Lindenmaier, Goetz
Subject: RE: RFR(S): 8050972: Concurrency problem in PcDesc cache


Hi Martin,

Is volatile enough though if the entries are read/written concurrently? What about needing, e.g., store-store barriers when writing an entry into the array?

Sent from my phone
On Jul 17, 2014 11:20 AM, "Doerr, Martin" <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi Vladimir,

the following line should also work:
PcDesc* volatile _pc_descs[cache_size];
But we thought that the typedef would improve readability.
The array elements must be volatile, not the PcDescs which are pointed to.

Best regards,
Martin

-----Original Message-----
From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net<mailto:hotspot-dev-bounces at openjdk.java.net>] On Behalf Of Vladimir Kozlov
Sent: Donnerstag, 17. Juli 2014 17:09
To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>
Subject: Re: RFR(S): 8050972: Concurrency problem in PcDesc cache

Hi Goetz,

What is the reason for new typedef?

Thanks,
Vladimir

On 7/17/14 1:54 AM, 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.
> Best regards,
>    Martin and Goetz.
>