diff --git a/kernel/src/context.rs b/kernel/src/context.rs index 760602024..dfc8b0ebd 100644 --- a/kernel/src/context.rs +++ b/kernel/src/context.rs @@ -174,7 +174,7 @@ impl<'a> CurrentUserSpace<'a> { /// Atomically updates a `PodAtomic` value with [`Ordering::Relaxed`] semantics. /// - /// This method internally uses an atomic compare-and-exchange operation.If the value changes + /// This method internally uses [`atomic_compare_exchange`]. If the value changes /// concurrently, this method will retry so the operation may be performed multiple times. /// /// # Panics @@ -183,18 +183,20 @@ impl<'a> CurrentUserSpace<'a> { /// boundary. /// /// [`Ordering::Relaxed`]: core::sync::atomic::Ordering::Relaxed + /// [`atomic_compare_exchange`]: VmWriter::atomic_compare_exchange pub fn atomic_update(&self, vaddr: Vaddr, op: impl Fn(T) -> T) -> Result where T: PodAtomic + Eq, { check_vaddr(vaddr)?; + let writer = self.writer(vaddr, core::mem::size_of::())?; + let reader = self.reader(vaddr, core::mem::size_of::())?; - let user_reader = self.reader(vaddr, core::mem::size_of::())?; - let mut user_writer = self.writer(vaddr, core::mem::size_of::())?; + let mut old_val = reader.atomic_load()?; loop { - match user_writer.atomic_update(&user_reader, &op)? { - (old_val, true) => return Ok(old_val), - (_, false) => continue, + match writer.atomic_compare_exchange(&reader, old_val, op(old_val))? { + (_, true) => return Ok(old_val), + (cur_val, false) => old_val = cur_val, } } } diff --git a/kernel/src/process/posix_thread/robust_list.rs b/kernel/src/process/posix_thread/robust_list.rs index f893be887..bb60752eb 100644 --- a/kernel/src/process/posix_thread/robust_list.rs +++ b/kernel/src/process/posix_thread/robust_list.rs @@ -124,38 +124,44 @@ const FUTEX_WAITERS: u32 = 0x8000_0000; const FUTEX_OWNER_DIED: u32 = 0x4000_0000; const FUTEX_TID_MASK: u32 = 0x3FFF_FFFF; -/// Wakeup one robust futex owned by the thread -/// FIXME: requires atomic operations here +/// Attempts to wake a robust futex owned by the given thread. +/// +/// If the futex at `futex_addr` is still owned by `tid`, it is marked with +/// `FUTEX_OWNER_DIED` and one waiter (if any) is woken. +/// If the futex is owned by another thread, the operation is canceled. pub fn wake_robust_futex(futex_addr: Vaddr, tid: Tid) -> Result<()> { + if futex_addr == 0 { + return_errno_with_message!(Errno::EINVAL, "invalid futext addr"); + } + let task = Task::current().unwrap(); let user_space = CurrentUserSpace::new(task.as_thread_local().unwrap()); - let futex_val = { - if futex_addr == 0 { - return_errno_with_message!(Errno::EINVAL, "invalid futext addr"); - } - user_space.read_val::(futex_addr)? - }; - let mut old_val = futex_val; + // Instantiate reader and writer pointing at the same `futex_addr`, set up + // for the same length: the length of an `u32`. + const U32_LEN: usize = size_of::(); + let writer = user_space.writer(futex_addr, U32_LEN)?; + let reader = user_space.reader(futex_addr, U32_LEN)?; + + let mut old_val: u32 = reader.atomic_load()?; loop { - // This futex may held by another thread, do nothing + // This futex may be held by another thread. If so, do nothing. if old_val & FUTEX_TID_MASK != tid { break; } + let new_val = (old_val & FUTEX_WAITERS) | FUTEX_OWNER_DIED; - let cur_val = user_space.read_val(futex_addr)?; - if cur_val != new_val { - // The futex value has changed, let's retry with current value - old_val = cur_val; - user_space.write_val(futex_addr, &new_val)?; - continue; + match writer.atomic_compare_exchange(&reader, old_val, new_val)? { + (cur_val, false) => old_val = cur_val, // Try again with `cur_val`. + (_, true) => { + // Wake up one waiter and break out from the loop. + if new_val & FUTEX_WAITERS != 0 { + debug!("wake robust futex addr: {:?}", futex_addr); + futex_wake(futex_addr, 1, None)?; + } + break; + } } - // Wakeup one waiter - if cur_val & FUTEX_WAITERS != 0 { - debug!("wake robust futex addr: {:?}", futex_addr); - futex_wake(futex_addr, 1, None)?; - } - break; } Ok(()) } diff --git a/ostd/src/mm/io.rs b/ostd/src/mm/io.rs index 290c1527f..a875b0647 100644 --- a/ostd/src/mm/io.rs +++ b/ostd/src/mm/io.rs @@ -781,7 +781,7 @@ impl<'a> VmWriter<'a, Infallible> { } // Currently, there are no volatile atomic operations in `core::intrinsics`. Therefore, we do - // not provide an infallible implementation of `VmWriter::atomic_update`. + // not provide an infallible implementation of `VmWriter::atomic_compare_exchange`. /// Writes `len` zeros to the target memory. /// @@ -859,27 +859,30 @@ impl VmWriter<'_, Fallible> { Ok(()) } - /// Atomically updates a `PodAtomic` value. + /// Atomically compares and exchanges a `PodAtomic` value. /// - /// This is implemented by performing an atomic load, applying the operation, and performing an - /// atomic compare-and-exchange. So this cannot prevent the [ABA - /// problem](https://en.wikipedia.org/wiki/ABA_problem). + /// This method compares `old_val` with the value pointed by `self` and, if they are equal, + /// updates it with `new_val`. /// - /// The caller is required to provide a reader which points to the exactly same memory location + /// The value that was previously in memory will be returned, along with a boolean denoting + /// whether the compare-and-exchange succeeds. The caller usually wants to retry if this + /// flag is false, passing the most recent value that was returned by this method. + /// + /// The caller is required to provide a reader which points to the exact same memory location /// to ensure that reading from the memory is allowed. /// - /// On success, the previous value will be returned with a boolean value denoting whether the - /// compare-and-exchange succeeds. The caller usually wants to retry if the flag is false. - /// - /// Regardless of whether it is successful, the cursor of the reader and writer will not move. + /// Regardless of whether it is successful, the cursors of the reader and writer will not move. /// /// This method only guarantees the atomicity of the specific operation. There are no /// synchronization constraints on other memory accesses. This aligns with the [Relaxed /// ordering](https://en.cppreference.com/w/cpp/atomic/memory_order.html#Relaxed_ordering) /// specified in the C++11 memory model. /// + /// Since the operation does not involve memory locks, it can't prevent the [ABA + /// problem](https://en.wikipedia.org/wiki/ABA_problem). + /// /// This method will fail with errors if: - /// 1. the remaining (avail) space of the reader (writer) is less than + /// 1. the remaining space of the reader or the available space of the writer are less than /// `core::mem::size_of::()` bytes, or /// 2. the memory operation fails due to an unresolvable page fault. /// @@ -888,10 +891,11 @@ impl VmWriter<'_, Fallible> { /// This method will panic if: /// 1. the reader and the writer does not point to the same memory location, or /// 2. the memory location is not aligned on a `core::mem::align_of::()`-byte boundary. - pub fn atomic_update( - &mut self, + pub fn atomic_compare_exchange( + &self, reader: &VmReader, - op: impl FnOnce(T) -> T, + old_val: T, + new_val: T, ) -> Result<(T, bool)> where T: PodAtomic + Eq, @@ -905,19 +909,12 @@ impl VmWriter<'_, Fallible> { let cursor = self.cursor.cast::(); assert!(cursor.is_aligned()); - // SAFETY: - // 1. The cursor is either valid for reading or in user space for `size_of::()` bytes. - // 2. The cursor is aligned on a `align_of::()`-byte boundary. - let old_val = unsafe { T::atomic_load_fallible(cursor)? }; - - let new_val = op(old_val); - // SAFETY: // 1. The cursor is either valid for reading and writing or in user space for 4 bytes. // 2. The cursor is aligned on a 4-byte boundary. let cur_val = unsafe { T::atomic_cmpxchg_fallible(cursor, old_val, new_val)? }; - Ok((old_val, old_val == cur_val)) + Ok((cur_val, old_val == cur_val)) } /// Writes `len` zeros to the target memory. diff --git a/ostd/src/mm/test.rs b/ostd/src/mm/test.rs index 63b36ff6b..168e41ea5 100644 --- a/ostd/src/mm/test.rs +++ b/ostd/src/mm/test.rs @@ -345,41 +345,49 @@ mod io { assert_eq!(reader_fallible.atomic_load::().unwrap(), 0x02020202); } - /// Tests the `atomic_update` method in Fallible mode. + /// Tests the `atomic_compare_exchange` method in Fallible mode. #[ktest] - fn atomic_update_fallible() { - type Segment = crate::mm::Segment<()>; - - fn update(segment: &Segment, old_val: u32, new_val: u32, f: fn(segment: &Segment)) -> bool { - let (val, is_succ) = segment - .writer() - .to_fallible() - .skip(4) - .atomic_update(segment.reader().to_fallible().skip(4), |val| { - assert_eq!(val, old_val); - f(segment); - new_val - }) - .unwrap(); - assert_eq!(val, old_val); - is_succ - } - + fn atomic_compare_exchange_fallible() { let segment = FrameAllocOptions::new() .zeroed(true) .alloc_segment(1) .unwrap(); - assert!(update(&segment, 0, 100, |_| ())); - assert!(update(&segment, 100, 200, |_| ())); + let cmpxchg = |expected, new| { + segment + .writer() + .to_fallible() + .atomic_compare_exchange(&segment.reader().to_fallible(), expected, new) + .unwrap() + }; - let is_succ = update(&segment, 200, 400, |segment| { - assert!(update(segment, 200, 300, |_| ())) - }); - assert!(!is_succ); + // Initially 0, expect 0 -> succeed and set to 100 + let (val, ok) = cmpxchg(0, 100); + assert_eq!(val, 0); + assert!(ok); + // Now 100, expect 100 -> succeed and set to 200 + let (val, ok) = cmpxchg(100, 200); + assert_eq!(val, 100); + assert!(ok); + + // Now 200, but we expect 300 -> fail, memory stays 200 + let (val, ok) = cmpxchg(300, 400); + assert_eq!(val, 200); + assert!(!ok); + + // Still 200, expect 200 -> succeed and set to 300 + let (val, ok) = cmpxchg(200, 300); + assert_eq!(val, 200); + assert!(ok); + + // Now 300, expect 200 -> fail, memory stays 300 + let (val, ok) = cmpxchg(200, 400); + assert_eq!(val, 300); + assert!(!ok); + + // Final check with raw reader let mut reader = segment.reader().to_fallible(); - reader.skip(4); assert_eq!(reader.atomic_load::().unwrap(), 300); assert_eq!(reader.read_val::().unwrap(), 300); }