Skip to content

Commit f090f0d

Browse files
Darksonnojeda
authored andcommitted
rust: sync: update integer types in CondVar
Reduce the chances of compilation failures due to integer type mismatches in `CondVar`. When an integer is defined using a #define in C, bindgen doesn't know which integer type it is supposed to be, so it will just use `u32` by default (if it fits in an u32). Whenever the right type is something else, we insert a cast in Rust. However, this means that the code has a lot of extra casts, and sometimes the code will be missing casts if u32 happens to be correct on the developer's machine, even though the type might be something else on a different platform. This patch updates all uses of such constants in `rust/kernel/sync/condvar.rs` to use constants defined with the right type. This allows us to remove various unnecessary casts, while also future-proofing for the case where `unsigned int != u32` (even though that is unlikely to ever happen in the kernel). I wrote this patch at the suggestion of Benno in [1]. Link: https://lore.kernel.org/all/nAEg-6vbtX72ZY3oirDhrSEf06TBWmMiTt73EklMzEAzN4FD4mF3TPEyAOxBZgZtjzoiaBYtYr3s8sa9wp1uYH9vEWRf2M-Lf4I0BY9rAgk=@proton.me/ [1] Suggested-by: Benno Lossin <benno.lossin@proton.me> Reviewed-by: Tiago Lam <tiagolam@gmail.com> Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Reviewed-by: Benno Lossin <benno.lossin@proton.me> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Alice Ryhl <aliceryhl@google.com> Link: https://lore.kernel.org/r/20240108-rb-new-condvar-methods-v4-4-88e0c871cc05@google.com [ Added note on the unlikeliness of `sizeof(int)` changing. ] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
1 parent e7b9b1f commit f090f0d

File tree

2 files changed

+34
-19
lines changed

2 files changed

+34
-19
lines changed

rust/kernel/sync/condvar.rs

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,17 @@
77
88
use super::{lock::Backend, lock::Guard, LockClassKey};
99
use crate::{
10-
bindings, init::PinInit, pin_init, str::CStr, task::MAX_SCHEDULE_TIMEOUT, time::Jiffies,
10+
bindings,
11+
init::PinInit,
12+
pin_init,
13+
str::CStr,
14+
task::{MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE, TASK_NORMAL, TASK_UNINTERRUPTIBLE},
15+
time::Jiffies,
1116
types::Opaque,
1217
};
13-
use core::ffi::c_long;
18+
use core::ffi::{c_int, c_long};
1419
use core::marker::PhantomPinned;
20+
use core::ptr;
1521
use macros::pin_data;
1622

