RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

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

RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Stefan Johansson
When adding a code heap the page size used with the underlying mapping is traced using `os::trace_page_sizes`. The old code tried to estimate the page-size based on the size, but the mapping has already been done so it is better to check the passed in `ReservedSpace`. Today we don't record the page size in the ReservedSpace, but we have a helper to do a good estimate: `ReservedSpace::actual_reserved_page_size()`. The proposal is to use this function.

When changing this I also realized that the traced min-size used un-aligned value while the actual `initialize`-call correctly uses the aligned size. Changed so that we also use the aligned size for tracing.

I'm currently doing some more work in this area and while I haven't added a specific test for this issue I have created a test I plan to integrate separately when a few more needed changes have gone in. The test is Linux-only and validates the output from `os::trace_page_sizes` against the information in `/proc/self/smaps`.

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

Commit messages:
 - 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Changes: https://git.openjdk.java.net/jdk/pull/2481/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2481&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261029
  Stats: 9 lines in 1 file changed: 1 ins; 6 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2481.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2481/head:pull/2481

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Vladimir Kozlov-2
On Tue, 9 Feb 2021 13:45:38 GMT, Stefan Johansson <[hidden email]> wrote:

> When adding a code heap the page size used with the underlying mapping is traced using `os::trace_page_sizes`. The old code tried to estimate the page-size based on the size, but the mapping has already been done so it is better to check the passed in `ReservedSpace`. Today we don't record the page size in the ReservedSpace, but we have a helper to do a good estimate: `ReservedSpace::actual_reserved_page_size()`. The proposal is to use this function.
>
> When changing this I also realized that the traced min-size used un-aligned value while the actual `initialize`-call correctly uses the aligned size. Changed so that we also use the aligned size for tracing.
>
> I'm currently doing some more work in this area and while I haven't added a specific test for this issue I have created a test I plan to integrate separately when a few more needed changes have gone in. The test is Linux-only and validates the output from `os::trace_page_sizes` against the information in `/proc/self/smaps`.

I think the difference comes from JDK-8087339 changes which did not update this code:
https://github.com/openjdk/jdk/commit/925a508b2bb1600909418405e9e5ea1a93a94580

I agree that alignment here should match one used in CodeCache::reserve_heap_memory() and not recalculated. But I am not sure actual_reserved_page_size() returns correct value.
May be we should record value in CodeHeap object when it is created.

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Stefan Johansson
On Tue, 9 Feb 2021 18:44:27 GMT, Vladimir Kozlov <[hidden email]> wrote:

> I agree that alignment here should match one used in CodeCache::reserve_heap_memory() and not recalculated. But I am not sure actual_reserved_page_size() returns correct value.
> May be we should record value in CodeHeap object when it is created.

