Skip to content

rust: add new api for initialization #909

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

Closed
wants to merge 11 commits into from
26 changes: 11 additions & 15 deletions drivers/android/context.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
// SPDX-License-Identifier: GPL-2.0

use kernel::{
bindings,
bindings, new_mutex,
prelude::*,
security,
sync::{Arc, Mutex, UniqueArc},
sync::{Arc, Mutex},
};

use crate::{
Expand All @@ -17,7 +17,9 @@ struct Manager {
uid: Option<bindings::kuid_t>,
}

#[pin_data]
pub(crate) struct Context {
#[pin]
manager: Mutex<Manager>,
}

Expand All @@ -27,21 +29,15 @@ unsafe impl Sync for Context {}

impl Context {
pub(crate) fn new() -> Result<Arc<Self>> {
let mut ctx = Pin::from(UniqueArc::try_new(Self {
// SAFETY: Init is called below.
manager: unsafe {
Mutex::new(Manager {
Arc::pin_init(pin_init!(Self {
manager: new_mutex!(
Manager {
node: None,
uid: None,
})
},
})?);

// SAFETY: `manager` is also pinned when `ctx` is.
let manager = unsafe { ctx.as_mut().map_unchecked_mut(|c| &mut c.manager) };
kernel::mutex_init!(manager, "Context::manager");

Ok(ctx.into())
},
"Contex::manager"
),
}))
}

pub(crate) fn set_manager_node(&self, node_ref: NodeRef) -> Result {
Expand Down
30 changes: 13 additions & 17 deletions drivers/android/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

use core::sync::atomic::{AtomicU64, Ordering};
use kernel::{
init::PinInit,
io_buffer::IoBufferWriter,
linked_list::{GetLinks, Links, List},
new_spinlock,
prelude::*,
sync::{Arc, Guard, LockedBy, Mutex, SpinLock},
user_ptr::UserSlicePtrWriter,
Expand Down Expand Up @@ -54,6 +56,7 @@ struct NodeDeathInner {
aborted: bool,
}

#[pin_data]
pub(crate) struct NodeDeath {
node: Arc<Node>,
process: Arc<Process>,
Expand All @@ -63,37 +66,30 @@ pub(crate) struct NodeDeath {
// TODO: Add the moment we're using this for two lists, which isn't safe because we want to
// remove from the list without knowing the list it's in. We need to separate this out.
death_links: Links<NodeDeath>,
#[pin]
inner: SpinLock<NodeDeathInner>,
}

impl NodeDeath {
/// Constructs a new node death notification object.
///
/// # Safety
///
/// The caller must call `NodeDeath::init` before using the notification object.
pub(crate) unsafe fn new(node: Arc<Node>, process: Arc<Process>, cookie: usize) -> Self {
Self {
#[allow(clippy::new_ret_no_self)]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to avoid having to disable this for every function where we return impl InInit<Self>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not naming it new? I am not sure if we should even name them new, maybe new_in_place or something might be more expressive. AFAIK we could add #![allow(clippy::new_ret_no_self)] to lib.rs, but then it would not warn any longer in other places... Maybe we could get in touch with the clippy folks and ask for some workaround (I am not aware of any currently). There is precedent for this: rust-lang/rust-clippy#3313

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to disable this globally until rust-lang/rust-clippy#7344 is fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed rust-lang/rust-clippy#9733 to fix the false positive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just add #![allow(clippy::new_ret_no_self)] to lib.rs?

pub(crate) fn new(node: Arc<Node>, process: Arc<Process>, cookie: usize) -> impl PinInit<Self> {
pin_init!(Self {
node,
process,
cookie,
work_links: Links::new(),
death_links: Links::new(),
inner: unsafe {
SpinLock::new(NodeDeathInner {
inner: new_spinlock!(
NodeDeathInner {
dead: false,
cleared: false,
notification_done: false,
aborted: false,
})
},
}
}

pub(crate) fn init(self: Pin<&mut Self>) {
// SAFETY: `inner` is pinned when `self` is.
let inner = unsafe { self.map_unchecked_mut(|n| &mut n.inner) };
kernel::spinlock_init!(inner, "NodeDeath::inner");
},
"NodeDeath::inner"
),
})
}

/// Sets the cleared flag to `true`.
Expand Down
44 changes: 18 additions & 26 deletions drivers/android/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use kernel::{
file::{self, File, IoctlCommand, IoctlHandler, PollTable},
io_buffer::{IoBufferReader, IoBufferWriter},
linked_list::List,
mm,
mm, new_mutex,
pages::Pages,
prelude::*,
rbtree::RBTree,
Expand Down Expand Up @@ -242,6 +242,7 @@ impl ProcessNodeRefs {
}
}

#[pin_data]
pub(crate) struct Process {
ctx: Arc<Context>,

Expand All @@ -255,10 +256,12 @@ pub(crate) struct Process {
// lock. We may want to split up the process state at some point to use a spin lock for the
// other fields.
// TODO: Make this private again.
#[pin]
pub(crate) inner: Mutex<ProcessInner>,

// References are in a different mutex to avoid recursive acquisition when
// incrementing/decrementing a node in another process.
#[pin]
node_refs: Mutex<ProcessNodeRefs>,
}

Expand All @@ -268,25 +271,13 @@ unsafe impl Sync for Process {}

impl Process {
fn new(ctx: Arc<Context>, cred: ARef<Credential>) -> Result<Arc<Self>> {
let mut process = Pin::from(UniqueArc::try_new(Self {
Arc::pin_init(pin_init!(Self {
ctx,
cred,
task: Task::current().group_leader().into(),
// SAFETY: `inner` is initialised in the call to `mutex_init` below.
inner: unsafe { Mutex::new(ProcessInner::new()) },
// SAFETY: `node_refs` is initialised in the call to `mutex_init` below.
node_refs: unsafe { Mutex::new(ProcessNodeRefs::new()) },
})?);

// SAFETY: `inner` is pinned when `Process` is.
let pinned = unsafe { process.as_mut().map_unchecked_mut(|p| &mut p.inner) };
kernel::mutex_init!(pinned, "Process::inner");

// SAFETY: `node_refs` is pinned when `Process` is.
let pinned = unsafe { process.as_mut().map_unchecked_mut(|p| &mut p.node_refs) };
kernel::mutex_init!(pinned, "Process::node_refs");

Ok(process.into())
task: ARef::from(Task::current().group_leader()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change caused by an inference issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

inner: new_mutex!(ProcessInner::new(), "Process::inner"),
node_refs: new_mutex!(ProcessNodeRefs::new(), "Process::node_refs"),
}))
}

/// Attempts to fetch a work item from the process queue.
Expand Down Expand Up @@ -714,14 +705,15 @@ impl Process {
return Ok(());
}

let death = {
let mut pinned = Pin::from(death.write(
// SAFETY: `init` is called below.
unsafe { NodeDeath::new(info.node_ref.node.clone(), self.clone(), cookie) },
));
pinned.as_mut().init();
Arc::<NodeDeath>::from(pinned)
};
let death = Arc::<NodeDeath>::from(
death
.pin_init_now::<core::convert::Infallible>(NodeDeath::new(
info.node_ref.node.clone(),
self.clone(),
cookie,
))
.unwrap_or_else(|(err, _)| match err {}),
);

info.death = Some(death.clone());

Expand Down
26 changes: 10 additions & 16 deletions drivers/android/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ use kernel::{
file::{File, PollTable},
io_buffer::{IoBufferReader, IoBufferWriter},
linked_list::{GetLinks, Links, List},
new_condvar, new_spinlock,
prelude::*,
security,
sync::{Arc, CondVar, SpinLock, UniqueArc},
sync::{Arc, CondVar, SpinLock},
user_ptr::{UserSlicePtr, UserSlicePtrWriter},
Either,
};
Expand Down Expand Up @@ -229,10 +230,13 @@ impl InnerThread {
}
}

#[pin_data]
pub(crate) struct Thread {
pub(crate) id: i32,
pub(crate) process: Arc<Process>,
#[pin]
inner: SpinLock<InnerThread>,
#[pin]
work_condvar: CondVar,
links: Links<Thread>,
}
Expand All @@ -241,31 +245,21 @@ impl Thread {
pub(crate) fn new(id: i32, process: Arc<Process>) -> Result<Arc<Self>> {
let return_work = Arc::try_new(ThreadError::new(InnerThread::set_return_work))?;
let reply_work = Arc::try_new(ThreadError::new(InnerThread::set_reply_work))?;
let mut thread = Pin::from(UniqueArc::try_new(Self {
let thread = Arc::pin_init(pin_init!(Self {
id,
process,
// SAFETY: `inner` is initialised in the call to `spinlock_init` below.
inner: unsafe { SpinLock::new(InnerThread::new()) },
// SAFETY: `work_condvar` is initialised in the call to `condvar_init` below.
work_condvar: unsafe { CondVar::new() },
inner: new_spinlock!(InnerThread::new(), "Thread::inner"),
work_condvar: new_condvar!("Thread::work_condvar"),
links: Links::new(),
})?);

// SAFETY: `inner` is pinned when `thread` is.
let inner = unsafe { thread.as_mut().map_unchecked_mut(|t| &mut t.inner) };
kernel::spinlock_init!(inner, "Thread::inner");

// SAFETY: `work_condvar` is pinned when `thread` is.
let condvar = unsafe { thread.as_mut().map_unchecked_mut(|t| &mut t.work_condvar) };
kernel::condvar_init!(condvar, "Thread::work_condvar");
}))?;

{
let mut inner = thread.inner.lock();
inner.set_reply_work(reply_work);
inner.set_return_work(return_work);
}

Ok(thread.into())
Ok(thread)
}

pub(crate) fn set_current_transaction(&self, transaction: Arc<Transaction>) {
Expand Down
41 changes: 17 additions & 24 deletions drivers/android/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use kernel::{
io_buffer::IoBufferWriter,
linked_list::List,
linked_list::{GetLinks, Links},
macros::pinned_drop,
new_spinlock,
prelude::*,
sync::{Arc, SpinLock, UniqueArc},
user_ptr::UserSlicePtrWriter,
Expand All @@ -26,7 +28,9 @@ struct TransactionInner {
file_list: List<Box<FileInfo>>,
}

#[pin_data(PinnedDrop)]
pub(crate) struct Transaction {
#[pin]
inner: SpinLock<TransactionInner>,
// TODO: Node should be released when the buffer is released.
node_ref: Option<NodeRef>,
Expand Down Expand Up @@ -55,26 +59,20 @@ impl Transaction {
let data_address = alloc.ptr;
let file_list = alloc.take_file_list();
alloc.keep_alive();
let mut tr = Pin::from(UniqueArc::try_new(Self {
// SAFETY: `spinlock_init` is called below.
inner: unsafe { SpinLock::new(TransactionInner { file_list }) },
let tr = UniqueArc::pin_init(pin_init!(Self {
inner: new_spinlock!(TransactionInner { file_list }, "Transaction::inner"),
node_ref: Some(node_ref),
stack_next,
from: from.clone(),
to,
code: tr.code,
flags: tr.flags,
data_size: tr.data_size as _,
data_size: tr.data_size as usize,
data_address,
offsets_size: tr.offsets_size as _,
offsets_size: tr.offsets_size as usize,
links: Links::new(),
free_allocation: AtomicBool::new(true),
})?);

// SAFETY: `inner` is pinned when `tr` is.
let pinned = unsafe { tr.as_mut().map_unchecked_mut(|t| &mut t.inner) };
kernel::spinlock_init!(pinned, "Transaction::inner");

}))?;
Ok(tr.into())
}

Expand All @@ -88,26 +86,20 @@ impl Transaction {
let data_address = alloc.ptr;
let file_list = alloc.take_file_list();
alloc.keep_alive();
let mut tr = Pin::from(UniqueArc::try_new(Self {
// SAFETY: `spinlock_init` is called below.
inner: unsafe { SpinLock::new(TransactionInner { file_list }) },
let tr = UniqueArc::pin_init(pin_init!(Self {
inner: new_spinlock!(TransactionInner { file_list }, "Transaction::inner"),
node_ref: None,
stack_next: None,
from: from.clone(),
to,
code: tr.code,
flags: tr.flags,
data_size: tr.data_size as _,
data_size: tr.data_size as usize,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

(To clarify, I'm not going into the question of whether it is better/worse to be explicit about the type, we can discuss that elsewhere. I'm asking if the new macros made things such that the compiler can't infer the types anymore.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is needed, otherwise we get a type inference error (have not investigated why).

data_address,
offsets_size: tr.offsets_size as _,
offsets_size: tr.offsets_size as usize,
links: Links::new(),
free_allocation: AtomicBool::new(true),
})?);

// SAFETY: `inner` is pinned when `tr` is.
let pinned = unsafe { tr.as_mut().map_unchecked_mut(|t| &mut t.inner) };
kernel::spinlock_init!(pinned, "Transaction::inner");

}))?;
Ok(tr.into())
}

Expand Down Expand Up @@ -285,8 +277,9 @@ impl DeliverToRead for Transaction {
}
}

impl Drop for Transaction {
fn drop(&mut self) {
#[pinned_drop]
impl PinnedDrop for Transaction {
fn drop(self: Pin<&mut Self>) {
if self.free_allocation.load(Ordering::Relaxed) {
self.to.buffer_get(self.data_address);
}
Expand Down
Loading