Re: RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

Daniel D. Daugherty
Hi Kim,

Thanks for the review.


On 7/8/19 7:08 PM, Kim Barrett wrote:

>> On Jul 6, 2019, at 9:53 AM, Daniel D. Daugherty <[hidden email]> wrote:
>>
>> Greetings,
>>
>> During the code review for the following fix:
>>
>>      JDK-8227117 normal interpreter table is not restored after single stepping with TLH
>>      https://bugs.openjdk.java.net/browse/JDK-8227117
>>
>> Erik O. noticed a potential race with templateInterpreter.cpp: copy_table()
>> depending on C++ compiler optimizations. The following bug is being used
>> to fix this issue:
>>
>>      JDK-8227338 templateInterpreter.cpp: copy_table() needs to be safer
>>      https://bugs.openjdk.java.net/browse/JDK-8227338
>>
>> Here's the webrev URL:
>>
>>      http://cr.openjdk.java.net/~dcubed/8227338-webrev/0_for_jdk14/
>>
>> This fix has been tested via Mach5 Tier[1-3] on Oracle's usual platforms.
>> Mach5 tier[4-6] is running now. It has also been tested with the manual
>> jdb test from JDK-8227117 using 'release' and 'fastdebug' bits.
>>
>> Thanks, in advance, for questions, comments or suggestions.
>>
>> Dan
> [This review is ignoring the issues around the current implementation
> of atomic copies discussed elsewhere in this thread. I assume those
> will be addressed elsewhere.]
>
> ------------------------------------------------------------------------------
> src/hotspot/share/interpreter/templateInterpreter.cpp
>   286     while (size-- > 0) *to++ = *from++;
>
> [pre-existing]
>
> This ought to be using Copy::disjoint_words.  That's even more obvious
> in conjunction with the change to use Copy::disjoint_words_atomic in
> the non-safepoint case.

I can make that change. Is there a specific advantage/reason that you
have in mind here?


