Re: [PATCH v2 00/14] hrtimer Rust API

From: Dirk Behme
Date: Thu Oct 03 2024 - 04:14:35 EST

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:

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 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?

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>(())




Note that
irq-off is needed for the lock, because otherwise we will hit a self
deadlock due to interrupts:

spin_lock(&a);
> timer interrupt
spin_lock(&a);

Also notice that the IrqDisabled<'_> token can be simply created by
::new(), because irq contexts should guarantee interrupt disabled (i.e.
we don't support nested interrupts*).

[*]: I vaguely remember we still have some driver code for slow devices
that will enable interrupts during an irq handler, but these are going
to be gone, we shouldn't really care about this in Rust code.

Regards,
Boqun

[1]: https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@xxxxxxxxxx/



Best regards

Dirk

[1]

# rust_doctest_kernel_hrtimer_rs_0.location: rust/kernel/hrtimer.rs:10
rust_doctests_kernel: Timer called

=============================
[ BUG: Invalid wait context ]
6.11.0-rc1-arm64 #28 Tainted: G N
-----------------------------
swapper/5/0 is trying to lock:
ffff0004409ab900 (rust/doctests_kernel_generated.rs:1238){+.+.}-{3:3}, at:
rust_helper_mutex_lock+0x10/0x18
other info that might help us debug this:
context-{2:2}
no locks held by swapper/5/0.
stack backtrace:
CPU: 5 UID: 0 PID: 0 Comm: swapper/5 Tainted: G N 6.11.0-rc1-arm64 #28
Tainted: [N]=TEST
Hardware name: ARM64 based board (DT)
Call trace:
$x.11+0x98/0xb4
show_stack+0x14/0x1c
$x.3+0x3c/0x94
dump_stack+0x14/0x1c
$x.205+0x538/0x594
$x.179+0xd0/0x18c
__mutex_lock+0xa0/0xa4
mutex_lock_nested+0x20/0x28
rust_helper_mutex_lock+0x10/0x18

_RNvXs_NvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_0NtB4_17ArcIntrusiveTimerNtNtCsclYTRz49wqv_6kernel7hrtimer13TimerCallback3run+0x5c/0xd0

_RNvXs1_NtNtCsclYTRz49wqv_6kernel7hrtimer3arcINtNtNtB9_4sync3arc3ArcNtNvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_017ArcIntrusiveTimerENtB7_16RawTimerCallback3runB1b_+0x20/0x2c
$x.90+0x64/0x70
hrtimer_interrupt+0x1d4/0x2ac
arch_timer_handler_phys+0x34/0x40
$x.62+0x50/0x54
generic_handle_domain_irq+0x28/0x40
$x.154+0x58/0x6c
$x.471+0x10/0x20
el1_interrupt+0x70/0x94
el1h_64_irq_handler+0x14/0x1c
el1h_64_irq+0x64/0x68
arch_local_irq_enable+0x4/0x8
cpuidle_enter+0x34/0x48
$x.37+0x58/0xe4
cpu_startup_entry+0x30/0x34
$x.2+0xf8/0x118
$x.13+0x0/0x4
rust_doctests_kernel: Timer called
rust_doctests_kernel: Timer called
rust_doctests_kernel: Timer called
rust_doctests_kernel: Timer called
rust_doctests_kernel: Counted to 5
ok 22 rust_doctest_kernel_hrtimer_rs_0
# rust_doctest_kernel_hrtimer_rs_1.location: rust/kernel/hrtimer.rs:137
rust_doctests_kernel: Hello from the future
rust_doctests_kernel: Flag raised
ok 23 rust_doctest_kernel_hrtimer_rs_1
# rust_doctest_kernel_hrtimer_rs_2.location: rust/kernel/hrtimer.rs:76
rust_doctests_kernel: Timer called
rust_doctests_kernel: Flag raised
ok 24 rust_doctest_kernel_hrtimer_rs_2