Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hugepage support for LCOW #1118

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

ninzavivek
Copy link
Contributor

@ninzavivek ninzavivek commented Aug 17, 2021

This adds hugepages support for LCOW.

A hugetlbfs drive can be mounted inside a LCOW. This is backed by a location in UVM. Currently, linux kernel shipped with containerd only supports hugepage size of 2M.

In order to specify a container path is a hugepage mount, hostpath needs to be specified as hugepages://2M.

In order for container to use hugepage successfully, it is expected UVM will be created appropriate KernelOptions annotation.

"io.microsoft.virtualmachine.lcow.kernelbootoptions" : "hugepagesz=2M hugepages=10"

UVM also has to be created with sufficient memory to host desired number of hugepages.

Ex:
"io.microsoft.virtualmachine.computetopology.memory.sizeinmb": "2048",
"io.microsoft.virtualmachine.lcow.kernelbootoptions" : "hugepagesz=2M hugepages=10"

In order to realize the performance of using hugepages inside the LCOW, memory backing the UVM has to ensured to use an approriate page size. The default page for physical memory backed UVM is 2MB. Hence, to ensure performance with using hugepage of size MB, UVM should be physcial memory backed.

"io.microsoft.virtualmachine.fullyphysicallybacked": "true"

Verification: Adhoc testing using updated containerd binaries/linux kernel /container image with a test app that tries to huge page.
Newly added functional test also passes.

@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch from 3ffee97 to 50866a2 Compare August 18, 2021 21:01
@ninzavivek ninzavivek changed the title hugepages handling Hugepage support for LCOW Aug 18, 2021
@ninzavivek ninzavivek marked this pull request as ready for review August 18, 2021 21:12
@ninzavivek ninzavivek requested a review from a team as a code owner August 18, 2021 21:12
hnsendpoint.go Outdated Show resolved Hide resolved
@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch from 50866a2 to 99e5783 Compare August 18, 2021 21:33
@katiewasnothere
Copy link
Contributor

katiewasnothere commented Aug 25, 2021

Do you know if we have to set hugepagesz=2M if that's the default size for the kernel we're running? Just curious -- not an issue

@katiewasnothere
Copy link
Contributor

katiewasnothere commented Aug 25, 2021

I think we want to do something like:

"host_path": “hugepages://2mb/somepathname"

where sourcepathinuvm/hugepages/2mb is where hugetlbfs is mounted, and somepathname is a subpath under that mount. This will allow us to better control propagation between containers. Otherwise with the current route, every container that requests hugepages of size 2mb will see any files (but potentially not mountpoints depending on configuration) under the 2mb sandbox source path.

@ninzavivek
Copy link
Contributor Author

Do you know if we have to set hugepagesz=2M if that's the default size for the kernel we're running? Just curious -- not an issue

I just did this to be explicit, so that in future if we add support for different pages at the same time, it is obvious, Let me know if you want me to remove it.

@ninzavivek
Copy link
Contributor Author

I think we want to do something like:

"host_path": “hugepages://2mb/somepathname"

where sourcepathinuvm/hugepages/2mb is where hugetlbfs is mounted, and somepathname is a subpath under that mount. This will allow us to better control propagation between containers. Otherwise with the current route, every container that requests hugepages of size 2mb will see any files (but potentially not mountpoints depending on configuration) under the 2mb sandbox source path.

  • Added a check in the CRI.

@dcantah
Copy link
Contributor

dcantah commented Aug 30, 2021

Oddly enough some of the bridge gcs tests are failing here

@ninzavivek
Copy link
Contributor Author

Oddly enough some of the bridge gcs tests are failing here

Kathryn, reran the tests, and they passed.

@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch 2 times, most recently from e2a5856 to 8761d81 Compare August 30, 2021 23:37
@ninzavivek
Copy link
Contributor Author

Oddly enough some of the bridge gcs tests are failing here

They are passing now, after pulling in latest changes.

@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch from 8761d81 to f7e1759 Compare August 31, 2021 01:03
@dcantah
Copy link
Contributor

dcantah commented Sep 1, 2021

This needs to get rebased to get Kathryns recent fixes

@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch from f7e1759 to e3f6ef1 Compare September 1, 2021 20:55
@ninzavivek
Copy link
Contributor Author

This needs to get rebased to get Kathryns recent fixes

Done.

Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see this design follows what was discussed, lgtm. Just go related comments

@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch from e3f6ef1 to f208d05 Compare September 7, 2021 19:04
@dcantah
Copy link
Contributor

dcantah commented Sep 7, 2021

Still lgtm

@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch from f208d05 to 5ad017e Compare September 7, 2021 20:22
Signed-off-by: Vivek Aggarwal <[email protected]>
@ninzavivek ninzavivek force-pushed the vivek_hugepages_mount branch from 5ad017e to 6fba53b Compare September 7, 2021 20:29
Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants