[PATCH] rust: sync: add wait_interruptible_freezable

[PATCH] rust: sync: add wait_interruptible_freezable

[Alice Ryhl]

Binder allows you to freeze a process where some of its threads are
blocked on the Binder driver. To make this work, we need to pass
TASK_FREEZABLE when going to sleep in the appropriate places. Thus, add
a new method wait_interruptible_freezable for the condition variable so
that sleeps where this is supported can be marked as such.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/condvar.rs | 18 +++++++++++++++++-
 rust/kernel/task.rs         |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index 7df565038d7d..a256cd74bd4b 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -11,7 +11,9 @@
     init::PinInit,
     pin_init,
     str::CStr,
-    task::{MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE, TASK_NORMAL, TASK_UNINTERRUPTIBLE},
+    task::{
+        MAX_SCHEDULE_TIMEOUT, TASK_FREEZABLE, TASK_INTERRUPTIBLE, TASK_NORMAL, TASK_UNINTERRUPTIBLE,
+    },
     time::Jiffies,
     types::Opaque,
 };
@@ -159,6 +161,20 @@ pub fn wait_interruptible<T: ?Sized, B: Backend>(&self, guard: &mut Guard<'_, T,
         crate::current!().signal_pending()
     }
 
+    /// Releases the lock and waits for a notification in interruptible and freezable mode.
+    #[must_use = "wait returns if a signal is pending, so the caller must check the return value"]
+    pub fn wait_interruptible_freezable<T: ?Sized, B: Backend>(
+        &self,
+        guard: &mut Guard<'_, T, B>,
+    ) -> bool {
+        self.wait_internal(
+            TASK_INTERRUPTIBLE | TASK_FREEZABLE,
+            guard,
+            MAX_SCHEDULE_TIMEOUT,
+        );
+        crate::current!().signal_pending()
+    }
+
     /// Releases the lock and waits for a notification in interruptible mode.
     ///
     /// Atomically releases the given lock (whose ownership is proven by the guard) and puts the
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 07bc22a7645c..ea43a3b8d9c5 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -23,6 +23,8 @@
 pub const TASK_INTERRUPTIBLE: c_int = bindings::TASK_INTERRUPTIBLE as c_int;
 /// Bitmask for tasks that are sleeping in an uninterruptible state.
 pub const TASK_UNINTERRUPTIBLE: c_int = bindings::TASK_UNINTERRUPTIBLE as c_int;
+/// Bitmask for tasks that are sleeping in a freezable state.
+pub const TASK_FREEZABLE: c_int = bindings::TASK_FREEZABLE as c_int;
 /// Convenience constant for waking up tasks regardless of whether they are in interruptible or
 /// uninterruptible sleep.
 pub const TASK_NORMAL: c_uint = bindings::TASK_NORMAL as c_uint;

---
base-commit: ceff0757f5dafb5be5205988171809c877b1d3e3
change-id: 20250130-condvar-freeze-f2ea5b308405

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust: sync: add wait_interruptible_freezable

[Peter Zijlstra]

On Thu, Jan 30, 2025 at 11:30:44AM +0000, Alice Ryhl wrote:
> Binder allows you to freeze a process where some of its threads are
> blocked on the Binder driver. To make this work, we need to pass
> TASK_FREEZABLE when going to sleep in the appropriate places. Thus, add
> a new method wait_interruptible_freezable for the condition variable so
> that sleeps where this is supported can be marked as such.

The constraint on freezable is that you must not hold locks. There is a
lockdep check for this in the code, but it would probably make sense to
teach Rust about this constraint as well, hmm?

Re: [PATCH] rust: sync: add wait_interruptible_freezable

[Alice Ryhl]

On Mon, Feb 3, 2025 at 12:54 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jan 30, 2025 at 11:30:44AM +0000, Alice Ryhl wrote:
> > Binder allows you to freeze a process where some of its threads are
> > blocked on the Binder driver. To make this work, we need to pass
> > TASK_FREEZABLE when going to sleep in the appropriate places. Thus, add
> > a new method wait_interruptible_freezable for the condition variable so
> > that sleeps where this is supported can be marked as such.
>
> The constraint on freezable is that you must not hold locks. There is a
> lockdep check for this in the code, but it would probably make sense to
> teach Rust about this constraint as well, hmm?

Unfortunately, I don't think there's any way to enforce this at
compile time, but I'm definitely happy to add this in the
documentation.

Thanks!
Alice

Re: [PATCH] rust: sync: add wait_interruptible_freezable

[Peter Zijlstra]

On Mon, Feb 03, 2025 at 12:56:05PM +0100, Alice Ryhl wrote:
> On Mon, Feb 3, 2025 at 12:54 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Thu, Jan 30, 2025 at 11:30:44AM +0000, Alice Ryhl wrote:
> > > Binder allows you to freeze a process where some of its threads are
> > > blocked on the Binder driver. To make this work, we need to pass
> > > TASK_FREEZABLE when going to sleep in the appropriate places. Thus, add
> > > a new method wait_interruptible_freezable for the condition variable so
> > > that sleeps where this is supported can be marked as such.
> >
> > The constraint on freezable is that you must not hold locks. There is a
> > lockdep check for this in the code, but it would probably make sense to
> > teach Rust about this constraint as well, hmm?
> 
> Unfortunately, I don't think there's any way to enforce this at
> compile time, but I'm definitely happy to add this in the
> documentation.

Ah, ISTR people talking about teaching Rust about the whole raw_spinlock
vs spinlock vs mutex nesting order and figured if it can do that, then
this should be doable too.

But perhaps that never quite happened.

Yes, documentation would be good. Just in case it isn't obviuos,
freezing a task that holds a lock can trivially deadlock vs another task
that needs that lock to complete before it too can hit freezable.