> ------------------------------------------------------------------------------
> src/hotspot/share/interpreter/templateInterpreter.cpp
>   284   if (SafepointSynchronize::is_at_safepoint()) {
>
> I wonder how much benefit we really get from having distinct safepoint
> and non-safepoint cases, rather than just unconditionally using
> Copy::disjoint_words_atomic.

Sorry, I don't know the answer to that. My intention was to use
Copy::disjoint_words_atomic() only in the case where I knew that
I needed it so no potential impact on existing uses at a safepoint.

Thanks for the review.

Dan


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

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

coleen.phillimore


On 7/9/19 9:46 AM, Daniel D. Daugherty wrote:

> On 7/9/19 9:13 AM, Daniel D. Daugherty wrote:
>> Hi Kim,
>>
>> Thanks for the review.
>>
>>
>> On 7/8/19 7:00 PM, Kim Barrett wrote:
>>>> On Jul 7, 2019, at 8:08 PM, David Holmes <[hidden email]>
>>>> wrote:
>>>>
>>>> On 7/07/2019 6:48 pm, Erik Osterlund wrote:
>>>>> The real danger is SPARC though and its BIS instructions. I don’t
>>>>> have the code in front of me, but I really hope not to see that
>>>>> switch statement and non-volatile loop in that
>>>>> pd_disjoint_words_atomic() function.
>>>> sparc uses the same loop.
>>>>
>>>> Let's face it, almost no body expects the compiler to do these
>>>> kinds of transformations. :(
>>> See JDK-8131330 and JDK-8142368, where we saw exactly this sort of
>>> transformation from a fill-loop
>>> to memset (which may use BIS, and indeed empirically does in some
>>> cases).  The loops in question
>>> seem trivially convertible to memcpy/memmove.
>>
>> Very interesting reads. Thanks for pointing those out.
>>
>> src/hotspot/share/interpreter/templateInterpreter.cpp:
>>
>> DispatchTable TemplateInterpreter::_active_table;
>> DispatchTable TemplateInterpreter::_normal_table;
>> DispatchTable TemplateInterpreter::_safept_table;
>>
>> So it seems like changing _active_table to:
>>
>> volatile DispatchTable TemplateInterpreter::_active_table;
>>
>> might be a good idea... Do you concur?
>
> This change would require a bunch of additional changes so I'm
> not planning to make it (way too intrusive).

Can you file an additional RFE to examine the uses of dispatch tables
for when we only have handshakes for safepoints?  And capture this idea
of making the tables volatile?

thanks,
Coleen

>
> Dan
>
>
>>
>>
>>> Also see JDK-8142349.
>>>
>>>>> And I agree that the atomic copying API should be used when we
>>>>> need atomic copying. And if it turns out the implementation of
>>>>> that API is not atomic, it should be fixed in that atomic copying
>>>>> API.
>>>> I agree to some extent, but we assume atomic load/stores of words
>>>> all over the place - and rightly so. The issue here is that we need
>>>> to hide the loop inside an API that we can somehow prevent the C++
>>>> compiler from screwing up. It's hardly intuitive or obvious when
>>>> this is needed e.g if I simply copy three adjacent words without a
>>>> loop could the compiler convert that to a block move that is
>>>> non-atomic?
>>>>
>>>>> So I think this change looks good. But it looks like we are not
>>>>> done yet. :c
>>>> I agree that changing the current code to use the atomic copy API
>>>> to convey intent is fine.
>>> I’ve been reserving Atomic::load/store for cases where the location
>>> “ought” to be declared std::atomic<T> if
>>> we were using C++11 atomics (or alternatively some homebrew
>>> equivalent).  Not all places where we do
>>> stuff “atomically” is appropriate for that though (consider card
>>> tables, being arrays of bytes, where using an
>>> atomic<T> type might impose alignment constraints that are
>>> undesirable here).  I *think* just using volatile
>>> here would likely be sufficient, e.g. we should have
>>>
>>>      Copy::disjoint_words_atomic(const HeapWord* from,volatile
>>> HeapWord* to, size_t count)
>>
>> I think this part should be taken up in the follow bug that I filed:
>>
>>     JDK-8227369 pd_disjoint_words_atomic() needs to be atomic
>>     https://bugs.openjdk.java.net/browse/JDK-8227369
>>
>> Thanks for chiming in on the review!
>>
>> Dan
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

Daniel D. Daugherty
In reply to this post by Daniel D. Daugherty
Greetings,

I've made one minor tweak based on Kim's code review.

Here's the full webrev:

http://cr.openjdk.java.net/~dcubed/8227338-webrev/1_for_jdk14.full/

Here's the incremental webrev:

http://cr.openjdk.java.net/~dcubed/8227338-webrev/1_for_jdk14.inc/

Here's the context diff:

$ hg diff
diff -r 32fe92d8b539 src/hotspot/share/interpreter/templateInterpreter.cpp
--- a/src/hotspot/share/interpreter/templateInterpreter.cpp    Mon Jul
08 16:58:27 2019 -0400
+++ b/src/hotspot/share/interpreter/templateInterpreter.cpp    Tue Jul
09 10:02:46 2019 -0400
@@ -283,7 +283,7 @@
    // Copy non-overlapping tables.
    if (SafepointSynchronize::is_at_safepoint()) {
      // Nothing is using the table at a safepoint so skip atomic word copy.
-    while (size-- > 0) *to++ = *from++;
+    Copy::disjoint_words((HeapWord*)from, (HeapWord*)to, (size_t)size);
    } else {
      // Use atomic word copy when not at a safepoint for safety.
      Copy::disjoint_words_atomic((HeapWord*)from, (HeapWord*)to,
(size_t)size);

Thanks, in advance, for questions, comments or suggestions.

Dan


On 7/6/19 9:53 AM, Daniel D. Daugherty wrote:

> Greetings,
>
> During the code review for the following fix:
>
>     JDK-8227117 normal interpreter table is not restored after single
> stepping with TLH
>     https://bugs.openjdk.java.net/browse/JDK-8227117
>
> Erik O. noticed a potential race with templateInterpreter.cpp:
> copy_table()
> depending on C++ compiler optimizations. The following bug is being used
> to fix this issue:
>
>     JDK-8227338 templateInterpreter.cpp: copy_table() needs to be safer
>     https://bugs.openjdk.java.net/browse/JDK-8227338
>
> Here's the webrev URL:
>
>     http://cr.openjdk.java.net/~dcubed/8227338-webrev/0_for_jdk14/
>
> This fix has been tested via Mach5 Tier[1-3] on Oracle's usual platforms.
> Mach5 tier[4-6] is running now. It has also been tested with the manual
> jdb test from JDK-8227117 using 'release' and 'fastdebug' bits.
>
> Thanks, in advance, for questions, comments or suggestions.
>
> Dan
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

Daniel D. Daugherty
In reply to this post by coleen.phillimore
> Can you file an additional RFE to examine the uses of dispatch tables
> for when we only have handshakes for safepoints?  And capture this
> idea of making the tables volatile?

Done.

     JDK-8227443 TemplateInterpreter::_active_table needs to be reexamined
     https://bugs.openjdk.java.net/browse/JDK-8227443

Feel free to update the new RFE.

Dan


On 7/9/19 10:05 AM, [hidden email] wrote:

>
>
> On 7/9/19 9:46 AM, Daniel D. Daugherty wrote:
>> On 7/9/19 9:13 AM, Daniel D. Daugherty wrote:
>>> Hi Kim,
>>>
>>> Thanks for the review.
>>>
>>>
>>> On 7/8/19 7:00 PM, Kim Barrett wrote:
>>>>> On Jul 7, 2019, at 8:08 PM, David Holmes <[hidden email]>
>>>>> wrote:
>>>>>
>>>>> On 7/07/2019 6:48 pm, Erik Osterlund wrote:
>>>>>> The real danger is SPARC though and its BIS instructions. I don’t
>>>>>> have the code in front of me, but I really hope not to see that
>>>>>> switch statement and non-volatile loop in that
>>>>>> pd_disjoint_words_atomic() function.
>>>>> sparc uses the same loop.
>>>>>
>>>>> Let's face it, almost no body expects the compiler to do these
>>>>> kinds of transformations. :(
>>>> See JDK-8131330 and JDK-8142368, where we saw exactly this sort of
>>>> transformation from a fill-loop
>>>> to memset (which may use BIS, and indeed empirically does in some
>>>> cases).  The loops in question
>>>> seem trivially convertible to memcpy/memmove.
>>>
>>> Very interesting reads. Thanks for pointing those out.
>>>
>>> src/hotspot/share/interpreter/templateInterpreter.cpp:
>>>
>>> DispatchTable TemplateInterpreter::_active_table;
>>> DispatchTable TemplateInterpreter::_normal_table;
>>> DispatchTable TemplateInterpreter::_safept_table;
>>>
>>> So it seems like changing _active_table to:
>>>
>>> volatile DispatchTable TemplateInterpreter::_active_table;
>>>
>>> might be a good idea... Do you concur?
>>
>> This change would require a bunch of additional changes so I'm
>> not planning to make it (way too intrusive).
>
> Can you file an additional RFE to examine the uses of dispatch tables
> for when we only have handshakes for safepoints?  And capture this
> idea of making the tables volatile?
>
> thanks,
> Coleen
>>
>> Dan
>>
>>
>>>
>>>
>>>> Also see JDK-8142349.
>>>>
>>>>>> And I agree that the atomic copying API should be used when we
>>>>>> need atomic copying. And if it turns out the implementation of
>>>>>> that API is not atomic, it should be fixed in that atomic copying
>>>>>> API.
>>>>> I agree to some extent, but we assume atomic load/stores of words
>>>>> all over the place - and rightly so. The issue here is that we
>>>>> need to hide the loop inside an API that we can somehow prevent
>>>>> the C++ compiler from screwing up. It's hardly intuitive or
>>>>> obvious when this is needed e.g if I simply copy three adjacent
>>>>> words without a loop could the compiler convert that to a block
>>>>> move that is non-atomic?
>>>>>
>>>>>> So I think this change looks good. But it looks like we are not
>>>>>> done yet. :c
>>>>> I agree that changing the current code to use the atomic copy API
>>>>> to convey intent is fine.
>>>> I’ve been reserving Atomic::load/store for cases where the location
>>>> “ought” to be declared std::atomic<T> if
>>>> we were using C++11 atomics (or alternatively some homebrew
>>>> equivalent).  Not all places where we do
>>>> stuff “atomically” is appropriate for that though (consider card
>>>> tables, being arrays of bytes, where using an
>>>> atomic<T> type might impose alignment constraints that are
>>>> undesirable here).  I *think* just using volatile
>>>> here would likely be sufficient, e.g. we should have
>>>>
>>>>      Copy::disjoint_words_atomic(const HeapWord* from,volatile
>>>> HeapWord* to, size_t count)
>>>
>>> I think this part should be taken up in the follow bug that I filed:
>>>
>>>     JDK-8227369 pd_disjoint_words_atomic() needs to be atomic
>>>     https://bugs.openjdk.java.net/browse/JDK-8227369
>>>
>>> Thanks for chiming in on the review!
>>>
>>> Dan
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

Daniel D. Daugherty
In reply to this post by Daniel D. Daugherty
Serguei,

Thanks for the quick re-review!

Dan


On 7/9/19 12:34 PM, [hidden email] wrote:

> Hi Dan,
>
> This looks good too.
>
> Thanks,
> Serguei
>
>
> n 7/9/19 07:09, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've made one minor tweak based on Kim's code review.
>>
>> Here's the full webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8227338-webrev/1_for_jdk14.full/
>>
>> Here's the incremental webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8227338-webrev/1_for_jdk14.inc/
>>
>> Here's the context diff:
>>
>> $ hg diff
>> diff -r 32fe92d8b539
>> src/hotspot/share/interpreter/templateInterpreter.cpp
>> --- a/src/hotspot/share/interpreter/templateInterpreter.cpp Mon Jul
>> 08 16:58:27 2019 -0400
>> +++ b/src/hotspot/share/interpreter/templateInterpreter.cpp Tue Jul
>> 09 10:02:46 2019 -0400
>> @@ -283,7 +283,7 @@
>>    // Copy non-overlapping tables.
>>    if (SafepointSynchronize::is_at_safepoint()) {
>>      // Nothing is using the table at a safepoint so skip atomic word
>> copy.
>> -    while (size-- > 0) *to++ = *from++;
>> +    Copy::disjoint_words((HeapWord*)from, (HeapWord*)to, (size_t)size);
>>    } else {
>>      // Use atomic word copy when not at a safepoint for safety.
>>      Copy::disjoint_words_atomic((HeapWord*)from, (HeapWord*)to,
>> (size_t)size);
>>
>> Thanks, in advance, for questions, comments or suggestions.
>>
>> Dan
>>
>>
>> On 7/6/19 9:53 AM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> During the code review for the following fix:
>>>
>>>     JDK-8227117 normal interpreter table is not restored after
>>> single stepping with TLH
>>>     https://bugs.openjdk.java.net/browse/JDK-8227117
>>>
>>> Erik O. noticed a potential race with templateInterpreter.cpp:
>>> copy_table()
>>> depending on C++ compiler optimizations. The following bug is being
>>> used
>>> to fix this issue:
>>>
>>>     JDK-8227338 templateInterpreter.cpp: copy_table() needs to be safer
>>>     https://bugs.openjdk.java.net/browse/JDK-8227338
>>>
>>> Here's the webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8227338-webrev/0_for_jdk14/
>>>
>>> This fix has been tested via Mach5 Tier[1-3] on Oracle's usual
>>> platforms.
>>> Mach5 tier[4-6] is running now. It has also been tested with the manual
>>> jdb test from JDK-8227117 using 'release' and 'fastdebug' bits.
>>>
>>> Thanks, in advance, for questions, comments or suggestions.
>>>
>>> Dan
>>>
>>
>