-
Notifications
You must be signed in to change notification settings - Fork 19
fs2: Fix statHugeTlb error when rsvd usage is present #24
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
fs2: Fix statHugeTlb error when rsvd usage is present #24
Conversation
e3d9c0a to
b502476
Compare
|
@AkihiroSuda @thaJeztah could you please take a look when you have a chance? Tests are passing locally. |
You are right, I've checked in my machine. |
|
hi @kolyshkin, @AkihiroSuda, @haircommander |
b502476 to
5e787da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, left a single nit.
Also, please change libct/cg/fs2: to fs2: in commit subject, as this is where it lives now (used to be part of runc's libcontainer/cgroups but now it's a separate repo).
I'm against these tests though, they are very non-real and do not actually test anything. I'd rather have something like a simple bats tests which checks that hugetlb is available, when runs runc events --stats $CTID and checks that hugetlb is not empty.
5e787da to
c1ab7c5
Compare
|
Thank you, @kolyshkin. I've updated the code and reworded the commit message accordingly. Regarding the tests, they are ported from fs/hugetlb_test.go, and other fs2 modules follow a similar testing pattern. I agree they could be more realistic, and what you described sounds like a solid direction. Would you prefer that be addressed in this PR, or do you see it as a broader improvement for the library? Since there are currently no bats tests in the repo, introducing them would involve a fair amount of work on its own. I'm also open to removing the tests and limiting this PR to the bug fix, given that this module didn't have any tests to begin with. What are your thoughts? |
|
Yes, let's remove the test, and then I'll add one to runc repo. |
Signed-off-by: Gavin Lam <[email protected]>
c1ab7c5 to
94067f2
Compare
|
Thanks @kolyshkin, I've removed the tests. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@kolyshkin @lifubang PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still LGTM
|
@kolyshkin does it look good to you? shall we merge this please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As promised in opencontainers/cgroups#24 (review) Signed-off-by: Kir Kolyshkin <[email protected]>
Better late than never, see opencontainers/runc#4898 |
As promised in opencontainers/cgroups#24 (review) Signed-off-by: Kir Kolyshkin <[email protected]>
As promised in opencontainers/cgroups#24 (review) Signed-off-by: Kir Kolyshkin <[email protected]>
HugeTLB stats for cgroup v2 is currently not working when rsvd usage is present. When the
.rsvd.currentfile exists, the code attempts to read a.rsvd.eventsfile to obtain failcnt stat. However, this file does not exist, resulting in the following error.open /sys/fs/cgroup/pod_123.slice/pod_123-456.slice/hugetlb.2MB.rsvd.events: no such file or directoryI’ve verified in kernel source code that the cgroup v2 HugeTLB controller does not create a
.rsvd.eventsfile.https://github.com/torvalds/linux/blob/v6.15/mm/hugetlb_cgroup.c#L711-L756
P.S. This is blocking cri-o/cri-o#9257