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

set cpu.rt_period_us and cpu.rt_runtime_us in the appropriate order #4235

Closed
wants to merge 1 commit into from

Conversation

ls-ggg
Copy link
Contributor

@ls-ggg ls-ggg commented Mar 29, 2024

Fix: #4094
when users set or update cpu.rt_period_us and cpu.rt_runtime_us at the same time,
runc should set these two values in the appropriate order to ensure that the intermediate value of cpu.rt_runtime_us/cpu.rt_period_us is less than the limit ratio

@ls-ggg ls-ggg force-pushed the fix-set-cpu.rt branch 8 times, most recently from e5c7a60 to e785c44 Compare March 29, 2024 06:03
@kolyshkin
Copy link
Contributor

This code is quite complicated. Can you please do something similar to commit 1b2adcf (and add a test case, too)?

@ls-ggg ls-ggg force-pushed the fix-set-cpu.rt branch 6 times, most recently from 4a16e8b to 28268ee Compare April 1, 2024 07:06
@ls-ggg
Copy link
Contributor Author

ls-ggg commented Apr 1, 2024

This code is quite complicated. Can you please do something similar to commit 1b2adcf (and add a test case, too)?

This code is quite complicated. Can you please do something similar to commit 1b2adcf (and add a test case, too)?
@kolyshkin Thanks for your suggestion, I have completed the comments and commit information. and added test cases

@ls-ggg ls-ggg force-pushed the fix-set-cpu.rt branch 2 times, most recently from c8a27c4 to 190d5dc Compare April 2, 2024 02:43
@ls-ggg
Copy link
Contributor Author

ls-ggg commented Apr 2, 2024

rebase

@ls-ggg
Copy link
Contributor Author

ls-ggg commented Apr 12, 2024

@kolyshkin Can this PR be approved?

libcontainer/cgroups/fs/cpu.go Show resolved Hide resolved
libcontainer/cgroups/fs/cpu.go Outdated Show resolved Hide resolved
As reported in issue opencontainers#4094, sometimes setting rt_period_us and
rt_runtime_us at the same time will fail. The reason is that in
 cgroupv1, these two values cannot be set atomically.

When we set a new rt_period_us, the kernel will determine
whether the current configuration of new_limit1 = old_quota/new_period
exceeds the limit. If it exceeds the limit, an error will be reported.
Maybe it is reasonable to set rt_runtime_us first so that the
new_limit2 = new_quota/old_period.
for example:
The original state of cgv1 is rt_period_us: 10000 rt_runtime_us: 5000
The user wants to change it to rt_period_us: 1000 rt_runtime_us:300.
The new rt_runtime_us should be set first.

In the opposite case, if rt_runtime_us is set first, new_limit2 may still
exceed the limit, but new_limit1 will be valid.
for example:
The original state of cgv1 is rt_period_us: 1000 rt_runtime_us: 500
The user wants to change it to rt_period_us: 10000 rt_runtime_us:3000.
The new rt_period_us should be set first.

Therefore, new_limit1 and new_limit2 should be calculated in advance,
and the smaller corresponding setting order should be selected to set
rt_period_us and rt_runtime_us.

Signed-off-by: ls-ggg <[email protected]>
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.

When runc updates the values ​​of cpu-rt-period and cpu-rt-runtime synchronously, an error occurs.
3 participants