Add support for rsvd hugetlb cgroup by omprakaash · Pull Request #2719 · youki-dev/youki

omprakaash · 2024-03-04T04:25:17Z

Adds support for rsvg hugetlb cgroup. More Info on why this is necessary: opencontainers/runtime-spec#1116. Should fix #1707.

omprakaash · 2024-03-05T18:23:26Z

I can also open up a PR in oci-spec-rs if necessary to update info about huge-tlb limits.

utam0k · 2024-03-06T12:20:10Z

I can also open up a PR in oci-spec-rs if necessary to update info about huge-tlb limits.

Let's do it.

omprakaash · 2024-03-09T19:54:14Z

Hey, sorry for the delay. Opened up a corresponding PR in oci-spec-rs repo as well.

utam0k · 2024-03-13T12:05:38Z

@omprakaash May I ask you to add a e2e test?
https://containers.github.io/youki/developer/e2e/rust_oci_test.html

omprakaash · 2024-03-13T13:53:55Z

Oh yes ! Will add one.

YJDoc2 · 2024-03-18T06:20:38Z

Hey, do we need a oci-spec release and update its version in this PR ?

utam0k · 2024-04-01T00:01:44Z

@omprakaash friendly remind ;)

utam0k · 2024-04-01T00:02:00Z

Hey, do we need a oci-spec release and update its version in this PR ?

If needed, I can do it.

codecov-commenter · 2024-04-01T06:13:44Z

Codecov Report

Merging #2719 (4e2a9cb) into main (ed2c08d) will increase coverage by 0.25%.
Report is 27 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2719      +/-   ##
==========================================
+ Coverage   65.21%   65.47%   +0.25%     
==========================================
  Files         133      133              
  Lines       16981    17099     +118     
==========================================
+ Hits        11074    11195     +121     
+ Misses       5907     5904       -3

utam0k · 2024-04-01T06:42:20Z

utam0k · 2024-04-01T06:56:01Z

+        let _ = common::write_cgroup_file(
+            root_path.join(format!(
+                "hugetlb.{}.rsvd.limit_in_bytes",
+                hugetlb.page_size()
+            )),
+            hugetlb.limit(),
+        );
+


Please take care of Result type.

Suggested change

let _ = common::write_cgroup_file(

root_path.join(format!(

"hugetlb.{}.rsvd.limit_in_bytes",

hugetlb.page_size()

)),

hugetlb.limit(),

);

common::write_cgroup_file(

root_path.join(format!(

"hugetlb.{}.rsvd.limit_in_bytes",

hugetlb.page_size()

)),

hugetlb.limit(),

)?;

Oh, I ignored the result because the desired behavior was falling back to just writing to the non-rsvd file if the rsvd file was not present.

utam0k · 2024-04-01T07:09:35Z

-
-        let usage_file = format!("hugetlb.{page_size}.usage_in_bytes");
-        let usage_content = common::read_cgroup_file(cgroup_path.join(usage_file))?;
+        let mut rsvd = ".rsvd";


How about this?

Suggested change

let mut rsvd = ".rsvd";

let mut file_prefix= format!("hugetlb.{page_size}.rsvd");

Oh that is better, will change it.

utam0k · 2024-04-01T07:10:31Z

+        let _ = common::write_cgroup_file(
+            root_path.join(format!("hugetlb.{}.rsvd.max", hugetlb.page_size())),
+            hugetlb.limit(),
+        );
+


Suggested change

let _ = common::write_cgroup_file(

root_path.join(format!("hugetlb.{}.rsvd.max", hugetlb.page_size())),

hugetlb.limit(),

);

common::write_cgroup_file(

root_path.join(format!("hugetlb.{}.rsvd.max", hugetlb.page_size())),

hugetlb.limit(),

)?;

utam0k · 2024-04-01T07:11:32Z

-        let events_file = format!("hugetlb.{page_size}.events");
-        let path = cgroup_path.join(events_file);
-        let events = common::read_cgroup_file(&path)?;
+        let mut rsvd = ".rsvd";


Same as comment in cgroup v1.

utam0k · 2024-04-02T12:02:32Z

@omprakaash It looks like the unit test failed. Please check it
https://github.com/containers/youki/actions/runs/8515994057/job/23342405969?pr=2719

omprakaash · 2024-04-03T02:03:43Z

Oh that is because the functions fails if the write to the new rsvd file fails. If rsvd is not supported, I think we should fallback to just writing to the non-rsvd file (https://github.com/opencontainers/runtime-spec/pull/1116/files). I left a follow up comment in the code review - review

utam0k · 2024-04-03T12:44:30Z

Oh that is because the functions fails if the write to the new rsvd file fails. If rsvd is not supported, I think we should fallback to just writing to the non-rsvd file (https://github.com/opencontainers/runtime-spec/pull/1116/files). I left a follow up comment in the code review - review

These unit tests use temporary dirs(/tmp). It means we don't use actual cgroup fs. You can put any file to simulate support rsvd in a temporary dir.

omprakaash · 2024-04-04T03:34:56Z

Hey, sorry I might be misunderstanding, but that is what the newly added tests do (eg: test_set_rsvd_hugetlb() ). So this test is testing that the application of hugetlb stats does not error out if rsvd-hugetlb file is not present.

utam0k · 2024-04-04T11:50:22Z

@omprakaash
Oh, I was wrong. I understood.

But there are possibilities of errors except for what you expect when writing the cgroup file, so we can't ignore this result.

Oh, I ignored the result because the desired behavior was falling back to just writing to the non-rsvd file if the rsvd file was not present.

How about checking if the rvsd is supported before writing rvsd's cgroup file?
https://github.com/omprakaash/youki/blob/f2bf35c0b4244bf4d1a73e56c85de3c7b3b11def/crates/libcgroups/src/v2/hugetlb.rs#L109-L112

omprakaash · 2024-04-07T04:09:11Z

Gotcha, changed it so that we first check if the rsvd file exists before attempting to write into it.

Signed-off-by: omprakaash <omsuseela@gmail.com> Signed-off-by: om <omsuseela@gmail.com> Signed-off-by: Om Prakaash <omsuseela@gmail.com>

utam0k

LGTM 👍 Thanks, @omprakaash

YJDoc2 added the kind/feature label Mar 11, 2024

omprakaash force-pushed the rsvd-hugetlb branch 2 times, most recently from 274287d to 4e2a9cb Compare April 1, 2024 00:46

utam0k reviewed Apr 1, 2024

View reviewed changes

Comment thread tests/contest/contest/src/tests/tlb/tlb_test.rs

utam0k Apr 1, 2024

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

👍

utam0k requested changes Apr 1, 2024

View reviewed changes

omprakaash force-pushed the rsvd-hugetlb branch from 4e2a9cb to f2bf35c Compare April 2, 2024 01:19

omprakaash force-pushed the rsvd-hugetlb branch from f2bf35c to 45929ed Compare April 7, 2024 04:08

Add support for rsvd hugetlb cgroup

f70f43d

Signed-off-by: omprakaash <omsuseela@gmail.com> Signed-off-by: om <omsuseela@gmail.com> Signed-off-by: Om Prakaash <omsuseela@gmail.com>

omprakaash force-pushed the rsvd-hugetlb branch from 45929ed to f70f43d Compare April 7, 2024 04:10

utam0k approved these changes Apr 7, 2024

View reviewed changes

utam0k enabled auto-merge (squash) April 7, 2024 05:26

utam0k merged commit e94e103 into youki-dev:main Apr 7, 2024

github-actions Bot mentioned this pull request Apr 7, 2024

Release for v0.3.3 #2665

Merged

	let mut rsvd = ".rsvd";
	let mut file_prefix= format!("hugetlb.{page_size}.rsvd");

Conversation

omprakaash commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omprakaash commented Mar 5, 2024

Uh oh!

utam0k commented Mar 6, 2024

Uh oh!

omprakaash commented Mar 9, 2024

Uh oh!

utam0k commented Mar 13, 2024

Uh oh!

omprakaash commented Mar 13, 2024

Uh oh!

YJDoc2 commented Mar 18, 2024

Uh oh!

utam0k commented Apr 1, 2024

Uh oh!

utam0k commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 1, 2024

Codecov Report

Uh oh!

utam0k Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

utam0k Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

omprakaash Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

utam0k Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

omprakaash Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

utam0k Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

utam0k Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

utam0k commented Apr 2, 2024

Uh oh!

omprakaash commented Apr 3, 2024

Uh oh!

utam0k commented Apr 3, 2024

Uh oh!

omprakaash commented Apr 4, 2024

Uh oh!

utam0k commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omprakaash commented Apr 7, 2024

Uh oh!

utam0k left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

omprakaash commented Mar 4, 2024 •

edited

Loading

utam0k commented Apr 1, 2024 •

edited

Loading

utam0k commented Apr 4, 2024 •

edited

Loading