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

cgroup limits not per surface #2084

Open
Syphdias opened this issue Aug 11, 2024 · 2 comments
Open

cgroup limits not per surface #2084

Syphdias opened this issue Aug 11, 2024 · 2 comments
Labels
needs-confirmation A reproduction has been reported, but the bug hasn't been confirmed or reproduced by a maintainer. os/linux

Comments

@Syphdias
Copy link
Contributor

The config documentation suggests that linux-cgroup-memory-limit works per surface but I believe it is shared by all surfaces.

ghostty/src/config/Config.zig

Lines 1270 to 1277 in 956b097

/// Memory limit for any individual terminal process (tab, split, window,
/// etc.) in bytes. If this is unset then no memory limit will be set.
///
/// Note that this sets the "memory.high" configuration for the memory
/// controller, which is a soft limit. You should configure something like
/// systemd-oom to handle killing processes that have too much memory
/// pressure.
@"linux-cgroup-memory-limit": ?u64 = null,

image

❯ tail /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-132142.scope/surfaces/**/memory.{current,max}
==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-132142.scope/surfaces/3084C1F0.service/memory.current <==
19464192

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-132142.scope/surfaces/30E906A0.service/memory.current <==
19013632

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-132142.scope/surfaces/memory.current <==
38535168

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-132142.scope/surfaces/3084C1F0.service/memory.max <==
max

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-132142.scope/surfaces/30E906A0.service/memory.max <==
max

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-132142.scope/surfaces/memory.max <==
max

I am not a 100% that this is true, but it makes not sense to me. I would expect the limit to be set for every creation of a new surface, not all surfaces.
Maybe I am misunderstanding how single-instance and cgroups interact or the controller does something.

// Create a cgroup that will contain all our surfaces. We will
// enable the controllers and configure resource limits for surfaces
// only on this cgroup so that it doesn't affect our main app.
try internal_os.cgroup.create(transient, "surfaces", null);
const surfaces = try std.fmt.allocPrint(alloc, "{s}/surfaces", .{transient});
defer alloc.free(surfaces);
// Enable all of our cgroup controllers. If these fail then
// we just log. We can't reasonably undo what we've done above
// so we log the warning and still return the transient group.
// I don't know a scenario where this fails yet.
try enableControllers(alloc, transient);
try enableControllers(alloc, surfaces);
// Configure the "high" memory limit. This limit is used instead
// of "max" because it's a soft limit that can be exceeded and
// can be monitored by things like systemd-oomd to kill if needed,
// versus an instant hard kill.
if (app.config.@"linux-cgroup-memory-limit") |limit| {
try internal_os.cgroup.configureMemoryLimit(surfaces, .{
.high = limit,
});
}

PS: Both settings (for all surfaces and per surface) could be useful, but the configuration documentation says it is per surface…

Version and Config

❯ ghostty +version
Ghostty 0.1.0-main+956b0977

Build Config
  - Zig version: 0.13.0
  - build mode : builtin.OptimizeMode.ReleaseFast
  - app runtime: apprt.Runtime.gtk
  - font engine: font.main.Backend.fontconfig_freetype
  - renderer   : renderer.OpenGL
  - libxev     : main.Backend.io_uring
❯ ghostty +show-config |grep -ve palette -e background -e foreground
font-feature = MesloLGS NF
font-size = 14
theme = tokyonight
cursor-color = #c0caf5
command = /usr/bin/zsh
focus-follows-mouse = true
click-repeat-interval = 500
linux-cgroup-memory-limit = 200000000
gtk-single-instance = true
gtk-titlebar = false
@mitchellh
Copy link
Contributor

Yeah the cgroup documentation did not make it clear to me what exactly the semantics were here. I thought this did apply to the children and is meant to be per surface but I'm not totally sure. If you (or anyone, me) could verify this that'd be very helpful.

@mitchellh mitchellh added bug os/linux needs-confirmation A reproduction has been reported, but the bug hasn't been confirmed or reproduced by a maintainer. labels Aug 12, 2024
@Syphdias
Copy link
Contributor Author

I tried to verify it with memory limit with kitten icat but since that is a softlimit I always overshot the limit. Easier to test with pid.max since it is a hard limit:

