Rust for Linux List
 help / color / mirror / Atom feed
* [PATCH v2] rust_binder: add TF_DEFER_COMPLETE flag for avoiding userspace roundtrip
@ 2026-07-22 21:09 Alice Ryhl
  2026-07-23 11:58 ` Alice Ryhl
  0 siblings, 1 reply; 2+ messages in thread
From: Alice Ryhl @ 2026-07-22 21:09 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Todd Kjos, Carlos Llamas
  Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
	Onur Özkan, rust-for-linux, linux-kernel, Alice Ryhl

Outgoing transactions are able to send a message and wait for its reply
in a single ioctl. Why not avoid a userspace roundtrip by applying the
same logic for replying to incoming messages and waiting for the next
incoming message?

Generally, when you send a reply using BC_REPLY, the kernel sends
BR_TRANSACTION_COMPLETE as a reply to BC_REPLY right away. The
BR_TRANSACTION_COMPLETE command indicates that it's safe for userspace
to free any resources associated with this message (such as embedded fds
or Binder nodes). However, the BR_TRANSACTION_COMPLETE message is
problematic because after BC_REPLY is issued, there will be a pending
message for userspace. The kernel will refuse to sleep for incoming
messages in this scenario.

The way this is handled for outgoing transaction is through a mechanism
known as deferred delivery of BR_TRANSACTION_COMPLETE. The idea is that
when you send an outgoing transaction, then we do not return to
userspace right away if BR_TRANSACTION_COMPLETE is the only pending
message. This patch adds a new flag called TF_DEFER_COMPLETE that lets
userspace opt-in to the same deferred delivery mechanism for
BR_TRANSACTION_COMPLETE when using BC_REPLY.

Given this new uapi, we can adjust sendReply in userspace libbinder
so that it writes the BC_REPLY command into mOut but does not flush the
buffer to the kernel. Then, userspace simply continues running until it
returns all the way out to the top-level joinThreadPool() loop, which
calls into the kernel to get the next incoming transaction. At this
point, mOut is flushed, sending the reply. The same ioctl then proceeds
to sleep for an incoming message.

Userspace only actually specifies TF_DEFER_COMPLETE when the Parcel does
not contain fds or refcounts on binder objects. This is because
otherwise said fd or binder node will not be freed until the binder
thread receives another incoming transaction, which could be a long
time. In the case of fds, this is especially important because delaying
fclose() can result in processes hanging because they read from a pipe
that isn't being closed due to fclose() not getting called. Note that
even if TF_DEFER_COMPLETE is not specified for this transaction, it can
still be useful to defer the BC_REPLY command, as it can still avoid a
userspace roundtrip when a new incoming transaction is available right
away.

Observing the cuttlefish logs while booting with this change shows that
there were 4297 opportunities for this optimization to kick in (that is,
boot invoked BC_REPLY 4297 times). Out of those, 3441 binder ioctls sent
and received a transaction in the same ioctl. This indicates that we
successfully eliminated a syscall on the server side for 80% of incoming
transactions. Generally, this means that a server is now able to handle
incoming messages using one syscall per incoming message (for each
incoming transaction, the syscall handles one BC_FREE_BUFFER and
BC_REPLY command, and then waits for the next incoming transaction).

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v2:
- Return deferred thread work instead of the process global work if
  there is work in the process global list.
- Link to v1: https://lore.kernel.org/r/20260716-defer-complete-v1-1-ce0e38d30dc6@google.com
---
 drivers/android/binder/defs.rs      |  3 +-
 drivers/android/binder/process.rs   |  8 ++++++
 drivers/android/binder/thread.rs    | 55 +++++++++++++++++++++++++++++++------
 include/uapi/linux/android/binder.h |  1 +
 4 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/drivers/android/binder/defs.rs b/drivers/android/binder/defs.rs
index 8ac9bdd7a499..cc4becd6e168 100644
--- a/drivers/android/binder/defs.rs
+++ b/drivers/android/binder/defs.rs
@@ -77,7 +77,8 @@ macro_rules! pub_no_prefix {
     TF_ONE_WAY,
     TF_ACCEPT_FDS,
     TF_CLEAR_BUF,
-    TF_UPDATE_TXN
+    TF_UPDATE_TXN,
+    TF_DEFER_COMPLETE,
 );
 
 pub(crate) use uapi::{
diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index 1778628d8acd..4f23a7cf7352 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -686,8 +686,16 @@ pub(crate) fn get_work(&self) -> Option<DLArc<dyn DeliverToRead>> {
     pub(crate) fn get_work_or_register<'a>(
         &'a self,
         thread: &'a Arc<Thread>,
+        thread_has_deferred_work: bool,
     ) -> GetWorkOrRegister<'a> {
         let mut inner = self.inner.lock();
+
+        if thread_has_deferred_work && !inner.work.is_empty() {
+            if let Some(work) = thread.pop_work_even_if_deferred() {
+                return GetWorkOrRegister::Work(work);
+            }
+        }
+
         // Try to get work from the process queue.
         if let Some(work) = inner.work.pop_front() {
             return GetWorkOrRegister::Work(work);
diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
index a51821dde0ad..c4d67b9ef39b 100644
--- a/drivers/android/binder/thread.rs
+++ b/drivers/android/binder/thread.rs
@@ -572,9 +572,21 @@ fn get_work_local(self: &Arc<Self>, wait: bool) -> Result<Option<DLArc<dyn Deliv
     // mangled symbol names.
     #[export_name = "rust_binder_wait"]
     fn get_work(self: &Arc<Self>, wait: bool) -> Result<Option<DLArc<dyn DeliverToRead>>> {
+        let thread_has_deferred_work;
+
         // Try to get work from the thread's work queue, using only a local lock.
         {
             let mut inner = self.inner.lock();
+
+            // The process_work_list boolean is used to make us go to sleep even if there is work
+            // in the thread todo-list, but it doesn't apply to the process todo-list. Furthermore,
+            // work in the thread todo-list must still be delivered before the process list.
+            //
+            // Thus, in some scenarios we must return the thread work now even if we were requested
+            // to wait. Adjust `process_work_list` to `true` accordingly.
+            inner.process_work_list |= inner.looper_need_return;
+            inner.process_work_list |= !wait;
+
             if let Some(work) = inner.pop_work() {
                 return Ok(Some(work));
             }
@@ -582,18 +594,26 @@ fn get_work(self: &Arc<Self>, wait: bool) -> Result<Option<DLArc<dyn DeliverToRe
                 drop(inner);
                 return Ok(self.process.get_work());
             }
+
+            // Note that if the thread list is empty, then the call to `pop_work()` has changed
+            // `process_work_list` back to `false` even if we set it to `true` above.
+            thread_has_deferred_work = inner.process_work_list;
         }
 
         // If the caller doesn't want to wait, try to grab work from the process queue.
         //
         // We know nothing will have been queued directly to the thread queue because it is not in
-        // a transaction and it is not in the process' ready list.
+        // a transaction and it is not in the process' ready list. We also know the thread list has
+        // no deferred work due to the `inner.process_work_list |= !wait` call above.
         if !wait {
             return self.process.get_work().ok_or(EAGAIN).map(Some);
         }
 
         // Get work from the process queue. If none is available, atomically register as ready.
-        let reg = match self.process.get_work_or_register(self) {
+        let reg = match self
+            .process
+            .get_work_or_register(self, thread_has_deferred_work)
+        {
             GetWorkOrRegister::Work(work) => return Ok(Some(work)),
             GetWorkOrRegister::Register(reg) => reg,
         };
@@ -609,14 +629,18 @@ fn get_work(self: &Arc<Self>, wait: bool) -> Result<Option<DLArc<dyn DeliverToRe
             inner.looper_flags &= !(LOOPER_WAITING | LOOPER_WAITING_PROC);
 
             if signal_pending || inner.looper_need_return {
-                // We need to return now. We need to pull the thread off the list of ready threads
-                // (by dropping `reg`), then check the state again after it's off the list to
-                // ensure that something was not queued in the meantime. If something has been
-                // queued, we just return it (instead of the error).
+                // We need to return now.
+                //
+                // We need to pull the thread off the list of ready threads (by dropping `reg`),
+                // then check the state again after it's off the list to ensure that something was
+                // not queued in the meantime. If something has been queued (or if there is
+                // deferred work), we just return it (instead of the error).
                 drop(inner);
                 drop(reg);
 
-                let res = match self.inner.lock().pop_work() {
+                inner = self.inner.lock();
+                inner.process_work_list = true;
+                let res = match inner.pop_work() {
                     Some(work) => Ok(Some(work)),
                     None if signal_pending => Err(EINTR),
                     None => Ok(None),
@@ -674,6 +698,12 @@ pub(crate) fn push_return_work(&self, reply: u32) {
         self.inner.lock().push_return_work(reply);
     }
 
+    pub(crate) fn pop_work_even_if_deferred(&self) -> Option<DLArc<dyn DeliverToRead>> {
+        let mut thread_inner = self.inner.lock();
+        thread_inner.process_work_list = true;
+        thread_inner.pop_work()
+    }
+
     fn translate_object(
         &self,
         obj_index: usize,
@@ -1398,8 +1428,15 @@ 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)?;
-            // Not notifying: Reply to current thread.
-            let _ = self.inner.lock().push_work(completion);
+            {
+                // This performs a deferred push so that `read` can wait for the next incoming
+                // transaction without a userspace roundtrip.
+                let mut inner = self.inner.lock();
+                inner.push_work_deferred(completion);
+                // However, if `TF_DEFER_COMPLETE` is not set, then set `process_work_list` to make
+                // the push non-deferred. This forces a userspace roundtrip.
+                inner.process_work_list |= info.flags & TF_DEFER_COMPLETE == 0;
+            }
             orig.from.deliver_reply(Ok(reply), &orig, None);
             Ok(())
         })()
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 701cad36de43..96e5b0184a1b 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -296,6 +296,7 @@ enum transaction_flags {
 	TF_ACCEPT_FDS	= 0x10,	/* allow replies with file descriptors */
 	TF_CLEAR_BUF	= 0x20,	/* clear buffer on txn complete */
 	TF_UPDATE_TXN	= 0x40,	/* update the outdated pending async txn */
+	TF_DEFER_COMPLETE = 0x80,	/* defer transaction complete to userspace */
 };
 
 struct binder_transaction_data {

---
base-commit: 2cedf2272f1bb42471e646868ac572cc5752bd91
change-id: 20260715-defer-complete-f1dea9af13a8

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


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] rust_binder: add TF_DEFER_COMPLETE flag for avoiding userspace roundtrip
  2026-07-22 21:09 [PATCH v2] rust_binder: add TF_DEFER_COMPLETE flag for avoiding userspace roundtrip Alice Ryhl
@ 2026-07-23 11:58 ` Alice Ryhl
  0 siblings, 0 replies; 2+ messages in thread
From: Alice Ryhl @ 2026-07-23 11:58 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Todd Kjos, Carlos Llamas
  Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
	Onur Özkan, rust-for-linux, linux-kernel

On Wed, Jul 22, 2026 at 09:09:22PM +0000, Alice Ryhl wrote:
> +            // Note that if the thread list is empty, then the call to `pop_work()` has changed
> +            // `process_work_list` back to `false` even if we set it to `true` above.
> +            thread_has_deferred_work = inner.process_work_list;

Oops, as sashiko points out, this should be:

	thread_has_deferred_work = !inner.work_list.is_empty();

Alice

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-07-23 11:58 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-22 21:09 [PATCH v2] rust_binder: add TF_DEFER_COMPLETE flag for avoiding userspace roundtrip Alice Ryhl
2026-07-23 11:58 ` Alice Ryhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox