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

fix: nix-shell environment for building spdk on arm #72

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

maxwnewcomer
Copy link
Contributor

@maxwnewcomer maxwnewcomer commented Dec 23, 2024

This PR documents the changes needed to get the ARM64 build working and running the example hello-world application. Here are some of the issues encountered along the way.

  1. Debugging the ARM Nix env

    • I spun up an arm64 machine on Hetzner and debugged the install process step by step.
    • First tried running nix-shell without SPDK and hit issues with pyelf. Replacing python3.withPackages with pkgs.python3Packages.pyelftools fixed that.
  2. Compiler Issues

    • The main problem in a manual build was the -msse4 flag, which is for x86 extended instruction sets and doesn’t work on ARM.
    • I split the nix-shell compiler flags into configurable strings so we can set optimization flags per architecture.
    • Then, in the default nix-shell spdk configure and build, Clang was the default compiler, this led to SVE compilation issues. In the "no-spdk-installed" nix environment, I noticed I was getting success w/ GCC, so migrated the nix script to remove Clang and use GCC by default. I don't want to get presumptuous that this is the best solution, very open to feedback on this.

Context

  • As mentioned in the parent OEP ([ OEP 3817 ] - Support arm64 Deployments openebs#3817), the goal here is just to make ARM builds possible—not optimized.
  • I have also tested the new changes on an amd64 system and everything went as expected. Not super familiar with what test suite I should be running.

Addresses


Please let me know if you'd rather me focus on getting the OEP to implementable rather than starting to implement fixes!

@maxwnewcomer maxwnewcomer marked this pull request as ready for review December 24, 2024 20:24
@maxwnewcomer maxwnewcomer marked this pull request as draft December 25, 2024 16:45
@maxwnewcomer maxwnewcomer marked this pull request as ready for review December 25, 2024 16:58
@tiagolobocastro
Copy link
Contributor

Then, in the default nix-shell spdk configure and build, Clang was the default compiler, this led to SVE compilation issues. In the "no-spdk-installed" nix environment, I noticed I was getting success w/ GCC, so migrated the nix script to remove Clang and use GCC by default. I don't want to get presumptuous that this is the best solution, very open to feedback on this.

Changing compiler doesn't sound like something to do lightly, even though it may compile.
I'd rather first try to explore other ways of fixing the SVE compilation, what do you think @dsharma-dc ?

@maxwnewcomer
Copy link
Contributor Author

Yeah, I totally understand how that is not ideal. I've been banging at it for a little bit hoping I've been missing something obvious.

@tiagolobocastro
Copy link
Contributor

Btw I've added github-arm64-2c-8gb runner to this repo, here's an example: bb3a764
And of course it fails to build...
Maybe you can also integrate this in your PR?

@maxwnewcomer
Copy link
Contributor Author

maxwnewcomer commented Dec 27, 2024

Btw I've added github-arm64-2c-8gb runner to this repo, here's an example: bb3a764 And of course it fails to build... Maybe you can also integrate this in your PR?

This is great, and hahah classic. Sorry that may be on me for not running it in with the --pure flag. I'll dig into it asap. I'll add that in here as well. Thanks for all of your help.

Edit: just noticed that failed run wasn't on this branch, my bad.

Copy link
Contributor

@tiagolobocastro tiagolobocastro left a comment

Choose a reason for hiding this comment

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

Alright, I had a better look at this and looks like up to release/2.7 the existing SPDK build was actually using gcc to build and not clang.
I'm not exactly sure why both gcc and some clang packages are included in the build, but it seems the order on which they appear changes the CC environment variable, and looks like after the recent refactoring on the develop branch, CC ends up pointing to clang and not gcc.
This is the PR that initially introduced llvm packages: openebs/mayastor#671. Funny enough that it was also to bring aarch64 support :)

So, I think using gcc is a good idea anyway since that's how it used to be, and was probably an unfortunate mistake that ended up changing that during the latest refactoring.

Also, please try to build mayastor io-engine with your spdk-rs code to ensure that is also working with this.

Thanks you!

nix/shell/spdk.nix Outdated Show resolved Hide resolved
@maxwnewcomer
Copy link
Contributor Author

Funny enough that it was also to bring aarch64 support :)

Thank you for doing the historical deep-dive to figure out the root of things. ^ This callout made me chuckle, so thanks for that.

So, I think using gcc is a good idea anyway since that's how it used to be, and was probably an unfortunate mistake that ended up changing that during the latest refactoring.

Great!

Also, please try to build mayastor io-engine with your spdk-rs code to ensure that is also working with this.

Yes of course, I'll do that asap. I noticed you guys were talking about merging the repos here #64, the CI for SPDK-rs also being tested downstream alone may be a convincing enough argument for the shift.

@maxwnewcomer
Copy link
Contributor Author

maxwnewcomer commented Dec 28, 2024

I tried building through the release script + just raw cargo build. The nix-shell, built fine as expected. However on the build of the io-engine I'm running into:

error[E0308]: mismatched types
   --> io-engine/src/bdev/ftl.rs:213:25
    |
213 |         ftl_conf.name = ftl_dev_name.as_ptr() as *mut i8;
    |         -------------   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `*mut u8`, found `*mut i8`
    |         |
    |         expected due to the type of this binding
    |
    = note: expected raw pointer `*mut u8`
               found raw pointer `*mut i8`

error[E0308]: mismatched types
   --> io-engine/src/bdev/ftl.rs:214:30
    |
214 |         ftl_conf.base_bdev = base_dev_name.as_ptr() as *mut i8;
    |         ------------------   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `*mut u8`, found `*mut i8`
    |         |
    |         expected due to the type of this binding
    |
    = note: expected raw pointer `*mut u8`
               found raw pointer `*mut i8`

error[E0308]: mismatched types
   --> io-engine/src/bdev/ftl.rs:215:31
    |
215 |         ftl_conf.cache_bdev = cache_dev_name.as_ptr() as *mut i8;
    |         -------------------   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `*mut u8`, found `*mut i8`
    |         |
    |         expected due to the type of this binding
    |
    = note: expected raw pointer `*mut u8`
               found raw pointer `*mut i8`

although the bdev/ftl.rs file does import spdk-rs bindings, these are all either String or CString .as_ptr() calls which seems auxilary to the spdk changes.

The docs for CStr do say:

Returns the inner pointer to this C string.

The returned pointer will be valid for as long as self is, and points to a contiguous region of memory terminated with a 0 byte to represent the end of the string.

The type of the returned pointer is [*const c_char], and whether it's an alias for *const i8 or *const u8 is platform-specific.

This may be able to be fixed by casting to std::os::raw::c_char or libc::c_char instead of hard coding i8 or u8

@dsharma-dc
Copy link
Contributor

This may be able to be fixed by casting to std::os::raw::c_char or libc::c_char instead of hard coding i8 or u8

Yes this should work with std::os::raw::c_char.
cc: @MaisenbacherD

@maxwnewcomer
Copy link
Contributor Author

maxwnewcomer commented Dec 31, 2024

Have a full mayastor release build on arm with this mayastor pr branch openebs/mayastor#1796 pointed to this pr's branch of spdk-rs!!

@tiagolobocastro
Copy link
Contributor

Awesome, great work @maxwnewcomer !!!

@MaisenbacherD
Copy link
Contributor

MaisenbacherD commented Dec 31, 2024

This may be able to be fixed by casting to std::os::raw::c_char or libc::c_char instead of hard coding i8 or u8

Yes this should work with std::os::raw::c_char. cc: @MaisenbacherD

Agree, as @maxwnewcomer already addressed this with openebs/mayastor#1796. Thanks! :)

spdk-rs used to not build on arm solely due to the
nix environment setup. There were issues with compiler
flags (-msse4 is x86 only) and some base environment
configuration. By fixing the environment we were able
to get a successful build of spdk-rs.

On top of these fixes we are able to test them in the
github action runner which runs nix-shell. This in turn
compiles spdk from source (openebs/spdk).

Signed-off-by: Max Newcomer <[email protected]>
@tiagolobocastro
Copy link
Contributor

@maxwnewcomer after the mayastor fix is this good to merge now?
@dsharma-dc please review again

@maxwnewcomer
Copy link
Contributor Author

Yes this should be good now. @tiagolobocastro. Might be good to do a mayastor CI run after this gets merged.

@tiagolobocastro tiagolobocastro merged commit 103dfc7 into openebs:develop Jan 8, 2025
9 checks passed
@tiagolobocastro
Copy link
Contributor

Thanks @maxwnewcomer !
Now you should raise PR on mayastor to update spdk-rs submodule:
git submodule update --remote spdk-rs

@maxwnewcomer
Copy link
Contributor Author

Thanks @maxwnewcomer ! Now you should raise PR on mayastor to update spdk-rs submodule: git submodule update --remote spdk-rs

Appreciate the direction! Thanks. Working on an "implementable" oep pr btw.

bors-openebs-mayastor bot pushed a commit to openebs/mayastor that referenced this pull request Jan 8, 2025
1797: chore: update spdk-rs for arm support r=tiagolobocastro a=maxwnewcomer

Updates `spdk-rs` to an "arm-buildable" `spdk-rs` version.

Update incorporates commits solely from openebs/spdk-rs#72

Co-authored-by: Max Newcomer <[email protected]>
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.

Will not build on ARM
4 participants