RFR: 8017629: G1: UseSHM in combination with a G1HeapRegionSize > os::large_page_size() falls back to use small pages

Tue Apr 12 14:23:27 UTC 2016

Hi Stefan,

On Mon, Apr 11, 2016 at 3:52 PM, Stefan Karlsson <stefan.karlsson at oracle.com
> wrote:

> Hi Thomas,
>
> On 2016-04-11 14:39, Thomas Stüfe wrote:
>
> Hi Stefan,
>
> short question, why the mmap before the shmat? Why not shmat right away at
> the requested address?
>
>
> If we have a requested_address we do exactly what you propose.
>
>   if (req_addr == NULL && alignment > os::large_page_size()) {
>     return shmat_with_large_alignment(shmid, bytes, alignment);
>   } else {
>     return shmat_with_normal_alignment(shmid, req_addr);
>   }
>
> ...
>
> static char* shmat_with_normal_alignment(int shmid, char* req_addr) {
>   char* addr = (char*)shmat(shmid, req_addr, 0);
>
>   if ((intptr_t)addr == -1) {
>     shm_warning_with_errno("Failed to attach shared memory.");
>     return NULL;
>   }
>
>   return addr;
> }
>
>
> It's when you don't have a requested address that mmap is used to find a
> large enough virtual memory area.
>
>
Sorry, seems I did not look at this coding thoroughly enough. I understand
now that you do mmap to allocate and then to cut away the extra
pre-/post-space, something which would not be possible with shmat, which
cannot be unmapped page-wise.

But I am still not sure why we do it his way:

3429 static char* shmat_with_alignment(int shmid, size_t bytes, size_t
alignment, char* req_addr) {
3430   // If there's no requested address, the shmat call can return memory
that is not
3431   // 'alignment' aligned, if the given alignment is larger than the
large page size.
3432   // Special care needs to be taken to ensure that we get aligned
memory back.
3433   if (req_addr == NULL && alignment > os::large_page_size()) {
3434     return shmat_with_large_alignment(shmid, bytes, alignment);
3435   } else {
3436     return shmat_with_normal_alignment(shmid, req_addr);
3437   }
3438 }

For req_addr==0 and big alignment, we attach at the given alignment
("shmat_with_large_alignment").
For req_addr!=0, we attach at the given requested address
("shmat_with_normal_alignment").
For req_addr==0 and smaller alignment, we ignore the alignment and attach
anywhere?

Maybe I am slow, but why does it matter if the alignment is large or small?
Why not just distinguish between:

1) address given (req_addr!=0): in this case we attach at this req_addr and
rely on the user having aligned the address properly for his purposes. We
specify 0 for flags, so we will attach at exactly the given address or
fail. In this case we could simply ignore the given alignment - if one was
given - or just use it to counter-check the req_addr.

2) alignment given (req_addr==0 and alignment > 0): attach at the given
alignment using mmap-before-shmat. This could be done for any alignment, be
it large or small.

Functions would become simpler and also could be clearer named (e.g.
"shmat_at_address" and "shmat_with_alignment", respectivly).

----

This:

3402   if ((intptr_t)addr == -1) {
3403     shm_warning_with_errno("Failed to attach shared memory.");
3404     // Since we don't know if the kernel unmapped the pre-reserved
memory area
3405     // we can't unmap it, since that would potentially unmap memory
that was
3406     // mapped from other threads.
3407     return NULL;
3408   }

seems scary. Means for every call this happens, we leak the reserved (not
committed) address space? For most cases (anything but ENOMEM, actually)
could we at least assert?:

EACCES - should not happen: we created the shared memory and are its owner
EIDRM  - should not happen.
EINVAL - should not happen. (you already check now the attach address for
alignment to SHMLBA, so this is covered)

---

Smaller nits:

Functions called "shmat_..." suggest shmat-like behaviour, so could we have
them return -1 instead of NULL in case of error?

Kind Regards, Thomas

>
> Also note that mmap- and shmat-allocated memory may have different
> alignment requirements: mmap requires a page-aligned request address,
> whereas shmat requires alignment to SHMLBA, which may be multiple pages
> (e.g. for ARM:
> <http://lxr.free-electrons.com/source/arch/arm/include/asm/shmparam.h#L9>
> http://lxr.free-electrons.com/source/arch/arm/include/asm/shmparam.h#L9).
> So, for this shat-over-mmap trick to work, request address has to be
> aligned to SHMLBA, not just page size.
>
> I see that you assert alignment of requ address to os::large_page_size(),
> which I would assume is a multiple of SHMLBA, but I am not sure of this.
>
>
> I've added some defensive code and asserts to catch this if/when this
> assumption fails:
>
> http://cr.openjdk.java.net/~stefank/8017629/webrev.02.delta/
> http://cr.openjdk.java.net/~stefank/8017629/webrev.02
>
> I need to verify that this works on other machines than my local Linux x64
> machine.
>
> Thanks,
> StefanK
>
>
> Kind Regards, Thomas
>
>
>
> On Mon, Apr 11, 2016 at 1:03 PM, Stefan Karlsson <
> stefan.karlsson at oracle.com> wrote:
>
>> Hi all,
>>
>> Please review this patch to enable SHM large page allocations even when
>> the requested alignment is larger than os::large_page_size().
>>
>> http://cr.openjdk.java.net/~stefank/8017629/webrev.01
>> https://bugs.openjdk.java.net/browse/JDK-8017629
>>
>> G1 is affected by this bug since it requires the heap to start at an
>> address that is aligned with the heap region size. The patch fixes this by
>> changing the UseSHM large pages allocation code. First, virtual memory with
>> correct alignment is pre-reserved and then the large pages are attached to
>> this memory area.
>>
>> Tested with vm.gc.testlist and ExecuteInternaVMTests
>>
>> Thanks,
>> StefanK
>>
>
>
>