RFR: 8267155: runtime/os/TestTracePageSizes times out [v4]

[Stefan Johansson]

On Tue, 18 May 2021 10:54:30 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> > 
> > I'm of course biased since I wrote the test. That said, I don't consider this _my_ test, so I won't object to changing it.
> 
> Sorry, I meant no offence. I think this test is really useful, since I want to be able to rely on the page sizes in the traces and not have to countercheck the smaps file. That's why I put work in it instead of just disabling it.
> 
None taken �� 

> What I meant was do you consider testing correct tracing of _explicit_ huge pages important enough on semi-old kernels before 3.8? If not, we can modify the test to skip older kernels completely, otherwise just the detect-THP-part?
> 
I'm good with keeping testing even on older kernels, since you run into this it obviously has some value testing it.

> > Personally I prefer the regex-parsing is clearer since it "parses" one segment at a time in the smaps file.
> 
> Unfortunately its not so easy since its smaps sections don't have a clear ending. I played around yesterday and gave up after a while. I also find regex code beyond a certain complexity really work intensive and difficult to debug.
> 
> The original regexp was also too loose, I think, since `(\\w+)-(\\w+)` - unanchored to the start of a line - matches the permissions if those contain dashes. And theoretically, you could accidentally match across secions, were one to miss a flag.
> 
True, the reg-ex approach has problems and as you say, future changes to the format might be more brittle with a reg-ex. 

> > I think if we go with this new parser, it would be nice to add some comments clarifying the steps.
> 
> Okay, I could do that.
> 
�� 

> > An alternative would be to have different regular expressions depending on the kernel version or first check if vmFlags are present and choose the correct expression. This would of course not help if this kind of parsing is still very slow on some systems.
> 
> I think its just more vulnerable against unknown or future smaps variants. E.g. relying on a specific order of entries.
> 
> > I like that you do a copy of smaps before parsing, that crossed my mind as well, to help debugging. Regarding smaller heap sizes. I don't see a problem if some of the test VMs fail getting large pages, since it tests that we still log the correct thing.
> 
> I prefer the "explicit-large-pages" test to really run a VM with explicit large pages. Otherwise we test with small pages, which would be the same as with the other tests.
> 
What I meant was that we can get the scenario where some reservations are small and some large, but that can still happen even with a smaller heap size, so I'm good with changing it.

> > Just wanted to share my thoughts before doing a proper review. I'm happy to review this patch if we decide to go with this new parser. If we do that I also think we should look at enabling parsing of `AnonHugePages` for the case where vmFlags are not present (similar to @shipilev approach in the last PR).
> 
> This test has been a bit of whack-a-mole; I am interested in a stable test which covers the important cases. E.g. I tried to find a clear definition of AnonHugePages but beyond that it requires the kernel to be compiled with THP I did not find any indication that this is synonymous with THP, or under which circumstances it is set, or whats its relationship to KernelPageSize is. I also suspect it is kernel-version-dependent like the other properties.
> 
> Frankly am interested in this test for testing trace of explicit huge pages correctly, not so much THP. Since this is what most of our customers use. THP have a bad rep here because they used to come with problems. And since hugetlbfs has gotten easier to use (eg with vm.nr_overcommit_hugepages) I usually use that instead of TPH.
> 

Yeah, the THP part might have been over-shooting it a bit, but I was quite happy when I found the `vmFlags` and never considered the older kernels. I'm good with disabling THP testing when vmFlags are not present.

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

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