rust: sync: Make Lock::locked_data() return raw pointer

[fbq]

@wedsonaf This PR is more for discussion purpose.

Currently Lock::locked_data returns &UnsafeCell<T>, I think we can change it to *mut T, reasons as follow:

• The solely purpose of &UnsafeCell<T> is to get a *mut T, why not directly return *mut T?
• Returning a &UnsafeCell<T> means we store underlying data in a UnsafeCell, and I think this is more an implementation detail, which we don't want to expose to API level.
• Besides, say we want to access data outside Rust world (e.g. a static C variable), we cannot put it in a Rust UnsafeCell, right? Because of that, we cannot implement a proper Lock struct for it.

An example for the third point: __cpu_online_mask is protected by cpus_write_lock() and cpus_write_unlock(), if we are about to implement a CPUOnlineLock, we could code as follow:

pub struct CPUOnlineLock {}

impl Lock for CPUOnlineLock {
    type Inner = bindings::cpumask;
    fn lock_noguard(&self) {
        unsafe {
            bindings::cpus_write_lock();
        }
    }
    unsafe fn unlock(&self) {
         bindings::cpus_write_unlock();
    }

    fn locked_data(&self) -> &UnsafeCell<Self::Inner> {
        // what we are going to return here?
        // if the API return `* mut T`, we can return
        // unsafe { &__cpu_online_mask }
        // here.
    }
}

Thoughts?

[jabedude]

Besides, say we want to access data outside Rust world (e.g. a static C variable), we cannot put it in a Rust UnsafeCell, right?

I think that is true, but I don't see that as a limitation on UnsafeCell. If a driver author has data that is accessed from C they would simply get the internal mutable pointer

[fbq]

Besides, say we want to access data outside Rust world (e.g. a static C variable), we cannot put it in a Rust UnsafeCell, right?

I think that is true, but I don't see that as a limitation on UnsafeCell. If a driver author has data that is accessed from C they would simply get the internal mutable pointer

What I mean is we have a static variable in C world:

static struct cpumask __cpu_online_mask;

And we cannot wrap it into a Rust UnsafeCell, whose ref is returned by some locked_data() implemention.

[wedsonaf]

@fbq I think you're right in that there's no harm in returning a raw pointer, in addition to enabling the scenario you describe where C controls the data storage.

Let me think about it tonight to try and convince myself. The thing that isn't clear yet is whether removing UnsafeCell may enable the compiler to do optimisations that shouldn't be allowed.

[197g]

The solely purpose of &UnsafeCell is to get a *mut T, why not directly return *mut T?

There is a semantic gap between &UnsafeCell<T> and a raw pointer. The latter guarantees nothing other than being an address while the former is a way to say, there surely is a valid T here that does not overlap with any other unique reference; even though different code may still share access to the inner contents and it might get mutated through different references at the same time and it is totally up to the programmer to ensure that all access to it is correct. Being a reference and thus having a lifetime also nudges the programmer into the direction of borrow checking where possible, such as ensuring that lock guards borrow from the original lock.

And we cannot wrap it into a Rust UnsafeCell, whose ref is returned by some locked_data() implemention.

Rust's memory model isn't typed and UnsafeCell doesn't change the ABI nor layout of the wrapped type—this is due to UnsafeCell being declared with the repr(transparent) attribute—so that declaring the static as an UnsafeCell instead of the contained type must work correctly:

extern "C" {
    static __cpu_online_mask: UnsafeCell<cpumask>;
}

It might also be fine to cast a pointer to the static to a &UnsafeCell<cpumask> if a declaration as such an extern static isn't feasilble for some reason.

[fbq]

Rust's memory model isn't typed and UnsafeCell doesn't change the ABI nor layout of the wrapped type—this is due to UnsafeCell being declared with the repr(transparent) attribute—so that declaring the static as an UnsafeCell instead of the contained type must work correctly:

extern "C" {
    static __cpu_online_mask: UnsafeCell<cpumask>;
}

Cool! I must admit I didn't know this works, thanks a lot!

[Soveu]

Also dereferencing *mut into &mut is very dangerous, because LLVM may assume that it is an unique reference that no other can modify (via noalias attribute)

[fbq]

Also dereferencing *mut into &mut is very dangerous, because LLVM may assume that it is an unique reference that no other can modify (via noalias attribute)

Yeah, @wedsonaf mentioned the similar concern too, and actually, I was also worried about this. Now seems we have a solution (thanks to @HeroicKatora), I will close this. Thank you all.