-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Enable panic_on_oom and requirements checks/warnings #1626
Enable panic_on_oom and requirements checks/warnings #1626
Conversation
@tlaurion This doesn't work :-/ This has to be done via a sysctl. I tested on Librem 14 (32 GB RAM), by dd'ing endlessly into a file (ramfs). The OOM killer still gets invoked several times. In this case the system does eventually panic (because it is the kernel actually using tons of memory here, if the OOM killer actually reclaimed significant memory it would not have panicked). That's the same on master too, no change. Enabling CONFIG_PROC_SYSCTL and checking So I think we have two options:
|
Hmmm. Agreed, it is not applied, and testing on qemu biased the results because we have TMPFS which restricts memory allocation and inhibited oom killer from kicking in if redoing
Otherwise dd op is stopped from exhausting memory completely and simply exits saying that:
As opposed to being killed by oom_killer without TPMFS:
Interestingly enough, newer kernels (5.8+) permit to apply sysctl configs directly from command line without the need of additional tooling nor hacks in init scripts https://serverfault.com/questions/1082967/can-i-change-the-default-sysctl-values-in-grub. I'm really confused about the kernel output of non-applied parameters nowadays:
As can be seen, not passed down to init as not applied to kernel, but also nothing kernel side saying that parameter was applied either. If we try something else invalid:
Here, we see that the invalid parameter is passed down as an environment variable to be picked up as expected. |
tmpfs doesn't restrict memory allocation in general, it only restricts the amount of memory used by a tmpfs mount. There is still a risk that we could run out of memory another way. Since this doesn't address OOM conditions in general (which scripts aren't written to handle, which is the problem), IMO we should focus on enabling panic on OOM.
That's a neat find, but I think that it would be more maintainable to do this from
It's documented this way, from https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html:
The intent AFAIK is just that the userspace tooling that will apply sysctls will go check the kernel command line on its own rather than having to plumb this through init and every layer in between. |
Alright. Agreed with all the above. So gonna make another PR that will warn if kernel config options are not enabled properly and supersedes this one. I think we are going in the path of having kconfig kernel requirements for Heads: this one would be part of the basics which if not present should scream being missing. |
Agree 👍 Opened #1628 with an idea how we might manage that |
(and we cannot simply expect all kernels to be 5.8+ anyways. Talos 2 comes to mind here and bumping kernel versions is not an easy task either until something else is figured out: kconfig all no+punching general requirements + board specifics and having boards similar in such ways) |
Signed-off-by: Thierry Laurion <[email protected]>
89988fa
to
045cedf
Compare
@JonathonHall-Purism I went ahead and applied No need to create another PR for this, just modified PR name here since branch was on goal name and still valid. When /proc/sys doesn't exist:
When /proc/sys/vm/panic_on_oom doesn't exist:
Which are alligned to #1628 future goals |
OP modified. @JonathonHall-Purism ready for review. |
Signed-off-by: Thierry Laurion <[email protected]>
045cedf
to
c73687a
Compare
Closes #1619
CONFIG_SYSCTL
andCONFIG_PROC_SYSCTL
added under all linux configsCONFIG_TMPFS
removed from all linux configs (to allign with purism config. Other configs need to be alligned across board configs as we already know which is Better manage Heads kernel config requirements #1628)Output in case config missing at #1626 (comment)
OLD: no, we cannot apply this directly through coreboot-> linux(heads payload) command line but if we were using kernel 5.8+.
OLD traces:
Had to launch qemu with
-m 75M
to simulate early crash:At -m 25M bootsplash doesn't show. No downside, maybe only on the x230-legacy-boars but anyway, going to be deprecated soon and there is no way someone would put such low memory sticks in their machines.
@JonathonHall-Purism please review!