| Message ID | 20240915-alice-file-v10-1-88484f7a3dcf@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | File abstractions needed by Rust Binder | expand |
On Sun, 15 Sep 2024 14:31:27 +0000 Alice Ryhl <aliceryhl@google.com> wrote: > This introduces a new marker type for types that shouldn't be thread > safe. By adding a field of this type to a struct, it becomes non-Send > and non-Sync, which means that it cannot be accessed in any way from > threads other than the one it was created on. > > This is useful for APIs that require globals such as `current` to remain > constant while the value exists. > > We update two existing users in the Kernel to use this helper: > > * `Task::current()` - moving the return type of this value to a > different thread would not be safe as you can no longer be guaranteed > that the `current` pointer remains valid. > * Lock guards. Mutexes and spinlocks should be unlocked on the same > thread as where they were locked, so we enforce this using the Send > trait. > > There are also additional users in later patches of this patchset. See > [1] and [2] for the discussion that led to the introduction of this > patch. > > Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [1] > Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [2] > Suggested-by: Benno Lossin <benno.lossin@proton.me> > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > Reviewed-by: Trevor Gross <tmgross@umich.edu> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com> > Reviewed-by: Gary Guo <gary@garyguo.net> > Signed-off-by: Alice Ryhl <aliceryhl@google.com> Miguel, can we apply this patch now without having it wait on the rest of file abstractions because it'll be useful to other? Best, Gary > --- > rust/kernel/sync/lock.rs | 13 +++++++++---- > rust/kernel/task.rs | 10 ++++++---- > rust/kernel/types.rs | 21 +++++++++++++++++++++ > 3 files changed, 36 insertions(+), 8 deletions(-)
On Sun, Sep 15, 2024 at 02:31:27PM +0000, Alice Ryhl wrote: > This introduces a new marker type for types that shouldn't be thread > safe. By adding a field of this type to a struct, it becomes non-Send > and non-Sync, which means that it cannot be accessed in any way from > threads other than the one it was created on. > > This is useful for APIs that require globals such as `current` to remain > constant while the value exists. > > We update two existing users in the Kernel to use this helper: > > * `Task::current()` - moving the return type of this value to a > different thread would not be safe as you can no longer be guaranteed > that the `current` pointer remains valid. > * Lock guards. Mutexes and spinlocks should be unlocked on the same > thread as where they were locked, so we enforce this using the Send > trait. Hi, this sounds useful, however from kernel side when I think thread-safe, I think must not be used across a sleep. Would something like ThreadLocked or LockedToThread make sense? (I could be way off base here...) thanks, -serge > There are also additional users in later patches of this patchset. See > [1] and [2] for the discussion that led to the introduction of this > patch. > > Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [1] > Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [2] > Suggested-by: Benno Lossin <benno.lossin@proton.me> > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > Reviewed-by: Trevor Gross <tmgross@umich.edu> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > Reviewed-by: Björn Roy Baron <bjorn3_gh@protonmail.com> > Reviewed-by: Gary Guo <gary@garyguo.net> > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > rust/kernel/sync/lock.rs | 13 +++++++++---- > rust/kernel/task.rs | 10 ++++++---- > rust/kernel/types.rs | 21 +++++++++++++++++++++ > 3 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs > index f6c34ca4d819..d6e9bab114b8 100644 > --- a/rust/kernel/sync/lock.rs > +++ b/rust/kernel/sync/lock.rs > @@ -6,8 +6,13 @@ > //! spinlocks, raw spinlocks) to be provided with minimal effort. > > use super::LockClassKey; > -use crate::{init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard}; > -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned}; > +use crate::{ > + init::PinInit, > + pin_init, > + str::CStr, > + types::{NotThreadSafe, Opaque, ScopeGuard}, > +}; > +use core::{cell::UnsafeCell, marker::PhantomPinned}; > use macros::pin_data; > > pub mod mutex; > @@ -139,7 +144,7 @@ pub fn lock(&self) -> Guard<'_, T, B> { > pub struct Guard<'a, T: ?Sized, B: Backend> { > pub(crate) lock: &'a Lock<T, B>, > pub(crate) state: B::GuardState, > - _not_send: PhantomData<*mut ()>, > + _not_send: NotThreadSafe, > } > > // SAFETY: `Guard` is sync when the data protected by the lock is also sync. > @@ -191,7 +196,7 @@ pub(crate) unsafe fn new(lock: &'a Lock<T, B>, state: B::GuardState) -> Self { > Self { > lock, > state, > - _not_send: PhantomData, > + _not_send: NotThreadSafe, > } > } > } > diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs > index 55dff7e088bf..278c623de0c6 100644 > --- a/rust/kernel/task.rs > +++ b/rust/kernel/task.rs > @@ -4,10 +4,12 @@ > //! > //! C header: [`include/linux/sched.h`](srctree/include/linux/sched.h). > > -use crate::types::Opaque; > +use crate::{ > + bindings, > + types::{NotThreadSafe, Opaque}, > +}; > use core::{ > ffi::{c_int, c_long, c_uint}, > - marker::PhantomData, > ops::Deref, > ptr, > }; > @@ -106,7 +108,7 @@ impl Task { > pub unsafe fn current() -> impl Deref<Target = Task> { > struct TaskRef<'a> { > task: &'a Task, > - _not_send: PhantomData<*mut ()>, > + _not_send: NotThreadSafe, > } > > impl Deref for TaskRef<'_> { > @@ -125,7 +127,7 @@ fn deref(&self) -> &Self::Target { > // that `TaskRef` is not `Send`, we know it cannot be transferred to another thread > // (where it could potentially outlive the caller). > task: unsafe { &*ptr.cast() }, > - _not_send: PhantomData, > + _not_send: NotThreadSafe, > } > } > > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs > index 9e7ca066355c..3238ffaab031 100644 > --- a/rust/kernel/types.rs > +++ b/rust/kernel/types.rs > @@ -532,3 +532,24 @@ unsafe impl AsBytes for str {} > // does not have any uninitialized portions either. > unsafe impl<T: AsBytes> AsBytes for [T] {} > unsafe impl<T: AsBytes, const N: usize> AsBytes for [T; N] {} > + > +/// Zero-sized type to mark types not [`Send`]. > +/// > +/// Add this type as a field to your struct if your type should not be sent to a different task. > +/// Since [`Send`] is an auto trait, adding a single field that is `!Send` will ensure that the > +/// whole type is `!Send`. > +/// > +/// If a type is `!Send` it is impossible to give control over an instance of the type to another > +/// task. This is useful to include in types that store or reference task-local information. A file > +/// descriptor is an example of such task-local information. > +/// > +/// This type also makes the type `!Sync`, which prevents immutable access to the value from > +/// several threads in parallel. > +pub type NotThreadSafe = PhantomData<*mut ()>; > + > +/// Used to construct instances of type [`NotThreadSafe`] similar to how `PhantomData` is > +/// constructed. > +/// > +/// [`NotThreadSafe`]: type@NotThreadSafe > +#[allow(non_upper_case_globals)] > +pub const NotThreadSafe: NotThreadSafe = PhantomData; > > -- > 2.46.0.662.g92d0881bb0-goog
On Tue, Sep 24, 2024 at 9:45 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > On Sun, Sep 15, 2024 at 02:31:27PM +0000, Alice Ryhl wrote: > > This introduces a new marker type for types that shouldn't be thread > > safe. By adding a field of this type to a struct, it becomes non-Send > > and non-Sync, which means that it cannot be accessed in any way from > > threads other than the one it was created on. > > > > This is useful for APIs that require globals such as `current` to remain > > constant while the value exists. > > > > We update two existing users in the Kernel to use this helper: > > > > * `Task::current()` - moving the return type of this value to a > > different thread would not be safe as you can no longer be guaranteed > > that the `current` pointer remains valid. > > * Lock guards. Mutexes and spinlocks should be unlocked on the same > > thread as where they were locked, so we enforce this using the Send > > trait. > > Hi, > > this sounds useful, however from kernel side when I think thread-safe, > I think must not be used across a sleep. Would something like ThreadLocked > or LockedToThread make sense? Hmm, those names seem pretty similar to the current name to me? Alice
On Wed, Sep 25, 2024 at 01:06:10PM +0200, Alice Ryhl wrote: > On Tue, Sep 24, 2024 at 9:45 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > > On Sun, Sep 15, 2024 at 02:31:27PM +0000, Alice Ryhl wrote: > > > This introduces a new marker type for types that shouldn't be thread > > > safe. By adding a field of this type to a struct, it becomes non-Send > > > and non-Sync, which means that it cannot be accessed in any way from > > > threads other than the one it was created on. > > > > > > This is useful for APIs that require globals such as `current` to remain > > > constant while the value exists. > > > > > > We update two existing users in the Kernel to use this helper: > > > > > > * `Task::current()` - moving the return type of this value to a > > > different thread would not be safe as you can no longer be guaranteed > > > that the `current` pointer remains valid. > > > * Lock guards. Mutexes and spinlocks should be unlocked on the same > > > thread as where they were locked, so we enforce this using the Send > > > trait. > > > > Hi, > > > > this sounds useful, however from kernel side when I think thread-safe, > > I think must not be used across a sleep. Would something like ThreadLocked > > or LockedToThread make sense? > > Hmm, those names seem pretty similar to the current name to me? Seems very different to me: If @foo is not threadsafe, it may be global or be usable by many threads, but must be locked to one thread during access. What you're describing here is (iiuc) that @foo must only be used by one particular thread. -serge
On Wed, 25 Sep 2024 08:59:04 -0500 "Serge E. Hallyn" <serge@hallyn.com> wrote: > On Wed, Sep 25, 2024 at 01:06:10PM +0200, Alice Ryhl wrote: > > On Tue, Sep 24, 2024 at 9:45 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > > > > > On Sun, Sep 15, 2024 at 02:31:27PM +0000, Alice Ryhl wrote: > > > > This introduces a new marker type for types that shouldn't be thread > > > > safe. By adding a field of this type to a struct, it becomes non-Send > > > > and non-Sync, which means that it cannot be accessed in any way from > > > > threads other than the one it was created on. > > > > > > > > This is useful for APIs that require globals such as `current` to remain > > > > constant while the value exists. > > > > > > > > We update two existing users in the Kernel to use this helper: > > > > > > > > * `Task::current()` - moving the return type of this value to a > > > > different thread would not be safe as you can no longer be guaranteed > > > > that the `current` pointer remains valid. > > > > * Lock guards. Mutexes and spinlocks should be unlocked on the same > > > > thread as where they were locked, so we enforce this using the Send > > > > trait. > > > > > > Hi, > > > > > > this sounds useful, however from kernel side when I think thread-safe, > > > I think must not be used across a sleep. Would something like ThreadLocked > > > or LockedToThread make sense? > > > > Hmm, those names seem pretty similar to the current name to me? > > Seems very different to me: > > If @foo is not threadsafe, it may be global or be usable by many > threads, but must be locked to one thread during access. > > What you're describing here is (iiuc) that @foo must only be used > by one particular thread. "locked to one thread during access" means it might be `Send` but not `!Sync`. What Alice has here is something is neither `Send` nor `Sync`, so I think the `NotThreadSafe` is a good name here because it cancels both guarantees. Best, Gary
On Sun, Sep 15, 2024 at 5:38 PM Gary Guo <gary@garyguo.net> wrote: > > Miguel, can we apply this patch now without having it wait on the rest > of file abstractions because it'll be useful to other? Sorry, I missed to reply to this during the conferences. If we need this for something else that does not go through VFS during this (same) cycle, then we can figure something out and apply it to rust-next too. Cheers, Miguel
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index f6c34ca4d819..d6e9bab114b8 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -6,8 +6,13 @@ //! spinlocks, raw spinlocks) to be provided with minimal effort. use super::LockClassKey; -use crate::{init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard}; -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned}; +use crate::{ + init::PinInit, + pin_init, + str::CStr, + types::{NotThreadSafe, Opaque, ScopeGuard}, +}; +use core::{cell::UnsafeCell, marker::PhantomPinned}; use macros::pin_data; pub mod mutex; @@ -139,7 +144,7 @@ pub fn lock(&self) -> Guard<'_, T, B> { pub struct Guard<'a, T: ?Sized, B: Backend> { pub(crate) lock: &'a Lock<T, B>, pub(crate) state: B::GuardState, - _not_send: PhantomData<*mut ()>, + _not_send: NotThreadSafe, } // SAFETY: `Guard` is sync when the data protected by the lock is also sync. @@ -191,7 +196,7 @@ pub(crate) unsafe fn new(lock: &'a Lock<T, B>, state: B::GuardState) -> Self { Self { lock, state, - _not_send: PhantomData, + _not_send: NotThreadSafe, } } } diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs index 55dff7e088bf..278c623de0c6 100644 --- a/rust/kernel/task.rs +++ b/rust/kernel/task.rs @@ -4,10 +4,12 @@ //! //! C header: [`include/linux/sched.h`](srctree/include/linux/sched.h). -use crate::types::Opaque; +use crate::{ + bindings, + types::{NotThreadSafe, Opaque}, +}; use core::{ ffi::{c_int, c_long, c_uint}, - marker::PhantomData, ops::Deref, ptr, }; @@ -106,7 +108,7 @@ impl Task { pub unsafe fn current() -> impl Deref<Target = Task> { struct TaskRef<'a> { task: &'a Task, - _not_send: PhantomData<*mut ()>, + _not_send: NotThreadSafe, } impl Deref for TaskRef<'_> { @@ -125,7 +127,7 @@ fn deref(&self) -> &Self::Target { // that `TaskRef` is not `Send`, we know it cannot be transferred to another thread // (where it could potentially outlive the caller). task: unsafe { &*ptr.cast() }, - _not_send: PhantomData, + _not_send: NotThreadSafe, } } diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index 9e7ca066355c..3238ffaab031 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -532,3 +532,24 @@ unsafe impl AsBytes for str {} // does not have any uninitialized portions either. unsafe impl<T: AsBytes> AsBytes for [T] {} unsafe impl<T: AsBytes, const N: usize> AsBytes for [T; N] {} + +/// Zero-sized type to mark types not [`Send`]. +/// +/// Add this type as a field to your struct if your type should not be sent to a different task. +/// Since [`Send`] is an auto trait, adding a single field that is `!Send` will ensure that the +/// whole type is `!Send`. +/// +/// If a type is `!Send` it is impossible to give control over an instance of the type to another +/// task. This is useful to include in types that store or reference task-local information. A file +/// descriptor is an example of such task-local information. +/// +/// This type also makes the type `!Sync`, which prevents immutable access to the value from +/// several threads in parallel. +pub type NotThreadSafe = PhantomData<*mut ()>; + +/// Used to construct instances of type [`NotThreadSafe`] similar to how `PhantomData` is +/// constructed. +/// +/// [`NotThreadSafe`]: type@NotThreadSafe +#[allow(non_upper_case_globals)] +pub const NotThreadSafe: NotThreadSafe = PhantomData;