Re: [PATCH] rust: sync: add wait_interruptible_freezable

[Alice Ryhl]

On Mon, Feb 3, 2025 at 2:38 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Feb 03, 2025 at 12:56:05PM +0100, Alice Ryhl wrote:
> > On Mon, Feb 3, 2025 at 12:54 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Thu, Jan 30, 2025 at 11:30:44AM +0000, Alice Ryhl wrote:
> > > > Binder allows you to freeze a process where some of its threads are
> > > > blocked on the Binder driver. To make this work, we need to pass
> > > > TASK_FREEZABLE when going to sleep in the appropriate places. Thus, add
> > > > a new method wait_interruptible_freezable for the condition variable so
> > > > that sleeps where this is supported can be marked as such.
> > >
> > > The constraint on freezable is that you must not hold locks. There is a
> > > lockdep check for this in the code, but it would probably make sense to
> > > teach Rust about this constraint as well, hmm?
> >
> > Unfortunately, I don't think there's any way to enforce this at
> > compile time, but I'm definitely happy to add this in the
> > documentation.
>
> Ah, ISTR people talking about teaching Rust about the whole raw_spinlock
> vs spinlock vs mutex nesting order and figured if it can do that, then
> this should be doable too.
>
> But perhaps that never quite happened.

There isn't too much progress on that front lately, but you are right
that this work could be extended to support this case too.

> Yes, documentation would be good. Just in case it isn't obviuos,
> freezing a task that holds a lock can trivially deadlock vs another task
> that needs that lock to complete before it too can hit freezable.

I'll include those details, thanks!

Alice

Re: [PATCH] rust: sync: add wait_interruptible_freezable

[Boqun Feng]

On Mon, Feb 03, 2025 at 02:41:37PM +0100, Alice Ryhl wrote:
> On Mon, Feb 3, 2025 at 2:38 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Feb 03, 2025 at 12:56:05PM +0100, Alice Ryhl wrote:
> > > On Mon, Feb 3, 2025 at 12:54 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > On Thu, Jan 30, 2025 at 11:30:44AM +0000, Alice Ryhl wrote:
> > > > > Binder allows you to freeze a process where some of its threads are
> > > > > blocked on the Binder driver. To make this work, we need to pass
> > > > > TASK_FREEZABLE when going to sleep in the appropriate places. Thus, add
> > > > > a new method wait_interruptible_freezable for the condition variable so
> > > > > that sleeps where this is supported can be marked as such.
> > > >
> > > > The constraint on freezable is that you must not hold locks. There is a
> > > > lockdep check for this in the code, but it would probably make sense to
> > > > teach Rust about this constraint as well, hmm?
> > >
> > > Unfortunately, I don't think there's any way to enforce this at
> > > compile time, but I'm definitely happy to add this in the
> > > documentation.
> >
> > Ah, ISTR people talking about teaching Rust about the whole raw_spinlock
> > vs spinlock vs mutex nesting order and figured if it can do that, then

Peter, are you talking about the POC idea I proposed on tracking irqsave
status:

	https://lore.kernel.org/rust-for-linux/20241018055125.2784186-1-boqun.feng@gmail.com/

? I'm working on this right now, however, I don't think this would help
spinlock or mutex nesting? Because there's no global(percpu) status of
acquiring these locks. Am I missing something here?

Regards,
Boqun

> > this should be doable too.
> >
> > But perhaps that never quite happened.
> 
> There isn't too much progress on that front lately, but you are right
> that this work could be extended to support this case too.
> 
> > Yes, documentation would be good. Just in case it isn't obviuos,
> > freezing a task that holds a lock can trivially deadlock vs another task
> > that needs that lock to complete before it too can hit freezable.
> 
> I'll include those details, thanks!
> 
> Alice

Re: [PATCH] rust: sync: add wait_interruptible_freezable

[Alice Ryhl]

On Mon, Feb 3, 2025 at 3:30 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Feb 03, 2025 at 02:41:37PM +0100, Alice Ryhl wrote:
> > On Mon, Feb 3, 2025 at 2:38 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Mon, Feb 03, 2025 at 12:56:05PM +0100, Alice Ryhl wrote:
> > > > On Mon, Feb 3, 2025 at 12:54 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > > > >
> > > > > On Thu, Jan 30, 2025 at 11:30:44AM +0000, Alice Ryhl wrote:
> > > > > > Binder allows you to freeze a process where some of its threads are
> > > > > > blocked on the Binder driver. To make this work, we need to pass
> > > > > > TASK_FREEZABLE when going to sleep in the appropriate places. Thus, add
> > > > > > a new method wait_interruptible_freezable for the condition variable so
> > > > > > that sleeps where this is supported can be marked as such.
> > > > >
> > > > > The constraint on freezable is that you must not hold locks. There is a
> > > > > lockdep check for this in the code, but it would probably make sense to
> > > > > teach Rust about this constraint as well, hmm?
> > > >
> > > > Unfortunately, I don't think there's any way to enforce this at
> > > > compile time, but I'm definitely happy to add this in the
> > > > documentation.
> > >
> > > Ah, ISTR people talking about teaching Rust about the whole raw_spinlock
> > > vs spinlock vs mutex nesting order and figured if it can do that, then
>
> Peter, are you talking about the POC idea I proposed on tracking irqsave
> status:
>
>         https://lore.kernel.org/rust-for-linux/20241018055125.2784186-1-boqun.feng@gmail.com/
>
> ? I'm working on this right now, however, I don't think this would help
> spinlock or mutex nesting? Because there's no global(percpu) status of
> acquiring these locks. Am I missing something here?

I assumed that Peter was talking about klint, but I don't know.

Alice