RFR: JDK-8270308: Amalloc aligns size but not return value (take 2) [v5]

[Coleen Phillimore]

On Sun, 25 Jul 2021 05:22:03 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi,
>> 
>> may I please have reviews for this fix. This fixes an issue with arena allocation alignment which can only happen on 32-bit. 
>> 
>> The underlying problem is that even though Arenas offer ways to allocate with different alignment (Amalloc and AmallocWords), allocation alignment is not guaranteed. This sequence will not work on 32-bit:
>> 
>> 
>> Arena ar;
>> p1 = ar.AmallocWords();  // return 32bit aligned address
>> p2 = ar.Amalloc();       // supposed to return 64bit aligned address but does not.
>> 
>> 
>> This patch is the bare minimum needed to fix the specific problem; I proposed a larger patch before which redid arena alignment handling but it was found too complex: https://github.com/openjdk/jdk/pull/4784 . 
>> 
>> This fix is limited to `Amalloc()` and aligns `_hwm` to be 64bit aligned before allocation. But since chunk boundaries are not guaranteed to be 64-bit aligned either, additional care must be taken to not overflow `_max`.  Since this adds instructions into a hot allocation path, I restricted this code to 32 bit - it is only needed there.
>> 
>> Remaining issues:
>> 
>> - `Amalloc...` align the allocation size in an attempt to ensure allocation alignment. This is not needed nor sufficient, we could just remove that code. I left it untouched to keep the patch minimal. I also left the `// ... for 32 bits this should align _hwm as well.` comment in `Amalloc()` though I think it is wrong.
>> - The chunk dimensions are not guaranteed to be 64-bit aligned:
>> 	1) Chunk bottom depends on Chunk start. We currently align the header size, but if the chunk starts at an unaligned address, this is not sufficient. It's not a real issue though as long as Chunks are C-heap allocated since malloc alignment is at least 64bit on our 32bit platforms. More of a beauty spot, since this is an implicit assumption which we don't really check.
>> 	2) Chunk top and hence Arena::_max are not guaranteed to be 64-bit aligned either. They depend on the input chunk length, which is not even aligned for the standard chunk sizes used in ChunkPool. And users can hand in any size they want. Fixing this would require more widespread changes to the ChunkPool logic though, so I left it as it is.
>> 	3) similarly, we cannot just align Arena::_max, because that is set in many places and we need to cover all of that; it ties in with Arena rollback as well.
>> 
>> Because of (2) and (3), I had to add the overflow check into `Amalloc()` - any other way to solve this would result in more widespread changes.
>> 	
>> -----
>> 
>> Tests:
>> 
>> - I tested the provided gtest on both 64-bit and 32-bit platforms (with and without the fix, without it shows the expected problem)
>> - GHA
>> - Tests are scheduled at SAP.
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix Arena::grow for misaligned large grow sizes

These are small comments but maybe important.

src/hotspot/share/memory/arena.cpp line 185:

> 183:   // |           |p |                                            |
> 184:   // +-----------+--+--------------------------------------------+
> 185:   // A           B  C                                            D

Hooray for ascii art!

src/hotspot/share/memory/arena.cpp line 197:

> 195:   assert(is_aligned(length, BytesPerLong), "chunk payload length not 64-bit aligned: "
> 196:                                            SIZE_FORMAT ".", length);
> 197:   size_t bytes = ARENA_ALIGN(sizeofChunk) + length;

Are Chunk::size, medium_size, init_size and tiny size all 64 bit aligned?  I see that they're aligned here again but it seems like those sizes being invariantly aligned to 64 bit would prevent future bugs (if they're not already).  Also maybe needing a static_assert ?

src/hotspot/share/memory/arena.cpp line 208:

> 206:        vm_exit_out_of_memory(bytes, OOM_MALLOC_ERROR, "Chunk::new");
> 207:      }
> 208:      assert(is_aligned(p, BytesPerLong), "Chunk start address not malloc aligned?");

Should this be ARENA_AMALLOC_ALIGNMENT, in case we have to change it back to 128 bits for some reason?

src/hotspot/share/memory/arena.hpp line 55:

> 53:     // default sizes; make them slightly smaller than 2**k to guard against
> 54:     // buddy-system style malloc implementations
> 55:     // Note: please keep these constants 64-bit aligned.

Can you add a static_assert(is_aligned(slack, ARENA_ALIGNMENT)?

test/hotspot/gtest/memory/test_arena.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.

Did you want an SAP copyright?

test/hotspot/gtest/memory/test_arena.cpp line 30:

> 28: #ifndef LP64
> 29: // These tests below are about alignment issues when mixing Amalloc and AmallocWords.
> 30: // Since on 64-bit these APIs offer the same alignment, they only matter for 32-bit.

Thank you so much for testing this on 32 bit platforms.

test/hotspot/gtest/memory/test_arena.cpp line 39:

> 37:   void* p2 = ar.Amalloc(BytesPerLong);
> 38:   ASSERT_TRUE(is_aligned(p1, BytesPerWord));
> 39:   ASSERT_TRUE(is_aligned(p2, BytesPerLong));

Should BytesPerLong in this test be ARENA_AMALLOC_ALIGNMENT?

-------------

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4835