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

libct/cg: add CFS bandwidth burst for CPU #3205

Closed
wants to merge 1 commit into from

Conversation

kailun-qin
Copy link
Contributor

Burstable CFS controller is introduced in Linux 5.14. This helps with
parallel workloads that might be bursty. They can get throttled even
when their average utilization is under quota. And they may be latency
sensitive at the same time so that throttling them is undesired.

This feature borrows time now against the future underrun, at the cost
of increased interference against the other system users, by introducing
cfs_burst_us into CFS bandwidth control to enact the cap on unused
bandwidth accumulation, which will then used additionally for burst.

The patch adds the support/control for CFS bandwidth burst.

runtime-spec: opencontainers/runtime-spec#1120

Signed-off-by: Kailun Qin kailun.qin@intel.com

@@ -89,6 +89,25 @@ func (s *CpuGroup) Set(path string, r *configs.Resources) error {
period = ""
}
}

var burst string
if r.CpuBurst != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

the spec (in opencontainers/runtime-spec#1120) says

A value of 0 for indicates that the cgroup can not accumulate any unused bandwidth. It makes the traditional bandwidth control behavior unchanged.

but since we ignore 0 here, this means we can't disable burst during runc update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! Yes, I just realized after submitting this so I updated the spec accordingly to elaborate.

update.go Outdated
Comment on lines 268 to 269
// sense, but historically runc allowed it and this needs to be
// supported to not break compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is historically true for period and quota, but not for burst. I think we should require period to be set when setting burst.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, make sense to me. JFYI, underneath in kernel implementation, it looks like quota, period and burst are all set together. As for systemd, it's not yet supported for burst.

@Zheaoli
Copy link
Contributor

Zheaoli commented Feb 28, 2022

Is there any new update about this PR?

I think maybe I can help about this PR

@kailun-qin
Copy link
Contributor Author

Is there any new update about this PR?

I think maybe I can help about this PR

@Zheaoli Thanks for raising this!

I'd love to move the PR forward but actually we need to have the spec (opencontainers/runtime-spec#1120) discussed and merged first.

Besides I also received the same feature request from @litianxi who aims to apply this in their production and we discussed further offline.

@kolyshkin would you please kindly help review the runtime-spec? Thanks a lot.

@Zheaoli
Copy link
Contributor

Zheaoli commented Jan 23, 2023

After opencontainers/runtime-spec#1120 has been completed, I think this PR is ready to go

@kailun-qin kailun-qin marked this pull request as ready for review January 28, 2023 02:23
@kailun-qin
Copy link
Contributor Author

Rebased on top of the latest main.

Burstable CFS controller is introduced in Linux 5.14. This helps with
parallel workloads that might be bursty. They can get throttled even
when their average utilization is under quota. And they may be latency
sensitive at the same time so that throttling them is undesired.

This feature borrows time now against the future underrun, at the cost
of increased interference against the other system users, by introducing
cfs_burst_us into CFS bandwidth control to enact the cap on unused
bandwidth accumulation, which will then used additionally for burst.

The patch adds the support/control for CFS bandwidth burst.

runtime-spec: opencontainers/runtime-spec#1120

Signed-off-by: Kailun Qin <kailun.qin@intel.com>
@AkihiroSuda
Copy link
Member

Can't be compiled

# github.com/opencontainers/runc/libcontainer/specconv
Error: libcontainer/specconv/spec_linux.go:740:14: r.CPU.Burst undefined (type *specs.LinuxCPU has no field or method Burst)
Error: libcontainer/specconv/spec_linux.go:741:36: r.CPU.Burst undefined (type *specs.LinuxCPU has no field or method Burst)

@AkihiroSuda
Copy link
Member

Also please consider adding an integration test

@AkihiroSuda
Copy link
Member

@kailun-qin Could you update the PR?

@Zheaoli
Copy link
Contributor

Zheaoli commented Feb 23, 2023

@AkihiroSuda, I will take handle of this PR...

AkihiroSuda added a commit that referenced this pull request Sep 6, 2023
[Carry #3205] libct/cg: add CFS bandwidth burst for CPU
@Zheaoli
Copy link
Contributor

Zheaoli commented Sep 9, 2023

@AkihiroSuda After #3749 has been merged, I think this PR and #3145 should be closed.

@AkihiroSuda AkihiroSuda closed this Sep 9, 2023
@kolyshkin kolyshkin mentioned this pull request Oct 23, 2023
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.

4 participants