Skip to content

Commit 6c70907

Browse files
Martijn Coenenpundiramit
authored andcommitted
ANDROID: binder: Add thread->process_todo flag.
This flag determines whether the thread should currently process the work in the thread->todo worklist. The prime usecase for this is improving the performance of synchronous transactions: all synchronous transactions post a BR_TRANSACTION_COMPLETE to the calling thread, but there's no reason to return that command to userspace right away - userspace anyway needs to wait for the reply. Likewise, a synchronous transaction that contains a binder object can cause a BC_ACQUIRE/BC_INCREFS to be returned to userspace; since the caller must anyway hold a strong/weak ref for the duration of the call, postponing these commands until the reply comes in is not a problem. Note that this flag is not used to determine whether a thread can handle process work; a thread should never pick up process work when thread work is still pending. Before patch: ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_sendVec_binderize/4 45959 ns 20288 ns 34351 BM_sendVec_binderize/8 45603 ns 20080 ns 34909 BM_sendVec_binderize/16 45528 ns 20113 ns 34863 BM_sendVec_binderize/32 45551 ns 20122 ns 34881 BM_sendVec_binderize/64 45701 ns 20183 ns 34864 BM_sendVec_binderize/128 45824 ns 20250 ns 34576 BM_sendVec_binderize/256 45695 ns 20171 ns 34759 BM_sendVec_binderize/512 45743 ns 20211 ns 34489 BM_sendVec_binderize/1024 46169 ns 20430 ns 34081 After patch: ------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------ BM_sendVec_binderize/4 42939 ns 17262 ns 40653 BM_sendVec_binderize/8 42823 ns 17243 ns 40671 BM_sendVec_binderize/16 42898 ns 17243 ns 40594 BM_sendVec_binderize/32 42838 ns 17267 ns 40527 BM_sendVec_binderize/64 42854 ns 17249 ns 40379 BM_sendVec_binderize/128 42881 ns 17288 ns 40427 BM_sendVec_binderize/256 42917 ns 17297 ns 40429 BM_sendVec_binderize/512 43184 ns 17395 ns 40411 BM_sendVec_binderize/1024 43119 ns 17357 ns 40432 Signed-off-by: Martijn Coenen <maco@android.com> Change-Id: Ia70287066d62aba64e98ac44ff1214e37ca75693
1 parent d61c22c commit 6c70907

1 file changed

Lines changed: 100 additions & 44 deletions

File tree

drivers/android/binder.c

