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

feat: add backoff feature #16

Merged
merged 2 commits into from Oct 17, 2023
Merged

feat: add backoff feature #16

merged 2 commits into from Oct 17, 2023

Conversation

mkroening
Copy link
Contributor

Closes #14

@mkroening
Copy link
Contributor Author

I chose backoff over exponential-backoff for the feature name for brevity. What do you think? 🤔

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks a lot! The implementation looks good to me, but I'm not sure if controlling the behavior through a feature gate is a good idea. Cargo unifies features across dependencies, so other dependencies that also depend on spinning_top could then silently change the behavior of your crate.

So I think it would be better to control this in a different way, e.g. through a new optional type parameter on RawSpinlock or a separate RawSpinlockBackoff type. What do you think?

@mkroening
Copy link
Contributor Author

mkroening commented Oct 16, 2023

Yeah, it makes sense to control this per mutex and not globally.

I gave optional type parameters a go.

Unfortunately, Rust cannot infer the default type in all cases, requiring Spinlock::<_>::new instead of Spinlock::new in some circumstances.
This is backwards incompatible and would require bumping the minor version on the next release.
Would you be okay with that?
If we did so, we could also clean up other things like removing const_spinlock or the nightly feature, which are not needed nowadays.

@phil-opp
Copy link
Member

phil-opp commented Oct 16, 2023

I'm fine with bumping the minor version, but I don't like the Spinlock::<_>::new requirement.

How about the following:

  • We hardcode the Spinlock type alias to Spin, i.e.:
    pub type Spinlock<T> = lock_api::Mutex<RawSpinlock<Spin>, T>;
  • We add a new BackoffSpinlock type alias that maps to lock_api::Mutex<RawSpinlock<Backoff>, T>.

?

@mkroening
Copy link
Contributor Author

Done. 👍

Similarly, one has to write let lock: RawSpinlock = RawSpinlock::INIT; instead of let lock = RawSpinlock::INIT;, but that might be fine, since using the raw mutex is rare?
If not, we could rename the struct and add type aliases here as well.

Another thing to decide: do we want to export all of these different type aliases at the root of the crate or do we want to put them in modules?

Type Definitions

We could move things to spinning_top::{spin,backoff_spin,rw_spin,rw_backoff_spin}::{Lock,LockGuard, MappedLockGuard} or similar, if you think the crate root was too cluttered. Using locks as spin::Lock or backoff_spin::Lock does not seem too bad, maybe?

@phil-opp
Copy link
Member

Thanks!

Similarly, one has to write let lock: RawSpinlock = RawSpinlock::INIT; instead of let lock = RawSpinlock::INIT;, but that might be fine, since using the raw mutex is rare?

Yeah, I think this is fine.

Another thing to decide: do we want to export all of these different type aliases at the root of the crate or do we want to put them in modules?

How about moving the guard types into a guard submodule? They are probably not imported most of the time, so making the import path a bit longer shouldn't be an issue. Then the root module would only contain the Spinlock and BackoffSpinlock type aliases.

@mkroening
Copy link
Contributor Author

How about moving the guard types into a guard submodule?

Sounds good. Done. 👍

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks!

@phil-opp phil-opp merged commit 3f995c6 into rust-osdev:master Oct 17, 2023
4 checks passed
@mkroening mkroening deleted the backoff branch October 17, 2023 11:47
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.

Exponential backoff
2 participants