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

Support Linux on MIPS and ARMv5TE #39

Merged
merged 9 commits into from
Sep 10, 2021
Merged

Support Linux on MIPS and ARMv5TE #39

merged 9 commits into from
Sep 10, 2021

Conversation

tatsuya6502
Copy link
Member

@tatsuya6502 tatsuya6502 commented Sep 9, 2021

This pull request is for supporting Linux on 32-bit MIPS and ARMv5TE. These platforms does not provide std::sync::atomic::AtomicU64 so currently both Moka and quanta will not compile.

After merging this PR, Moka will compile on 32-bit MIPS. For ARMv5TE, you need to add default-features = false to "moka" dependency in Cargo.toml and then Moka will compile.

After this PR is merged, you can resolve the errors by disabling the default features by adding default-features = false to the dependency declaration in Cargo.toml. This will remove quanta from the dependencies and enable a fall-back implementation in Moka.

Changes

  • Add a cargo feature quanta atomic64 and make it a default feature.
  • Add a module common::time and conditionally choose the source file from "time_quanta.rs" or "time_compat.rs".
    • time_quanta.rs file has type aliases to quanta::{Instant, Clock, Mock}. It also has AtomicInstant holding an AtomicU64.
    • time_compat.rs file has Instant based on std::time::Instant and quanta-like Clock and Mock. It also has AtomicInstant holding a RwLock<Option<Instant>>.
  • Update Cache, etc. modules to use common::time instead of quanta.
  • Add a step to GitHub Actions to test future feature without quanta/AtomicU64.

Fixes #38.

These platforms does not provide `std::sync::atomic::AtomicU64`.

- Add a cargo feature "quanta" and make it a default feature.
- Add a module `common::time` and conditionally choose the source file
  from "time_quanta.rs" or "time_compat.rs".
    - time_quanta.rs file has type aliases to `quanta::{Instant, Clock, Mock}`.
      It also has `AtomicInstant` holding an `AtomicU64`.
    - time_compat.rs file has `Instant` based on `std::time::Instant`
      and quanta-like `Clock` and `Mock`. It also has `AtomicInstant`
      holding a `RwLock<Option<Instant>>`.
- Update Cache, etc. modules to use `common::time` instead of `quanta`.
- Add a step to GitHub Actions to test future feature without quanta/
  `AtomicU64`.
@tatsuya6502 tatsuya6502 self-assigned this Sep 9, 2021
@tatsuya6502 tatsuya6502 added the enhancement New feature or request label Sep 9, 2021
@tatsuya6502 tatsuya6502 added this to the v0.5.3 milestone Sep 9, 2021
@tatsuya6502 tatsuya6502 changed the title Mips armv5 Support Linux on MIPS and ARMv5TE Sep 9, 2021
Remove some old codes.
Remove target_arch = "mips" from the cfg_attr for the time module. (It will
not work when quanta feature is still enabled it Cargo.toml)
Add a section to the README to explain that `default-features = false` will
be needed on these platforms.
@messense
Copy link
Contributor

messense commented Sep 10, 2021

Note that I've also opened metrics-rs/quanta#55 to fix quanta on these platforms, so maybe after that lands moka can still use quanta but not AtomicU64. In this case I think the feature name should not be quanta.

@tatsuya6502
Copy link
Member Author

Thanks. Good point.

Hmm. I am having hard time to come up with a good short name for the feature. Moka uses AtomicU64 as a lightweight atomic type for timestamps. It is for space efficiency and also high performance.

So... perhaps, atomic-time, ... or just atomic-u64/atomic64?

I think atomic64 might be OK. It may be too straightforward, but works. It will be easy to understand that you need to disable atomic64 feature if platform does not support 64-bit atomic types.

- Rename the Cargo feature "quanta" to "atomic64".
- Update the change log and readme.
- Update a comment in the yml for GitHub Actions.
Copy link
Contributor

@messense messense left a comment

Choose a reason for hiding this comment

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

Thank you.

@messense
Copy link
Contributor

messense commented Sep 10, 2021

I've just tested this on a mipsel router, it compiles and runs, but hits seanmonstar/num_cpus#69 , I think this line should be changed to use 1 if num_cpus::get() returns 0

let num_threads = num_cpus::get();

See also rust-lang/futures-rs#1835

Ensure to set num threads to 1 for thread pools when `num_cpus::get()` returns 0.
@tatsuya6502 tatsuya6502 marked this pull request as ready for review September 10, 2021 13:14
@tatsuya6502
Copy link
Member Author

Thanks for testing on the real hardware. I did not realize it could return 0. Changed to use 1 when it returns 0.

@tatsuya6502
Copy link
Member Author

Verified the followings:

  1. Ran cargo build for the following targets using Docker images messense/rust-musl-cross and verified they succeeded:
    • armv5te-unknown-linux-musleabi
    • mips-unknown-linux-musl
    • mipsel-unknown-linux-musl
    • commit: d91c5ce
  2. Ran regular prerelease load tests using mokabench on x86_64-unknown-linux-gnu. Verified that the test runs with and without the default features found no new issue.

Test 1

% git remote -v
origin	git@github.com:moka-rs/moka.git (fetch)
origin	git@github.com:moka-rs/moka.git (push)

$ git log -1
commit d91c5ce4bbd6d403e1fe37fa9ed1831225b18434 (HEAD -> mips-armv5, origin/mips-armv5)
Author: Tatsuya Kawano
Date:   Fri Sep 10 20:19:23 2021 +0800

    Bump the version to v0.5.3


$ sudo rm -rf release

$ docker run -it --rm -v $(pwd):/home/rust/src \
  --mount source=cargo-registry,target=/root/.cargo/registry \
  messense/rust-musl-cross:armv5te-musleabi \
  cargo build --no-default-features --features future --target armv5te-unknown-linux-musleabi
   ...
   Compiling moka v0.5.3 (/home/rust/src)
   ...
    Finished dev [unoptimized + debuginfo] target(s) in 32.67s

$ echo $?
0

$ docker run -it --rm -v $(pwd):/home/rust/src \
  --mount source=cargo-registry,target=/root/.cargo/registry \
  messense/rust-musl-cross:mips-musl \
  sh -c "RUSTFLAGS='-C target-feature=+crt-static -C link-arg=-s' \
    cargo build --no-default-features --features future --target mips-unknown-linux-musl"
   ...
   Compiling moka v0.5.3 (/home/rust/src)
   ...
    Finished dev [unoptimized + debuginfo] target(s) in 8.96s
$ echo $?
0

$ docker run -it --rm -v $(pwd):/home/rust/src \
  --mount source=cargo-registry,target=/root/.cargo/registry \
  messense/rust-musl-cross:mipsel-musl \
  sh -c "RUSTFLAGS='-C target-feature=+crt-static -C link-arg=-s' \
    cargo build --no-default-features --features future --target mipsel-unknown-linux-musl"
   ...
   Compiling moka v0.5.3 (/home/rust/src)
   ...
    Finished dev [unoptimized + debuginfo] target(s) in 8.82s
$ echo $?
0

@messense
Copy link
Contributor

Test again on mipsel router and I'm happy to report that it works™.

@tatsuya6502
Copy link
Member Author

Thank you for testing. I am merging this and will publish v0.5.3 to crate.io soon.

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

Successfully merging this pull request may close these issues.

2 participants