Originally cros_async was not permitted to depend on base, and as such
was precluded from using RawDescriptor/AsRawDescriptor. Now that we can
use base, we are switching over to those types/traits as they allow for
much cleaner cross platform code.
BUG=b:228904576
TEST=builds (also tested applying downstream).
Change-Id: Id5da60b127656568069795707a48beca1a015151
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3584139
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
Major changes:
* Adds Windows implementations
* Refactors cros_async to use the styleguide for cross platform code.
* Adds platform specific Descriptor impls to Timer & Event (short term;
this will go away in the next CL in the series).
Minor adjustments:
* The doctests for the Executor were passing the wrong type of seek
parameter to write_from_vec. It was disabling seeking (None) when it
should have been seeking to zero (Some(0)).
BUG=b:213147081
TEST=tested by Windows & Linux bots.
Change-Id: Id7e025ceb9f1be4a165de1e9ba824cf60dd076ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3579735
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
This removes sys_util(_core), which moved into base::unix/common, as
well as common/(cros_async,io_uring), which moved into the root
directory.
The only reason the code was still around is that they were still
used in the ChromeOS codebase.
ChromeOS has pinned the version of crosvm it uses for these libraries
so we can go ahead and remove the code.
A few remaining references to sys_util have been updated to base.
BUG=b:227226222,b:229016539
TEST=presubmit
Change-Id: I7a711044de7e067b217287f2bac822d6ac7d3964
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3593852
Reviewed-by: Allen Webb <allenwebb@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
This reverts commit aac461e25e.
Reason for revert: Breaks audio_streams portage build
Original change's description:
> Remove temporarily duplicated code from codebase
>
> This removes sys_util(_core), which moved into base::unix/common, as
> well as common/(cros_async,io_uring), which moved into the root
> directory.
>
> The only reason the code was still around is that they were still
> used in the ChromeOS codebase.
> ChromeOS has pinned the version of crosvm it uses for these libraries
> so we can go ahead and remove the code.
>
> A few remaining references to sys_util have been updated to base.
>
> BUG=b:227226222,b:229016539
> TEST=presubmit
>
> Change-Id: I35a3d1f0ea28182b77abf9b423fcab4cad525981
> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3580118
> Reviewed-by: Allen Webb <allenwebb@google.com>
> Tested-by: kokoro <noreply+kokoro@google.com>
> Commit-Queue: Dennis Kempin <denniskempin@google.com>
Bug: b:227226222,b:229016539
Change-Id: I907279ab47718355cd57915830580929dc157f84
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3593846
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Maciek Swiech <drmasquatch@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
fallocate modes were previously unix specific constants, which won't work after
we add support for Winodws. Here, we've put a generic abstraction in place so
that each platform can implement the details as appropriate.
Thanks to acourbot@ for suggesting the splits in this series.
BUG=b:213147081
TEST=see final CL in series.
Change-Id: I822ad6c4a26eea716482029e8a6c0489aa72c595
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3583613
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
TimerFd is the sys_util era Unix-specific timer, which doesn't support Windows.
This CL moves us to base::Timer, which is the cross platform equivalent.
Thanks to acourbot@ for suggesting the splits in this series.
BUG=b:213147081
TEST=see final CL in series.
Change-Id: I41c8ad88da77c48397ed466ff11aecd703c470a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3583612
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
EventFd is the original sys_util era Unix-specific Event. We need to use
base::Event, which is the cross platform event that works on Windows.
Thanks to acourbot@ for suggesting the splits in this series.
BUG=b:213147081
TEST=see final CL in series.
Change-Id: I174313cb544c5fc3768810365a42e6eaac1ca91a
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3583611
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
This removes sys_util(_core), which moved into base::unix/common, as
well as common/(cros_async,io_uring), which moved into the root
directory.
The only reason the code was still around is that they were still
used in the ChromeOS codebase.
ChromeOS has pinned the version of crosvm it uses for these libraries
so we can go ahead and remove the code.
A few remaining references to sys_util have been updated to base.
BUG=b:227226222,b:229016539
TEST=presubmit
Change-Id: I35a3d1f0ea28182b77abf9b423fcab4cad525981
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3580118
Reviewed-by: Allen Webb <allenwebb@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Dennis Kempin <denniskempin@google.com>
There are two copies of io_uring and cros_async that are slightly
different. Bump the version number of new one.
BUG=None
TEST=cargo build
Change-Id: I7f82d3c5f01633bef7d0ce14ab777bbb50d4fbd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3565625
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Commit-Queue: Junichi Uekawa <uekawa@chromium.org>
This is currently only used in tests, and removing it makes the
cross-platform Timer API simpler to implement. The Windows version of
Timer did not implement this API, so it was already unusable in portable
code.
BUG=b:215618361
TEST=tools/presubmit
TEST=cargo test -p base timer
Change-Id: I57ab15e8b652d0df3664d95bc7759b9c84fe5e10
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3570178
Reviewed-by: Noah Gold <nkgold@google.com>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Upstreams support for Tubes on Windows, splitting Tube into platform
specific files. This contains several critical enhancements:
* POSIX Tubes support multi producer multi consumer configurations, but
Windows has remained strictly SPSC for each direction. Windows cannot
support MPMC, and that configuration is not really something we want
either. To address that, this CL introduces directional Tubes. A
SendTube is clonable, and a RecvTube is not, which gives us MPSC.
* This CL also fixes multiple interface conflicts that have developed
between Linux & Windows:
+ send wasn't async on the Linux AsyncTube.
+ send data wasn't passed as owned on the Linux AsyncTube.
+ Adds the 'static constraint for AsyncTube::send on POSIX. This is an
requirement on Windows.
+ Event::read_timeout doesn't need to take &mut self, and it wasn't
downstream. This CL switches to &self.
* Adds the missing notifier.rs file in base.
Note that this CL does not attempt to remove balloon's usage of
Tube::try_clone. That's a somewhat involved issue that should be tackled in
its own CL.
Test: tested downstream on Windows & Linux bots, upstream on Linux bots.
Bug: b:221484449
Change-Id: I288dbc1d1e42f8ce08258cdaaf85100ca93721ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3536897
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
base was previously providing some async types which now would
cause a circular dependency. Those have been moved into cros_async.
BUG=b:22320646
TEST=presubmit
Change-Id: I1f526ccfc5882f3a64404f714b13ac92ebfddcd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3533614
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
This change contains the results of running
./tools/contib/cargo_refactor.py
This will break the next uprev, and needs to be synchronizized
with the corresponding ebuild changes in https://crrev.com/c/3248925
BUG=b:195126527
TEST=./tools/run_tests
Change-Id: Ied15a1841887bb8f59fba65b912b81acf69beb73
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3248129
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Use the crates.io implementation of tempfile instead of our own version.
Our reimplementation is kept in the tree for now in case of dependencies
outside of the crosvm tree; it can be removed later once those are fully
switched over to the crates.io implementation.
BUG=b:199204746
TEST=emerge-hatch crosvm
Change-Id: I07d3404239302ab9a17f4ddc82a9479b256e4eb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3209839
Reviewed-by: Dennis Kempin <denniskempin@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
The `num_idle` field of the shared state between BlockingPool worker
threads can underflow in the following case:
* state.num_idle == 2.
* We spawn 2 new tasks into the BlockingPool.
* Both idle worker threads are woken up. `state.num_idle` goes to 0.
* The first worker thread wakes up and pulls a task from the queue.
That task finishes very quickly so the worker thread pulls the second
task from the queue before the second worker thread is scheduled.
* The second worker thread is scheduled. It sees that
`s.tasks.is_empty() == true` so it goes back to waiting on the
Condvar.
* The second worker thread's wait times out and it tries to decrement
`state.num_idle` leading to underflow.
Fix this by adding a `num_notified` field to the shared worker state.
This field acts like a counter for the number of idle worker threads
that have been woken up.
When an idle thread is waiting on a Condvar, rather than checking if the
task queue is empty, it will instead check if num_notified > 0. When an
idle worker thread observes that num_notified > 0 it decrements it by 1
and then goes back to processing tasks from the queue. num_idle is only
decremented when num_notified is 0.
Change the num_idle decrement to a checked_sub so that we can catch it
even when -Coverflow_checks=off. Also add a test for this case. This
test consistently panics without the num_notified changes.
BUG=none
TEST=unit tests
Change-Id: Ia1b348605e0d02415635cdd023db1c10201ab661
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3139159
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Richard Zhang <rizhang@google.com>
Reviewed-by: Noah Gold <nkgold@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Add an optional deadline to BlockingPool::shutdown. Any worker threads
that have not yet exited once the deadline expires will be detached.
This ensures that we don't end up blocking indefinitely while waiting
for worker threads to exit.
BUG=none
TEST=unit tests
Change-Id: I6d7e73e1c95a934a4fd80825a9d44187532408b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3058842
Auto-Submit: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Noah Gold <nkgold@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
p{read,write} cannot be used on sockets even if the file offset is 0.
Make the file offset optional and fall back to regular read/write when
it is not set.
BUG=none
TEST=cargo test
Change-Id: Iff938aabe613b6164782714cfac94743d64f551a
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3033233
Auto-Submit: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Noah Gold <nkgold@google.com>
Reviewed-by: Woody Chow <woodychow@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Add a spawn_blocking() method to the Executor so that users don't need
to manually create a BlockingPool every time they need to do some
blocking work.
BUG=b:179755651
TEST=cargo test
Change-Id: I70d111d98a4c51af4bc8ed8181a4b102bf3c2ffa
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2987586
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Auto-Submit: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Noah Gold <nkgold@google.com>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
This fixes:
* version mismatches in Cargo.lock
* style issues
* implementations of Into that should be From
* deprecated protobuf APIs
It also adds RUST_BACKTRACE=1 to the kokoro tests.
BUG=None
TEST=./bin/preupload-clippy
Change-Id: I8e9157c903f2080a5fdcc4d3e4ed72fbad41c64f
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3024427
Auto-Submit: Allen Webb <allenwebb@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Allen Webb <allenwebb@google.com>
Kernels before 5.10 had known bugs in the io_uring implementation.
Don't use io_uring when we detect this. Also skip all the io_uring
tests in this case.
BUG=none
TEST=cargo test
Change-Id: I5fd6203ad25a6fb85ff28f1a6ddb0181f836ad89
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3006309
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Woody Chow <woodychow@google.com>
BlockingPool provides a dedicated thread pool for running blocking
operations. This is useful when an async task wants to do some
CPU-intensive computation, run some IO operation that cannot be
performed asynchronously, or to call a blocking API in a dependency that
doesn't have an asynchronous variant.
BUG=b:179755651
TEST=cargo test
Change-Id: I389fc504f380d66325739d2d6b7afe58e024194d
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2987585
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Sometimes users of cros_async need to return an io::Error. Provide a
conversion function to convert the various async errors into an
io::Error. This allows callers to access the underlying error without
forcing us to expose these internal implementation details.
BUG=none
TEST=unit tests
Change-Id: Ie0ab00cb80ea58f628a38c173e28babf30b8d5b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3006308
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
This doesn't really have anything to do with synchronization so move it
into its own module and export it as a top-level function.
BUG=b:179755651
TEST=cargo test
Change-Id: Icb733c36ee1d4cebcb445e47289c92b9b77a278b
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2987583
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Noah Gold <nkgold@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
This makes it easier to implement IntoAsync for foreign types.
BUG=none
TEST=Use it in a later change
Change-Id: I070fe3b63ac7458e21fa38b3a1b1bdb318c44d5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2891120
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Noah Gold <nkgold@google.com>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Replace the deprecated std::sync::spin_loop_hint with
std::hint::spin_loop.
Fixes this new warning from Rust 1.51:
warning: use of deprecated function `std::sync::atomic::spin_loop_hint`:
use hint::spin_loop instead
BUG=None
TEST=bin/clippy
Change-Id: Ic975c77ea19bcf256f73c0b3b89f85866abfe0bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2864362
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Tested-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
The syscall_defines crate is redundant with an up to date libc. This
change removes any dependency on syscall_defines. A new libc is required
to bring in some new syscall numbers like the ones for io_uring.
TEST=./test_all
BUG=None
Cq-Depend: chromium:2832000
Change-Id: I6df7fb992bacb5efd54cefca08836d52f4bfcd8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2832001
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Allen Webb <allenwebb@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Commit-Queue: Zach Reizner <zachr@chromium.org>
This allows the compiler to generate more efficient assembly for arm.
It also maintains consistency for the whole file since we were already
using compare_exchange_weak in some places and not others.
BUG=none
TEST=`FEATURES=test emerge-kukui-arc-r cros_async`. Also copy the unit
test binary onto the device and run in a loop to see that there are
no failures.
Change-Id: Ia8c942c419ac2989a5653875d78c48003fb757d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2805754
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Checking the state with a relaxed load before doing a
compare_exchange_weak can reduce unnecessary coherence traffic on the
CPU and improve performance.
BUG=none
TEST=unit tests
Change-Id: Icabd9863ceb5ba674dbec601afee8f7962f69413
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2805753
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Drop order in rust is weird. Temporaries created in an if let
expression are dropped _after_ the else branch. In this case that meant
we were sleeping while holding the spin lock, which could potentially
lead to the test hanging ~forever if the thread trying to update the
value repeatedly failed to acquire the lock.
Move the sleep out of the else branch so that the lock is dropped after
checking for the waiter but before the thread goes to sleep.
BUG=none
TEST=Run unit tests and see that they no longer get randomly stuck for
several seconds.
Change-Id: I08aa80169071959593bee157acda39411472cf11
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2804870
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Updating an atomic value invalidates the entire cache line to which it
belongs, which can make the next access to that cache line slower on
other CPU cores. This can lead to "destructive interference" or "false
sharing", where atomic operations on two or more unrelated values on the
same cache line cause hardware interference with each other, reducing
overall performance.
Deal with this by aligning atomic primitives to the cache line width so
that two primitives are not placed on the same cache line. This also
has the benefit of causing *constructive* interference between the
atomic value and the data it protects. Since the user of the atomic
primitive likely wants to access the protected data after acquiring
access, having them both on the same cache line makes the subsequent
access to the data faster.
A common pattern for synchronization primitives is to put them inside an
Arc. However, if the primitive did not specify cache line alignment then
both the atomic reference count and the atomic state could end up on the
same cache line. In this case, changing the reference count of the
primitive would cause destructive interference with its operation. With
the proper alignment, both the atomic state and the reference count end
up on different cache lines so there would be no interference between
them.
Since we can't query the cache line width of the target machine at build
time, we pessimistically use an alignment of 128 bytes based on the
following observations:
* On x86, the cache line is usually 64 bytes. However, on Intel cpus the
spatial prefetcher "strives to complete every cache line fetched to
the L2 cache with the pair line that completes it to a 128-byte
aligned chunk" (section 2.3.5.4 of [1]). So to avoid destructive
interference we need to align on every pair of cache lines.
* On ARM, both cortex A-15 (armv7 [2]) and cortex A-77 (aarch64 [3])
have 64-byte data cache lines. However, Qualcomm Snapdragon CPUs can
have 128-byte data cache lines [4]. Since Chrome OS code compiled for
armv7 can still run on aarch64 cpus with 128-byte cache lines assume
we need 128-byte alignment there as well.
[1]: https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf
[2]: https://developer.arm.com/documentation/ddi0438/d/Level-2-Memory-System/About-the-L2-memory-system
[3]: https://developer.arm.com/documentation/101111/0101/functional-description/level-2-memory-system/about-the-l2-memory-system
[4]: https://www.7-cpu.com/cpu/Snapdragon.html
BUG=none
TEST=unit tests
Change-Id: Iaf6a29ad0d35411c70fd0e833cc6a49eda029bbc
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2804869
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Now that we're not transferring waiters between the Condvar and the
Mutex we can simplify how the cancel function works. Also, now that it
never changes we can drop the Spinlock around it and treat it like a
const field.
BUG=none
TEST=Run unit tests in a loop on both x86 and arm and observe no
failures or hangs
Change-Id: I0851c4eeb0b9462098ed1ac186a25f1c5791511a
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2804868
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
A performance optimization should never change the observable behavior
and yet that's what this one did. Canceling a `cv.wait()` call after
the waiter was already transferred to the Mutex's wait list should still
result in us waking up the next waiter in the Condvar's wait list.
Instead, the `cancel_after_transfer` test was checking for the opposite
behavior.
Additionally, the transfer was racy with concurrent cancellation.
Consider the following sequence of events:
Thread A Thread B
-------- --------
drop WaitFuture cv.notify_all()
waiter.cancel.lock() raw_mutex.transfer_waiters()
c = cancel.c
data = cancel.data
waiter.cancel.unlock()
waiter.cancel.lock()
cancel.c = mu_cancel_waiter
cancel.data = mutex_ptr
waiter.cancel.unlock()
waiter.is_waiting_for = Mutex
mu.unlock_slow()
get_wake_list()
waiter.is_waiting_for = None
waiter.wake()
c(data, waiter, false)
cancel_waiter(cv, waiter, false)
waiter.is_waiting_for == None
get_wake_list
There are 2 issues in the above sequence:
1. Thread A has stale information about the state of the waiter. Since
the waiter was woken, it needs to set `wake_next` in the cancel
function to true but instead incorrectly sets it to false. By
itself, this isn't that big of an issue because the cancel function
also checks if the waiter was already removed from the wait
list (i.e., it was woken up) but that check is problematic because of
the next issue.
2. The Condvar's cancel function can detect when a waiter has been moved
to the Mutex's wait list (waiter.is_waiting_for == Mutex) and can
request to retry the cancellation. However, when
waiter.is_waiting_for == None (which means it was removed from the
wait list), it doesn't know whether the waiter was woken up from the
Mutex's wait list or the Condvar's wait list. It incorrectly assumes
that the waiter was in the Condvar's wait list and does not retry the
cancel. As a result, the Mutex's cancel function is never called,
which means any waiters still in the Mutex's wait list will never get
woken up.
I haven't been able to come up with a way to fix these issues without
making everything way more complicated so for now let's just drop the
transfer optimization.
The initial motivation for this optimization was to avoid having to make
a FUTEX_WAKE syscall for every thread that needs to be woken up and to
avoid a thundering herd problem where the newly woken up threads all
cause a bunch of contention on the mutex. However, waking up futures
tends to be cheaper than waking up a whole thread. If no executor
threads are blocked then it doesn't even involve making a syscall as the
executor will simply add the future to its ready list. Additionally,
it's unlikely that multi-threaded executors will have more threads than
the # of cpus on the system so that should also reduce the amount of
contention on the mutex.
If this code starts showing up as a hotspot in perf traces then we
should consider figuring out a way to re-enable this optimization.
BUG=chromium:1157860
TEST=unit tests. Also run the tests in a loop for an hour on a kukui
and see that it didn't hang
Cq-Depend: chromium:2793844
Change-Id: Iee3861a40c8d9a45d3a01863d804efc82d4467ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2804867
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Add magic comments so that we can have separate ebuilds for the io_uring
and cros_async crates.
BUG=none
TEST=`FEATURES=test emerge-$BOARD cros_async`
Change-Id: I8e4befc90d44b4b021864f4358c8f9b3ec5a87d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2794162
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
These didn't get moved in the original move of cros_async::sync. This CL
adds them.
BUG=None
TEST=builds
Change-Id: I08204a9aedd960e0e8e7befc930076df065b74ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2776214
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Commit-Queue: Noah Gold <nkgold@google.com>
Some judgement calls were made about unnecessary wrapping. Usually they
would get resolved by removing the wrapping or returning a convenient
error, but the ones that returned results for consistency with other
functions were added to the allow list.
The error handling in the usb code had a lot of unit error types which
is now a clippy lint. This was resolved by either removing the result
entirely or returning a convenient error.
The field_reassign_with_default lint is faulty and was added to the list
of supressions. This affected virtio-wayland code.
BUG=b:179277332
TEST=cargo clippy with rustc 1.50+
Change-Id: Ie812cdeaf7c42f4f2b47b1dc87f05a7c87a60f8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2757510
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Auto-Submit: Zach Reizner <zachr@chromium.org>
Commit-Queue: Dennis Kempin <denniskempin@google.com>
PollSource keeps a strong reference to the FdExecutor, which can lead to
a memory leak via a circular reference if the caller spawns a future
that owns a PollSource and then detaches it. Avoid this by using weak
references instead.
With this change, we now only use weak references internally. The only
way to increase the strong reference count is by cloning the FdExecutor.
BUG=none
TEST=unit tests
Change-Id: Ic58ff475a31c6fca831c3ced73b26b87ceeda028
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2760378
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Previously each executor had to call RunnableQueue::set_waker at the
beginning of every loop before it started executing futures, which was a
bit tedious. This call was needed so that the RunnableQueue would wake
up the executor if a new future was scheduled.
Instead move responsibility for waking the executor when a future is
scheduled back to the executor. This lets us get rid of the set_waker
method and is arguably a better separation of responsibilities.
BUG=none
TEST=unit tests
Change-Id: Ica46437f78c822d87096eaa215954d118c6578f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2760377
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Noah Gold <nkgold@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
To allow for porting to non POSIX platforms, we've brought the
libchromeos::sync module into cros_async (which was the only
consumer).
BUG=b:180978556
TEST=builds
Change-Id: I97256b1dc37124cebc693c035e63d2c5b29e94b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2757280
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
This CL replaces BorrowedIoVec with VolatileSlice, since VolatileSlice
is a superset of the BorrowedIoVec interface. Also uring_mem -> mem
since that interface will not be exclusively used by uring.
BUG=none
TEST=builds
Change-Id: I33e23483e7afc263c76d71f88736cf38fd5e520e
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2724863
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
Commit "1fcc8a8f io_uring: Make UringContext Sync" uncovered an issue
where wakeups were lost if a uring operation was added from a different
thread while the executor thread was blocked inside an io_uring_enter
call. To fix this, PendingOperation calls UringContext::submit
whenever the pending IO is not yet completed and has not been submitted.
Unfortunately, since we tend to write code like
`r.read_to_vec(..).await`, this meant that we would call
UringContext::submit every time after adding a new operation to the
submit queue, kind of defeating the purpose of batching multiple IO ops
in a single syscall. Instead only call UringContext::submit when the
current thread is not the same as the executor thread. The executor
will submit all pending operations anyway the next time it calls
UringContext::wait so there's no need to do it from PendingOperation
when it is polled on the executor thread.
BUG=none
TEST=unit tests
Change-Id: Ia95f3844790d3392e074e3ab55a9c6ef59f29db2
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2684063
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Closing the uring fd should release the resources borrowed by the kernel
so there's no reason to call io_uring_enter to drive any pending IO to
completion. Doing so may end up blocking the thread indefinitely.
Instead just make sure that the URingContext is dropped before anything
else. Rust's drop order guarantees that struct fields are dropped in
the order of declaration so this just means putting it first.
Also, since the RawExecutor is wrapped in an Arc it may end up being
dropped from a different thread than the one that called run() or
run_until(). We know there are no other references so just clear the
cached thread_id so that we don't panic in the drop impl. We don't
currently have any users that want to run the executor concurrently on
multiple threads so we'll just punt that problem until we actually need
to deal with it.
BUG=none
TEST=unit tests
Change-Id: Icf6a23fc433128dd00adbd56a715dbae24cd8ea2
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2643845
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Noah Gold <nkgold@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
