RFR: 8261230: GC tracing of page sizes are wrong in a few places

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

RFR: 8261230: GC tracing of page sizes are wrong in a few places

Stefan Johansson
The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.

In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.

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

Commit messages:
 - 8261230-test-fix
 - 8261230: GC tracing of page sizes are wrong in a few places

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

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places

Albert Mingkun Yang
On Tue, 9 Feb 2021 19:42:15 GMT, Stefan Johansson <[hidden email]> wrote:

> The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.
>
> In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.

In the test, there's some string matching to detect if large page is properly set up. I think it's best to include an excerpt of the log showing both the success and failure modes in the comments. This way even readers who are not intimately familiar with the gc-logs output could still feel fairly confident that the output parsing part is indeed correct.

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

Changes requested by ayang (Author).

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places

Stefan Johansson
In reply to this post by Stefan Johansson
On Tue, 9 Feb 2021 19:42:15 GMT, Stefan Johansson <[hidden email]> wrote:

> The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.
>
> In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.

test/hotspot/jtreg/gc/g1/TestLargePageUseForAuxMemory.java line 59:

> 57:     static void checkSize(OutputAnalyzer output, long expectedSize, String pattern) {
> 58:         // First check if there is a large page failure associated with
> 59:         // the data structure being checked.

Are you thinking something like this @albertnetymk?
Suggestion:

        // First check if there is a large page failure associated with
        // the data structure being checked. In case of a large page
        // allocation failure the output will include logs like this for
        // the affected data structure:
        // [0.048s][debug][gc,heap,coops] Reserve regular memory without large pages
        // [0.048s][info ][pagesize     ] Next Bitmap: ... page_size=2M ...

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

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places

Albert Mingkun Yang
On Thu, 11 Feb 2021 16:20:07 GMT, Stefan Johansson <[hidden email]> wrote:

>> The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.
>>
>> In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.
>
> test/hotspot/jtreg/gc/g1/TestLargePageUseForAuxMemory.java line 59:
>
>> 57:     static void checkSize(OutputAnalyzer output, long expectedSize, String pattern) {
>> 58:         // First check if there is a large page failure associated with
>> 59:         // the data structure being checked.
>
> Are you thinking something like this @albertnetymk?
> Suggestion:
>
>         // First check if there is a large page failure associated with
>         // the data structure being checked. In case of a large page
>         // allocation failure the output will include logs like this for
>         // the affected data structure:
>         // [0.048s][debug][gc,heap,coops] Reserve regular memory without large pages
>         // [0.048s][info ][pagesize     ] Next Bitmap: ... page_size=4K ...

Yes, and also for `checkLargePagesEnabled`. It's not obvious to me why we parse the output in those two places, one looking for the failure mode, and the other looking for the success mode. That's why I asked for an sample of the "expected" log output.

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

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places

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

> The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.
>
> In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.

Looks good. `os::trace_page_sizes_for_requested_size` is not easy to understand, especially with the alignment vs preferred_page_size semantic. Not sure what alignment has to do with preferred page size.

We could prevent these kind of errors and make the code more readable by introducing a page size enum. We only have a handful of valid values anyway.

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

Marked as reviewed by stuefe (Reviewer).

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places

Stefan Johansson
In reply to this post by Albert Mingkun Yang
On Thu, 11 Feb 2021 16:29:53 GMT, Albert Mingkun Yang <[hidden email]> wrote:

>> test/hotspot/jtreg/gc/g1/TestLargePageUseForAuxMemory.java line 59:
>>
>>> 57:     static void checkSize(OutputAnalyzer output, long expectedSize, String pattern) {
>>> 58:         // First check if there is a large page failure associated with
>>> 59:         // the data structure being checked.
>>
>> Are you thinking something like this @albertnetymk?
>> Suggestion:
>>
>>         // First check if there is a large page failure associated with
>>         // the data structure being checked. In case of a large page
>>         // allocation failure the output will include logs like this for
>>         // the affected data structure:
>>         // [0.048s][debug][gc,heap,coops] Reserve regular memory without large pages
>>         // [0.048s][info ][pagesize     ] Next Bitmap: ... page_size=4K ...
>
> Yes, and also for `checkLargePagesEnabled`. It's not obvious to me why we parse the output in those two places, one looking for the failure mode, and the other looking for the success mode. That's why I asked for an sample of the "expected" log output.

I think the check if large pages are enable is pretty straight forward. We should never expect large page sizes in the output unless large pages are enabled. I do however agree that this check is a bit clunky. Would it help to extract it to a separate function? Something like `largePagesAllocationFailure(pattern)`, I could also change the name of the function above to just be `largePagesEnabled()` then the code reads even better?

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

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places [v2]

Stefan Johansson
In reply to this post by Stefan Johansson
> The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.
>
> In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.

Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:

  Albert review
 
  Renamed helper to improve how the code read. Also extracted the failure check into a separate function.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2486/files
  - new: https://git.openjdk.java.net/jdk/pull/2486/files/76faa2e4..b46f6a75

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2486&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2486&range=00-01

  Stats: 22 lines in 1 file changed: 15 ins; 1 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2486.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2486/head:pull/2486

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places [v2]

Stefan Johansson
In reply to this post by Thomas Stuefe
On Thu, 11 Feb 2021 17:31:57 GMT, Thomas Stuefe <[hidden email]> wrote:

> Looks good. `os::trace_page_sizes_for_requested_size` is not easy to understand, especially with the alignment vs preferred_page_size semantic. Not sure what alignment has to do with preferred page size.

Thanks. I agree that it is not that straight forward but I think the intention here is to pass in both page size and alignment to make it easier to understand why the actual size might be large than the requested size. For example in cases like this:
[debug][gc,heap,coops] Reserve regular memory without large pages
[info ][pagesize     ] Next Bitmap: req_size=32000K base=0x00007fa4ef400000 page_size=4K alignment=2M size=32M
 

> We could prevent these kind of errors and make the code more readable by introducing a page size enum. We only have a handful of valid values anyway.

Yes, there is certainly room for improvement in this area =)

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

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places [v2]

Albert Mingkun Yang
In reply to this post by Stefan Johansson
On Thu, 11 Feb 2021 17:29:34 GMT, Stefan Johansson <[hidden email]> wrote:

>> Yes, and also for `checkLargePagesEnabled`. It's not obvious to me why we parse the output in those two places, one looking for the failure mode, and the other looking for the success mode. That's why I asked for an sample of the "expected" log output.
>
> I think the check if large pages are enable is pretty straight forward. We should never expect large page sizes in the output unless large pages are enabled. I do however agree that this check is a bit clunky. Would it help to extract it to a separate function? Something like `largePagesAllocationFailure(pattern)`, I could also change the name of the function above to just be `largePagesEnabled()` then the code reads even better?

I overlooked the fact that `largePagesAllocationFailure` is pattern/data structure specific. I am happy with the current patch. Thank you.

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

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places [v2]

Albert Mingkun Yang
In reply to this post by Stefan Johansson
On Thu, 11 Feb 2021 18:10:57 GMT, Stefan Johansson <[hidden email]> wrote:

>> The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.
>>
>> In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.
>
> Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:
>
>   Albert review
>  
>   Renamed helper to improve how the code read. Also extracted the failure check into a separate function.

Marked as reviewed by ayang (Author).

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

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

Integrated: 8261230: GC tracing of page sizes are wrong in a few places

Stefan Johansson
In reply to this post by Stefan Johansson
On Tue, 9 Feb 2021 19:42:15 GMT, Stefan Johansson <[hidden email]> wrote:

> The usage of `os::trace_page_sizes()` and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper `ReservedSpace::actual_reserved_page_size()` instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the `ReservedSpace`, but for now the helper is good enough.
>
> In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.

This pull request has now been integrated.

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

8261230: GC tracing of page sizes are wrong in a few places

Reviewed-by: ayang, stuefe

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

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

Re: RFR: 8261230: GC tracing of page sizes are wrong in a few places [v2]

Stefan Johansson
In reply to this post by Albert Mingkun Yang
On Fri, 12 Feb 2021 12:24:29 GMT, Albert Mingkun Yang <[hidden email]> wrote:

>> Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Albert review
>>  
>>   Renamed helper to improve how the code read. Also extracted the failure check into a separate function.
>
> Marked as reviewed by ayang (Author).

Thanks for the reviews @albertnetymk and @tstuefe!

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

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