loom icon indicating copy to clipboard operation
loom copied to clipboard

What to do when loom::AtomicUsize do not implement as_mut_ptr()

Open BowenXiao1999 opened this issue 2 years ago • 7 comments

Hi, I was tring to use loom for a special ref counter. Here is the sample code, basically trying to deallocate a AtomicUsize when down to 0.

But I found that loom::AtomicUsize do not have api .as_mut_ptr() to get the mutable pointer. What should I do in this case? I read the doc and found that we should implement API for std type if there is API difference. But for this case, it is the loom type that miss the function, how can I implement this? It seems not feasible via with_mut.

Thanks!

#[repr(transparent)]
#[derive(Clone, Copy, Debug)]
pub struct TaskLocalBytesAllocated(Option<&'static AtomicUsize>);
impl TaskLocalBytesAllocated {
    /// Subtracts from the counter value, and `drop` the counter while the count reaches zero.
    #[inline(always)]
    pub(crate) fn sub(&self, val: usize) {
        if let Some(bytes) = self.0 {
            // Use Release to synchronize with the below deletion.
            let old_bytes = bytes.fetch_sub(val, Ordering::Release);
            // If the counter reaches zero, delete the counter. Note that we've ensured there's no
            // zero deltas in `wrap_layout`, so there'll be no more uses of the counter.
            if old_bytes == val {
                // This fence is needed to prevent reordering of use of the counter and deletion of
                // the counter. Because it is marked `Release`, the decreasing of the counter
                // synchronizes with this `Acquire` fence. This means that use of the counter
                // happens before decreasing the counter, which happens before this fence, which
                // happens before the deletion of the counter.
                fence(Ordering::Acquire);

                unsafe { Box::from_raw_in(bytes.as_mut_ptr(), System) };
            }
        }
    }
}

BowenXiao1999 avatar Dec 08 '22 12:12 BowenXiao1999

I just found that #154 maybe relates to this.

BowenXiao1999 avatar Dec 08 '22 13:12 BowenXiao1999

  • as_mut_ptr is an unstable API, and loom does not usually attempt to emulate unstable APIs.
  • Even if loom provides as_mut_ptr, I think the use of as_mut_ptr in your code is unsound with loom because it relies on std atomic type layout: In your code, the pointer returned by as_mut_ptr must also be a pointer to the allocated atomic type, but in loom, the pointer to the value and the pointer to the atomic type point to different locations (see note in docs).

taiki-e avatar Dec 08 '22 13:12 taiki-e

but in loom, the pointer to the value and the pointer to the atomic type point to different locations

So to achieve the same semantics and effect, for loom, we should free the 2 pointers (assuming we can get both) instead of one?

BowenXiao1999 avatar Dec 08 '22 14:12 BowenXiao1999

No. When loom's atomic type is freed, the value is also freed, so there is only one pointer that needs to be freed.

However, you need to call Box::from_raw* with a pointer to the object you actually allocated, instead of the pointer returned from Atomic*::as_mut_ptr. Perhaps you need to store a pointer instead of a reference (something like the following).

#[repr(transparent)]
#[derive(Clone, Copy, Debug)]
// Note that NoneNull is used here. 
pub struct TaskLocalBytesAllocated(Option<NoneNull<AtomicUsize>>);
impl TaskLocalBytesAllocated {
    #[inline(always)]
    pub(crate) fn sub(&self, val: usize) {
        if let Some(bytes_ptr) = self.0 {
            let bytes_ref: &AtomicUsize = unsafe { bytes_ptr.as_ref() };
            let old_bytes = bytes_ref.fetch_sub(val, Ordering::Release);
            if old_bytes == val {
                fence(Ordering::Acquire);
                // Note that this is a pointer to AtomicUsize. 
                // Since I don't know the full context of your code, 
                // I'm assuming here that the object allocated is only AtomicUsize,
                // but if you allocate a structure containing AtomicUsize, you need 
                // to call from_raw with a pointer to that entire structure.
                let ptr: *mut AtomicUsize = bytes_ptr.as_ptr();
                unsafe { Box::from_raw_in(ptr, System) };
            }
        }
    }
}

taiki-e avatar Dec 08 '22 15:12 taiki-e

Thanks for your help.

Another question:

  • I have add a unit test above. But it seems that when I change the spawn threads from 3 to 4, the unit test will be aborted by signal unexpectedly and hard to debug.

error messages: process didn't exit successfully: `/Users/bowen/risingwave-dev/target/debug/deps/loom-2e78f6234f7ddb77` (signal: 6, SIGABRT: process abort signal)

Is it becasue the MAX_THREADS limit? So we only allowed to spawn 3 thread in max? [3 + 1 (the original thread) = 4?]

BowenXiao1999 avatar Dec 09 '22 06:12 BowenXiao1999

SIGABRT: process abort signal

I think this is due to panic during panic as well as https://github.com/tokio-rs/loom/issues/291.

taiki-e avatar Dec 09 '22 10:12 taiki-e

I opened PR #297 which should fix that panic-in-panic SIGABRT error :)

Pointerbender avatar Dec 14 '22 08:12 Pointerbender