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

[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 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.

Regards,
Boqun

> 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>(())
> 
> 
> 
[...]