Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
BowenXiao1999 opened this issue Dec 8, 2022 · 7 comments
Open

Comments

@BowenXiao1999
Copy link

BowenXiao1999 commented Dec 8, 2022

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
Copy link
Author

I just found that #154 maybe relates to this.

@taiki-e
Copy link
Member

taiki-e commented Dec 8, 2022

  • 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).

@BowenXiao1999
Copy link
Author

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?

@taiki-e
Copy link
Member

taiki-e commented Dec 8, 2022

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) };
            }
        }
    }
}

@BowenXiao1999
Copy link
Author

BowenXiao1999 commented Dec 9, 2022

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?]

@taiki-e
Copy link
Member

taiki-e commented Dec 9, 2022

SIGABRT: process abort signal

I think this is due to panic during panic as well as #291.

@Pointerbender
Copy link
Contributor

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

mergify bot pushed a commit to risingwavelabs/risingwave that referenced this issue Dec 15, 2022
Basically same as #6822. But we reuse the Counter to avoid code duplicate. Copy the content here.

Use https://github.com/tokio-rs/loom for concurrency test. Maybe can make the correctness more confident. But I'm not so sure how to write the best test.

it requires some code change for previous structure, so may need discuss:

remove workspacke hack in task_stats_alloc crate. Otherwise there will be package conflict if you run RUSTFLAGS="--cfg loom" cargo test --test loom. Have not investigate deeply but simple remove just works.

Refactor &'static AtomicUsize to be NonNull<AtomicUsize>. This is because the AtomicUsize in loom type do not support .as_mut_ptr (described in What to do when loom::AtomicUsize do not implement as_mut_ptr() tokio-rs/loom#298), so we use NonNull as intermediate workaround, that will add some unsafe code in .add() and .sub(). But seems OK.

To test the drop, I have to add a flag var AtomicUsize in .sub() to ensure that the value is dropped. Please provide some suggestions if you have better ideas on how to write test.


Approved-By: BugenZhao
Approved-By: liurenjie1024

Co-Authored-By: BowenXiao1999 <[email protected]>
Co-Authored-By: Bowen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants