From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id 1C9779DC7; Tue, 28 Jul 2026 10:41:31 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id C111B9DB0; Tue, 28 Jul 2026 10:41:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=0.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_MISSING,FROM_SUSPICIOUS_NTLD,SPF_HELO_NONE, T_PDS_OTHER_BAD_TLD autolearn=no autolearn_force=no version=4.0.1 Received: from mail-108-mta211.mxroute.com (mail-108-mta211.mxroute.com [136.175.108.211]) by atuin.qyliss.net (Postfix) with ESMTPS id 84F869DAF for ; Tue, 28 Jul 2026 10:41:26 +0000 (UTC) Received: from filter006.mxroute.com ([136.175.111.3] filter006.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta211.mxroute.com (ZoneMTA) with ESMTPSA id 19fa850a07c000c8ef.003 for (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384); Tue, 28 Jul 2026 10:41:24 +0000 X-Zone-Loop: 41e1006c0f1a1afa92ffe05bf8e66b42d09fe7560b0c X-Originating-IP: [136.175.111.3] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gagarin.work; s=x; h=Message-Id:Date:References:In-Reply-To:Cc:To:From: Subject:Content-Transfer-Encoding:Content-Type:MIME-Version:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=VjgQ8uyMfgHMqvgbBF5Snt8sC0+LeoTOfcFEPdJDCOY=; b=udpv2AnKys2KfLSy/VbcFnS5tW oUYEhbc3eTqzaFDpKjV4CYVUWWmFBC87G2ChPsDtbwup3c4iPsL4xT7vEsutdJ3x0plOWR4qaCDBp ePLkjmofYp/pEYK30DnBmpsDa+s00psNx436ydE6ZIaX56Kp5FXPTdMssU1DmP6E1eQSHoUqAkGD3 i0Y4hLlRZ5kUiuIgtkJEVZRP9jXCyrfbb0yeoBEt9bAIMO0Bh44xKOQbPaNTuMsahda2tFYLmmuOZ Upx0ISOAk4QJi/w549yjXGdlkAFrX5M94TQ/kbbq27aJ0MGRR8kasaYB5CI7fTsBF2ct49HGG8Pnd Ho/svKcw==; MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH v4 03/20] Documentation: Mention control groups From: Valentin Gagarin To: Demi Marie Obenour In-Reply-To: <20260721-cgroups-v4-3-46b2e5fff7b6@gmail.com> References: <20260721-cgroups-v4-0-46b2e5fff7b6@gmail.com> <20260721-cgroups-v4-3-46b2e5fff7b6@gmail.com> Date: Tue, 28 Jul 2026 12:41:02 +0200 Message-Id: <178523526292.177091.4277895723800005859.b4-review@b4> X-Authenticated-Id: valentin@gagarin.work Message-ID-Hash: IS4LVVSJMWN3JUPMH6PNLI36SRVJ5TBL X-Message-ID-Hash: IS4LVVSJMWN3JUPMH6PNLI36SRVJ5TBL X-MailFrom: valentin@gagarin.work X-Mailman-Rule-Hits: member-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; emergency CC: Spectrum OS Development , Alyssa Ross X-Mailman-Version: 3.3.10 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, 21 Jul 2026 21:59:08 -0400, Demi Marie Obenour wrote: > diff --git a/Documentation/doc/development/control-groups.adoc b/Documentation/doc/development/control-groups.adoc > new file mode 100644 > index 0000000..6ce33f2 > --- /dev/null > +++ b/Documentation/doc/development/control-groups.adoc > @@ -0,0 +1,88 @@ > += Control groups in Spectrum > + > +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour > +// SPDX-License-Identifier: GFDL-1.3-no-invariants-or-later OR CC-BY-SA-4.0 > + > +Linux control groups (cgroups) can be used for several purposes: > + > +1. They allow waiting for a group of processes to exit. > +2. They allow terminating a group of processes. > +3. They allow limiting a group of processes' access to resources. Not repeating prefixes helps readability. And a link to upstream documentation is always good. https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html[Linux control groups] (cgroups) can be used for several purposes: 1. Waiting for a group of processes to exit. 2. Terminating a group of processes. 3. Limiting a group of processes' access to resources. > [ ... skip 10 lines ... ] > +2. The per-VM services for each VM are under `/vm-services.slice/vm-${VM}.slice`, > + where `${VM}` is replaced by the VM's ID. > +3. Each per-VM service is under `/vm-services.slice/vm-${VM}.slice/${SERVICE_NAME}`, > + where `${VM}` is replaced by the VM's ID and `${SERVICE_NAME}` is replaced by > + the name of the service. > +4. The VMM runs under `/vm-services.slice/vm-${VM}.slice/vmm`. I don't think we introduce VMM anywhere else in the documentation, so let's write it out. 4. The virtual machine manager (VMM) runs under `/vm-services.slice/vm-${VM}.slice/vmm`. > + > +If a cgroup contains child cgroups, it likely contains a `$inner.service` > +cgroup. This is where programs that would otherwise run in the cgroup itself > +are placed. Generally, these programs are instances of `s6-svscan` and/or > +`s6-supervise`. Since we're not discussing s6 anywhere, please link to what these things mean. Also I think it's fine to imply logical OR when using "or" with our audience, and reduce the visual noise (the general recommendation is to avoid slashes in prose). are placed. Generally, these programs are instances of https://skarnet.org/software/s6/s6-svscan.html[`s6-svscan`] or https://skarnet.org/software/s6/s6-supervise.html[`s6-supervise`]. > + > +== Using Control Groups > + > +When adding a new s6 service, one should carefully consider whether it > +should be placed in a control group. Most services should be placed in > +a control group, with only a few exceptions: The first sentence reads as if one now has to make serious decisions, but the second one amounts to "mostly no". This was confusing to read, and took attention away from the relevant parts, which seem to revolve around "how to do that". I recommend removing the first sentence. Most services should be placed in a control group, with only a few exceptions: > + > +1. Services, such as `getty`, that spawn background processes. > +2. Loggers. > +3. Trivial services that don't do anything. > + > +Generally, it's best to set the control group up as the first thing > +the service does. To do that, use `cgroup-setup --leaf -- $1 COMMAND_LINE`, > +where `$1` should be the service name and `COMMAND_LINE` is the program > +to run in a cgroup. It took me a while to figure out that this is a new tool that comes with Spectrum. Not sure how to handle that best, but here's an attempt: Generally, it's best to set the control group up as the first thing the service does, with Spectrum's https://spectrum-os.org/git/spectrum/tree/tools/cgroup-setup[`cgroup-setup`]. Call `cgroup-setup --leaf -- $1 COMMAND_LINE`, where `$1` should be the service name and `COMMAND_LINE` is the program to run in a cgroup. > + > +If you use execline for your run script, this is as simple as: For example, using https://skarnet.org/software/execline/[execline] for your run script: > + > +.run > +.... But I'd prefer "manually" to make that unambiguous. > +#!/bin/execlineb -WS1 That renders as a separate paragraph only containing "run". Let's remove that, it doesn't make sense visuallyr > + > +cgroup-setup --leaf -- $1 > +# rest of script comes here > +.... > + > +If the service exits, it's usually best to terminate any programs left > +behind with SIGKILL and remove the control group. In Spectrum, this is To keep the convention of displaying code items in monospace. Also shouldn't it be "when a service exits", since it's usually a matter of time and holds for any service? When a service exits, it's usually best to terminate any programs left behind with `SIGKILL` and remove the control group. In Spectrum, this is This new (sub-)section could use a heading, such as "Purging control groups". A separate section also removes the implication that we're still talking about "the" same service as before. > +called "purging" the cgroup. To purge the cgroup when a service exits, > +make the `finish` script invoke `/usr/bin/cgroup-s6-finish`. The first > +two command line arguments must be the first two arguments passed to the > +`finish` script. The third argument must be the path to the cgroup to To purge the cgroup when a service exits, make the `finish` script in the https://skarnet.org/software/s6/servicedir.html[s6 service directory] invoke Spectrum's `/usr/bin/cgroup-s6-finish`. This also needs an example. It took me a couple of times to visualise that it's just `cgroup-s6-finish $1 $2 $3`. And is it necessary to say `/usr/bin/...`? Ideally we'd already indicate here that `cgroup-s6-finish` is a symlink to `cgroup-setup`. > +be purged relative to the cgroup the program itself is in. This is > +usually, but not always, the third argument to the `finish` script. This needs more information. When is it the third argument, when is it not? I'd be helpless at this point. > + > +When invoked as `cgroup-s6-finish`, `cgroup-setup` checks if > +the service exited due to a signal that caused it to dump core. If it > +did, `cgroup-s6-finish` exits with status 125, ensuring that > +`s6-supervise` will *not* restart it. This is intentional: if a service +crashes due to a fatal signal, this is possibly a sign of memory > +crashes due to a fatal signal, this is possibly a sign of memory > +corruption. Restarting the service in this case can turn an unreliable This should probably go into reference documentation for that program, but let's keep it here until we have a place to list it and a way to do that automatically. > +memory corruption exploit into a reliable one. Rust panics do not cause > +core dumps, so the service will be restarted afterwards. I don't understand what that last sentence means for me as a user. This seems detached from the previous explanations. > + > +One can also use `cgroup-purge` to purge a cgroup explicitly. This is > +used to stop the VMM and all per-VM services when a VM is shut down. Do you mean that command is intended to be used manually? I'm not sure here, because "explicitly" doesn't intuitively convey that to me. If you want to keep the terminology, an example with a bit of introduction would help, such as: > + > +== Future plans > + > +Control groups are designed around a single writer process controlling each > +of them. Many Linux distros use systemd for this, but Spectrum doesn't use > +systemd. The only persistent per-service process is s6-supervise, but that > +doesn't have control group support. s/distros/distributions/ and maybe highlight command names with monospace. Control groups are designed around a single writer process controlling each of them. Many Linux distributions use `systemd` for this, but Spectrum doesn't use `systemd`. The only persistent per-service process is `s6-supervise`, but that doesn't have control group support. Generally I strongly recommend one sentence per line for documentation, a recommendation that apparently goes back to early Unix[1]. That makes review a lot easier. As you can observe, otherwise one has to cut random unrelated pieces from the diff to discuss one word or phrase from a sentence. We don't have to introduce it now, but should start this as a convention. [1]: https://rhodesmill.org/brandon/2012/one-sentence-per-line/ Reviewed-by: Valentin Gagarin -- Valentin Gagarin