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

linux: enable MGLRU in config options #8275

Merged
merged 1 commit into from Nov 6, 2023
Merged

Conversation

smp79
Copy link
Contributor

@smp79 smp79 commented Nov 2, 2023

@chewitt
Copy link
Member

chewitt commented Nov 2, 2023

Looks interesting, but the project has a long history of people showing up with the latest fashionable kernel tweak only to discover that in a minimalist (non-server, non-desktop) OS where everything already runs in RAM; there's no meaningful improvement. I'd like to see a higher standard of performance testing than "doesn't seem to break anything" please :)

@smp79
Copy link
Contributor Author

smp79 commented Nov 2, 2023

That was a forum user request. The change looks harmless enough, so I don't see why not.

@chewitt
Copy link
Member

chewitt commented Nov 3, 2023

If we implemented every user request from the forums LE would be a 1.2GB image download and we'd still be supporting i386 hardware. If you read more on this topic, i.e. https://lore.kernel.org/linux-mm/be281873-7c28-12b4-7eb5-50d08042549f@intel.com/ it's quite clear that the change targets and benefits systems under heavy memory pressure. Due to the low footprint of LE (450MB used on a 1GB system) and 1GB being the minimum hardware spec that we support now; most LE systems are not under memory pressure. Thus it would be good to run some performance testing with this change to see whether it has any real world benefit, or understand what conditions that it benefits. If there is proveable benefit we should add the change. If there isn't, we generally choose not to alter the dynamics of a stable kernel config.

@rsalvaterra
Copy link

@chewitt, that goes both ways, I guess. Case in point: why does the LibreELEC kernel need to reserve 256 MiB of CMA by default on the x86-64 architecture? Or why does it need to have any cgroup controllers enabled (aside from CONFIG_CGROUPS=y, which is required by systemd)?

@chewitt
Copy link
Member

chewitt commented Nov 3, 2023

I'm 100% sure there questionnable defconfig options set among our codebase. it would be great to rationalise them, but anyone doing that will be equally requested to show evidence their removal or change does no harm and brings a benefit to the disro. Otherwise we desire minimum change to stable/proven configuration and have a "trust, but verify" approach for suggested changes.

@popcornmix
Copy link
Contributor

We do have this enabled on RPiOS kernel.

There was an issue where OOM became common with CONFIG_VMSPLIT_3G (only used on 32-bit Pi4 kernel), where we had to disable MGLRU, but that shouldn't affect you.

We've had this enabled for 6 months or so, so I'm pretty happy it doesn't break anything.
Some anecdotes of it being better, but not a lot of hard evidence (outside of Phoronix benchmarks).

@HiassofT
Copy link
Member

HiassofT commented Nov 3, 2023

FYI: Debian x86_64 kernel is built with CONFIG_LRU_GEN=y but doesn't enable it by default so it's opt-in (via sysfs). That seems like a reasonable choice but likely adds some overhead compared to LRU_GEN=n

hias@lenny:~$ grep LRU_GEN /boot/config-6.1.0-13-amd64 
CONFIG_LRU_GEN=y
# CONFIG_LRU_GEN_ENABLED is not set
# CONFIG_LRU_GEN_STATS is not set

@rsalvaterra
Copy link

There was an issue where OOM became common with CONFIG_VMSPLIT_3G (only used on 32-bit Pi4 kernel), where we had to disable MGLRU, but that shouldn't affect you.

Why would anyone run a 32-bit kernel on a 64-bit CPU? A 32-bit userspace I understand, but the kernel…? That's just insanity.

@popcornmix
Copy link
Contributor

popcornmix commented Nov 3, 2023

Why would anyone run a 32-bit kernel on a 64-bit CPU? A 32-bit userspace I understand, but the kernel…? That's just insanity

32-bit userspace is needed by some if sdcard is used on older Pi's.
We have since switched the default kernel to 64-bit, but it has introduced a number of issues.
Mostly build files using arch (which is kernel's bit-ness) to choose gcc flags (which depends on userspace bit-ness).

@lrusak
Copy link
Member

lrusak commented Nov 3, 2023

@chewitt, that goes both ways, I guess. Case in point: why does the LibreELEC kernel need to reserve 256 MiB of CMA by default on the x86-64 architecture?

Because x86 can still use drm prime in Kodi which allocates from the cma pool (although it's for sw rendering only where inputstream is used).

Or why does it need to have any cgroup controllers enabled (aside from CONFIG_CGROUPS=y, which is required by systemd)?

Not 100% sure on this one but it could be for docker.

@smp79
Copy link
Contributor Author

smp79 commented Nov 5, 2023

I'm changing this to # CONFIG_LRU_GEN_ENABLED is not set as suggested by @HiassofT. Anyone who wants MGLRU can just add echo y >/sys/kernel/mm/lru_gen/enabled to autostart.sh.

@rsalvaterra
Copy link

I'm changing this to # CONFIG_LRU_GEN_ENABLED is not set as suggested by @HiassofT. Anyone who wants MGLRU can just add echo y >/sys/kernel/mm/lru_gen/enabled to autostart.sh.

That'd be an acceptable compromise, in my humble opinion.

@chewitt chewitt merged commit a5c8d95 into LibreELEC:master Nov 6, 2023
@lrusak
Copy link
Member

lrusak commented Nov 6, 2023

Should this be added to distributions/LibreELEC/kernel_options?

@smp79 smp79 deleted the mglru branch December 3, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants