[PATCH v6 0/2] Avoid synchronize_rcu() for every thread drop in Rust Binder

[PATCH v6 0/2] Avoid synchronize_rcu() for every thread drop in Rust Binder

[Alice Ryhl]

Right now Rust Binder calls synchronize_rcu() more often than is
necessary. Most processes do not use epoll at all, so they don't require
rcu here. Back in Kangrejos I came up with a way to avoid this. Idea is
to move the value that needs rcu to a separate allocation that's easy to
kfree_rcu(). We pay the allocation only when the proc uses epoll using
an "upgrade" strategy - most processes don't.

Based on top of:
https://lore.kernel.org/rust-for-linux/20260707-binder-noderefs-spin-v4-0-7c3c8bc16339@google.com/

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v6:
- Rebase on v7.2-rc2 and node_ref spinlock change.
- Fix conflict with commit 77bfebf11077 ("rust_binder: fix BINDER_GET_EXTENDED_ERROR")
- Link to v5: https://lore.kernel.org/r/20260611-upgrade-poll-v5-0-497a11c06828@google.com

Changes in v5:
- Rebase on top of series converting node_refs to spinlock.
- Reword comment claiming node_refs is a mutex.
- Link to v4: https://lore.kernel.org/r/20260523-upgrade-poll-v4-0-f5b4c747eac2@google.com

Changes in v4:
- Use SetOnce for PollCondVar variable instead of storing inside
  spinlock. This avoids calling poll_wait() under the spinlock.
- Link to v3: https://lore.kernel.org/r/20260508-upgrade-poll-v3-0-0c619fe846e8@google.com

Changes in v3:
- This series was almost entirely rewritten to use a different
  implementation strategy. By moving the PollCondVar to the process we
  can avoid the upgrade logic entirely.
- Link to v2: https://lore.kernel.org/r/20260213-upgrade-poll-v2-0-984a0fb184fb@google.com

Changes in v2:
- Change how Rust Binder handles the lock class key.
- Rebase.
- Link to v1: https://lore.kernel.org/r/20260117-upgrade-poll-v1-0-179437b7bd49@google.com

---
Alice Ryhl (2):
      rust: poll: use kfree_rcu() for PollCondVar
      rust_binder: move (e)poll wait queue to Process

 drivers/android/binder/node.rs        |  4 +-
 drivers/android/binder/process.rs     | 67 ++++++++++++++++++++++--------
 drivers/android/binder/thread.rs      | 78 +++++++++++++++++------------------
 drivers/android/binder/transaction.rs |  6 ++-
 rust/kernel/sync/poll.rs              | 73 +++++++++++++++++++++++++++++++-
 5 files changed, 167 insertions(+), 61 deletions(-)
---
base-commit: 0e77a935cf137e791c672d21bb80441043017ff2
change-id: 20260117-upgrade-poll-37ee2a7a79dd

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

[PATCH v6 1/2] rust: poll: use kfree_rcu() for PollCondVar

[Alice Ryhl]

Rust Binder currently uses PollCondVar, but it calls synchronize_rcu()
in the destructor, which we would like to avoid. Add a variation of
PollCondVar that kfree_rcu() instead.

One could avoid the `rcu` field and allocate the rcu_head on drop using
a fallback to synchronize_rcu() on ENOMEM. However, I'd prefer to avoid
the potential for synchronize_rcu(), and Binder will only use this for a
small fraction of processes, so even if it changes which kmalloc bucket
it falls into, the extra memory is not a problem.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/poll.rs | 73 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/poll.rs b/rust/kernel/sync/poll.rs
index 0ec985d560c8..684dfa242b1a 100644
--- a/rust/kernel/sync/poll.rs
+++ b/rust/kernel/sync/poll.rs
@@ -5,12 +5,18 @@
 //! Utilities for working with `struct poll_table`.
 
 use crate::{
+    alloc::AllocError,
     bindings,
     fs::File,
     prelude::*,
     sync::{CondVar, LockClassKey},
+    types::Opaque, //
+};
+use core::{
+    marker::PhantomData,
+    mem::ManuallyDrop,
+    ops::Deref, //
 };
-use core::{marker::PhantomData, ops::Deref};
 
 /// Creates a [`PollCondVar`] initialiser with the given name and a newly-created lock class.
 #[macro_export]
