Read exactly two tokens from Linux transparent huge pages sysfs config

[jayaddison]

There was an unexpected and hard-to-spot issue here: the /sys/kernel/mm/transparent_hugepage/enabled file contains three entries, and the std::ifstream reader was reading two values on each loop iteration, resulting in incorrect behaviour.

Resolves#37064

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

[RaisinTen]

Thanks for fixing this! 🎉

Note to whoever lands this

The commit message needs to be amended to include:

Fixes: https://github.com/nodejs/node/issues/37064 

and be wrapped at 72 columns.

[jayaddison]

Thanks @RaisinTen, and my mistake regarding the commit message linewrapping, I'll note that for next time.

I was considering adding a small amount of test coverage here; it's been a while since I've coded much C++ so it could take a little while, but that offer stands if it'd be useful. I'd also vaguely considered refactoring this to support an arbitrary number of tokens in the hugepages flag file (by checking for presence of any of a -- possibly std::set -- of valid 'positive' values while reading the file token-by-token).

[RaisinTen]

I was considering adding a small amount of test coverage here; it's been a while since I've coded much C++ so it could take a little while, but that offer stands if it'd be useful.

I'm curious to know how you would approach testing this. For reference, here is an already existing test file to test
the --use-largepages option: https://github.com/nodejs/node/blob/master/test/parallel/test-startup-large-pages.js.

I'd also vaguely considered refactoring this to support an arbitrary number of tokens in the hugepages flag file (by checking for presence of any of a -- possibly std::set -- of valid 'positive' values while reading the file token-by-token).

I tried to edit the file locally by following this guide:
https://www.kernel.org/doc/html/latest/admin-guide/mm/transhuge.html#global-thp-controls
and it seems, the file has a format that allows it to have only these three tokens in this sequence separated by spaces
with only one of them surrounded by [] according to what we echo into it:

always madvise never 

So, I don't think using a std::set here is necessary.

[jayaddison]

I'm curious to know how you would approach testing this.

It looks tricky to test this logic while providing fixtures that simulate the contents of /sys/kernel/mm/transparent_hugepage/enabled, outside of a host system.

I noticed that there is existing use of the gtest framework within the test suite, and understand that it may be possible to use that to mock file streams. If those conditions hold true, we might be able to extract the logic into a function that accepts a file stream as an argument, and confirm the behaviour of that function without any filesystem reads on a host system.

So, I don't think using a std::set here is necessary.

Ok, agreed - using a set may be over-engineering the problem.

[RaisinTen]

Would be interesting to have that test. 👍
Removing the author ready label now since you're still working on this.

[jayaddison]

For reference (it doesn't look like this should affect the changes suggested here) - the logic to produce the contents of the /sys/kernel/mm/transparent_hugepage/enabled kernel configuration file is available here: https://github.com/torvalds/linux/blob/13391c60da3308ed9980de0168f74cce6c62ac1d/mm/huge_memory.c#L162-L177

[jayaddison]

The most recent commits (excluding lint fixes) extract two separate functions to, respectively, read the Linux huge tables configuration, and determine whether huge tables are configured as enabled.

I've now learned that although gtest is present in the codebase, gmock is not. Either way, gmock looks intended for use for testing C++ classes, not file-level functions.

One possibility might be to change the linkage of the IsTransparentHugePagesConfigured function (via use of static / extern keywords) so that the cctests can link to and exercise the logic within it. That makes me a little wary because it doesn't seem to be an existing pattern within the test suite, and also it has the potential to affect the contents of distributed binaries.

I'll puzzle over this for a little while and any guidance would be appreciated.

[jayaddison]

Despite some further investigation, I'm currently uncertain about how to proceed, given questions around how to make the (non-public) IsTransparentHugePagesConfigured unit-test-able. I'm planning to pause work on this until we have some more ideas about how to resolve that.

[RaisinTen]

Thanks for trying to test this out. I'm not sure about testing this either if there is no way to mock the file.

If the code isn't testable, I think it would be better to stick to your initial commit as a fix for the issue you brought up as it solves its purpose and is more concise and also readable. Additionally, I would consider adding a comment mentioning the link to the logic behind producing the kernel config file that you mentioned as I didn't come across any better explanation of the file format.

I'm adding a review wanted label. Let's wait and see what others have to say about this.

cc @gabrielschulhof would you please take a look at this fix when you're free?

[jayaddison]

Thanks @RaisinTen; I agree with all of your analysis there. For the purposes of not hiding history, I'll walk back to the initial change by reverting the subsequent commits, and then I'll add an explanatory comment.

[jayaddison]

As an aside: reading this Percona article about the performance impact of transparent huge pages has led me to look further back to the Linux v2.6.38 series kernel where it looks like the feature was introduced (ref).

It would be good to take time and aim for maximal version compatibility in any changes applied here.

[jayaddison]

(initially it does appear that the file format has remained stable since introduction, but there could be debug options or other good reasons why the format changed in the interim)

[jayaddison]

There are one or two further simplifications possible here:

• A std::ifstream object's close member function will automatically be called when the object is destroyed; this may enable some conditional-clause rewrites
• Since we are only reading a fixed number of tokens, we could sequentially read these using a single variable

[jayaddison]

The edits described in my previous comment are available to read as code here.

After checking that the changes in this pull request are valid, we could discuss whether the extra edits are worth including.

[jayaddison]

@RaisinTen Please could you re-add the author ready label to this pull request? I'm comfortable that this is ready for review again now.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35816/

[RaisinTen]

LGTM!

[RaisinTen]

Landed in a0c0875 🎉