Lines changed: 100 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,8 @@ enum {
601601
* (protected by @proc->inner_lock)
602602
* @todo: list of work to do for this thread
603603
* (protected by @proc->inner_lock)
604+
* @process_todo: whether work in @todo should be processed
605+
* (protected by @proc->inner_lock)
604606
* @return_error: transaction errors reported by this thread
605607
* (only accessed by this thread)
606608
* @reply_error: transaction errors reported by target thread
@@ -627,6 +629,7 @@ struct binder_thread {
627629
bool looper_need_return; /* can be written by other thread */
628630
struct binder_transaction *transaction_stack;
629631
struct list_head todo;
632+
bool process_todo;
630633
struct binder_error return_error;
631634
struct binder_error reply_error;
632635
wait_queue_head_t wait;
@@ -814,6 +817,16 @@ static bool binder_worklist_empty(struct binder_proc *proc,
814817
return ret;
815818
}
816819

820+
/**
821+
* binder_enqueue_work_ilocked() - Add an item to the work list
822+
* @work: struct binder_work to add to list
823+
* @target_list: list to add work to
824+
*
825+
* Adds the work to the specified list. Asserts that work
826+
* is not already on a list.
827+
*
828+
* Requires the proc->inner_lock to be held.
829+
*/
817830
static void
818831
binder_enqueue_work_ilocked(struct binder_work *work,
819832
struct list_head *target_list)
@@ -824,22 +837,56 @@ binder_enqueue_work_ilocked(struct binder_work *work,
824837
}
825838

826839
/**
827-
* binder_enqueue_work() - Add an item to the work list
828-
* @proc: binder_proc associated with list
840+
* binder_enqueue_thread_work_ilocked_nowake() - Add thread work
841+
* @thread: thread to queue work to
829842
* @work: struct binder_work to add to list
830-
* @target_list: list to add work to
831843
*
832-
* Adds the work to the specified list. Asserts that work
833-
* is not already on a list.
844+
* Adds the work to the todo list of the thread. Doesn't set the process_todo
845+
* flag, which means that (if it wasn't already set) the thread will go to
846+
* sleep without handling this work when it calls read.
847+
*
848+
* Requires the proc->inner_lock to be held.
834849
*/
835850
static void
836-
binder_enqueue_work(struct binder_proc *proc,
837-
struct binder_work *work,
838-
struct list_head *target_list)
851+
binder_enqueue_thread_work_ilocked_nowake(struct binder_thread *thread,
852+
struct binder_work *work)
839853
{
840-
binder_inner_proc_lock(proc);
841-
binder_enqueue_work_ilocked(work, target_list);
842-
binder_inner_proc_unlock(proc);
854+
binder_enqueue_work_ilocked(work, &thread->todo);
855+
}
856+
857+
/**
858+
* binder_enqueue_thread_work_ilocked() - Add an item to the thread work list
859+
* @thread: thread to queue work to
860+
* @work: struct binder_work to add to list
861+
*
862+
* Adds the work to the todo list of the thread, and enables processing
863+
* of the todo queue.
864+
*
865+
* Requires the proc->inner_lock to be held.
866+
*/
867+
static void
868+
binder_enqueue_thread_work_ilocked(struct binder_thread *thread,
869+
struct binder_work *work)
870+
{
871+
binder_enqueue_work_ilocked(work, &thread->todo);
872+
thread->process_todo = true;
873+
}
874+
875+
/**
876+
* binder_enqueue_thread_work() - Add an item to the thread work list
877+
* @thread: thread to queue work to
878+
* @work: struct binder_work to add to list
879+
*
880+
* Adds the work to the todo list of the thread, and enables processing
881+
* of the todo queue.
882+
*/
883+
static void
884+
binder_enqueue_thread_work(struct binder_thread *thread,
885+
struct binder_work *work)
886+
{
887+
binder_inner_proc_lock(thread->proc);
888+
binder_enqueue_thread_work_ilocked(thread, work);
889+
binder_inner_proc_unlock(thread->proc);
843890
}
844891

845892
static void
@@ -954,7 +1001,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd)
9541001
static bool binder_has_work_ilocked(struct binder_thread *thread,
9551002
bool do_proc_work)
9561003
{
957-
return !binder_worklist_empty_ilocked(&thread->todo) ||
1004+
return thread->process_todo ||
9581005
thread->looper_need_return ||
9591006
(do_proc_work &&
9601007
!binder_worklist_empty_ilocked(&thread->proc->todo));
@@ -1371,6 +1418,17 @@ static int binder_inc_node_nilocked(struct binder_node *node, int strong,
13711418
node->local_strong_refs++;
13721419
if (!node->has_strong_ref && target_list) {
13731420
binder_dequeue_work_ilocked(&node->work);
1421+
/*
1422+
* Note: this function is the only place where we queue
1423+
* directly to a thread->todo without using the
1424+
* corresponding binder_enqueue_thread_work() helper
1425+
* functions; in this case it's ok to not set the
1426+
* process_todo flag, since we know this node work will
1427+
* always be followed by other work that starts queue
1428+
* processing: in case of synchronous transactions, a
1429+
* BR_REPLY or BR_ERROR; in case of oneway
1430+
* transactions, a BR_TRANSACTION_COMPLETE.
1431+
*/
13741432
binder_enqueue_work_ilocked(&node->work, target_list);
13751433
}
13761434
} else {
@@ -1382,6 +1440,9 @@ static int binder_inc_node_nilocked(struct binder_node *node, int strong,
13821440
node->debug_id);
13831441
return -EINVAL;
13841442
}
1443+
/*
1444+
* See comment above
1445+
*/
13851446
binder_enqueue_work_ilocked(&node->work, target_list);
13861447
}
13871448
}
@@ -2071,9 +2132,9 @@ static void binder_send_failed_reply(struct binder_transaction *t,
20712132
binder_pop_transaction_ilocked(target_thread, t);
20722133
if (target_thread->reply_error.cmd == BR_OK) {
20732134
target_thread->reply_error.cmd = error_code;
2074-
binder_enqueue_work_ilocked(
2075-
&target_thread->reply_error.work,
2076-
&target_thread->todo);
2135+
binder_enqueue_thread_work_ilocked(
2136+
target_thread,
2137+
&target_thread->reply_error.work);
20772138
wake_up_interruptible(&target_thread->wait);
20782139
} else {
20792140
WARN(1, "Unexpected reply error: %u\n",
@@ -2712,11 +2773,10 @@ static bool binder_proc_transaction(struct binder_transaction *t,
27122773
struct binder_proc *proc,
27132774
struct binder_thread *thread)
27142775
{
2715-
struct list_head *target_list = NULL;
27162776
struct binder_node *node = t->buffer->target_node;
27172777
struct binder_priority node_prio;
27182778
bool oneway = !!(t->flags & TF_ONE_WAY);
2719-
bool wakeup = true;
2779+
bool pending_async = false;
27202780

27212781
BUG_ON(!node);
27222782
binder_node_lock(node);
@@ -2726,8 +2786,7 @@ static bool binder_proc_transaction(struct binder_transaction *t,
27262786
if (oneway) {
27272787
BUG_ON(thread);
27282788
if (node->has_async_transaction) {
2729-
target_list = &node->async_todo;
2730-
wakeup = false;
2789+
pending_async = true;
27312790
} else {
27322791
node->has_async_transaction = 1;
27332792
}
@@ -2741,22 +2800,20 @@ static bool binder_proc_transaction(struct binder_transaction *t,
27412800
return false;
27422801
}
27432802

2744-
if (!thread && !target_list)
2803+
if (!thread && !pending_async)
27452804
thread = binder_select_thread_ilocked(proc);
27462805

27472806
if (thread) {
2748-
target_list = &thread->todo;
27492807
binder_transaction_priority(thread->task, t, node_prio,
27502808
node->inherit_rt);
2751-
} else if (!target_list) {
2752-
target_list = &proc->todo;
2809+
binder_enqueue_thread_work_ilocked(thread, &t->work);
2810+
} else if (!pending_async) {
2811+
binder_enqueue_work_ilocked(&t->work, &proc->todo);
27532812
} else {
2754-
BUG_ON(target_list != &node->async_todo);
2813+
binder_enqueue_work_ilocked(&t->work, &node->async_todo);
27552814
}
27562815

2757-
binder_enqueue_work_ilocked(&t->work, target_list);
2758-
2759-
if (wakeup)
2816+
if (!pending_async)
27602817
binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
27612818

27622819
binder_inner_proc_unlock(proc);
@@ -3258,25 +3315,26 @@ static void binder_transaction(struct binder_proc *proc,
32583315
}
32593316
}
32603317
tcomplete->type = BINDER_WORK_TRANSACTION_COMPLETE;
3261-
binder_enqueue_work(proc, tcomplete, &thread->todo);
32623318
t->work.type = BINDER_WORK_TRANSACTION;
32633319

32643320
if (reply) {
3321+
binder_enqueue_thread_work(thread, tcomplete);
32653322
binder_inner_proc_lock(target_proc);
32663323
if (target_thread->is_dead) {
32673324
binder_inner_proc_unlock(target_proc);
32683325
goto err_dead_proc_or_thread;
32693326
}
32703327
BUG_ON(t->buffer->async_transaction != 0);
32713328
binder_pop_transaction_ilocked(target_thread, in_reply_to);
3272-
binder_enqueue_work_ilocked(&t->work, &target_thread->todo);
3329+
binder_enqueue_thread_work_ilocked(target_thread, &t->work);
32733330
binder_inner_proc_unlock(target_proc);
32743331
wake_up_interruptible_sync(&target_thread->wait);
32753332
binder_restore_priority(current, in_reply_to->saved_priority);
32763333
binder_free_transaction(in_reply_to);
32773334
} else if (!(t->flags & TF_ONE_WAY)) {
32783335
BUG_ON(t->buffer->async_transaction != 0);
32793336
binder_inner_proc_lock(proc);
3337+
binder_enqueue_thread_work_ilocked_nowake(thread, tcomplete);
32803338
t->need_reply = 1;
32813339
t->from_parent = thread->transaction_stack;
32823340
thread->transaction_stack = t;
@@ -3290,6 +3348,7 @@ static void binder_transaction(struct binder_proc *proc,
32903348
} else {
32913349
BUG_ON(target_node == NULL);
32923350
BUG_ON(t->buffer->async_transaction != 1);
3351+
binder_enqueue_thread_work(thread, tcomplete);
32933352
if (!binder_proc_transaction(t, target_proc, NULL))
32943353
goto err_dead_proc_or_thread;
32953354
}
@@ -3369,15 +3428,11 @@ static void binder_transaction(struct binder_proc *proc,
33693428
if (in_reply_to) {
33703429
binder_restore_priority(current, in_reply_to->saved_priority);
33713430
thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
3372-
binder_enqueue_work(thread->proc,
3373-
&thread->return_error.work,
3374-
&thread->todo);
3431+
binder_enqueue_thread_work(thread, &thread->return_error.work);
33753432
binder_send_failed_reply(in_reply_to, return_error);
33763433
} else {
33773434
thread->return_error.cmd = return_error;
3378-
binder_enqueue_work(thread->proc,
3379-
&thread->return_error.work,
3380-
&thread->todo);
3435+
binder_enqueue_thread_work(thread, &thread->return_error.work);
33813436
}
33823437
}
33833438

@@ -3681,10 +3736,9 @@ static int binder_thread_write(struct binder_proc *proc,
36813736
WARN_ON(thread->return_error.cmd !=
36823737
BR_OK);
36833738
thread->return_error.cmd = BR_ERROR;
3684-
binder_enqueue_work(
3685-
thread->proc,
3686-
&thread->return_error.work,
3687-
&thread->todo);
3739+
binder_enqueue_thread_work(
3740+
thread,
3741+
&thread->return_error.work);
36883742
binder_debug(
36893743
BINDER_DEBUG_FAILED_TRANSACTION,
36903744
"%d:%d BC_REQUEST_DEATH_NOTIFICATION failed\n",
@@ -3764,9 +3818,9 @@ static int binder_thread_write(struct binder_proc *proc,
37643818
if (thread->looper &
37653819
(BINDER_LOOPER_STATE_REGISTERED |
37663820
BINDER_LOOPER_STATE_ENTERED))
3767-
binder_enqueue_work_ilocked(
3768-
&death->work,
3769-
&thread->todo);
3821+
binder_enqueue_thread_work_ilocked(
3822+
thread,
3823+
&death->work);
37703824
else {
37713825
binder_enqueue_work_ilocked(
37723826
&death->work,
@@ -3821,8 +3875,8 @@ static int binder_thread_write(struct binder_proc *proc,
38213875
if (thread->looper &
38223876
(BINDER_LOOPER_STATE_REGISTERED |
38233877
BINDER_LOOPER_STATE_ENTERED))
3824-
binder_enqueue_work_ilocked(
3825-
&death->work, &thread->todo);
3878+
binder_enqueue_thread_work_ilocked(
3879+
thread, &death->work);
38263880
else {
38273881
binder_enqueue_work_ilocked(
38283882
&death->work,
@@ -3996,6 +4050,8 @@ static int binder_thread_read(struct binder_proc *proc,
39964050
break;
39974051
}
39984052
w = binder_dequeue_work_head_ilocked(list);
4053+
if (binder_worklist_empty_ilocked(&thread->todo))
4054+
thread->process_todo = false;
39994055

40004056
switch (w->type) {
40014057
case BINDER_WORK_TRANSACTION: {

0 commit comments

Comments
 (0)