Yes, the `actual_reserved_page_size()` is far from perfect and I plan to update `ReservedSpace` to have a page size member that can be queried in places like this and then we can remove this helper. This will be required once we allow multiple large page sizes ([PR#1153](https://github.com/openjdk/jdk/pull/1153)). That said, `actual_reserved_page_size()` is currently doing a good job returning the correct page size since it is considering both if the space is "special", what alignment it used and if transparent huge pages are enabled.

I would prefer doing the change that records the page size i `ReservedSpace` as a separate patch and in that patch also remove all uses of `actual_reserved_page_size()`. Doing this change now is required to be able to integrate the [new test](https://github.com/openjdk/jdk/compare/master...kstefanj:test-for-trace-page-sizes) I mentioned, and I think it will be helpful for work in this area going forward.

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Thomas Stuefe
In reply to this post by Stefan Johansson
On Tue, 9 Feb 2021 13:45:38 GMT, Stefan Johansson <[hidden email]> wrote:

> When adding a code heap the page size used with the underlying mapping is traced using `os::trace_page_sizes`. The old code tried to estimate the page-size based on the size, but the mapping has already been done so it is better to check the passed in `ReservedSpace`. Today we don't record the page size in the ReservedSpace, but we have a helper to do a good estimate: `ReservedSpace::actual_reserved_page_size()`. The proposal is to use this function.
>
> When changing this I also realized that the traced min-size used un-aligned value while the actual `initialize`-call correctly uses the aligned size. Changed so that we also use the aligned size for tracing.
>
> I'm currently doing some more work in this area and while I haven't added a specific test for this issue I have created a test I plan to integrate separately when a few more needed changes have gone in. The test is Linux-only and validates the output from `os::trace_page_sizes` against the information in `/proc/self/smaps`.

Hi Stefan,

I think this is okay if we keep in mind that `ReservedSpace::actual_reserved_page_size` needs fixing up. At the latest after JDK-8256155 hits. Rather use one function than to have to hunt down all the places where caller code tries to guess the page size.

Are you currently working on this? If yes, what are your plans? I have a half finished prototype where I changed os::reserve_xxx() to return meta information about the reservation alongside the pointer, one of them the reserved page size.

Have you guys decided whether its okay to remove the "multiple page sizes per reservation" feature?

There is also another possible simplification I was thinking about, which is to remove the "UseSHM" feature from Linux. I honestly do not know why we still need it. That would simplify rework of large page handling on Linux a lot. I did ask around in December: https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-December/046885.html , but did not get many answers.

Cheers, Thomas

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

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Stefan Johansson
On Wed, 10 Feb 2021 06:45:51 GMT, Thomas Stuefe <[hidden email]> wrote:

> Are you currently working on this? If yes, what are your plans? I have a half finished prototype where I changed os::reserve_xxx() to return meta information about the reservation alongside the pointer, one of them the reserved page size.
>

Yes, I also have a prototype that I've been playing around with. It currently uses an out-parameter to return the page size from `os::reserve_memory_special*` calls. The page sizes is then saved in the ReservedSpace for later use.

> Have you guys decided whether its okay to remove the "multiple page sizes per reservation" feature?
>

I've done some investigations but nothing have been decided. For now my prototype will return the smallest page size used by a mapping. Going forward I would like to do this, but it feels more urgent to get the other things in place first.

> There is also another possible simplification I was thinking about, which is to remove the "UseSHM" feature from Linux. I honestly do not know why we still need it. That would simplify rework of large page handling on Linux a lot. I did ask around in December: https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-December/046885.html , but did not get many answers.
>

I know and I've been digging around in this area and agree even more now that it would be great to get rid of `UseSHM`. Not sure how if everybody agrees though and I'm currently working on small fix for `UseSHM` so that at least we don't leave it enabled everytime someone sets `+UseLargePages` without having any explicit large pages enabled ([PR#2488](https://github.com/openjdk/jdk/pull/2488)).

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Thomas Stuefe
On Wed, 10 Feb 2021 10:25:43 GMT, Stefan Johansson <[hidden email]> wrote:

> > Are you currently working on this? If yes, what are your plans? I have a half finished prototype where I changed os::reserve_xxx() to return meta information about the reservation alongside the pointer, one of them the reserved page size.
>
> Yes, I also have a prototype that I've been playing around with. It currently uses an out-parameter to return the page size from `os::reserve_memory_special*` calls. The page sizes is then saved in the ReservedSpace for later use.

I think that makes sense as a solution for this. My attempt was along this:
void* os::reserve_xxx(size, ... blabla..., reservation_info_t* info = NULL);
with reservation_info_t being a holder for information both "public" and opaque: e.g. whether this is executable memory (e.g. for MacOS MAP_JIT issue on committiong), the page size of course, as well as a way for platforms to piggyback internal information (eg. memory type used on AIX).

But your solution sounds simpler, and its sufficient at least for now. So I don't think we work at cross purposes.

>
> > Have you guys decided whether its okay to remove the "multiple page sizes per reservation" feature?
>
> I've done some investigations but nothing have been decided. For now my prototype will return the smallest page size used by a mapping. Going forward I would like to do this, but it feels more urgent to get the other things in place first.

No problem, but good to know its not forgotten.

>
> > There is also another possible simplification I was thinking about, which is to remove the "UseSHM" feature from Linux. I honestly do not know why we still need it. That would simplify rework of large page handling on Linux a lot. I did ask around in December: https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-December/046885.html , but did not get many answers.
>
> I know and I've been digging around in this area and agree even more now that it would be great to get rid of `UseSHM`. Not sure how if everybody agrees though and I'm currently working on small fix for `UseSHM` so that at least we don't leave it enabled everytime someone sets `+UseLargePages` without having any explicit large pages enabled ([PR#2488](https://github.com/openjdk/jdk/pull/2488)).

Nice that you think the same. I am not sure many people are around which know the history. Maybe we should ask Andrew Haley, I believe he wrote some of that coding.

I commented on your PR in your PR.

Cheers, Thomas

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Vladimir Kozlov-2
On Wed, 10 Feb 2021 14:45:55 GMT, Thomas Stuefe <[hidden email]> wrote:

>>> Are you currently working on this? If yes, what are your plans? I have a half finished prototype where I changed os::reserve_xxx() to return meta information about the reservation alongside the pointer, one of them the reserved page size.
>>>
>>
>> Yes, I also have a prototype that I've been playing around with. It currently uses an out-parameter to return the page size from `os::reserve_memory_special*` calls. The page sizes is then saved in the ReservedSpace for later use.
>>
>>> Have you guys decided whether its okay to remove the "multiple page sizes per reservation" feature?
>>>
>>
>> I've done some investigations but nothing have been decided. For now my prototype will return the smallest page size used by a mapping. Going forward I would like to do this, but it feels more urgent to get the other things in place first.
>>
>>> There is also another possible simplification I was thinking about, which is to remove the "UseSHM" feature from Linux. I honestly do not know why we still need it. That would simplify rework of large page handling on Linux a lot. I did ask around in December: https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-December/046885.html , but did not get many answers.
>>>
>>
>> I know and I've been digging around in this area and agree even more now that it would be great to get rid of `UseSHM`. Not sure how if everybody agrees though and I'm currently working on small fix for `UseSHM` so that at least we don't leave it enabled everytime someone sets `+UseLargePages` without having any explicit large pages enabled ([PR#2488](https://github.com/openjdk/jdk/pull/2488)).
>
>> > Are you currently working on this? If yes, what are your plans? I have a half finished prototype where I changed os::reserve_xxx() to return meta information about the reservation alongside the pointer, one of them the reserved page size.
>>
>> Yes, I also have a prototype that I've been playing around with. It currently uses an out-parameter to return the page size from `os::reserve_memory_special*` calls. The page sizes is then saved in the ReservedSpace for later use.
>
> I think that makes sense as a solution for this. My attempt was along this:
> void* os::reserve_xxx(size, ... blabla..., reservation_info_t* info = NULL);
> with reservation_info_t being a holder for information both "public" and opaque: e.g. whether this is executable memory (e.g. for MacOS MAP_JIT issue on committiong), the page size of course, as well as a way for platforms to piggyback internal information (eg. memory type used on AIX).
>
> But your solution sounds simpler, and its sufficient at least for now. So I don't think we work at cross purposes.
>
>>
>> > Have you guys decided whether its okay to remove the "multiple page sizes per reservation" feature?
>>
>> I've done some investigations but nothing have been decided. For now my prototype will return the smallest page size used by a mapping. Going forward I would like to do this, but it feels more urgent to get the other things in place first.
>
> No problem, but good to know its not forgotten.
>
>>
>> > There is also another possible simplification I was thinking about, which is to remove the "UseSHM" feature from Linux. I honestly do not know why we still need it. That would simplify rework of large page handling on Linux a lot. I did ask around in December: https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-December/046885.html , but did not get many answers.
>>
>> I know and I've been digging around in this area and agree even more now that it would be great to get rid of `UseSHM`. Not sure how if everybody agrees though and I'm currently working on small fix for `UseSHM` so that at least we don't leave it enabled everytime someone sets `+UseLargePages` without having any explicit large pages enabled ([PR#2488](https://github.com/openjdk/jdk/pull/2488)).
>
> Nice that you think the same. I am not sure many people are around which know the history. Maybe we should ask Andrew Haley, I believe he wrote some of that coding.
>
> I commented on your PR in your PR.
>
> Cheers, Thomas

> > I agree that alignment here should match one used in CodeCache::reserve_heap_memory() and not recalculated. But I am not sure actual_reserved_page_size() returns correct value.
> > May be we should record value in CodeHeap object when it is created.
>
> Yes, the `actual_reserved_page_size()` is far from perfect and I plan to update `ReservedSpace` to have a page size member that can be queried in places like this and then we can remove this helper. This will be required once we allow multiple large page sizes ([PR#1153](https://github.com/openjdk/jdk/pull/1153)). That said, `actual_reserved_page_size()` is currently doing a good job returning the correct page size since it is considering both if the space is "special", what alignment it used and if transparent huge pages are enabled.
>
> I would prefer doing the change that records the page size i `ReservedSpace` as a separate patch and in that patch also remove all uses of `actual_reserved_page_size()`. Doing this change now is required to be able to integrate the [new test](https://github.com/openjdk/jdk/compare/master...kstefanj:test-for-trace-page-sizes) I mentioned, and I think it will be helpful for work in this area going forward.

Are you talking about [8261230](https://bugs.openjdk.java.net/browse/JDK-8261230) to do recording?
Okay, then I am fine with this change.

What testing you did for current changes?

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Stefan Johansson
In reply to this post by Thomas Stuefe
On Wed, 10 Feb 2021 14:45:55 GMT, Thomas Stuefe <[hidden email]> wrote:

>
> ```
> void* os::reserve_xxx(size, ... blabla..., reservation_info_t* info = NULL);
> ```
>
> with reservation_info_t being a holder for information both "public" and opaque: e.g. whether this is executable memory (e.g. for MacOS MAP_JIT issue on committiong), the page size of course, as well as a way for platforms to piggyback internal information (eg. memory type used on AIX).
>
> But your solution sounds simpler, and its sufficient at least for now. So I don't think we work at cross purposes.
>

Yes, we'll see who gets around to making it happen first :)

> > > There is also another possible simplification I was thinking about, which is to remove the "UseSHM" feature from Linux. I honestly do not know why we still need it. That would simplify rework of large page handling on Linux a lot. I did ask around in December: https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-December/046885.html , but did not get many answers.
> >
> >
> > I know and I've been digging around in this area and agree even more now that it would be great to get rid of `UseSHM`. Not sure how if everybody agrees though and I'm currently working on small fix for `UseSHM` so that at least we don't leave it enabled everytime someone sets `+UseLargePages` without having any explicit large pages enabled ([PR#2488](https://github.com/openjdk/jdk/pull/2488)).
>
> Nice that you think the same. I am not sure many people are around which know the history. Maybe we should ask Andrew Haley, I believe he wrote some of that coding.
>
> I commented on your PR in your PR.

Thanks for all your input, very helpful =)

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Stefan Johansson
In reply to this post by Vladimir Kozlov-2
On Wed, 10 Feb 2021 17:55:00 GMT, Vladimir Kozlov <[hidden email]> wrote:

> > I would prefer doing the change that records the page size i `ReservedSpace` as a separate patch and in that patch also remove all uses of `actual_reserved_page_size()`. Doing this change now is required to be able to integrate the [new test](https://github.com/openjdk/jdk/compare/master...kstefanj:test-for-trace-page-sizes) I mentioned, and I think it will be helpful for work in this area going forward.
>
> Are you talking about [8261230](https://bugs.openjdk.java.net/browse/JDK-8261230) to do recording?
> Okay, then I am fine with this change.
>

No that change is also just fixing places where the traced page size is incorrect. Recording the page size in `ReservedSpace` will be handled by [JDK-8261527](https://bugs.openjdk.java.net/browse/JDK-8261527) going forward.

> What testing you did for current changes?

I've done a fair amount of local JTREG testing as well as manual testing to verify we get the correct result with and without explicit large pages enabled. Are there any specific compiler tests you know of that rely on the output from this tracing?

I've also done mach5 tier 1-3 but we have few to none systems in there with explicit huge pages enabled, so that is mostly for sanity.

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Vladimir Kozlov-2
In reply to this post by Stefan Johansson
On Tue, 9 Feb 2021 13:45:38 GMT, Stefan Johansson <[hidden email]> wrote:

> When adding a code heap the page size used with the underlying mapping is traced using `os::trace_page_sizes`. The old code tried to estimate the page-size based on the size, but the mapping has already been done so it is better to check the passed in `ReservedSpace`. Today we don't record the page size in the ReservedSpace, but we have a helper to do a good estimate: `ReservedSpace::actual_reserved_page_size()`. The proposal is to use this function.
>
> When changing this I also realized that the traced min-size used un-aligned value while the actual `initialize`-call correctly uses the aligned size. Changed so that we also use the aligned size for tracing.
>
> I'm currently doing some more work in this area and while I haven't added a specific test for this issue I have created a test I plan to integrate separately when a few more needed changes have gone in. The test is Linux-only and validates the output from `os::trace_page_sizes` against the information in `/proc/self/smaps`.

Good.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Tobias Hartmann-3
In reply to this post by Stefan Johansson
On Tue, 9 Feb 2021 13:45:38 GMT, Stefan Johansson <[hidden email]> wrote:

> When adding a code heap the page size used with the underlying mapping is traced using `os::trace_page_sizes`. The old code tried to estimate the page-size based on the size, but the mapping has already been done so it is better to check the passed in `ReservedSpace`. Today we don't record the page size in the ReservedSpace, but we have a helper to do a good estimate: `ReservedSpace::actual_reserved_page_size()`. The proposal is to use this function.
>
> When changing this I also realized that the traced min-size used un-aligned value while the actual `initialize`-call correctly uses the aligned size. Changed so that we also use the aligned size for tracing.
>
> I'm currently doing some more work in this area and while I haven't added a specific test for this issue I have created a test I plan to integrate separately when a few more needed changes have gone in. The test is Linux-only and validates the output from `os::trace_page_sizes` against the information in `/proc/self/smaps`.

Looks good to me, thanks for fixing!

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Stefan Johansson
In reply to this post by Vladimir Kozlov-2
On Wed, 10 Feb 2021 22:57:28 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> When adding a code heap the page size used with the underlying mapping is traced using `os::trace_page_sizes`. The old code tried to estimate the page-size based on the size, but the mapping has already been done so it is better to check the passed in `ReservedSpace`. Today we don't record the page size in the ReservedSpace, but we have a helper to do a good estimate: `ReservedSpace::actual_reserved_page_size()`. The proposal is to use this function.
>>
>> When changing this I also realized that the traced min-size used un-aligned value while the actual `initialize`-call correctly uses the aligned size. Changed so that we also use the aligned size for tracing.
>>
>> I'm currently doing some more work in this area and while I haven't added a specific test for this issue I have created a test I plan to integrate separately when a few more needed changes have gone in. The test is Linux-only and validates the output from `os::trace_page_sizes` against the information in `/proc/self/smaps`.
>
> Good.

Thanks for the reviews, @vnkozlov, @tstuefe and @TobiHartmann

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

PR: https://git.openjdk.java.net/jdk/pull/2481
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Stefan Johansson
In reply to this post by Stefan Johansson
On Tue, 9 Feb 2021 13:45:38 GMT, Stefan Johansson <[hidden email]> wrote:

> When adding a code heap the page size used with the underlying mapping is traced using `os::trace_page_sizes`. The old code tried to estimate the page-size based on the size, but the mapping has already been done so it is better to check the passed in `ReservedSpace`. Today we don't record the page size in the ReservedSpace, but we have a helper to do a good estimate: `ReservedSpace::actual_reserved_page_size()`. The proposal is to use this function.
>
> When changing this I also realized that the traced min-size used un-aligned value while the actual `initialize`-call correctly uses the aligned size. Changed so that we also use the aligned size for tracing.
>
> I'm currently doing some more work in this area and while I haven't added a specific test for this issue I have created a test I plan to integrate separately when a few more needed changes have gone in. The test is Linux-only and validates the output from `os::trace_page_sizes` against the information in `/proc/self/smaps`.

This pull request has now been integrated.

Changeset: eef86a80
Author:    Stefan Johansson <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/eef86a80
Stats:     9 lines in 1 file changed: 1 ins; 6 del; 2 mod

8261029: Code heap page sizes not traced correctly using os::trace_page_sizes

Reviewed-by: kvn, stuefe, thartmann

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

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