@@ -66,6 +72,7 @@ pub fn register_wait(&self, file: &File, cv: &PollCondVar) {
 ///
 /// [`CondVar`]: crate::sync::CondVar
 #[pin_data(PinnedDrop)]
+#[repr(transparent)]
 pub struct PollCondVar {
     #[pin]
     inner: CondVar,
@@ -104,3 +111,67 @@ fn drop(self: Pin<&mut Self>) {
         unsafe { bindings::synchronize_rcu() };
     }
 }
+
+/// A [`KBox<PollCondVar>`] that uses `kfree_rcu`.
+///
+/// [`KBox<PollCondVar>`]: PollCondVar
+pub struct PollCondVarBox {
+    inner: ManuallyDrop<Pin<KBox<PollCondVarBoxInner>>>,
+}
+
+#[pin_data]
+#[repr(C)]
+struct PollCondVarBoxInner {
+    #[pin]
+    inner: PollCondVar,
+    rcu: Opaque<bindings::callback_head>,
+}
+
+// SAFETY: PollCondVar is Send
+unsafe impl Send for PollCondVarBoxInner {}
+// SAFETY: PollCondVar is Sync
+unsafe impl Sync for PollCondVarBoxInner {}
+
+impl PollCondVarBox {
+    /// Constructs a new boxed [`PollCondVar`].
+    pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) -> Result<Self, AllocError> {
+        let b = KBox::pin_init(
+            pin_init!(PollCondVarBoxInner {
+                inner <- PollCondVar::new(name, key),
+                rcu: Opaque::uninit(),
+            }),
+            GFP_KERNEL,
+        )
+        .map_err(|_| AllocError)?;
+
+        Ok(PollCondVarBox {
+            inner: ManuallyDrop::new(b),
+        })
+    }
+}
+
+impl Deref for PollCondVarBox {
+    type Target = PollCondVar;
+    fn deref(&self) -> &PollCondVar {
+        &self.inner.inner
+    }
+}
+
+impl Drop for PollCondVarBox {
+    #[inline]
+    fn drop(&mut self) {
+        // SAFETY: ManuallyDrop::take ok because not already taken.
+        let boxed = unsafe { ManuallyDrop::take(&mut self.inner) };
+
+        // SAFETY: The code below frees the box without calling the actual destructor of the type,
+        // but it's okay because it re-implements the destructor using `kfree_rcu()` in place of
+        // `synchronize_rcu()`.
+        let ptr = KBox::into_raw(unsafe { Pin::into_inner_unchecked(boxed) });
+
+        // SAFETY: The pointer points at a valid `wait_queue_head`.
+        unsafe { bindings::__wake_up_pollfree((*ptr).inner.inner.wait_queue_head.get()) };
+
+        // SAFETY: This was allocated using `KBox::pin_init`, so it can be freed with `kvfree`.
+        unsafe { bindings::kvfree_call_rcu((*ptr).rcu.get(), ptr.cast::<ffi::c_void>()) };
+    }
+}

-- 
2.55.0.rc2.803.g1fd1e6609c-goog

[PATCH v6 2/2] rust_binder: move (e)poll wait queue to Process

[Alice Ryhl]

Most processes do not use Rust Binder with epoll, so avoid paying the
synchronize_rcu() cost in drop for those that don't need it. For those
that do, we also manage to replace synchronize_rcu() with kfree_rcu(),
though we introduce an extra allocation.

In case the last ref to an Arc<Thread> is dropped outside of
deferred_release(), this also ensures that synchronize_rcu() is not
called in destructor of Arc<Thread> in other places. Theoretically that
could lead to jank by making other syscalls slow, which would be
problematic.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/node.rs        |  4 +-
 drivers/android/binder/process.rs     | 67 ++++++++++++++++++++++--------
 drivers/android/binder/thread.rs      | 78 +++++++++++++++++------------------
 drivers/android/binder/transaction.rs |  6 ++-
 4 files changed, 95 insertions(+), 60 deletions(-)

diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
index 59c5ab747bf4..2b0618938a0a 100644
--- a/drivers/android/binder/node.rs
+++ b/drivers/android/binder/node.rs
@@ -538,7 +538,7 @@ pub(crate) fn submit_oneway(
             inner.oneway_todo.push_back(transaction);
         } else {
             inner.has_oneway_transaction = true;
-            guard.push_work(transaction)?;
+            guard.push_work(&self.owner, transaction)?;
         }
         Ok(())
     }
@@ -570,7 +570,7 @@ pub(crate) fn pending_oneway_finished(&self) {
         let transaction = inner.oneway_todo.pop_front();
         inner.has_oneway_transaction = transaction.is_some();
         if let Some(transaction) = transaction {
-            match guard.push_work(transaction) {
+            match guard.push_work(&self.owner, transaction) {
                 Ok(()) => {}
                 Err((_err, work)) => {
                     // Process is dead.
diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index 0555c4bd503e..f855d8d9818c 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -30,7 +30,8 @@
     sync::{
         aref::ARef,
         lock::{spinlock::SpinLockBackend, Guard},
-        Arc, ArcBorrow, CondVar, CondVarTimeoutResult, SpinLock, UniqueArc,
+        poll::PollCondVarBox,
+        Arc, ArcBorrow, CondVar, CondVarTimeoutResult, SetOnce, SpinLock, UniqueArc,
     },
     task::{Pid, Task},
     uaccess::{UserSlice, UserSliceReader},
@@ -172,21 +173,26 @@ fn new() -> Self {
     /// taken while holding the inner process lock.
     pub(crate) fn push_work(
         &mut self,
+        proc: &Process,
         work: DLArc<dyn DeliverToRead>,
     ) -> Result<(), (BinderError, DLArc<dyn DeliverToRead>)> {
+        let sync = work.should_sync_wakeup();
+
         // Try to find a ready thread to which to push the work.
         if let Some(thread) = self.ready_threads.pop_front() {
             // Push to thread while holding state lock. This prevents the thread from giving up
             // (for example, because of a signal) when we're about to deliver work.
-            match thread.push_work(work) {
+            match thread.push_work_inner(work, sync) {
                 PushWorkRes::Ok => Ok(()),
+                PushWorkRes::OkNotifyPoll => {
+                    proc.notify_poll(sync);
+                    Ok(())
+                }
                 PushWorkRes::FailedDead(work) => Err((BinderError::new_dead(), work)),
             }
         } else if self.is_dead {
             Err((BinderError::new_dead(), work))
         } else {
-            let sync = work.should_sync_wakeup();
-
             // Didn't find a thread waiting for proc work; this can happen
             // in two scenarios:
             // 1. All threads are busy handling transactions
@@ -194,17 +200,12 @@ pub(crate) fn push_work(
             //    the kernel driver soon and pick up this work.
             // 2. Threads are using the (e)poll interface, in which case
             //    they may be blocked on the waitqueue without having been
-            //    added to waiting_threads. For this case, we just iterate
-            //    over all threads not handling transaction work, and
-            //    wake them all up. We wake all because we don't know whether
-            //    a thread that called into (e)poll is handling non-binder
-            //    work currently.
+            //    added to waiting_threads. For this case, we wake it up
+            //    directly.
             self.work.push_back(work);
 
             // Wake up polling threads, if any.
-            for thread in self.threads.values() {
-                thread.notify_if_poll_ready(sync);
-            }
+            proc.notify_poll(sync);
 
             Ok(())
         }
@@ -227,11 +228,11 @@ pub(crate) fn update_node_refcount(
 
         // If we decided that we need to push work, push either to the process or to a thread if
         // one is specified.
-        if let Some(node) = push {
+        if let Some(pnode) = push {
             if let Some(thread) = othread {
-                thread.push_work_deferred(node);
+                thread.push_work_deferred(pnode);
             } else {
-                let _ = self.push_work(node);
+                let _ = self.push_work(&node.owner, pnode);
                 // Nothing to do: `push_work` may fail if the process is dead, but that's ok as in
                 // that case, it doesn't care about the notification.
             }
@@ -457,6 +458,12 @@ pub(crate) struct Process {
     #[pin]
     node_refs: SpinLock<ProcessNodeRefs>,
 
+    // Synchronizes `register_wait` calls to the `PollCondVarBox`.
+    //
+    // The `PollCondVarBox` is not stored here because synchronization is
+    // done for `register_wait` only. Wakeups do not take this lock.
+    poll: SetOnce<PollCondVarBox>,
+
     // Work node for deferred work item.
     #[pin]
     defer_work: Work<Process>,
@@ -516,6 +523,7 @@ fn new(ctx: Arc<Context>, cred: ARef<Credential>) -> Result<Arc<Self>> {
                 defer_work <- kernel::new_work!("Process::defer_work"),
                 links <- ListLinks::new(),
                 stats: BinderStats::new(),
+                poll: SetOnce::new(),
             }),
             GFP_KERNEL,
         )?;
@@ -715,7 +723,7 @@ fn get_current_thread(self: ArcBorrow<'_, Self>) -> Result<Arc<Thread>> {
 
     pub(crate) fn push_work(&self, work: DLArc<dyn DeliverToRead>) -> BinderResult {
         // If push_work fails, drop the work item outside the lock.
-        let res = self.inner.lock().push_work(work);
+        let res = self.inner.lock().push_work(self, work);
         match res {
             Ok(()) => Ok(()),
             Err((err, work)) => {
@@ -1018,7 +1026,7 @@ pub(crate) fn inc_ref_done(&self, reader: &mut UserSliceReader, strong: bool) ->
         if let Ok(Some(node)) = inner.get_existing_node(ptr, cookie) {
             if let Some(node) = node.inc_ref_done_locked(strong, &mut inner) {
                 // This only fails if the process is dead.
-                let _ = inner.push_work(node);
+                let _ = inner.push_work(self, node);
             }
         }
         Ok(())
@@ -1535,6 +1543,15 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
             }
         }
     }
+
+    pub(crate) fn notify_poll(&self, sync: bool) {
+        if let Some(poll) = self.poll.as_ref() {
+            if sync {
+                poll.notify_sync();
+            }
+            poll.notify_all();
+        }
+    }
 }
 
 fn get_frozen_status(data: UserSlice) -> Result {
@@ -1726,7 +1743,21 @@ pub(crate) fn poll(
         table: PollTable<'_>,
     ) -> Result<u32> {
         let thread = this.get_current_thread()?;
-        let (from_proc, mut mask) = thread.poll(file, table);
+        {
+            let poll = loop {
+                if let Some(poll) = this.poll.as_ref() {
+                    break poll;
+                }
+
+                let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?;
+                // Reuse our existing lock to synchronize callers initializing.
+                let _guard = this.node_refs.lock();
+                this.poll.populate(poll);
+            };
+
+            table.register_wait(file, poll);
+        }
+        let (from_proc, mut mask) = thread.poll()?;
         if mask == 0 && from_proc && !this.inner.lock().work.is_empty() {
             mask |= bindings::POLLIN;
         }
diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
index 33368f7fd551..541ca947cc52 100644
--- a/drivers/android/binder/thread.rs
+++ b/drivers/android/binder/thread.rs
@@ -9,15 +9,14 @@
 
 use kernel::{
     bindings,
-    fs::{File, LocalFile},
+    fs::LocalFile,
     list::{AtomicTracker, List, ListArc, ListLinks, TryNewListArc},
     prelude::*,
     security,
     seq_file::SeqFile,
     seq_print,
     sync::atomic::{ordering::Relaxed, Atomic},
-    sync::poll::{PollCondVar, PollTable},
-    sync::{aref::ARef, Arc, SpinLock},
+    sync::{aref::ARef, Arc, CondVar, SpinLock},
     task::Task,
     uaccess::{UserPtr, UserSlice, UserSliceReader},
     uapi,
@@ -225,8 +224,10 @@ fn claim_next(&mut self, size: usize) -> Result<usize> {
     }
 }
 
+#[must_use]
 pub(crate) enum PushWorkRes {
     Ok,
+    OkNotifyPoll,
     FailedDead(DLArc<dyn DeliverToRead>),
 }
 
@@ -234,6 +235,7 @@ impl PushWorkRes {
     fn is_ok(&self) -> bool {
         match self {
             PushWorkRes::Ok => true,
+            PushWorkRes::OkNotifyPoll => true,
             PushWorkRes::FailedDead(_) => false,
         }
     }
@@ -310,27 +312,32 @@ fn pop_work(&mut self) -> Option<DLArc<dyn DeliverToRead>> {
 
     fn push_work(&mut self, work: DLArc<dyn DeliverToRead>) -> PushWorkRes {
         if self.is_dead {
-            PushWorkRes::FailedDead(work)
+            return PushWorkRes::FailedDead(work);
+        }
+        self.work_list.push_back(work);
+        self.process_work_list = true;
+        if self.looper_flags & LOOPER_POLL != 0 {
+            PushWorkRes::OkNotifyPoll
         } else {
-            self.work_list.push_back(work);
-            self.process_work_list = true;
             PushWorkRes::Ok
         }
     }
 
-    fn push_reply_work(&mut self, code: u32) {
+    fn push_reply_work(&mut self, code: u32) -> PushWorkRes {
         if let Ok(work) = ListArc::try_from_arc(self.reply_work.clone()) {
             work.set_error_code(code);
-            self.push_work(work);
+            self.push_work(work)
         } else {
             pr_warn!("Thread reply work is already in use.");
+            PushWorkRes::Ok
         }
     }
 
     fn push_return_work(&mut self, reply: u32) {
         if let Ok(work) = ListArc::try_from_arc(self.return_work.clone()) {
             work.set_error_code(reply);
-            self.push_work(work);
+            // Not notifying: Reply to current thread.
+            let _ = self.push_work(work);
         } else {
             pr_warn!("Thread return work is already in use.");
         }
@@ -422,7 +429,7 @@ pub(crate) struct Thread {
     #[pin]
     inner: SpinLock<InnerThread>,
     #[pin]
-    work_condvar: PollCondVar,
+    work_condvar: CondVar,
     /// Used to insert this thread into the process' `ready_threads` list.
     ///
     /// INVARIANT: May never be used for any other list than the `self.process.ready_threads`.
@@ -453,7 +460,7 @@ pub(crate) fn new(id: i32, process: Arc<Process>) -> Result<Arc<Self>> {
                 process,
                 task: ARef::from(&**kernel::current!()),
                 inner <- kernel::new_spinlock!(inner, "Thread::inner"),
-                work_condvar <- kernel::new_poll_condvar!("Thread::work_condvar"),
+                work_condvar <- kernel::new_condvar!("Thread::work_condvar"),
                 links <- ListLinks::new(),
                 links_track <- AtomicTracker::new(),
             }),
@@ -624,7 +631,14 @@ fn get_work(self: &Arc<Self>, wait: bool) -> Result<Option<DLArc<dyn DeliverToRe
     /// Returns whether the item was successfully pushed. This can only fail if the thread is dead.
     pub(crate) fn push_work(&self, work: DLArc<dyn DeliverToRead>) -> PushWorkRes {
         let sync = work.should_sync_wakeup();
+        self.push_work_inner(work, sync)
+    }
 
+    pub(crate) fn push_work_inner(
+        &self,
+        work: DLArc<dyn DeliverToRead>,
+        sync: bool,
+    ) -> PushWorkRes {
         let res = self.inner.lock().push_work(work);
 
         if res.is_ok() {
@@ -643,7 +657,8 @@ pub(crate) fn push_work(&self, work: DLArc<dyn DeliverToRead>) -> PushWorkRes {
     pub(crate) fn push_work_if_looper(&self, work: DLArc<dyn DeliverToRead>) -> BinderResult {
         let mut inner = self.inner.lock();
         if inner.is_looper() && !inner.is_dead {
-            inner.push_work(work);
+            // Not notifying: Reply to current thread.
+            let _ = inner.push_work(work);
             Ok(())
         } else {
             drop(inner);
@@ -1154,7 +1169,7 @@ fn deliver_single_reply(
             transaction.set_outstanding(&mut self.process.inner.lock());
         }
 
-        {
+        let ret = {
             let mut inner = self.inner.lock();
             if !inner.pop_transaction_replied(transaction) {
                 return false;
@@ -1171,15 +1186,16 @@ fn deliver_single_reply(
             }
 
             match reply {
-                Ok(work) => {
-                    inner.push_work(work);
-                }
+                Ok(work) => inner.push_work(work),
                 Err(code) => inner.push_reply_work(code),
             }
-        }
+        };
 
         // Notify the thread now that we've released the inner lock.
         self.work_condvar.notify_sync();
+        if matches!(ret, PushWorkRes::OkNotifyPoll) {
+            self.process.notify_poll(true);
+        }
         false
     }
 
@@ -1349,7 +1365,8 @@ fn reply_inner(self: &Arc<Self>, info: &mut TransactionInfo) -> BinderResult {
             let process = orig.from.process.clone();
             let allow_fds = orig.flags & TF_ACCEPT_FDS != 0;
             let reply = Transaction::new_reply(self, process, info, allow_fds)?;
-            self.inner.lock().push_work(completion);
+            // Not notifying: Reply to current thread.
+            let _ = self.inner.lock().push_work(completion);
             orig.from.deliver_reply(Ok(reply), &orig, None);
             Ok(())
         })()
@@ -1387,7 +1404,8 @@ fn oneway_transaction_inner(self: &Arc<Self>, info: &mut TransactionInfo) -> Bin
         };
         let list_completion = DTRWrap::arc_try_new(DeliverCode::new(code))?;
         let completion = list_completion.clone_arc();
-        self.inner.lock().push_work(list_completion);
+        // Not notifying: Reply to current thread.
+        let _ = self.inner.lock().push_work(list_completion);
         match transaction.submit(info) {
             Ok(()) => Ok(()),
             Err(err) => {
@@ -1589,10 +1607,9 @@ pub(crate) fn write_read(self: &Arc<Self>, data: UserSlice, wait: bool) -> Resul
         ret
     }
 
-    pub(crate) fn poll(&self, file: &File, table: PollTable<'_>) -> (bool, u32) {
-        table.register_wait(file, &self.work_condvar);
+    pub(crate) fn poll(&self) -> Result<(bool, u32)> {
         let mut inner = self.inner.lock();
-        (inner.should_use_process_work_queue(), inner.poll())
+        Ok((inner.should_use_process_work_queue(), inner.poll()))
     }
 
     /// Make the call to `get_work` or `get_work_local` return immediately, if any.
@@ -1609,26 +1626,9 @@ pub(crate) fn exit_looper(&self) {
         }
     }
 
-    pub(crate) fn notify_if_poll_ready(&self, sync: bool) {
-        // Determine if we need to notify. This requires the lock.
-        let inner = self.inner.lock();
-        let notify = inner.looper_flags & LOOPER_POLL != 0 && inner.should_use_process_work_queue();
-        drop(inner);
-
-        // Now that the lock is no longer held, notify the waiters if we have to.
-        if notify {
-            if sync {
-                self.work_condvar.notify_sync();
-            } else {
-                self.work_condvar.notify_one();
-            }
-        }
-    }
-
     pub(crate) fn release(self: &Arc<Self>) {
         self.inner.lock().is_dead = true;
 
-        //self.work_condvar.clear();
         self.unwind_transaction_stack();
 
         // Cancel all pending work items.
diff --git a/drivers/android/binder/transaction.rs b/drivers/android/binder/transaction.rs
index 6cb7b745c52e..2643cca4a7d6 100644
--- a/drivers/android/binder/transaction.rs
+++ b/drivers/android/binder/transaction.rs
@@ -371,11 +371,15 @@ pub(crate) fn submit(self: DLArc<Self>, info: &mut TransactionInfo) -> BinderRes
             crate::trace::trace_transaction(false, &self, Some(&thread.task));
             match thread.push_work(self) {
                 PushWorkRes::Ok => Ok(()),
+                PushWorkRes::OkNotifyPoll => {
+                    process.notify_poll(true);
+                    Ok(())
+                }
                 PushWorkRes::FailedDead(me) => Err((BinderError::new_dead(), me)),
             }
         } else {
             crate::trace::trace_transaction(false, &self, None);
-            process_inner.push_work(self)
+            process_inner.push_work(&process, self)
         };
         drop(process_inner);
 

-- 
2.55.0.rc2.803.g1fd1e6609c-goog

Re: [PATCH v6 1/2] rust: poll: use kfree_rcu() for PollCondVar

[Boqun Feng]

On Tue, Jul 07, 2026 at 10:43:12AM +0000, Alice Ryhl wrote:
> Rust Binder currently uses PollCondVar, but it calls synchronize_rcu()
> in the destructor, which we would like to avoid. Add a variation of
> PollCondVar that kfree_rcu() instead.
> 
> One could avoid the `rcu` field and allocate the rcu_head on drop using
> a fallback to synchronize_rcu() on ENOMEM. However, I'd prefer to avoid
> the potential for synchronize_rcu(), and Binder will only use this for a
> small fraction of processes, so even if it changes which kmalloc bucket
> it falls into, the extra memory is not a problem.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>


Let's switch to a better abstraction like RcuFreeBox later, but I don't
see any issue of this approach in general.

Reviewed-by: Boqun Feng <boqun@kernel.org>

Regards,
Boqun

> ---
>  rust/kernel/sync/poll.rs | 73 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 1 deletion(-)
> 
[..]

Re: [PATCH v6 2/2] rust_binder: move (e)poll wait queue to Process

[Boqun Feng]

On Tue, Jul 07, 2026 at 10:43:13AM +0000, Alice Ryhl wrote:
> Most processes do not use Rust Binder with epoll, so avoid paying the
> synchronize_rcu() cost in drop for those that don't need it. For those
> that do, we also manage to replace synchronize_rcu() with kfree_rcu(),
> though we introduce an extra allocation.
> 
> In case the last ref to an Arc<Thread> is dropped outside of
> deferred_release(), this also ensures that synchronize_rcu() is not
> called in destructor of Arc<Thread> in other places. Theoretically that
> could lead to jank by making other syscalls slow, which would be
> problematic.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
[...]
> diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
> index 0555c4bd503e..f855d8d9818c 100644
> --- a/drivers/android/binder/process.rs
> +++ b/drivers/android/binder/process.rs
[...]
>              }
>          }
>      }
> +
> +    pub(crate) fn notify_poll(&self, sync: bool) {
> +        if let Some(poll) = self.poll.as_ref() {
> +            if sync {
> +                poll.notify_sync();
> +            }
> +            poll.notify_all();
> +        }
> +    }
>  }
>  
>  fn get_frozen_status(data: UserSlice) -> Result {
> @@ -1726,7 +1743,21 @@ pub(crate) fn poll(
>          table: PollTable<'_>,
>      ) -> Result<u32> {
>          let thread = this.get_current_thread()?;
> -        let (from_proc, mut mask) = thread.poll(file, table);
> +        {
> +            let poll = loop {
> +                if let Some(poll) = this.poll.as_ref() {
> +                    break poll;
> +                }
> +
> +                let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?;
> +                // Reuse our existing lock to synchronize callers initializing.
> +                let _guard = this.node_refs.lock();

Note sure whether this lock is needed? SetOnce::populate() should be
atomic, i.e. only one populate() would win?

Also seems we should have a SetOnce::as_ref_or_populate(&self, default:
T).

The rest looks good to me. FWIW,

Reviewed-by: Boqun Feng <boqun@kernel.org>

Regards,
Boqun

> +                this.poll.populate(poll);
> +            };
> +
> +            table.register_wait(file, poll);
> +        }
> +        let (from_proc, mut mask) = thread.poll()?;
>          if mask == 0 && from_proc && !this.inner.lock().work.is_empty() {
>              mask |= bindings::POLLIN;
>          }
[..]

Re: [PATCH v6 2/2] rust_binder: move (e)poll wait queue to Process

[Alice Ryhl]

On Fri, Jul 10, 2026 at 05:30:30PM -0700, Boqun Feng wrote:
> On Tue, Jul 07, 2026 at 10:43:13AM +0000, Alice Ryhl wrote:
> > +        {
> > +            let poll = loop {
> > +                if let Some(poll) = this.poll.as_ref() {
> > +                    break poll;
> > +                }
> > +
> > +                let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?;
> > +                // Reuse our existing lock to synchronize callers initializing.
> > +                let _guard = this.node_refs.lock();
> > +                this.poll.populate(poll);
> > +            };
> 
> Note sure whether this lock is needed? SetOnce::populate() should be
> atomic, i.e. only one populate() would win?
> 
> Also seems we should have a SetOnce::as_ref_or_populate(&self, default:
> T).

I'm taking this lock because I want to ensure that losers only loop
once. The problem is that just because you lost the race in populate(),
it's not guaranteed that as_ref() will return Some on the next
iteration, since the winner of the race may still be busy executing
populate(). Taking the loop avoids this possibility.

With regards to as_ref_or_populate(), I point you to this discussion for
reasons why this is hard:
https://lore.kernel.org/all/aZLZbN5C3wXgt3kL@google.com/

> The rest looks good to me. FWIW,
> 
> Reviewed-by: Boqun Feng <boqun@kernel.org>

Thanks!

Alice

Re: [PATCH v6 2/2] rust_binder: move (e)poll wait queue to Process

[Boqun Feng]

On Sat, Jul 11, 2026 at 10:32:39AM +0000, Alice Ryhl wrote:
> On Fri, Jul 10, 2026 at 05:30:30PM -0700, Boqun Feng wrote:
> > On Tue, Jul 07, 2026 at 10:43:13AM +0000, Alice Ryhl wrote:
> > > +        {
> > > +            let poll = loop {
> > > +                if let Some(poll) = this.poll.as_ref() {
> > > +                    break poll;
> > > +                }
> > > +
> > > +                let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?;
> > > +                // Reuse our existing lock to synchronize callers initializing.
> > > +                let _guard = this.node_refs.lock();
> > > +                this.poll.populate(poll);
> > > +            };
> > 
> > Note sure whether this lock is needed? SetOnce::populate() should be
> > atomic, i.e. only one populate() would win?
> > 
> > Also seems we should have a SetOnce::as_ref_or_populate(&self, default:
> > T).
> 
> I'm taking this lock because I want to ensure that losers only loop
> once. The problem is that just because you lost the race in populate(),

Then probably you could add some comment explaining this. For example:

    // Reuse our existing lock to synchronize callers initializing to
    // make sure in the next iteration `as_ref()` will return `Some`.

> it's not guaranteed that as_ref() will return Some on the next
> iteration, since the winner of the race may still be busy executing
> populate(). Taking the loop avoids this possibility.
> 
> With regards to as_ref_or_populate(), I point you to this discussion for
> reasons why this is hard:
> https://lore.kernel.org/all/aZLZbN5C3wXgt3kL@google.com/
> 

Seems to me you really want OnceLock behavior here, and other code may
have the same requirement in the future. So maybe we just add OnceLock
for it? If the space cost is the concern, we can add a 
SetOnce::populate_with_lock(&self, lock: &Lock<..>) to provide users an
option. I think it's better than open-code here.

Thoughts?

Regards,
Boqun

> > The rest looks good to me. FWIW,
> > 
> > Reviewed-by: Boqun Feng <boqun@kernel.org>
> 
> Thanks!
> 
> Alice

Re: [PATCH v6 2/2] rust_binder: move (e)poll wait queue to Process

[Alice Ryhl]

On Sat, Jul 11, 2026 at 06:37:14AM -0700, Boqun Feng wrote:
> On Sat, Jul 11, 2026 at 10:32:39AM +0000, Alice Ryhl wrote:
> > On Fri, Jul 10, 2026 at 05:30:30PM -0700, Boqun Feng wrote:
> > > On Tue, Jul 07, 2026 at 10:43:13AM +0000, Alice Ryhl wrote:
> > > > +        {
> > > > +            let poll = loop {
> > > > +                if let Some(poll) = this.poll.as_ref() {
> > > > +                    break poll;
> > > > +                }
> > > > +
> > > > +                let poll = PollCondVarBox::new(c"Process::poll", kernel::static_lock_class!())?;
> > > > +                // Reuse our existing lock to synchronize callers initializing.
> > > > +                let _guard = this.node_refs.lock();
> > > > +                this.poll.populate(poll);
> > > > +            };
> > > 
> > > Note sure whether this lock is needed? SetOnce::populate() should be
> > > atomic, i.e. only one populate() would win?
> > > 
> > > Also seems we should have a SetOnce::as_ref_or_populate(&self, default:
> > > T).
> > 
> > I'm taking this lock because I want to ensure that losers only loop
> > once. The problem is that just because you lost the race in populate(),
> 
> Then probably you could add some comment explaining this. For example:
> 
>     // Reuse our existing lock to synchronize callers initializing to
>     // make sure in the next iteration `as_ref()` will return `Some`.

I don't mind adding the comment.

> > it's not guaranteed that as_ref() will return Some on the next
> > iteration, since the winner of the race may still be busy executing
> > populate(). Taking the loop avoids this possibility.
> > 
> > With regards to as_ref_or_populate(), I point you to this discussion for
> > reasons why this is hard:
> > https://lore.kernel.org/all/aZLZbN5C3wXgt3kL@google.com/
> > 
> 
> Seems to me you really want OnceLock behavior here, and other code may
> have the same requirement in the future. So maybe we just add OnceLock
> for it? If the space cost is the concern, we can add a 
> SetOnce::populate_with_lock(&self, lock: &Lock<..>) to provide users an
> option. I think it's better than open-code here.
> 
> Thoughts?

Well, I don't think it's a bad idea.

The current code drops the value under the spinlock if populate() loses
the race, and a populate_with_lock() could help avoid that, so that
seems like a reasonable idea. It's not a big problem in this case (it's
just a kfree_rcu() call after all), but could be a problem for other
SetOnce users.

I mean, it'd be ideal if SetOnce could just properly support this, but
like I discussed on the thread, there are a bunch of things to keep in
mind if we want to actually do that. You want preemption disabled while
you memcpy() in the value in populate(). I guess preemption doesn't
matter for as_ref(), though. Not sure how that would interact with
PREEMPT_RT.

Alice