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

pahole: patch to force single-threaded mode if reproducibility is desired #231768

Merged
merged 1 commit into from
May 15, 2023

Conversation

delroth
Copy link
Contributor

@delroth delroth commented May 14, 2023

Description of changes

#223584 #227800

pahole processes DWARF -> BTF in a pseudo-random order if multi
threading is enabled (default in the Linux kernel). This can be fixed in dependent
derivation by making sure users don't ever pass "-j" or "-j N", but it
seems easier and safer to just detect whether reprodubility is desired
(by proxy: if SOURCE_DATE_EPOCH is set) and ignore the multi-threading
flag.

Tested with a zfs --check build, it passes with this patch applied!

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@delroth
Copy link
Contributor Author

delroth commented May 14, 2023

@ofborg build linuxPackages.zfs linux_latest

Copy link
Member

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

I guess it makes sense -- have you brought it up with upstream?
I'm sure the merge process could be made reproducible if they tried.

@delroth
Copy link
Contributor Author

delroth commented May 14, 2023

I guess it makes sense -- have you brought it up with upstream? I'm sure the merge process could be made reproducible if they tried.

I think it could be but it would not be trivial from what I can tell -- the threads just grab DWARF info from the input in an uncoordinated fashion, so you'd need some major refactoring to split up the input in a consistent way, or to sort the output while merging.

I've not communicated with upstream at all at this point.

@martinetd
Copy link
Member

I've opened acmel/dwarves#42 for now

@delroth delroth force-pushed the pahole-btf-repro branch from db5e997 to 2c4c9d1 Compare May 14, 2023 04:02
@delroth delroth changed the title pahole: patch to default to single-threaded mode if reproducibility is desired pahole: patch to force single-threaded mode if reproducibility is desired May 14, 2023
@delroth
Copy link
Contributor Author

delroth commented May 14, 2023

Thanks a lot for the insightful review, I've updated this PR:

  • Comment in the patch updated to mention it's forcing single-threaded mode, not just changing the default.
  • Commit message is updated to match.
  • Commit message also mentions that the issue comes from user (in this case the Linux kernel build scripts) using -j or -jN.

@martinetd
Copy link
Member

Thanks!

Unless there's any reason to hurry let's give upstream a couple of days to reply (at least Arnaldo does this for work so we should wait for weekdays); otherwise LGTM

To whoever merges this: might want to retarget staging instead given number of packages involved

@delroth delroth changed the base branch from master to staging May 14, 2023 04:11
@delroth
Copy link
Contributor Author

delroth commented May 14, 2023

Unless there's any reason to hurry let's give upstream a couple of days to reply (at least Arnaldo does this for work so we should wait for weekdays); otherwise LGTM

No objection, I don't think there's any hurry. @raboof any particular timeline we should try to keep in mind re: reproducible builds and 23.05 release?

To whoever merges this: might want to retarget staging instead given number of packages involved

Done.

@ofborg ofborg bot requested a review from martinetd May 14, 2023 04:21
…ired

pahole's processes DWARF -> BTF in a pseudo-random order if multi
threading is enabled (default in the Linux kernel). This can be fixed in dependent
derivation by making sure users don't ever pass "-j" or "-j N", but it
seems easier and safer to just detect whether reprodubility is desired
(by proxy: if SOURCE_DATE_EPOCH is set) and ignore the multi-threading
flag.
@raboof
Copy link
Member

raboof commented May 14, 2023

Great find! Does this slow down the kernel build or is the parallelism inconsequential here?

Unless there's any reason to hurry let's give upstream a couple of days to reply (at least Arnaldo does this for work so we should wait for weekdays); otherwise LGTM

No objection, I don't think there's any hurry. @raboof any particular timeline we should try to keep in mind re: reproducible builds and 23.05 release?

I don't think we expect to fix #188978 for 23.05, so I agree there's no hurry and we can take this at its 'natural pace' 👍

@martinetd
Copy link
Member

Great find! Does this slow down the kernel build or is the parallelism inconsequential here?

This is more a developer comfort thing imo -- from the commit message in acmel/dwarves@f104790 it should be something like 20s -> 5s

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

LGTM

@RaitoBezarius RaitoBezarius merged commit b94e599 into NixOS:staging May 15, 2023
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.

4 participants