RwLock::read for UFT hits, kstats use AtomicU64
#636
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR replaces the per-`Port` `KMutex` with a `KRwLock`. This allows ioctls to read port state without preventing packets from being served. This also prevents successful UFT hits from blocking one another on the port for any length of time. A requirement is that we move to atomic accesses on kstat values, which allows us to update kstats from outside a write-lock on the port. The odd thing is that there doesn't seem to be a performance improvement (in Mbps). Contention according to lockstat is solved, and apparently the time spent in `xde_rx`/`xde_mc_tx` is reduced -- we're no longer: * S2C -- the 3rd and 10th most spin-happy locks, * C2S -- the 2rd and 3rd most spin-happy locks. Nor are we egregiously blocking on the port lock.
At the very least, this compiles. We needed to regenerate `ip.rs`, on account of the many `extern` -> `unsafe extern` block changes.
FelixMcFelix
added a commit
that referenced
this pull request
Feb 21, 2025
Pulled out of #636, we were creating some `Spin` locks which would end up falling back to adaptive behaviour as part of LFT creation. Moreover, they would hit a debug assert and panic on test bits. (!!!) Given that we have no means of specifying priority in `mutex_init`, we make sure that all created `KMutex`es are corret by removing the illusion of free choice.
FelixMcFelix
added a commit
that referenced
this pull request
Feb 21, 2025
Pulled out of #636, we were creating some `Spin` locks which would end up falling back to adaptive behaviour as part of LFT creation. Moreover, they would hit a debug assert and panic on test bits. (!!!) Given that we have no means of specifying priority in `mutex_init`, we make sure that all created `KMutex`es are corret by removing the illusion of free choice.
FelixMcFelix
added a commit
that referenced
this pull request
Feb 21, 2025
Pulled out of #636. We were creating some `Spin` locks which, misconfigured, would end up falling back to adaptive behaviour as part of LFT creation. Moreover, they would hit a debug assert and panic on test bits. (!!!) Given that we have no means of specifying priority in `mutex_init`, we now make sure that all created `KMutex`es are correct by removing the illusion of free choice.
CI on recent PRs is breaking, due to rustup 1.28.0+ no longer autoinstalling the correct rust toolchain version. This hurts us immediately since we have *two* toolchains (pinned nightly and stable), and deliberately specified the nightly for some tooling. This PR changes this over to use buildomat's auto-installation for the stable variant, and the new toolchain show -> install pattern for nightly. This also lets us place `$NIGHTLY` into most of our `cargo fmt` invocations, which should reduce the busywork in future compiler bumps for XDE.
Makes some aspects clearer -- no more fixing up wrongly incremented stats, and makes it more obvious that a `reprocess` is a downgrade to the slowpath. Catches one locking invariant that was being upset by an accidental epoch re-check.
FelixMcFelix
commented
Mar 11, 2025
Forgot an `UnsafeCell`, everything's good now!
Includes some changes discussed out-of-band on Just Including The Atomic Types in the union definition, getting us away from the explicit UnsafeCell. A couple of nice things fell out of this -- we have setup a pattern for if/when we need the other KStat variants, but we can now deduplicate the code between test/kernel since we always have an `AtomicU64` now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces the per-
PortKMutexwith aKRwLock. This allowsioctls to read port state without preventing packets from being served.
This also prevents successful UFT hits from blocking one another on the
port for any length of time.
A requirement is that we move to atomic accesses on kstat values, which
allows us to update kstats from outside a write-lock on the port.
The odd thing is that there doesn't seem to be a performanceContention according to lockstat is solved, andimprovement (in Mbps).
apparently the time spent in
xde_rx/xde_mc_txis reduced -- we're nolonger:
After some refactoring, this has moved from 2.8->2.9 Gbps (pre-LSO) on
glasgow.Nor are we egregiously blocking on the port lock.
Closes #435.