Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1723960 > unrolled thread
| Started by | Martijn Coenen <maco@android.com> |
|---|---|
| First post | 2017-08-31 10:10 +0200 |
| Last post | 2017-08-31 10:20 +0200 |
| Articles | 6 — 1 participant |
Back to article view | Back to linux.kernel
[PATCH v2 00/13] ANDROID: binder: RT priority inheritance and small fixes. Martijn Coenen <maco@android.com> - 2017-08-31 10:10 +0200
[PATCH v2 12/13] ANDROID: binder: don't queue async transactions to thread. Martijn Coenen <maco@android.com> - 2017-08-31 10:10 +0200
[PATCH v2 08/13] ANDROID: binder: don't check prio permissions on restore. Martijn Coenen <maco@android.com> - 2017-08-31 10:20 +0200
[PATCH v2 01/13] ANDROID: binder: remove proc waitqueue Martijn Coenen <maco@android.com> - 2017-08-31 10:20 +0200
[PATCH v2 09/13] ANDROID: binder: Don't BUG_ON(!spin_is_locked()). Martijn Coenen <maco@android.com> - 2017-08-31 10:20 +0200
[PATCH v2 02/13] ANDROID: binder: push new transactions to waiting threads. Martijn Coenen <maco@android.com> - 2017-08-31 10:20 +0200
| From | Martijn Coenen <maco@android.com> |
|---|---|
| Date | 2017-08-31 10:10 +0200 |
| Subject | [PATCH v2 00/13] ANDROID: binder: RT priority inheritance and small fixes. |
| Message-ID | <uksI1-38N-3@gated-at.bofh.it> |
Changes since v1 [1]: - added more detailed commit messages and comments to the priority inheritance patches, including rationale for not using schet_setscheduler() directly, or rt_mutex prio inheritance. No functional changes. [1]: https://lkml.kernel.org/r/20170825093335.100892-1-maco@android.com --- The first six patches in this set introduce support for priority inheritance of real-time scheduling policies in binder. With the introduction of Android Treble, functionality that used to be in a single process is now split over two or more processes, which communicate using binder IPC. For latency sensitive operations such as sensor events, Bluetooth audio and rendering, inheriting the (real-time) priority of the caller is crucial to meet requirements. The implementation in this series directly calls into the scheduler to modify priorities, since I haven't found a way to make this work correctly with rt_mutex or other existing priority inheritance mechanisms. I have found the current approach to be reliable, but I'm happy to look into suggestions to make this work with existing infrastructure. More details in the patches themselves. Colin's patch adds a debug ioctl that allows us to more accurately track memory leaks, as it allows us to identify objects to which only remote processes have a reference. The subsequent patches are mostly small fixes and (hopefully) well explained in the commit messages. All patches except 'Add tracing for binder priority inheritance' have already been reviewed by Android engineers and are merged in Android's common kernel trees. --- Colin Cross (1): ANDROID: binder: Add BINDER_GET_NODE_DEBUG_INFO ioctl Martijn Coenen (12): ANDROID: binder: remove proc waitqueue ANDROID: binder: push new transactions to waiting threads. ANDROID: binder: add support for RT prio inheritance. ANDROID: binder: add min sched_policy to node. ANDROID: binder: improve priority inheritance. ANDROID: binder: add RT inheritance flag to node. ANDROID: binder: don't check prio permissions on restore. ANDROID: binder: Don't BUG_ON(!spin_is_locked()). ANDROID: binder: call poll_wait() unconditionally. ANDROID: binder: don't enqueue death notifications to thread todo. ANDROID: binder: don't queue async transactions to thread. ANDROID: binder: Add tracing for binder priority inheritance. drivers/android/binder.c | 773 ++++++++++++++++++++++++++++-------- drivers/android/binder_trace.h | 24 ++ include/uapi/linux/android/binder.h | 63 ++- 3 files changed, 689 insertions(+), 171 deletions(-) -- 2.14.1.581.gf28d330327-goog
[toc] | [next] | [standalone]
| From | Martijn Coenen <maco@android.com> |
|---|---|
| Date | 2017-08-31 10:10 +0200 |
| Subject | [PATCH v2 12/13] ANDROID: binder: don't queue async transactions to thread. |
| Message-ID | <uksI2-38N-33@gated-at.bofh.it> |
| In reply to | #1723960 |
This can cause issues with processes using the poll()
interface:
1) client sends two oneway transactions
2) the second one gets queued on async_todo
(because the server didn't handle the first one
yet)
3) server returns from poll(), picks up the
first transaction and does transaction work
4) server is done with the transaction, sends
BC_FREE_BUFFER, and the second transaction gets
moved to thread->todo
5) libbinder's handlePolledCommands() only handles
the commands in the current data buffer, so
doesn't see the new transaction
6) the server continues running and issues a new
outgoing transaction. Now, it suddenly finds
the incoming oneway transaction on its thread
todo, and returns that to userspace.
7) userspace does not expect this to happen; it
may be holding a lock while making the outgoing
transaction, and if handling the incoming
trasnaction requires taking the same lock,
userspace will deadlock.
By queueing the async transaction to the proc
workqueue, we make sure it's only picked up when
a thread is ready for proc work.
Signed-off-by: Martijn Coenen <maco@android.com>
---
drivers/android/binder.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 2d23f8699d40..7768ba280177 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3570,11 +3570,13 @@ static int binder_thread_write(struct binder_proc *proc,
BUG_ON(buf_node->proc != proc);
w = binder_dequeue_work_head_ilocked(
&buf_node->async_todo);
- if (!w)
+ if (!w) {
buf_node->has_async_transaction = 0;
- else
+ } else {
binder_enqueue_work_ilocked(
- w, &thread->todo);
+ w, &proc->todo);
+ binder_wakeup_proc_ilocked(proc);
+ }
binder_node_inner_unlock(buf_node);
}
trace_binder_transaction_buffer_release(buffer);
--
2.14.1.581.gf28d330327-goog
[toc] | [prev] | [next] | [standalone]
| From | Martijn Coenen <maco@android.com> |
|---|---|
| Date | 2017-08-31 10:20 +0200 |
| Subject | [PATCH v2 08/13] ANDROID: binder: don't check prio permissions on restore. |
| Message-ID | <uksRH-3bX-1@gated-at.bofh.it> |
| In reply to | #1723960 |
Because we have disabled RT priority inheritance for
the regular binder domain, the following can happen:
1) thread A (prio 98) calls into thread B
2) because RT prio inheritance is disabled, thread B
runs at the lowest nice (prio 100) instead
3) thread B calls back into A; A will run at prio 100
for the duration of the transaction
4) When thread A is done with the call from B, we will
try to restore the prio back to 98. But, we fail
because the process doesn't hold CAP_SYS_NICE,
neither is RLIMIT_RT_PRIO set.
While the proper fix going forward will be to
correctly apply CAP_SYS_NICE or RLIMIT_RT_PRIO,
for now it seems reasonable to not check permissions
on the restore path.
Signed-off-by: Martijn Coenen <maco@android.com>
---
drivers/android/binder.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 017693dd4ec1..0c0ecd78b9a7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1109,9 +1109,10 @@ static int to_kernel_prio(int policy, int user_priority)
}
/**
- * binder_set_priority() - sets the scheduler priority of a task
+ * binder_do_set_priority() - sets the scheduler priority of a task
* @task: task to set priority on
* @desired: desired priority to run at
+ * @verify: verify whether @task is allowed to run at @desired prio
*
* The scheduler policy of tasks is changed explicitly, because we want to
* support a few distinct features:
@@ -1145,8 +1146,9 @@ static int to_kernel_prio(int policy, int user_priority)
* temporarily runs at its original priority.
* 5) rt_mutex does not currently support PI for CFS tasks.
*/
-static void binder_set_priority(struct task_struct *task,
- struct binder_priority desired)
+static void binder_do_set_priority(struct task_struct *task,
+ struct binder_priority desired,
+ bool verify)
{
int priority; /* user-space prio value */
bool has_cap_nice;
@@ -1170,7 +1172,7 @@ static void binder_set_priority(struct task_struct *task,
priority = to_userspace_prio(policy, desired.prio);
- if (is_rt_policy(policy) && !has_cap_nice) {
+ if (verify && is_rt_policy(policy) && !has_cap_nice) {
long max_rtprio = task_rlimit(task, RLIMIT_RTPRIO);
if (max_rtprio == 0) {
@@ -1182,7 +1184,7 @@ static void binder_set_priority(struct task_struct *task,
}
}
- if (is_fair_policy(policy) && !has_cap_nice) {
+ if (verify && is_fair_policy(policy) && !has_cap_nice) {
long min_nice = rlimit_to_nice(task_rlimit(task, RLIMIT_NICE));
if (min_nice > MAX_NICE) {
@@ -1215,6 +1217,18 @@ static void binder_set_priority(struct task_struct *task,
set_user_nice(task, priority);
}
+static void binder_set_priority(struct task_struct *task,
+ struct binder_priority desired)
+{
+ binder_do_set_priority(task, desired, /* verify = */ true);
+}
+
+static void binder_restore_priority(struct task_struct *task,
+ struct binder_priority desired)
+{
+ binder_do_set_priority(task, desired, /* verify = */ false);
+}
+
static void binder_transaction_priority(struct task_struct *task,
struct binder_transaction *t,
struct binder_priority node_prio,
@@ -3251,7 +3265,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_enqueue_work_ilocked(&t->work, &target_thread->todo);
binder_inner_proc_unlock(target_proc);
wake_up_interruptible_sync(&target_thread->wait);
- binder_set_priority(current, in_reply_to->saved_priority);
+ binder_restore_priority(current, in_reply_to->saved_priority);
binder_free_transaction(in_reply_to);
} else if (!(t->flags & TF_ONE_WAY)) {
BUG_ON(t->buffer->async_transaction != 0);
@@ -3340,7 +3354,7 @@ static void binder_transaction(struct binder_proc *proc,
BUG_ON(thread->return_error.cmd != BR_OK);
if (in_reply_to) {
- binder_set_priority(current, in_reply_to->saved_priority);
+ binder_restore_priority(current, in_reply_to->saved_priority);
thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
binder_enqueue_work(thread->proc,
&thread->return_error.work,
@@ -3940,7 +3954,7 @@ static int binder_thread_read(struct binder_proc *proc,
wait_event_interruptible(binder_user_error_wait,
binder_stop_on_user_error < 2);
}
- binder_set_priority(current, proc->default_priority);
+ binder_restore_priority(current, proc->default_priority);
}
if (non_block) {
--
2.14.1.581.gf28d330327-goog
[toc] | [prev] | [next] | [standalone]
| From | Martijn Coenen <maco@android.com> |
|---|---|
| Date | 2017-08-31 10:20 +0200 |
| Subject | [PATCH v2 01/13] ANDROID: binder: remove proc waitqueue |
| Message-ID | <uksRI-3bX-19@gated-at.bofh.it> |
| In reply to | #1723960 |
Removes the process waitqueue, so that threads
can only wait on the thread waitqueue. Whenever
there is process work to do, pick a thread and
wake it up. Having the caller pick a thread is
helpful for things like priority inheritance.
This also fixes an issue with using epoll(),
since we no longer have to block on different
waitqueues.
Signed-off-by: Martijn Coenen <maco@android.com>
---
drivers/android/binder.c | 255 +++++++++++++++++++++++++++++++++--------------
1 file changed, 181 insertions(+), 74 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ba9e613b42d6..56211d025c2d 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -28,10 +28,10 @@
* binder_node_lock() and binder_node_unlock() are
* used to acq/rel
* 3) proc->inner_lock : protects the thread and node lists
- * (proc->threads, proc->nodes) and all todo lists associated
- * with the binder_proc (proc->todo, thread->todo,
- * proc->delivered_death and node->async_todo), as well as
- * thread->transaction_stack
+ * (proc->threads, proc->waiting_threads, proc->nodes)
+ * and all todo lists associated with the binder_proc
+ * (proc->todo, thread->todo, proc->delivered_death and
+ * node->async_todo), as well as thread->transaction_stack
* binder_inner_proc_lock() and binder_inner_proc_unlock()
* are used to acq/rel
*
@@ -475,6 +475,8 @@ enum binder_deferred_state {
* (protected by @outer_lock)
* @refs_by_node: rbtree of refs ordered by ref->node
* (protected by @outer_lock)
+ * @waiting_threads: threads currently waiting for proc work
+ * (protected by @inner_lock)
* @pid PID of group_leader of process
* (invariant after initialized)
* @tsk task_struct for group_leader of process
@@ -504,8 +506,6 @@ enum binder_deferred_state {
* (protected by @inner_lock)
* @requested_threads_started: number binder threads started
* (protected by @inner_lock)
- * @ready_threads: number of threads waiting for proc work
- * (protected by @inner_lock)
* @tmp_ref: temporary reference to indicate proc is in use
* (protected by @inner_lock)
* @default_priority: default scheduler priority
@@ -526,6 +526,7 @@ struct binder_proc {
struct rb_root nodes;
struct rb_root refs_by_desc;
struct rb_root refs_by_node;
+ struct list_head waiting_threads;
int pid;
struct task_struct *tsk;
struct files_struct *files;
@@ -540,7 +541,6 @@ struct binder_proc {
int max_threads;
int requested_threads;
int requested_threads_started;
- int ready_threads;
int tmp_ref;
long default_priority;
struct dentry *debugfs_entry;
@@ -556,6 +556,7 @@ enum {
BINDER_LOOPER_STATE_EXITED = 0x04,
BINDER_LOOPER_STATE_INVALID = 0x08,
BINDER_LOOPER_STATE_WAITING = 0x10,
+ BINDER_LOOPER_STATE_POLL = 0x20,
};
/**
@@ -564,6 +565,8 @@ enum {
* (invariant after initialization)
* @rb_node: element for proc->threads rbtree
* (protected by @proc->inner_lock)
+ * @waiting_thread_node: element for @proc->waiting_threads list
+ * (protected by @proc->inner_lock)
* @pid: PID for this thread
* (invariant after initialization)
* @looper: bitmap of looping state
@@ -593,6 +596,7 @@ enum {
struct binder_thread {
struct binder_proc *proc;
struct rb_node rb_node;
+ struct list_head waiting_thread_node;
int pid;
int looper; /* only modified by this thread */
bool looper_need_return; /* can be written by other thread */
@@ -920,6 +924,86 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd)
return retval;
}
+static bool binder_has_work_ilocked(struct binder_thread *thread,
+ bool do_proc_work)
+{
+ return !binder_worklist_empty_ilocked(&thread->todo) ||
+ thread->looper_need_return ||
+ (do_proc_work &&
+ !binder_worklist_empty_ilocked(&thread->proc->todo));
+}
+
+static bool binder_has_work(struct binder_thread *thread, bool do_proc_work)
+{
+ bool has_work;
+
+ binder_inner_proc_lock(thread->proc);
+ has_work = binder_has_work_ilocked(thread, do_proc_work);
+ binder_inner_proc_unlock(thread->proc);
+
+ return has_work;
+}
+
+static bool binder_available_for_proc_work_ilocked(struct binder_thread *thread)
+{
+ return !thread->transaction_stack &&
+ binder_worklist_empty_ilocked(&thread->todo) &&
+ (thread->looper & (BINDER_LOOPER_STATE_ENTERED |
+ BINDER_LOOPER_STATE_REGISTERED));
+}
+
+static void binder_wakeup_poll_threads_ilocked(struct binder_proc *proc,
+ bool sync)
+{
+ struct rb_node *n;
+ struct binder_thread *thread;
+
+ for (n = rb_first(&proc->threads); n != NULL; n = rb_next(n)) {
+ thread = rb_entry(n, struct binder_thread, rb_node);
+ if (thread->looper & BINDER_LOOPER_STATE_POLL &&
+ binder_available_for_proc_work_ilocked(thread)) {
+ if (sync)
+ wake_up_interruptible_sync(&thread->wait);
+ else
+ wake_up_interruptible(&thread->wait);
+ }
+ }
+}
+
+static void binder_wakeup_proc_ilocked(struct binder_proc *proc, bool sync)
+{
+ struct binder_thread *thread;
+
+ BUG_ON(!spin_is_locked(&proc->inner_lock));
+ thread = list_first_entry_or_null(&proc->waiting_threads,
+ struct binder_thread,
+ waiting_thread_node);
+
+ if (thread) {
+ list_del_init(&thread->waiting_thread_node);
+ if (sync)
+ wake_up_interruptible_sync(&thread->wait);
+ else
+ wake_up_interruptible(&thread->wait);
+ return;
+ }
+
+ /* Didn't find a thread waiting for proc work; this can happen
+ * in two scenarios:
+ * 1. All threads are busy handling transactions
+ * In that case, one of those threads should call back into
+ * 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.
+ */
+ binder_wakeup_poll_threads_ilocked(proc, sync);
+}
+
static void binder_set_nice(long nice)
{
long min_nice;
@@ -1138,7 +1222,7 @@ static bool binder_dec_node_nilocked(struct binder_node *node,
if (proc && (node->has_strong_ref || node->has_weak_ref)) {
if (list_empty(&node->work.entry)) {
binder_enqueue_work_ilocked(&node->work, &proc->todo);
- wake_up_interruptible(&node->proc->wait);
+ binder_wakeup_proc_ilocked(proc, false);
}
} else {
if (hlist_empty(&node->refs) && !node->local_strong_refs &&
@@ -2399,7 +2483,6 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_thread *target_thread = NULL;
struct binder_node *target_node = NULL;
struct list_head *target_list;
- wait_queue_head_t *target_wait;
struct binder_transaction *in_reply_to = NULL;
struct binder_transaction_log_entry *e;
uint32_t return_error = 0;
@@ -2409,6 +2492,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_size_t last_fixup_min_off = 0;
struct binder_context *context = proc->context;
int t_debug_id = atomic_inc_return(&binder_last_id);
+ bool wakeup_for_proc_work = false;
e = binder_transaction_log_add(&binder_transaction_log);
e->debug_id = t_debug_id;
@@ -2572,10 +2656,9 @@ static void binder_transaction(struct binder_proc *proc,
if (target_thread) {
e->to_thread = target_thread->pid;
target_list = &target_thread->todo;
- target_wait = &target_thread->wait;
} else {
target_list = &target_proc->todo;
- target_wait = &target_proc->wait;
+ wakeup_for_proc_work = true;
}
e->to_proc = target_proc->pid;
@@ -2882,7 +2965,7 @@ static void binder_transaction(struct binder_proc *proc,
binder_node_lock(target_node);
if (target_node->has_async_transaction) {
target_list = &target_node->async_todo;
- target_wait = NULL;
+ wakeup_for_proc_work = false;
} else
target_node->has_async_transaction = 1;
/*
@@ -2901,11 +2984,13 @@ static void binder_transaction(struct binder_proc *proc,
binder_inner_proc_unlock(target_proc);
binder_node_unlock(target_node);
}
- if (target_wait) {
- if (reply || !(tr->flags & TF_ONE_WAY))
- wake_up_interruptible_sync(target_wait);
- else
- wake_up_interruptible(target_wait);
+ if (target_thread) {
+ wake_up_interruptible_sync(&target_thread->wait);
+ } else if (wakeup_for_proc_work) {
+ binder_inner_proc_lock(target_proc);
+ binder_wakeup_proc_ilocked(target_proc,
+ !(tr->flags & TF_ONE_WAY));
+ binder_inner_proc_unlock(target_proc);
}
if (target_thread)
binder_thread_dec_tmpref(target_thread);
@@ -3345,12 +3430,14 @@ static int binder_thread_write(struct binder_proc *proc,
&ref->death->work,
&thread->todo);
else {
- binder_enqueue_work(
- proc,
+ binder_inner_proc_lock(proc);
+ binder_enqueue_work_ilocked(
&ref->death->work,
&proc->todo);
- wake_up_interruptible(
- &proc->wait);
+ binder_wakeup_proc_ilocked(
+ proc,
+ false);
+ binder_inner_proc_unlock(proc);
}
}
} else {
@@ -3385,8 +3472,9 @@ static int binder_thread_write(struct binder_proc *proc,
binder_enqueue_work_ilocked(
&death->work,
&proc->todo);
- wake_up_interruptible(
- &proc->wait);
+ binder_wakeup_proc_ilocked(
+ proc,
+ false);
}
} else {
BUG_ON(death->work.type != BINDER_WORK_DEAD_BINDER);
@@ -3441,7 +3529,8 @@ static int binder_thread_write(struct binder_proc *proc,
binder_enqueue_work_ilocked(
&death->work,
&proc->todo);
- wake_up_interruptible(&proc->wait);
+ binder_wakeup_proc_ilocked(
+ proc, false);
}
}
binder_inner_proc_unlock(proc);
@@ -3468,13 +3557,6 @@ static void binder_stat_br(struct binder_proc *proc,
}
}
-static int binder_has_proc_work(struct binder_proc *proc,
- struct binder_thread *thread)
-{
- return !binder_worklist_empty(proc, &proc->todo) ||
- thread->looper_need_return;
-}
-
static int binder_has_thread_work(struct binder_thread *thread)
{
return !binder_worklist_empty(thread->proc, &thread->todo) ||
@@ -3512,6 +3594,38 @@ static int binder_put_node_cmd(struct binder_proc *proc,
return 0;
}
+static int binder_wait_for_work(struct binder_thread *thread,
+ bool do_proc_work)
+{
+ DEFINE_WAIT(wait);
+ struct binder_proc *proc = thread->proc;
+ int ret = 0;
+
+ freezer_do_not_count();
+ binder_inner_proc_lock(proc);
+ for (;;) {
+ prepare_to_wait(&thread->wait, &wait, TASK_INTERRUPTIBLE);
+ if (binder_has_work_ilocked(thread, do_proc_work))
+ break;
+ if (do_proc_work)
+ list_add(&thread->waiting_thread_node,
+ &proc->waiting_threads);
+ binder_inner_proc_unlock(proc);
+ schedule();
+ binder_inner_proc_lock(proc);
+ list_del_init(&thread->waiting_thread_node);
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
+ }
+ finish_wait(&thread->wait, &wait);
+ binder_inner_proc_unlock(proc);
+ freezer_count();
+
+ return ret;
+}
+
static int binder_thread_read(struct binder_proc *proc,
struct binder_thread *thread,
binder_uintptr_t binder_buffer, size_t size,
@@ -3532,10 +3646,7 @@ static int binder_thread_read(struct binder_proc *proc,
retry:
binder_inner_proc_lock(proc);
- wait_for_proc_work = thread->transaction_stack == NULL &&
- binder_worklist_empty_ilocked(&thread->todo);
- if (wait_for_proc_work)
- proc->ready_threads++;
+ wait_for_proc_work = binder_available_for_proc_work_ilocked(thread);
binder_inner_proc_unlock(proc);
thread->looper |= BINDER_LOOPER_STATE_WAITING;
@@ -3552,23 +3663,15 @@ static int binder_thread_read(struct binder_proc *proc,
binder_stop_on_user_error < 2);
}
binder_set_nice(proc->default_priority);
- if (non_block) {
- if (!binder_has_proc_work(proc, thread))
- ret = -EAGAIN;
- } else
- ret = wait_event_freezable_exclusive(proc->wait, binder_has_proc_work(proc, thread));
+ }
+
+ if (non_block) {
+ if (!binder_has_work(thread, wait_for_proc_work))
+ ret = -EAGAIN;
} else {
- if (non_block) {
- if (!binder_has_thread_work(thread))
- ret = -EAGAIN;
- } else
- ret = wait_event_freezable(thread->wait, binder_has_thread_work(thread));
+ ret = binder_wait_for_work(thread, wait_for_proc_work);
}
- binder_inner_proc_lock(proc);
- if (wait_for_proc_work)
- proc->ready_threads--;
- binder_inner_proc_unlock(proc);
thread->looper &= ~BINDER_LOOPER_STATE_WAITING;
if (ret)
@@ -3854,7 +3957,8 @@ static int binder_thread_read(struct binder_proc *proc,
*consumed = ptr - buffer;
binder_inner_proc_lock(proc);
- if (proc->requested_threads + proc->ready_threads == 0 &&
+ if (proc->requested_threads == 0 &&
+ list_empty(&thread->proc->waiting_threads) &&
proc->requested_threads_started < proc->max_threads &&
(thread->looper & (BINDER_LOOPER_STATE_REGISTERED |
BINDER_LOOPER_STATE_ENTERED)) /* the user-space code fails to */
@@ -3965,7 +4069,7 @@ static struct binder_thread *binder_get_thread_ilocked(
thread->return_error.cmd = BR_OK;
thread->reply_error.work.type = BINDER_WORK_RETURN_ERROR;
thread->reply_error.cmd = BR_OK;
-
+ INIT_LIST_HEAD(&new_thread->waiting_thread_node);
return thread;
}
@@ -4078,28 +4182,24 @@ static unsigned int binder_poll(struct file *filp,
{
struct binder_proc *proc = filp->private_data;
struct binder_thread *thread = NULL;
- int wait_for_proc_work;
+ bool wait_for_proc_work;
thread = binder_get_thread(proc);
binder_inner_proc_lock(thread->proc);
- wait_for_proc_work = thread->transaction_stack == NULL &&
- binder_worklist_empty_ilocked(&thread->todo);
+ thread->looper |= BINDER_LOOPER_STATE_POLL;
+ wait_for_proc_work = binder_available_for_proc_work_ilocked(thread);
+
binder_inner_proc_unlock(thread->proc);
- if (wait_for_proc_work) {
- if (binder_has_proc_work(proc, thread))
- return POLLIN;
- poll_wait(filp, &proc->wait, wait);
- if (binder_has_proc_work(proc, thread))
- return POLLIN;
- } else {
- if (binder_has_thread_work(thread))
- return POLLIN;
- poll_wait(filp, &thread->wait, wait);
- if (binder_has_thread_work(thread))
- return POLLIN;
- }
+ if (binder_has_work(thread, wait_for_proc_work))
+ return POLLIN;
+
+ poll_wait(filp, &thread->wait, wait);
+
+ if (binder_has_thread_work(thread))
+ return POLLIN;
+
return 0;
}
@@ -4146,8 +4246,10 @@ static int binder_ioctl_write_read(struct file *filp,
&bwr.read_consumed,
filp->f_flags & O_NONBLOCK);
trace_binder_read_done(ret);
- if (!binder_worklist_empty(proc, &proc->todo))
- wake_up_interruptible(&proc->wait);
+ binder_inner_proc_lock(proc);
+ if (!binder_worklist_empty_ilocked(&proc->todo))
+ binder_wakeup_proc_ilocked(proc, false);
+ binder_inner_proc_unlock(proc);
if (ret < 0) {
if (copy_to_user(ubuf, &bwr, sizeof(bwr)))
ret = -EFAULT;
@@ -4389,7 +4491,6 @@ static int binder_open(struct inode *nodp, struct file *filp)
get_task_struct(current->group_leader);
proc->tsk = current->group_leader;
INIT_LIST_HEAD(&proc->todo);
- init_waitqueue_head(&proc->wait);
proc->default_priority = task_nice(current);
binder_dev = container_of(filp->private_data, struct binder_device,
miscdev);
@@ -4399,6 +4500,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
binder_stats_created(BINDER_STAT_PROC);
proc->pid = current->group_leader->pid;
INIT_LIST_HEAD(&proc->delivered_death);
+ INIT_LIST_HEAD(&proc->waiting_threads);
filp->private_data = proc;
mutex_lock(&binder_procs_lock);
@@ -4450,7 +4552,6 @@ static void binder_deferred_flush(struct binder_proc *proc)
}
}
binder_inner_proc_unlock(proc);
- wake_up_interruptible_all(&proc->wait);
binder_debug(BINDER_DEBUG_OPEN_CLOSE,
"binder_flush: %d woke %d threads\n", proc->pid,
@@ -4519,7 +4620,7 @@ static int binder_node_release(struct binder_node *node, int refs)
ref->death->work.type = BINDER_WORK_DEAD_BINDER;
binder_enqueue_work_ilocked(&ref->death->work,
&ref->proc->todo);
- wake_up_interruptible(&ref->proc->wait);
+ binder_wakeup_proc_ilocked(ref->proc, false);
binder_inner_proc_unlock(ref->proc);
}
@@ -5007,23 +5108,29 @@ static void print_binder_proc_stats(struct seq_file *m,
struct binder_proc *proc)
{
struct binder_work *w;
+ struct binder_thread *thread;
struct rb_node *n;
- int count, strong, weak;
+ int count, strong, weak, ready_threads;
size_t free_async_space =
binder_alloc_get_free_async_space(&proc->alloc);
seq_printf(m, "proc %d\n", proc->pid);
seq_printf(m, "context %s\n", proc->context->name);
count = 0;
+ ready_threads = 0;
binder_inner_proc_lock(proc);
for (n = rb_first(&proc->threads); n != NULL; n = rb_next(n))
count++;
+
+ list_for_each_entry(thread, &proc->waiting_threads, waiting_thread_node)
+ ready_threads++;
+
seq_printf(m, " threads: %d\n", count);
seq_printf(m, " requested threads: %d+%d/%d\n"
" ready threads %d\n"
" free async space %zd\n", proc->requested_threads,
proc->requested_threads_started, proc->max_threads,
- proc->ready_threads,
+ ready_threads,
free_async_space);
count = 0;
for (n = rb_first(&proc->nodes); n != NULL; n = rb_next(n))
--
2.14.1.581.gf28d330327-goog
[toc] | [prev] | [next] | [standalone]
| From | Martijn Coenen <maco@android.com> |
|---|---|
| Date | 2017-08-31 10:20 +0200 |
| Subject | [PATCH v2 09/13] ANDROID: binder: Don't BUG_ON(!spin_is_locked()). |
| Message-ID | <uksRI-3bX-23@gated-at.bofh.it> |
| In reply to | #1723960 |
Because is_spin_locked() always returns false on UP
systems.
Use assert_spin_locked() instead, and remove the
WARN_ON() instances, since those were easy to verify.
Signed-off-by: Martijn Coenen <maco@android.com>
---
drivers/android/binder.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 0c0ecd78b9a7..7da9c42583f7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1013,7 +1013,7 @@ binder_select_thread_ilocked(struct binder_proc *proc)
{
struct binder_thread *thread;
- BUG_ON(!spin_is_locked(&proc->inner_lock));
+ assert_spin_locked(&proc->inner_lock);
thread = list_first_entry_or_null(&proc->waiting_threads,
struct binder_thread,
waiting_thread_node);
@@ -1044,7 +1044,7 @@ static void binder_wakeup_thread_ilocked(struct binder_proc *proc,
struct binder_thread *thread,
bool sync)
{
- BUG_ON(!spin_is_locked(&proc->inner_lock));
+ assert_spin_locked(&proc->inner_lock);
if (thread) {
if (sync)
@@ -1273,7 +1273,7 @@ static struct binder_node *binder_get_node_ilocked(struct binder_proc *proc,
struct rb_node *n = proc->nodes.rb_node;
struct binder_node *node;
- BUG_ON(!spin_is_locked(&proc->inner_lock));
+ assert_spin_locked(&proc->inner_lock);
while (n) {
node = rb_entry(n, struct binder_node, rb_node);
@@ -1319,7 +1319,8 @@ static struct binder_node *binder_init_node_ilocked(
__u32 flags = fp ? fp->flags : 0;
s8 priority;
- BUG_ON(!spin_is_locked(&proc->inner_lock));
+ assert_spin_locked(&proc->inner_lock);
+
while (*p) {
parent = *p;
@@ -1398,9 +1399,9 @@ static int binder_inc_node_nilocked(struct binder_node *node, int strong,
{
struct binder_proc *proc = node->proc;
- BUG_ON(!spin_is_locked(&node->lock));
+ assert_spin_locked(&node->lock);
if (proc)
- BUG_ON(!spin_is_locked(&proc->inner_lock));
+ assert_spin_locked(&proc->inner_lock);
if (strong) {
if (internal) {
if (target_list == NULL &&
@@ -1451,9 +1452,9 @@ static bool binder_dec_node_nilocked(struct binder_node *node,
{
struct binder_proc *proc = node->proc;
- BUG_ON(!spin_is_locked(&node->lock));
+ assert_spin_locked(&node->lock);
if (proc)
- BUG_ON(!spin_is_locked(&proc->inner_lock));
+ assert_spin_locked(&proc->inner_lock);
if (strong) {
if (internal)
node->internal_strong_refs--;
@@ -1977,7 +1978,7 @@ static void binder_pop_transaction_ilocked(struct binder_thread *target_thread,
struct binder_transaction *t)
{
BUG_ON(!target_thread);
- BUG_ON(!spin_is_locked(&target_thread->proc->inner_lock));
+ assert_spin_locked(&target_thread->proc->inner_lock);
BUG_ON(target_thread->transaction_stack != t);
BUG_ON(target_thread->transaction_stack->from != target_thread);
target_thread->transaction_stack =
@@ -5123,7 +5124,6 @@ static void print_binder_transaction_ilocked(struct seq_file *m,
struct binder_proc *to_proc;
struct binder_buffer *buffer = t->buffer;
- WARN_ON(!spin_is_locked(&proc->inner_lock));
spin_lock(&t->lock);
to_proc = t->to_proc;
seq_printf(m,
@@ -5212,7 +5212,6 @@ static void print_binder_thread_ilocked(struct seq_file *m,
size_t start_pos = m->count;
size_t header_pos;
- WARN_ON(!spin_is_locked(&thread->proc->inner_lock));
seq_printf(m, " thread %d: l %02x need_return %d tr %d\n",
thread->pid, thread->looper,
thread->looper_need_return,
@@ -5249,10 +5248,6 @@ static void print_binder_node_nilocked(struct seq_file *m,
struct binder_work *w;
int count;
- WARN_ON(!spin_is_locked(&node->lock));
- if (node->proc)
- WARN_ON(!spin_is_locked(&node->proc->inner_lock));
-
count = 0;
hlist_for_each_entry(ref, &node->refs, node_entry)
count++;
@@ -5279,7 +5274,6 @@ static void print_binder_node_nilocked(struct seq_file *m,
static void print_binder_ref_olocked(struct seq_file *m,
struct binder_ref *ref)
{
- WARN_ON(!spin_is_locked(&ref->proc->outer_lock));
binder_node_lock(ref->node);
seq_printf(m, " ref %d: desc %d %snode %d s %d w %d d %pK\n",
ref->data.debug_id, ref->data.desc,
--
2.14.1.581.gf28d330327-goog
[toc] | [prev] | [next] | [standalone]
| From | Martijn Coenen <maco@android.com> |
|---|---|
| Date | 2017-08-31 10:20 +0200 |
| Subject | [PATCH v2 02/13] ANDROID: binder: push new transactions to waiting threads. |
| Message-ID | <uksRI-3bX-35@gated-at.bofh.it> |
| In reply to | #1723960 |
Instead of pushing new transactions to the process
waitqueue, select a thread that is waiting on proc
work to handle the transaction. This will make it
easier to improve priority inheritance in future
patches, by setting the priority before we wake up
a thread.
If we can't find a waiting thread, submit the work
to the proc waitqueue instead as we did previously.
Signed-off-by: Martijn Coenen <maco@android.com>
---
drivers/android/binder.c | 181 +++++++++++++++++++++++++++++++++--------------
1 file changed, 127 insertions(+), 54 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 56211d025c2d..5366726db968 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -970,7 +970,20 @@ static void binder_wakeup_poll_threads_ilocked(struct binder_proc *proc,
}
}
-static void binder_wakeup_proc_ilocked(struct binder_proc *proc, bool sync)
+/**
+ * binder_select_thread_ilocked() - selects a thread for doing proc work.
+ * @proc: process to select a thread from
+ *
+ * Note that calling this function moves the thread off the waiting_threads
+ * list, so it can only be woken up by the caller of this function, or a
+ * signal. Therefore, callers *should* always wake up the thread this function
+ * returns.
+ *
+ * Return: If there's a thread currently waiting for process work,
+ * returns that thread. Otherwise returns NULL.
+ */
+static struct binder_thread *
+binder_select_thread_ilocked(struct binder_proc *proc)
{
struct binder_thread *thread;
@@ -979,8 +992,35 @@ static void binder_wakeup_proc_ilocked(struct binder_proc *proc, bool sync)
struct binder_thread,
waiting_thread_node);
- if (thread) {
+ if (thread)
list_del_init(&thread->waiting_thread_node);
+
+ return thread;
+}
+
+/**
+ * binder_wakeup_thread_ilocked() - wakes up a thread for doing proc work.
+ * @proc: process to wake up a thread in
+ * @thread: specific thread to wake-up (may be NULL)
+ * @sync: whether to do a synchronous wake-up
+ *
+ * This function wakes up a thread in the @proc process.
+ * The caller may provide a specific thread to wake-up in
+ * the @thread parameter. If @thread is NULL, this function
+ * will wake up threads that have called poll().
+ *
+ * Note that for this function to work as expected, callers
+ * should first call binder_select_thread() to find a thread
+ * to handle the work (if they don't have a thread already),
+ * and pass the result into the @thread parameter.
+ */
+static void binder_wakeup_thread_ilocked(struct binder_proc *proc,
+ struct binder_thread *thread,
+ bool sync)
+{
+ BUG_ON(!spin_is_locked(&proc->inner_lock));
+
+ if (thread) {
if (sync)
wake_up_interruptible_sync(&thread->wait);
else
@@ -1004,6 +1044,13 @@ static void binder_wakeup_proc_ilocked(struct binder_proc *proc, bool sync)
binder_wakeup_poll_threads_ilocked(proc, sync);
}
+static void binder_wakeup_proc_ilocked(struct binder_proc *proc)
+{
+ struct binder_thread *thread = binder_select_thread_ilocked(proc);
+
+ binder_wakeup_thread_ilocked(proc, thread, /* sync = */false);
+}
+
static void binder_set_nice(long nice)
{
long min_nice;
@@ -1222,7 +1269,7 @@ static bool binder_dec_node_nilocked(struct binder_node *node,
if (proc && (node->has_strong_ref || node->has_weak_ref)) {
if (list_empty(&node->work.entry)) {
binder_enqueue_work_ilocked(&node->work, &proc->todo);
- binder_wakeup_proc_ilocked(proc, false);
+ binder_wakeup_proc_ilocked(proc);
}
} else {
if (hlist_empty(&node->refs) && !node->local_strong_refs &&
@@ -2468,6 +2515,73 @@ static int binder_fixup_parent(struct binder_transaction *t,
return 0;
}
+/**
+ * binder_proc_transaction() - sends a transaction to a process and wakes it up
+ * @t: transaction to send
+ * @proc: process to send the transaction to
+ * @thread: thread in @proc to send the transaction to (may be NULL)
+ *
+ * This function queues a transaction to the specified process. It will try
+ * to find a thread in the target process to handle the transaction and
+ * wake it up. If no thread is found, the work is queued to the proc
+ * waitqueue.
+ *
+ * If the @thread parameter is not NULL, the transaction is always queued
+ * to the waitlist of that specific thread.
+ *
+ * Return: true if the transactions was successfully queued
+ * false if the target process or thread is dead
+ */
+static bool binder_proc_transaction(struct binder_transaction *t,
+ struct binder_proc *proc,
+ struct binder_thread *thread)
+{
+ struct list_head *target_list = NULL;
+ struct binder_node *node = t->buffer->target_node;
+ bool oneway = !!(t->flags & TF_ONE_WAY);
+ bool wakeup = true;
+
+ BUG_ON(!node);
+ binder_node_lock(node);
+ if (oneway) {
+ BUG_ON(thread);
+ if (node->has_async_transaction) {
+ target_list = &node->async_todo;
+ wakeup = false;
+ } else {
+ node->has_async_transaction = 1;
+ }
+ }
+
+ binder_inner_proc_lock(proc);
+
+ if (proc->is_dead || (thread && thread->is_dead)) {
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(node);
+ return false;
+ }
+
+ if (!thread && !target_list)
+ thread = binder_select_thread_ilocked(proc);
+
+ if (thread)
+ target_list = &thread->todo;
+ else if (!target_list)
+ target_list = &proc->todo;
+ else
+ BUG_ON(target_list != &node->async_todo);
+
+ binder_enqueue_work_ilocked(&t->work, target_list);
+
+ if (wakeup)
+ binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
+
+ binder_inner_proc_unlock(proc);
+ binder_node_unlock(node);
+
+ return true;
+}
+
static void binder_transaction(struct binder_proc *proc,
struct binder_thread *thread,
struct binder_transaction_data *tr, int reply,
@@ -2482,7 +2596,6 @@ static void binder_transaction(struct binder_proc *proc,
struct binder_proc *target_proc = NULL;
struct binder_thread *target_thread = NULL;
struct binder_node *target_node = NULL;
- struct list_head *target_list;
struct binder_transaction *in_reply_to = NULL;
struct binder_transaction_log_entry *e;
uint32_t return_error = 0;
@@ -2492,7 +2605,6 @@ static void binder_transaction(struct binder_proc *proc,
binder_size_t last_fixup_min_off = 0;
struct binder_context *context = proc->context;
int t_debug_id = atomic_inc_return(&binder_last_id);
- bool wakeup_for_proc_work = false;
e = binder_transaction_log_add(&binder_transaction_log);
e->debug_id = t_debug_id;
@@ -2653,13 +2765,8 @@ static void binder_transaction(struct binder_proc *proc,
}
binder_inner_proc_unlock(proc);
}
- if (target_thread) {
+ if (target_thread)
e->to_thread = target_thread->pid;
- target_list = &target_thread->todo;
- } else {
- target_list = &target_proc->todo;
- wakeup_for_proc_work = true;
- }
e->to_proc = target_proc->pid;
/* TODO: reuse incoming transaction for reply */
@@ -2938,8 +3045,9 @@ static void binder_transaction(struct binder_proc *proc,
}
BUG_ON(t->buffer->async_transaction != 0);
binder_pop_transaction_ilocked(target_thread, in_reply_to);
- binder_enqueue_work_ilocked(&t->work, target_list);
+ binder_enqueue_work_ilocked(&t->work, &target_thread->todo);
binder_inner_proc_unlock(target_proc);
+ wake_up_interruptible_sync(&target_thread->wait);
binder_free_transaction(in_reply_to);
} else if (!(t->flags & TF_ONE_WAY)) {
BUG_ON(t->buffer->async_transaction != 0);
@@ -2948,49 +3056,17 @@ static void binder_transaction(struct binder_proc *proc,
t->from_parent = thread->transaction_stack;
thread->transaction_stack = t;
binder_inner_proc_unlock(proc);
- binder_inner_proc_lock(target_proc);
- if (target_proc->is_dead ||
- (target_thread && target_thread->is_dead)) {
- binder_inner_proc_unlock(target_proc);
+ if (!binder_proc_transaction(t, target_proc, target_thread)) {
binder_inner_proc_lock(proc);
binder_pop_transaction_ilocked(thread, t);
binder_inner_proc_unlock(proc);
goto err_dead_proc_or_thread;
}
- binder_enqueue_work_ilocked(&t->work, target_list);
- binder_inner_proc_unlock(target_proc);
} else {
BUG_ON(target_node == NULL);
BUG_ON(t->buffer->async_transaction != 1);
- binder_node_lock(target_node);
- if (target_node->has_async_transaction) {
- target_list = &target_node->async_todo;
- wakeup_for_proc_work = false;
- } else
- target_node->has_async_transaction = 1;
- /*
- * Test/set of has_async_transaction
- * must be atomic with enqueue on
- * async_todo
- */
- binder_inner_proc_lock(target_proc);
- if (target_proc->is_dead ||
- (target_thread && target_thread->is_dead)) {
- binder_inner_proc_unlock(target_proc);
- binder_node_unlock(target_node);
+ if (!binder_proc_transaction(t, target_proc, NULL))
goto err_dead_proc_or_thread;
- }
- binder_enqueue_work_ilocked(&t->work, target_list);
- binder_inner_proc_unlock(target_proc);
- binder_node_unlock(target_node);
- }
- if (target_thread) {
- wake_up_interruptible_sync(&target_thread->wait);
- } else if (wakeup_for_proc_work) {
- binder_inner_proc_lock(target_proc);
- binder_wakeup_proc_ilocked(target_proc,
- !(tr->flags & TF_ONE_WAY));
- binder_inner_proc_unlock(target_proc);
}
if (target_thread)
binder_thread_dec_tmpref(target_thread);
@@ -3435,8 +3511,7 @@ static int binder_thread_write(struct binder_proc *proc,
&ref->death->work,
&proc->todo);
binder_wakeup_proc_ilocked(
- proc,
- false);
+ proc);
binder_inner_proc_unlock(proc);
}
}
@@ -3473,8 +3548,7 @@ static int binder_thread_write(struct binder_proc *proc,
&death->work,
&proc->todo);
binder_wakeup_proc_ilocked(
- proc,
- false);
+ proc);
}
} else {
BUG_ON(death->work.type != BINDER_WORK_DEAD_BINDER);
@@ -3529,8 +3603,7 @@ static int binder_thread_write(struct binder_proc *proc,
binder_enqueue_work_ilocked(
&death->work,
&proc->todo);
- binder_wakeup_proc_ilocked(
- proc, false);
+ binder_wakeup_proc_ilocked(proc);
}
}
binder_inner_proc_unlock(proc);
@@ -4248,7 +4321,7 @@ static int binder_ioctl_write_read(struct file *filp,
trace_binder_read_done(ret);
binder_inner_proc_lock(proc);
if (!binder_worklist_empty_ilocked(&proc->todo))
- binder_wakeup_proc_ilocked(proc, false);
+ binder_wakeup_proc_ilocked(proc);
binder_inner_proc_unlock(proc);
if (ret < 0) {
if (copy_to_user(ubuf, &bwr, sizeof(bwr)))
@@ -4620,7 +4693,7 @@ static int binder_node_release(struct binder_node *node, int refs)
ref->death->work.type = BINDER_WORK_DEAD_BINDER;
binder_enqueue_work_ilocked(&ref->death->work,
&ref->proc->todo);
- binder_wakeup_proc_ilocked(ref->proc, false);
+ binder_wakeup_proc_ilocked(ref->proc);
binder_inner_proc_unlock(ref->proc);
}
--
2.14.1.581.gf28d330327-goog
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web