Message-ID: <ZDbBsBIZwMhbquRG@Boquns-Mac-mini.local>
Date:   Wed, 12 Apr 2023 07:35:28 -0700
From:   Boqun Feng <boqun.feng@...il.com>
To:     Wedson Almeida Filho <wedsonaf@...il.com>
Cc:     rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        linux-kernel@...r.kernel.org,
        Wedson Almeida Filho <walmeida@...rosoft.com>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>
Subject: Re: [PATCH v4 11/13] rust: lock: add `Guard::do_unlocked`

On Wed, Apr 12, 2023 at 08:07:40AM -0300, Wedson Almeida Filho wrote:
> On Wed, 12 Apr 2023 at 03:25, Boqun Feng <boqun.feng@...il.com> wrote:
> >
> > On Tue, Apr 11, 2023 at 02:45:41AM -0300, Wedson Almeida Filho wrote:
> > [...]
> > > +
> > > +    unsafe fn relock(ptr: *mut Self::State, guard_state: &mut Self::GuardState) {
> > > +        let _ = match guard_state {
> > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > +            // initialised.
> > > +            None => unsafe { Self::lock(ptr) },
> > > +            // SAFETY: The safety requiments of this function ensure that `ptr` has been
> > > +            // initialised.
> > > +            Some(_) => unsafe { Self::lock_irqsave(ptr) },
> > > +        };
> > > +    }
> > >  }
> > >
> >
> > One thing I'm little worried about the above is that we don't store back
> > the new GuardState into `guard_state`, the particular case I'm worried
> > about is as follow:
> >
> >         // IRQ is enabled.
> >         // Disabling IRQ
> >         unsafe { bindings::local_irq_disable(); }
> >
> >         let mut g = unsafe { SpinLockBackend::lock(&mut lock as *mut _) };
> >         // `g` records irq state is "irq disabled"
> >
> >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> >         // restore into "irq disabled" mode.
> >         // IRQ is disabled.
> >
> >         // Enabling IRQ
> >         unsafe { bindings::local_irq_enable(); }
> >         // IRQ is enabled.
> >
> >         unsafe { SpinLockBackend::relock(&mut lock as *mut _, &mut g) }
> >         // `g` still records irq state is "irq disabled"
> 
> Yes, that's by design. If you want it to record the new "irq enabled"
> state, then you should call `lock()`, not `relock()`.
> 
> >         unsafe { SpinLockBackend::unlock(&mut lock as *mut _, &g); }
> >         // restore into "irq disabled" mode.
> >         // IRQ is disabled.
> >
> >
> > This looks pretty scary to me, I would expect `relock()` updates the
> > latest GuardState to the guard. Any reason it's implemented this way?
> 
> A `relock()` followed by an `unlock()` takes the state back to how it
> was when `lock()` was originally called: this is precisely why
> `relock()` exists.
> 
> Consider the following case:
> 
> ```
> local_disable_irq();
> let mut guard = spinlock.lock();

I think you meant `spinlock.lock_irqsave()` here, right?

> 
> guard.do_unlocked(|| {
>     local_irq_enable();
>     schedule();
> });
> 
> drop(guard);
> ```
> 
> What would you expect the state to be? It's meant to be the state

I understand your point but I would expect people to code like:

```
local_disable_irq();
let mut guard = spinlock.lock(); // or lock_irqsave(), doesn't matter

guard.do_unlocked(|| {
    local_irq_enable();
    schedule();
    local_irq_disable();
});

drop(guard);
```

And the closure in do_unlocked() can also be something like:
```
	guard.do_unlocked(|| {
	    local_irq_enabled();
	    let _g = ScopeGuard::new(|| {
	        local_irq_disabled();
	    });

	    schedule();

	    if (some_cond) {
	    	return; // return early
	    }

	    if (other_cond) {
	    	return;
	    }
	})

```

One benefit (other that code looks symmetric) is we can use the same
closure in other place. Also it helps klint since we keep the irq
enablement state change as local as possible: we can go ahead and
require irq enabled state should not be changed between the closure in
do_unlock().

Maybe I'm missing something, but the current `relock` semantics is
really tricky to get ;-)

Regards,
Boqun

> right before `spinlock.lock()` was called, that's what the guard
> represents.
> 
> If you want to preserve a new state, then you don't want `relock()`,
> you just want a new `lock()` call.
> 
> > Regards,
> > Boqun
> >
> > >  // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. We use the `irqsave`
> > >  // variant of the C lock acquisition functions to disable interrupts and retrieve the original
> > >  // interrupt state, and the `irqrestore` variant of the lock release functions to restore the state
> > >  // in `unlock` -- we use the guard context to determine which method was used to acquire the lock.
> > > -unsafe impl super::IrqSaveBackend for SpinLockBackend {
> > > +unsafe impl IrqSaveBackend for SpinLockBackend {
> > >      unsafe fn lock_irqsave(ptr: *mut Self::State) -> Self::GuardState {
> > >          // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
> > >          // memory, and that it has been initialised before.
> > > --
> > > 2.34.1
> > >

Powered by blists - more mailing lists