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

deterministic hugepage size, set without kexec #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

gehoern
Copy link
Contributor

@gehoern gehoern commented Nov 22, 2024

hugepaes have not been set deterministically, fixed by adding it to the kernel cmdline

added a direct application of hugepages to dracut -> no kexec needed anymore

has a slight delay but in initrd this still should be fast.

@NotTheEvilOne NotTheEvilOne mentioned this pull request Nov 24, 2024
@@ -0,0 +1 @@
CMDLINE_LINUX="$CMDLINE_LINUX default_hugepagesz=1G"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want to change the hugepagesz, the default (2MB) is fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to follow up on the discussion we had recently:

On cascade lake we have:

  • L1 TLB:
    • 4x32 2MB Entries covering 128MB of memory
    • 1x4 1G entries covering 4GB of memory

It pretty much depends on the locality of your workload whether 2M or 1G pages are the better option. My suggestion would be the same as @fwiesel's. Let's stick with 2MB pages for the time being.

When we have everything working, we can still do benchmarks to figure out whether 1G pages are actually helping, or not.

Comment on lines +44 to +54
#cmdline="$(</proc/cmdline) hugepages=$hugepages"
#release=$(uname -r)

#NEWROOT=${NEWROOT:-/sysroot}

#kexec \
# -l $NEWROOT/boot/vmlinuz-${release} \
# --initrd=$NEWROOT/boot/initrd.img-${release} \
# --command-line="$cmdline"

#kexec -e --reset-vga
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove it instead?

@@ -12,5 +12,5 @@ depends() {
# Install the required file(s) and directories for the module in the initramfs.
install() {
inst_hook pre-pivot 00 "$moddir/ensure-hugepages.sh"
inst kexec
#inst kexec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove that comment as well.

@NotTheEvilOne
Copy link
Contributor

@fwiesel, @gehoern and @toanju can we integrate the changes @fwiesel mentioned into the script to merge this PR?


NEWROOT=${NEWROOT:-/sysroot}
echo $hugepages > /proc/sys/vm/nr_hugepages
# might get more hugepages and faster with the kernel cmdline, but like this you avoid a kexec reboot.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding: What exactly do you mean with this comment? Why do we avoid a kexec reboot when we use it like this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gehoern moved applying hugepages to a slightly later point in booting meaning that the Linux kernel might move around some pages in that process (if than only a few at this moment in time).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NotTheEvilOne this part I understand, I just don't understand why we would need a kexec cycle otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kexec has been used originally to apply the changes. This has been rewritten in that PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, then the comment makes sense. Thanks for elaborating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants