RFR(S): 8231612: 100% cpu on arm32 in Service Thread

[Kim Barrett]

> On Dec 6, 2019, at 8:32 AM, Aleksei Voitylov <aleksei.voitylov at bell-sw.com> wrote:
> 
> Hi Kim,
> 
> (cced hotspot-runtime as the updated webrev has code changes).
> 
> Thank you for the suggestion to address this at the code level. The
> following change makes GCC generate correct code:
> 
> webrev: http://cr.openjdk.java.net/~avoitylov/webrev.8231612.03/
> issue: https://bugs.openjdk.java.net/browse/JDK-8231612
> 
> GCC alias analysis at RTL level has some limitations [1]. In particular,
> it sometimes cannot differentiate between pointer and integer and can
> make incorrect assumptions about whether two pointers actually alias
> each other. We are hitting this in
> Atomic::CmpxchgByteUsingInt::operator(), which is used on some platforms.
> 
> The solution is to reference the same memory region using only one
> pointer. Anything with a second pointer here continues to confuse GCC,
> and would anyway be fragile in view of this GCC bug which surfaces
> easily on ARM32.
> 
> I considered making get/set byte operations inline functions but
> preferred locality defines bring.
> 
> Tested with JTReg, JCK and jcstress on ARM32 and Solaris SPARC with no
> regressions. The original problem also disappeared.
> No regressions in JCK VM,Lang, JTReg HotSpot on Linux x86, x86_64,
> AArch64, PPC, Mac.
> 
> Thanks,
> 
> -Aleksei
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49330
> 
> On 19/11/2019 19:01, Kim Barrett wrote:
>>> On Nov 19, 2019, at 10:18 AM, Aleksei Voitylov <aleksei.voitylov at bell-sw.com> wrote:
>>> 
>>> Kim,
>>> 
>>> you are right, to play it safe we need to -fno-tree-pre all relevant
>>> bool occurrances. I'll get back with an updated webrev when testing is
>>> finished.
>> I think just sprinkling the build system with -fno-tree-pre to address this doesn’t
>> scale in the face of people writing new code.
>> 
>> I think we need to find a way to write CmpxchgByteUsingInt in such a way that
>> it doesn’t tickle this bug, or forbid byte-sized cmpxchg (which would be painful),
>> or provide some entirely other implementation approach.

This looks like a good solution to me.

(I like this approach even without the gcc bug (which was an
entertaining read itself); this change lets me delete an item from my
followup list from templatizing Atomic.)

A couple of really minor nits:

(1) In the macros there are various expressions of the form

   x << BitsPerByte * idx 

Please put parens around the shift calculation, e.g.

   x << (BitsPerByte * idx)

so readers don't need to remember the operator precedence order.

(2) In this line:

 871   uint32_t cur = SET_BYTE_IN_INT(*aligned_dest, canon_compare_value, idx);

I'd like "*aligned_dest" replaced with "Atomic::load(aligned_dest)".

In addition, I think functions would be preferable to macros for the
helpers.  Put them in the Atomic::CmpxchgByteUsingInt class, with
inline definitions where you put the macro definitions.