NotificationsYou must be signed in to change notification settings
Fork2.2k
Star12.9k

skip set cpu.weight when cpu.idle is enabled in cgroups v2#4143

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

qdaxb wants to merge1 commit intoopencontainers:main

base:main

Choose a base branch

fromqdaxb:main

Open

skip set cpu.weight when cpu.idle is enabled in cgroups v2#4143

qdaxb wants to merge1 commit intoopencontainers:mainfromqdaxb:main

Conversation

Copy link

qdaxb commentedDec 12, 2023

In cgroups v2, settingcpu.weight whencpu.idle==1 will fail withInvalid argument

This pr fixed this situation, consistent withhttps://github.com/opencontainers/runc/blob/main/libcontainer/cgroups/systemd/v2.go#L119

qdaxb force-pushed themain branch 2 times, most recently from9079199 tod255be9Compare

December 12, 2023 16:48

Copy link

Contributor

mrunalp commentedDec 12, 2023

Instead of silently skipping we should fail as a validation error.

Copy link

Author

qdaxb commentedDec 13, 2023•
edited
Loading

Instead of silently skipping we should fail as a validation error.

@mrunalp Thanks for reply.

I'm still a little confused about theidle parameter.

There is no description in thespec thatshares andidle cannot be applied at the same time. But If I set bothshares andidle, systemd driver and fs2 driver will behave differently.

system\driver	systemd/v2	fs2/cpu
support idle & systemd < 252	systemd:set cpu.weight=1 fsMgr:set cpu.idle=1 & return error	set cpu.idle=1 & return error
support idle & systemd > 252	systemd:set cpu.idle=1 fsMgr:set cpu.idle=1 & return error	set cpu.idle=1 & return error
not support idle	return error	return error

So I think fs2 should be compatible with this situation like systemd, or directly add a hint in the document that they cannot be used at the same time?

Copy link

Member

AkihiroSuda commentedDec 15, 2023

Instead of silently skipping we should fail as a validation error.

Should be a warning to avoid breaking compatibility?

Copy link

Contributor

kolyshkin commentedMar 19, 2024

Yes, this should probably be a warning. I'm going to carry this into#4227.

skip set cpu.weight when cpu.idle is enabled in cgroups v2

669f940

Signed-off-by: axb <uaxb@hotmail.com>

kolyshkin force-pushed themain branch fromd255be9 to669f940Compare

May 8, 2024 17:15

kolyshkin added the area/cgroupv2 label

May 8, 2024

Copy link

Contributor

kolyshkin commentedMay 8, 2024

There's no need for a warning. The problem is,Set is called duringrunc update with a combination of old and new settings, and thus it's normal for cpuweight and cpuidle both being set.

This PR needs a test case though. Can you add one@qdaxb ?

Labels

area/cgroupv2

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

skip set cpu.weight when cpu.idle is enabled in cgroups v2#4143

Are you sure you want to change the base?

skip set cpu.weight when cpu.idle is enabled in cgroups v2#4143

Uh oh!