1723
/// Creates a [`CondVar`] initialiser with the given name and a newly-created lock class.
@@ -108,7 +114,7 @@ impl CondVar {
108114

109115
fn wait_internal<T: ?Sized, B: Backend>(
110116
&self,
111-
wait_state: u32,
117+
wait_state: c_int,
112118
guard: &mut Guard<'_, T, B>,
113119
timeout_in_jiffies: c_long,
114120
) -> c_long {
@@ -119,11 +125,7 @@ impl CondVar {
119125

120126
// SAFETY: Both `wait` and `wait_queue_head` point to valid memory.
121127
unsafe {
122-
bindings::prepare_to_wait_exclusive(
123-
self.wait_queue_head.get(),
124-
wait.get(),
125-
wait_state as _,
126-
)
128+
bindings::prepare_to_wait_exclusive(self.wait_queue_head.get(), wait.get(), wait_state)
127129
};
128130

129131
// SAFETY: Switches to another thread. The timeout can be any number.
@@ -142,7 +144,7 @@ impl CondVar {
142144
/// [`CondVar::notify_one`] or [`CondVar::notify_all`]. Note that it may also wake up
143145
/// spuriously.
144146
pub fn wait<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) {
145-
self.wait_internal(bindings::TASK_UNINTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
147+
self.wait_internal(TASK_UNINTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
146148
}
147149

148150
/// Releases the lock and waits for a notification in interruptible mode.
@@ -153,7 +155,7 @@ impl CondVar {
153155
/// Returns whether there is a signal pending.
154156
#[must_use = "wait_interruptible returns if a signal is pending, so the caller must check the return value"]
155157
pub fn wait_interruptible<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T, B>) -> bool {
156-
self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
158+
self.wait_internal(TASK_INTERRUPTIBLE, guard, MAX_SCHEDULE_TIMEOUT);
157159
crate::current!().signal_pending()
158160
}
159161

@@ -169,7 +171,7 @@ impl CondVar {
169171
jiffies: Jiffies,
170172
) -> CondVarTimeoutResult {
171173
let jiffies = jiffies.try_into().unwrap_or(MAX_SCHEDULE_TIMEOUT);
172-
let res = self.wait_internal(bindings::TASK_INTERRUPTIBLE, guard, jiffies);
174+
let res = self.wait_internal(TASK_INTERRUPTIBLE, guard, jiffies);
173175

174176
match (res as Jiffies, crate::current!().signal_pending()) {
175177
(jiffies, true) => CondVarTimeoutResult::Signal { jiffies },
@@ -178,15 +180,15 @@ impl CondVar {
178180
}
179181
}
180182

181-
/// Calls the kernel function to notify the appropriate number of threads with the given flags.
182-
fn notify(&self, count: i32, flags: u32) {
183+
/// Calls the kernel function to notify the appropriate number of threads.
184+
fn notify(&self, count: c_int) {
183185
// SAFETY: `wait_queue_head` points to valid memory.
184186
unsafe {
185187
bindings::__wake_up(
186188
self.wait_queue_head.get(),
187-
bindings::TASK_NORMAL,
189+
TASK_NORMAL,
188190
count,
189-
flags as _,
191+
ptr::null_mut(),
190192
)
191193
};
192194
}
@@ -198,23 +200,23 @@ impl CondVar {
198200
/// CPU.
199201
pub fn notify_sync(&self) {
200202
// SAFETY: `wait_queue_head` points to valid memory.
201-
unsafe { bindings::__wake_up_sync(self.wait_queue_head.get(), bindings::TASK_NORMAL) };
203+
unsafe { bindings::__wake_up_sync(self.wait_queue_head.get(), TASK_NORMAL) };
202204
}
203205

204206
/// Wakes a single waiter up, if any.
205207
///
206208
/// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
207209
/// completely (as opposed to automatically waking up the next waiter).
208210
pub fn notify_one(&self) {
209-
self.notify(1, 0);
211+
self.notify(1);
210212
}
211213

212214
/// Wakes all waiters up, if any.
213215
///
214216
/// This is not 'sticky' in the sense that if no thread is waiting, the notification is lost
215217
/// completely (as opposed to automatically waking up the next waiter).
216218
pub fn notify_all(&self) {
217-
self.notify(0, 0);
219+
self.notify(0);
218220
}
219221
}
220222

rust/kernel/task.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,24 @@
55
//! C header: [`include/linux/sched.h`](srctree/include/linux/sched.h).
66
77
use crate::{bindings, types::Opaque};
8-
use core::{ffi::c_long, marker::PhantomData, ops::Deref, ptr};
8+
use core::{
9+
ffi::{c_int, c_long, c_uint},
10+
marker::PhantomData,
11+
ops::Deref,
12+
ptr,
13+
};
914

1015
/// A sentinel value used for infinite timeouts.
1116
pub const MAX_SCHEDULE_TIMEOUT: c_long = c_long::MAX;
1217

18+
/// Bitmask for tasks that are sleeping in an interruptible state.
19+
pub const TASK_INTERRUPTIBLE: c_int = bindings::TASK_INTERRUPTIBLE as c_int;
20+
/// Bitmask for tasks that are sleeping in an uninterruptible state.
21+
pub const TASK_UNINTERRUPTIBLE: c_int = bindings::TASK_UNINTERRUPTIBLE as c_int;
22+
/// Convenience constant for waking up tasks regardless of whether they are in interruptible or
23+
/// uninterruptible sleep.
24+
pub const TASK_NORMAL: c_uint = bindings::TASK_NORMAL as c_uint;
25+
1326
/// Returns the currently running task.
1427
#[macro_export]
1528
macro_rules! current {

0 commit comments

Comments
 (0)