With linux-cgroup-processes-limit = 100, I opened two ghostty tabs and started a couple of sleep inf & in both tabs until I got an error in one of the terminals:

❯ sleep inf &
zsh: fork failed: resource temporarily unavailable

Looking at cgroups

❯ systemd-cgls -u [email protected] |grep app.slice -A45
├─app.slice
│ ├─app-dbus\x2d:1.26\x2dorg.a11y.atspi.Registry.slice
│ │ └─dbus-:[email protected]
│ │   └─1816 /usr/lib/at-spi2-registryd --use-gnome-session
│ ├─app-ghostty-transient-80378.scope
│ │ ├─surfaces
│ │ │ ├─29759100.service
│ │ │ │ ├─ 80744 /usr/bin/zsh
│ │ │ │ ├─ 80751 /usr/bin/zsh
│ │ │ │ ├─ 80783 /usr/bin/zsh
│ │ │ │ ├─ 80784 /usr/bin/zsh
│ │ │ │ ├─ 80786 /home/syphdias/.cache/zsh4humans/v5/cache/gitstatus/gitstatusd…
│ │ │ │ ├─ 84465 sleep inf
│ │ │ │ ├─ 84522 sleep inf
│ │ │ │ ├─ 84581 sleep inf
│ │ │ │ ├─ 84600 sleep inf
│ │ │ │ ├─ 84630 sleep inf
│ │ │ │ ├─ 84641 sleep inf
│ │ │ │ ├─ 84650 sleep inf
│ │ │ │ ├─ 84741 sleep inf
│ │ │ │ ├─ 84745 sleep inf
│ │ │ │ ├─ 84749 sleep inf
│ │ │ │ ├─ 84758 sleep inf
│ │ │ │ ├─ 84822 sleep inf
│ │ │ │ ├─ 84861 sleep inf
│ │ │ │ ├─ 84866 sleep inf
│ │ │ │ └─107598 sleep inf
│ │ │ └─29D8BEC0.service
│ │ │   ├─80387 /usr/bin/zsh
│ │ │   ├─80393 /usr/bin/zsh
│ │ │   ├─80426 /usr/bin/zsh
│ │ │   ├─80428 /usr/bin/zsh
│ │ │   ├─80429 /home/syphdias/.cache/zsh4humans/v5/cache/gitstatus/gitstatusd-…
│ │ │   ├─83712 sleep inf
│ │ │   ├─83713 sleep inf
│ │ │   ├─83714 sleep inf
│ │ │   ├─83715 sleep inf
│ │ │   ├─83716 sleep inf
│ │ │   ├─83717 sleep inf
│ │ │   ├─83718 sleep inf
│ │ │   ├─83719 sleep inf
│ │ │   ├─83720 sleep inf
│ │ │   ├─83721 sleep inf
│ │ │   └─84349 sleep inf
│ │ └─app
│ │   └─80378 ghostty

Current pids:

❯ tail  /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-*.scope/{,app,surfaces,surfaces/*.service}/pids.current
==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope//pids.current <==
113

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/app/pids.current <==
13

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/surfaces/pids.current <==
100

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/surfaces/29759100.service/pids.current <==
52

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/surfaces/29D8BEC0.service/pids.current <==
48

Pid.Max:

❯ tail  /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-*.scope/{,app,surfaces,surfaces/*.service}/pids.max
==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope//pids.max <==
38394

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/app/pids.max <==
max

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/surfaces/pids.max <==
100

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/surfaces/29759100.service/pids.max <==
max

==> /sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-ghostty-transient-80378.scope/surfaces/29D8BEC0.service/pids.max <==
max

So I hit the limit of surfaces with processes from different surfaces not just from one. After I hit the error in one tab I also hit it in the other

@Syphdias Syphdias changed the title cgroup limit not per surface cgroup limits not per surface Aug 13, 2024
@mitchellh mitchellh removed the bug label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-confirmation A reproduction has been reported, but the bug hasn't been confirmed or reproduced by a maintainer. os/linux
Projects
None yet
Development

No branches or pull requests

2 participants