The libminijail version in AOSP complains when there are multiple entries for
the same system call, which was the case for virtio-fs's policy.
BUG=b/185811304
Change-Id: I389c07c86e7d79f16e4f47a893abad598033352a
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2837307
Commit-Queue: Jorge Moreira Broche <jemoreira@google.com>
Tested-by: Jorge Moreira Broche <jemoreira@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
The recv thread was started immediately after the client object was created,
which caused minijail to abort refusing to fork a multithreaded process.
BUG=b/185811304
Change-Id: I5608e3b89eb4dfd944542d163e60b78937d37ba1
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2837306
Tested-by: Jorge Moreira Broche <jemoreira@google.com>
Commit-Queue: Jorge Moreira Broche <jemoreira@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
For vfio-platform we will have many platform IRQs per vfio-platform
device, so we need to pass the irq index to these functions in this
commit, rather than inferring the IRQ index from the IRQ type (intx
vs msi vs msix).
In other words, this commit eliminates some assumptions in the common
vfio code that we are working with vfio-pci devices when doing vfio
passthrough.
BUG=b:185504618
TEST=cros_workon_make
Change-Id: Iaa02c387fb8a679217d4cc9dabecf7fc61f9c9fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2829293
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Tomasz Jeznach <tjeznach@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Micah Morton <mortonm@chromium.org>
set_memory_policy() method accepts MemoryPolicy bitmask - a
set of hints what kind of memory advices GuestMemory can
provide to the kernel. For the time being we support only
one advice - MADV_HUGEPAGE.
BUG=b:174206107
TEST=arc.Boot.vm on hatch-arc-r
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Change-Id: Ibc3e4a2bb2ac7059de80dfba45e1cee3af4c3bcc
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2812546
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Add use_hugepages() MemoryMapping method, which will advice the
kernel to use Huge Pages for memory mappings.
BUG=b:174206107
TEST=arc.Boot.vm on hatch-arc-r
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Change-Id: I8cf61d71bb7e135594c1a4bc3c68674a1576b766
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2812545
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
hugepages (--hugepages) argument will be passed down to arch layer so that
it can configure GuestMemory to support Transparent Huge Pages.
BUG=b:174206107
TEST=arc.Boot.vm on hatch-arc-r
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Change-Id: I1590238f7be3f12c9aa22b4c9061a56bfc381332
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2812544
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
In this CL, the decoder gets the buffer modifier via resource_bridge,
and then passes the value to the backend.
BUG=b:79682290
TEST=run media CTS and check the modifier is passed to GAVDA
Cq-Depend: chromium:2820734
Change-Id: I27b845251bf4a12efb74193945a6cb952c8f14af
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2821108
Tested-by: Chih-Yu Huang <akahuang@chromium.org>
Commit-Queue: Chih-Yu Huang <akahuang@chromium.org>
Reviewed-by: David Stevens <stevensd@chromium.org>
Reviewed-by: Alex Lau <alexlau@chromium.org>
Reviewed-by: David Staessens <dstaessens@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
New rustc(1.53) introduces a warning:
warning: derive helper attribute is used before it is introduced
--> /home/dgreid/src/chromiumos/src/platform/crosvm/sys_util/src/descriptor_reflection.rs:391:3
|
391 | #[serde(transparent)]
| ^^^^^
392 | #[derive(Serialize, Deserialize)]
| --------- the attribute is introduced here
Fix that by reversing the order.
Change-Id: Ifd77322bb9552a3ba6c1c86019e9cb366328b173
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2818897
Reviewed-by: Zach Reizner <zachr@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Dylan Reid <dgreid@chromium.org>
Rust's libc considers android to be different than linux and provides
certain functions exclusively for target_os = "linux".
BUG=b:185155959
Change-Id: I664821fd678f0c911deb9312fe5fcfc9faf00053
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2822209
Tested-by: Jorge Moreira Broche <jemoreira@google.com>
Commit-Queue: Jorge Moreira Broche <jemoreira@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Because the default value for mode is GpuMode::ModeVirglRenderer, make
the default value for use_vulkan false as well.
Set use_vulkan to true when mode is set to GpuMode::ModeGfxstream for
backward compatibility.
BUG=b:178104043
TEST=cargo build
Change-Id: Idf1417f04d23999cf5a03b0bf640973b69de93e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2823012
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: David Riley <davidriley@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Tested-by: Chia-I Wu <olv@google.com>
Commit-Queue: Chia-I Wu <olv@google.com>
This adds a scoped signal handler struct that performs some basic checks
to avoid accidental replacement of an existing handler and cleanup when
the handler goes out of scope.
Also, adds scoped_signal_handler::wait_for_interrupt() which blocks
until SIGINT is received (e.g. via Ctrl-C).
BUG=None
TEST=cargo test --workspace -- --test-threads=1
Change-Id: I5e649eac3d3ee0b842b200fc553acac44b2dfe94
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2807306
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: Allen Webb <allenwebb@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Allen Webb <allenwebb@google.com>
This replaces an Into<A> for B implementation with an From<B> for A
implementation.
BUG=None
TEST=cargo test && cargo clippy
Change-Id: I9891a2fe62bd63857006c6dca88aceb8d72ed6da
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2826414
Tested-by: Allen Webb <allenwebb@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Auto-Submit: Allen Webb <allenwebb@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Reviewed-by: Stephen Barber <smbarber@chromium.org>
Commit-Queue: Allen Webb <allenwebb@google.com>
Having a trait for interrupts used by queue and the devices allows for a
slightly different implementation to handle interrupting the guest when
using vhost-user.
Change devices to handle the resample event being optional as it is
handled on the VMM side with vhost_user.
Change-Id: I511d3db66a7986e7a2a8bce5f48285171dee3388
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2795284
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Dylan Reid <dgreid@chromium.org>
In multi-process mode, we currently rely on dma-buf mmap() to
map GPU buffers into the guest. We usually have to fix the
Mesa driver, and maybe even the kernel to get to work. That's
"kind of" fine for ChromeOS, which owns the entire stack.
For their Virtual Graphics Interface (VGI) initiative, Android
upstream has requested multi-process mode to work in a
cross-platform, generic way. Using Vulkan is the only option
that meets the rigorous, uncompromising, strict, meticulous and
bone-crushing requirements of Android upstream.
This has possible two benefits:
1) We can enable multi-process mode on Nvidia or other
closed-source drivers, which is nice for Cuttlefish.
2) On open-source drivers, dma-buf memory is pinned to the
GTT (amdgpu), even when ideally it can be moved into faster
vram regions. This atleast gives the implementation the
chance to do the smarter and faster option.
We shouldn't run into any SELinux issues since the main crosvm
process is not sandboxed.
Incidentals:
* Changes vulkano_gralloc to consider integrated GPUs and dGPUs.
Metadata query is preferred done on the integrated GPU.
* Update vulkano_gralloc to match top of tree vulkano.
BUG=b:173630595
TEST=used Vulkano allocator and mapped memory into the guest
Change-Id: I78b069c7478d11b3201397894dcccd13bdc61f2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2792042
Tested-by: Gurchetan Singh <gurchetansingh@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Fds created via dup don't share file descriptor flags with the original
fd, which means that they don't have the FD_CLOEXEC flag set. Use
fcntl(F_DUPFD_CLOEXEC) so that this flag gets set for the duplicated fds
as well.
BUG=none
TEST=unit tests
Change-Id: Ib471cf40acac1eacf72969ba45247f50b349ed58
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2809687
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Help message does not include some of the useful subcommands.
BUG=None
TEST=crosvm
Change-Id: Ibdf5db85fc9b912f9b2ba28e42ce5f65db932215
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2817696
Tested-by: Junichi Uekawa <uekawa@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Junichi Uekawa <uekawa@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
This CL addresses some minor issues with the existing interface:
1. from_descriptor is too generic for some platforms that require
special handling for file/File backed mappings.
2. Nearly all call sites pass either File or SharedMemory. Now
we just have from_ methods for those types to preserve type
information.
3. Other platforms require additional fields in MemoryMapping, so a
tuple struct no longer makes sense.
4. The mmap syscall error message was misleading as we use it for more
than just the mmap syscall.
BUG=None
TEST=builds
Change-Id: I74c41bad52bb81880a11231cd18f47e233548a24
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2815614
Reviewed-by: Udam Saini <udam@google.com>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
WaitContext and Reader/Writer are in the critical path for every device.
Use small vector optimization to avoid making unnecessary small heap
allocations.
The smallvec crate is maintained by the servo authors and only has an
optional dependency on serde.
BUG=none
TEST=pre-cq
Cq-Depend: chromium:2687076
Change-Id: Ic0c57ac949e263b70b76495e3c9121dd8c2e1177
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2684062
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
VFIO is updated to use try_clone() to duplicate a File recently.
The try_clone() implementation will use fcntl with the argument
F_DUPFD_CLOEXEC to duplicate the File, so need to add one more
rule in vfio_device.policy to allow it otherwise VFIO will be
failed when sandbox is enabled.
BUG=None
TEST=boot VM with VFIO passthrough + sandbox enabled
Change-Id: I55cce937f1c12a32537aaff8d3ddafa135a674d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2816822
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
This requires exporting the memory layout from the arch crates, but it
does simplify the bloated build_vm interface a bit. It also will allow
for more fine-grained control the backing memory of GuestMemory.
TEST=test_all
BUG=b:183988204
Change-Id: Ie76755198d2fdc2a41bd538650939d6550686b88
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2809434
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Zach Reizner <zachr@chromium.org>
With multiple regions potentially backing a single `GuestMemory`
instance, `AsRef` doesn't make sense any more. Switch the one user to
`shm_region` which returns the region for a given address.
The `AsRef` implementation was accidentally left in the change to allow
multiple regions in guest memory.
Change-Id: I1246de004315a44f1f9d58995d837f3fbecb5d6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2808745
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Chih-Yang Hsia <paulhsia@chromium.org>
validate_raw_fd assumes that the fd passed in was not created by crosvm
and returns EBADF if it sees that the fd has the FD_CLOEXEC flag set.
We can't use it for fds created by the fs device since those do have
that flag set.
We're already taking a `&dyn AsRawFd` as the parameter so just assume
it's valid and clone it directly since there's no safe way to create an
invalid one.
BUG=none
TEST=vm.Fio.virtiofs_dax* tests are no longer failing
Change-Id: I10d9752e0960143fb58a63d2b76f64d34ec464d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2809686
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
This patch adds support for creating udmabufs via a guest provided
sg-list. Ideally, we'd create the udmabuf from a virtio-gpu guest
dedicated heap, but that needs further investigation.
In terms of the protocol, these following prototype items are added:
BLOB_CREATE_GUEST_HANDLE: "create an udmabuf" or an OS-specific
equivalent. This can be used with the guest dedicated heap or system
memory. Right now, only system memory is used.
We also want to associate the udmabuf with any host side metadata. For
example, SET_SCANOUT_BLOB doesn't passthrough the modifiers since
virtio-gpu KMS + modifiers is annoying. Simple solution: just ask the
host for the modifier. This could also enable different caching types
if the guest blob is mappable (for example, the MSM GPU driver currently
only supports WC mappings. We might also want cached mappings for
camera).
Incidentals:
* Add a placeholder for RESOURCE_SYNC
BUG=chromium:892806, b:173630595
TEST=create a bunch of udmabufs from the guest
Change-Id: I4686d9f48938f10e7c265750a931d7d2d2d9611b
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2786291
Tested-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Zach Reizner <zachr@chromium.org>
Commit-Queue: Gurchetan Singh <gurchetansingh@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>
This allows other languages to communicate directly with the control
socket without having to invoke `crosvm`
BUG=None
TEST=Ran ./run_tests
Change-Id: Icbf5905c41643b080bae3613b73a032467db1c4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2772798
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Kevin Hamacher <hamacher@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Convert the pub member to private and provide an accessor.
Prevents the spread of poking in to a private member from vhost.
Change-Id: Ib2070e990dc91c532164cc83f5af72bfbc9b2e89
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2795283
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Keiichi Watanabe <keiichiw@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>
Allowing each region to have a separate backing FD will make it possible
to build GuestMemory from the vhost `SET_MEM_TABLE` message that
transmits the memory regions for virtio queues in vhost-user devices.
Change-Id: I6f9bc6136915da9d873ea896823e3b8f426ca69d
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2795282
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Dylan Reid <dgreid@chromium.org>
Create the virtio video-decoder and video-encoder devices after the GPU
device so that the device number of GPU will be consistent on ARCVM
platforms where video devices may or may not exist.
BUG=b:178348623
TEST=boot arcvm on betty and hatch, check gpu pci id is equal
Change-Id: I99d9d0befe6e5aea16fc4e85ed527e4954010466
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2773655
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
A new background thread is added to the client to receive buffer
status messages from the server. The VioSClient struct is made thread
safe and can now be kept inside an Arc instead of a Mutex.
BUG=b/163867676
Change-Id: I52c6d93d36096699906dfc95821dc1834ff6f7bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2795292
Tested-by: Jorge Moreira Broche <jemoreira@google.com>
Auto-Submit: Jorge Moreira Broche <jemoreira@google.com>
Commit-Queue: Jorge Moreira Broche <jemoreira@google.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Now that there is more than one binary target, set the default-run to
the classic crosvm target, which is a superset of all the binary
targets.
TEST=cargo run (without --bin)
BUG=None
Change-Id: I199e8938d6a8bb69175391971bdf4f809bbe19e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2806225
Auto-Submit: Zach Reizner <zachr@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-by: Tomasz Jeznach <tjeznach@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Commit-Queue: Dylan Reid <dgreid@chromium.org>
Commit-Queue: Zach Reizner <zachr@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
