Compact memory before requesting huge pages

[odinuge]

What type of PR is this?
/kind flake

What this PR does / why we need it:
Compact memory to make bigger contiguous blocks of memory available
before allocating huge pages.

https://www.kernel.org/doc/Documentation/sysctl/vm.txt

Which issue(s) this PR fixes:

ref #81726

Special notes for your reviewer:

/sig node
/sig testing

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[odinuge]

/cc mattjmcnaughton
/cc oomichi

[mattjmcnaughton]

Thanks for this pr :)

Some high level requests from me around comments/documentation before I review :) want to be sure I know what I'm reviewing haha and I'm far from a huge pages expert :)

[mattjmcnaughton]

Could you please expand on how dropping the kernel caches is necessary to ensure we get the requested amount of huge pages?

[mattjmcnaughton]

One thing I forgot to say... thank you for linking the kernel docs! That is helpful :)

[mattjmcnaughton]

Still curious about the answer to the specific q in my first comment when you get a chance :) Thanks!

[odinuge]

Hi,

After some more research, it looks like running memory compaction is more reasonable.

This is the docs from kernel.org about /proc/sys/vm/compact_memory:

Available only when CONFIG_COMPACTION is set. When 1 is written to the file,
all zones are compacted such that free memory is available in contiguous
blocks where possible. This can be important for example in the allocation of
huge pages although processes will also directly compact memory as required.

I have updated the PR to use that instead. Thoughts?

[mattjmcnaughton]

Are the k8s tests running as root? I'm a little surprised that a test would have permissions to run this command (especially on the k8s CI)?

[odinuge]

Hi,
Yes, and they have to!

The same permissions apply to the pre allocation of huge pages (see 2-3 lines below) by writing to /proc/sys/vm/nr_hugepages 😄

[mattjmcnaughton]

On Mon, Sep 16, 2019 at 07:36:47AM -0700, Odin Ugedal wrote: odinuge commented on this pull request. > @@ -85,7 +85,14 @@ func makePodToVerifyHugePages(baseName string, hugePagesLimit resource.Quantity) // configureHugePages attempts to allocate 10Mi of 2Mi hugepages for testing purposes func configureHugePages() error { - err := exec.Command("/bin/sh", "-c", "echo 5 > /proc/sys/vm/nr_hugepages").Run() + // Drop all kernel caches to make sure we can get the requested amount of huge pages + // This will only affect kernel level cache hit, and nothing visible in userspace + // https://www.kernel.org/doc/Documentation/sysctl/vm.txt + err := exec.Command("/bin/sh", "-c", "echo 3 > /proc/sys/vm/drop_caches").Run() Hi, Yes, and they have to! The same permissions apply to the pre allocation of huge pages (see 2-3 lines below) by writing to `/proc/sys/vm/nr_hugepages` 😄 -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #82656 (comment)

Interesting! I suppose if we are already doing this for the pre allocation of huge pages, then we aren't introducing any new behavior here.

[oomichi]

The change itself is good for me according to the Linux doc which is specified in this PR:

compact_memory

Available only when CONFIG_COMPACTION is set. When 1 is written to the file,
all zones are compacted such that free memory is available in contiguous
blocks where possible. This can be important for example in the allocation of
huge pages although processes will also directly compact memory as required.

/lgtm

[odinuge]

/assign @tallclair

[mattjmcnaughton]

/lgtm

I don't see any harm in trying this out. It's still not immediately clear if we are seeing actual errors because this doesn't exist, or its more of a preventative measure.

Also, confirming that CONFIG_COMPACTION is set re the documentation?

[odinuge]

Thanks for the review @mattjmcnaughton!

I don't see any harm in trying this out. It's still not immediately clear if we are seeing actual errors because this doesn't exist, or its more of a preventative measure.

Tests like this will flake once in a while, but compacting memory before trying to allocate huge page memory should reduce the amount of flakes by a fair share. Since we only allocate 10MiB of 2MiB pages, it should more or less never flake.

Also, confirming that CONFIG_COMPACTION is set re the documentation?

It is quite difficult to check kernel config at runtime since it depends on what version it is running, and what distro it is. The file /proc/sys/vm/compact_memory will only be present if CONFIG_COMPACTION=y, and the code verifies that the file exists before trying to write to it. 😄

[odinuge]

/cc @alejandrox1

[mattjmcnaughton]

On Thu, Sep 19, 2019 at 03:29:07AM -0700, Odin Ugedal wrote: Thanks for the review @mattjmcnaughton! > I don't see any harm in trying this out. It's still not immediately clear if we are seeing actual errors because this doesn't exist, or its more of a preventative measure. Tests like this will flake once in a while, but compacting memory before trying to allocate huge page memory should reduce the amount of flakes by a fair share. Since we only allocate `10MiB` of `2MiB` pages, it should more or less never flake. > Also, confirming that CONFIG_COMPACTION is set re the documentation? It is quite difficult to check kernel config at runtime since it depends on what version it is running, and what distro it is. The file `/proc/sys/vm/compact_memory` will only be present if `CONFIG_COMPACTION=y`, and the code verifies that the file exists before trying to write to it. 😄 -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #82656 (comment)

Good stuff :) Thanks for your helpful answers as always!

[alejandrox1]

im a bit late to the party but thank you for working on this @odinuge !
/priority important-longterm
/milestone v1.17

/assign @Random-Liu @derekwaynecarr @ConnorDoyle
ptal

[oomichi]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: odinuge, oomichi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• test/e2e_node/OWNERS [oomichi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment