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

[Vitaly Davidovich]

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> 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]
> *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> 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] On Behalf
> Of Vladimir Kozlov
> Sent: Donnerstag, 17. Juli 2014 17:09
> To: Lindenmaier, Goetz; 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.
> >
>