Re: [PATCH v2 00/14] hrtimer Rust API
From: Dirk Behme
Date: Thu Oct 03 2024 - 12:19:58 EST
Am 03.10.24 um 15:03 schrieb Boqun Feng:
On Thu, Oct 03, 2024 at 10:14:17AM +0200, Dirk Behme wrote:
Am 01.10.24 um 16:42 schrieb Boqun Feng:
On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
On 18.09.2024 00:27, Andreas Hindborg wrote:
Hi!
This series adds support for using the `hrtimer` subsystem from Rust code.
I tried breaking up the code in some smaller patches, hopefully that will
ease the review process a bit.
Just fyi, having all 14 patches applied I get [1] on the first (doctest)
Example from hrtimer.rs.
This is from lockdep:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
Having just a quick look I'm not sure what the root cause is. Maybe mutex in
interrupt context? Or a more subtle one?
I think it's calling mutex inside an interrupt context as shown by the
callstack:
] __mutex_lock+0xa0/0xa4
] ...
] hrtimer_interrupt+0x1d4/0x2ac
, it is because:
+//! struct ArcIntrusiveTimer {
+//! #[pin]
+//! timer: Timer<Self>,
+//! #[pin]
+//! flag: Mutex<bool>,
+//! #[pin]
+//! cond: CondVar,
+//! }
has a Mutex<bool>, which actually should be a SpinLockIrq [1].
Two understanding questions:
Good questions. ;-)
:-)
1. In the main thread (full example for reference below [2]) where is the
lock released? After the while loop? I.e. is the lock held until guard
With the current implementation, there are two places the lock will be
released: 1) inside CondVar::wait() and
CondVar::wait() releases *and* reaquires, the lock then? So that
outside of CondVar::wait() but inside the while() loop the lock is
held until the while loop is exit?
Would that lock handling inside CondVar::wait() handle the irq stuff
(irq enable and disable) of SpinLockIrq correctly, then?
2) after `guard` is eventually
drop after the loop.
reaches 5?
let mut guard = has_timer.flag.lock(); // <= lock taken here?
while *guard != 5 {
has_timer.cond.wait(&mut guard);
} // <= lock
released here?
I wonder what this would mean for the interrupt TimerCallback in case we
would use an irq-off SpinLock instead here?
Or maybe:
2. The only place where the guard is modified (*guard += 1;) is in the
TimerCallback which runs in interrupt context as we learned. With that
writing the guard value can't be interrupted. Couldn't we drop the whole
lock, then?
No, because the main thread can run on another CPU, so disabling
interrupts (because of the interrupt handlers) doesn't mean exclusive
access to value.
Yes. I agree if the main thread would write. But that main thread does
read-only accesses, only? So it reads either the old or the new value,
indepenent on the locking? Only the interrupt handler does
read/modify/write. But thats protected by the interrupt context, already.
Dirk
Best regards
Dirk
[2]
//! #[pin_data]
//! struct ArcIntrusiveTimer {
//! #[pin]
//! timer: Timer<Self>,
//! #[pin]
//! flag: Mutex<u64>,
//! #[pin]
//! cond: CondVar,
//! }
//!
//! impl ArcIntrusiveTimer {
//! fn new() -> impl PinInit<Self, kernel::error::Error> {
//! try_pin_init!(Self {
//! timer <- Timer::new(),
//! flag <- new_mutex!(0),
//! cond <- new_condvar!(),
//! })
//! }
//! }
//!
//! impl TimerCallback for ArcIntrusiveTimer {
//! type CallbackTarget<'a> = Arc<Self>;
//! type CallbackPointer<'a> = Arc<Self>;
//!
//! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
//! pr_info!("Timer called\n");
//! let mut guard = this.flag.lock();
//! *guard += 1;
//! this.cond.notify_all();
//! if *guard == 5 {
//! TimerRestart::NoRestart
//! }
//! else {
//! TimerRestart::Restart
//!
//! }
//! }
//! }
//!
//! impl_has_timer! {
//! impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
//! }
//!
//!
//! let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
//! let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
//! let mut guard = has_timer.flag.lock();
//!
//! while *guard != 5 {
//! has_timer.cond.wait(&mut guard);
//! }
//!
//! pr_info!("Counted to 5\n");
//! # Ok::<(), kernel::error::Error>(())
[...]