This && usage is a bit confusing. I reworked these to be actual
if statements so it's clear what is going on.
Signed-off-by: Daniel Walker <[email protected]>
---
drivers/staging/android/binder.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 17d89a8..c37336d 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2146,7 +2146,7 @@ static int binder_thread_read(struct binder_proc *proc,
void __user *end = buffer + size;
int ret = 0;
- int wait_for_proc_work;
+ int wait_for_proc_work = 0;
if (*consumed == 0) {
if (put_user(BR_NOOP, (uint32_t __user *)ptr))
@@ -2155,8 +2155,8 @@ static int binder_thread_read(struct binder_proc *proc,
}
retry:
- wait_for_proc_work = thread->transaction_stack == NULL &&
- list_empty(&thread->todo);
+ if (list_empty(&thread->todo) && thread->transaction_stack == NULL)
+ wait_for_proc_work = 1;
if (thread->return_error != BR_OK && ptr < end) {
if (thread->return_error2 != BR_OK) {
@@ -2539,13 +2539,15 @@ 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;
+ int wait_for_proc_work = 0;
mutex_lock(&binder_lock);
thread = binder_get_thread(proc);
- wait_for_proc_work = thread->transaction_stack == NULL &&
- list_empty(&thread->todo) && thread->return_error == BR_OK;
+ if (list_empty(&thread->todo) && thread->transaction_stack == NULL &&
+ thread->return_error == BR_OK)
+ wait_for_proc_work = 1;
+
mutex_unlock(&binder_lock);
if (wait_for_proc_work) {
--
1.5.4.3
I removed the binder_transaction_buffer_release predefine, and put
the actual function in place of it.
Signed-off-by: Daniel Walker <[email protected]>
---
drivers/staging/android/binder.c | 138 ++++++++++++++++++-------------------
1 files changed, 67 insertions(+), 71 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index c4a596f..ec86808 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1270,7 +1270,73 @@ static void binder_send_failed_reply(struct binder_transaction *t,
static void binder_transaction_buffer_release(struct binder_proc *proc,
struct binder_buffer *buffer,
- size_t *failed_at);
+ size_t *failed_at)
+{
+ size_t *offp, *off_end;
+ int debug_id = buffer->debug_id;
+
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ "binder: %d buffer release %d, size %zd-%zd, failed at %p\n",
+ proc->pid, buffer->debug_id,
+ buffer->data_size, buffer->offsets_size, failed_at);
+
+ if (buffer->target_node)
+ binder_dec_node(buffer->target_node, 1, 0);
+
+ offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));
+ if (failed_at)
+ off_end = failed_at;
+ else
+ off_end = (void *)offp + buffer->offsets_size;
+ for (; offp < off_end; offp++) {
+ struct flat_binder_object *fp;
+ if (*offp > buffer->data_size - sizeof(*fp) ||
+ buffer->data_size < sizeof(*fp) ||
+ !IS_ALIGNED(*offp, sizeof(void *))) {
+ printk(KERN_ERR "binder: transaction release %d bad"
+ "offset %zd, size %zd\n", debug_id, *offp, buffer->data_size);
+ continue;
+ }
+ fp = (struct flat_binder_object *)(buffer->data + *offp);
+ switch (fp->type) {
+ case BINDER_TYPE_BINDER:
+ case BINDER_TYPE_WEAK_BINDER: {
+ struct binder_node *node = binder_get_node(proc, fp->binder);
+ if (node == NULL) {
+ printk(KERN_ERR "binder: transaction release %d bad node %p\n", debug_id, fp->binder);
+ break;
+ }
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " node %d u%p\n",
+ node->debug_id, node->ptr);
+ binder_dec_node(node, fp->type == BINDER_TYPE_BINDER, 0);
+ } break;
+ case BINDER_TYPE_HANDLE:
+ case BINDER_TYPE_WEAK_HANDLE: {
+ struct binder_ref *ref = binder_get_ref(proc, fp->handle);
+ if (ref == NULL) {
+ printk(KERN_ERR "binder: transaction release %d bad handle %ld\n", debug_id, fp->handle);
+ break;
+ }
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " ref %d desc %d (node %d)\n",
+ ref->debug_id, ref->desc, ref->node->debug_id);
+ binder_dec_ref(ref, fp->type == BINDER_TYPE_HANDLE);
+ } break;
+
+ case BINDER_TYPE_FD:
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " fd %ld\n", fp->handle);
+ if (failed_at)
+ task_close_fd(proc, fp->handle);
+ break;
+
+ default:
+ printk(KERN_ERR "binder: transaction release %d bad object type %lx\n", debug_id, fp->type);
+ break;
+ }
+ }
+}
static void binder_transaction(struct binder_proc *proc,
struct binder_thread *thread,
@@ -1679,76 +1745,6 @@ err_no_context_mgr_node:
thread->return_error = return_error;
}
-static void binder_transaction_buffer_release(struct binder_proc *proc,
- struct binder_buffer *buffer,
- size_t *failed_at)
-{
- size_t *offp, *off_end;
- int debug_id = buffer->debug_id;
-
- binder_debug(BINDER_DEBUG_TRANSACTION,
- "binder: %d buffer release %d, size %zd-%zd, failed at %p\n",
- proc->pid, buffer->debug_id,
- buffer->data_size, buffer->offsets_size, failed_at);
-
- if (buffer->target_node)
- binder_dec_node(buffer->target_node, 1, 0);
-
- offp = (size_t *)(buffer->data + ALIGN(buffer->data_size, sizeof(void *)));
- if (failed_at)
- off_end = failed_at;
- else
- off_end = (void *)offp + buffer->offsets_size;
- for (; offp < off_end; offp++) {
- struct flat_binder_object *fp;
- if (*offp > buffer->data_size - sizeof(*fp) ||
- buffer->data_size < sizeof(*fp) ||
- !IS_ALIGNED(*offp, sizeof(void *))) {
- printk(KERN_ERR "binder: transaction release %d bad"
- "offset %zd, size %zd\n", debug_id, *offp, buffer->data_size);
- continue;
- }
- fp = (struct flat_binder_object *)(buffer->data + *offp);
- switch (fp->type) {
- case BINDER_TYPE_BINDER:
- case BINDER_TYPE_WEAK_BINDER: {
- struct binder_node *node = binder_get_node(proc, fp->binder);
- if (node == NULL) {
- printk(KERN_ERR "binder: transaction release %d bad node %p\n", debug_id, fp->binder);
- break;
- }
- binder_debug(BINDER_DEBUG_TRANSACTION,
- " node %d u%p\n",
- node->debug_id, node->ptr);
- binder_dec_node(node, fp->type == BINDER_TYPE_BINDER, 0);
- } break;
- case BINDER_TYPE_HANDLE:
- case BINDER_TYPE_WEAK_HANDLE: {
- struct binder_ref *ref = binder_get_ref(proc, fp->handle);
- if (ref == NULL) {
- printk(KERN_ERR "binder: transaction release %d bad handle %ld\n", debug_id, fp->handle);
- break;
- }
- binder_debug(BINDER_DEBUG_TRANSACTION,
- " ref %d desc %d (node %d)\n",
- ref->debug_id, ref->desc, ref->node->debug_id);
- binder_dec_ref(ref, fp->type == BINDER_TYPE_HANDLE);
- } break;
-
- case BINDER_TYPE_FD:
- binder_debug(BINDER_DEBUG_TRANSACTION,
- " fd %ld\n", fp->handle);
- if (failed_at)
- task_close_fd(proc, fp->handle);
- break;
-
- default:
- printk(KERN_ERR "binder: transaction release %d bad object type %lx\n", debug_id, fp->type);
- break;
- }
- }
-}
-
int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
void __user *buffer, int size, signed long *consumed)
{
--
1.5.4.3
An initial cleanup of all the binder_stat statements. The binder
command and return stats still need some assistance tho.
Signed-off-by: Daniel Walker <[email protected]>
---
drivers/staging/android/binder.c | 54 ++++++++++++++++++++++---------------
1 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 01711e3..027f17e 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -117,7 +117,7 @@ module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
binder_stop_on_user_error = 2; \
} while (0)
-enum {
+enum binder_stat_types {
BINDER_STAT_PROC,
BINDER_STAT_THREAD,
BINDER_STAT_NODE,
@@ -137,6 +137,16 @@ struct binder_stats {
static struct binder_stats binder_stats;
+static inline void binder_stats_deleted(enum binder_stat_types type)
+{
+ binder_stats.obj_deleted[type]++;
+}
+
+static inline void binder_stats_created(enum binder_stat_types type)
+{
+ binder_stats.obj_created[type]++;
+}
+
struct binder_transaction_log_entry {
int debug_id;
int call_type;
@@ -937,7 +947,7 @@ static struct binder_node *binder_new_node(struct binder_proc *proc,
node = kzalloc(sizeof(*node), GFP_KERNEL);
if (node == NULL)
return NULL;
- binder_stats.obj_created[BINDER_STAT_NODE]++;
+ binder_stats_created(BINDER_STAT_NODE);
rb_link_node(&node->rb_node, parent, p);
rb_insert_color(&node->rb_node, &proc->nodes);
node->debug_id = ++binder_last_id;
@@ -1025,7 +1035,7 @@ static int binder_dec_node(struct binder_node *node, int strong, int internal)
node->debug_id);
}
kfree(node);
- binder_stats.obj_deleted[BINDER_STAT_NODE]++;
+ binder_stats_deleted(BINDER_STAT_NODE);
}
}
@@ -1074,7 +1084,7 @@ static struct binder_ref *binder_get_ref_for_node(struct binder_proc *proc,
new_ref = kzalloc(sizeof(*ref), GFP_KERNEL);
if (new_ref == NULL)
return NULL;
- binder_stats.obj_created[BINDER_STAT_REF]++;
+ binder_stats_created(BINDER_STAT_REF);
new_ref->debug_id = ++binder_last_id;
new_ref->proc = proc;
new_ref->node = node;
@@ -1139,10 +1149,10 @@ static void binder_delete_ref(struct binder_ref *ref)
ref->debug_id, ref->desc);
list_del(&ref->death->work.entry);
kfree(ref->death);
- binder_stats.obj_deleted[BINDER_STAT_DEATH]++;
+ binder_stats_deleted(BINDER_STAT_DEATH);
}
kfree(ref);
- binder_stats.obj_deleted[BINDER_STAT_REF]++;
+ binder_stats_deleted(BINDER_STAT_REF);
}
static int binder_inc_ref(struct binder_ref *ref, int strong,
@@ -1214,7 +1224,7 @@ static void binder_pop_transaction(struct binder_thread *target_thread,
if (t->buffer)
t->buffer->transaction = NULL;
kfree(t);
- binder_stats.obj_deleted[BINDER_STAT_TRANSACTION]++;
+ binder_stats_deleted(BINDER_STAT_TRANSACTION);
}
static void binder_send_failed_reply(struct binder_transaction *t,
@@ -1471,14 +1481,14 @@ static void binder_transaction(struct binder_proc *proc,
return_error = BR_FAILED_REPLY;
goto err_alloc_t_failed;
}
- binder_stats.obj_created[BINDER_STAT_TRANSACTION]++;
+ binder_stats_created(BINDER_STAT_TRANSACTION);
tcomplete = kzalloc(sizeof(*tcomplete), GFP_KERNEL);
if (tcomplete == NULL) {
return_error = BR_FAILED_REPLY;
goto err_alloc_tcomplete_failed;
}
- binder_stats.obj_created[BINDER_STAT_TRANSACTION_COMPLETE]++;
+ binder_stats_created(BINDER_STAT_TRANSACTION_COMPLETE);
t->debug_id = ++binder_last_id;
e->debug_id = t->debug_id;
@@ -1720,10 +1730,10 @@ err_copy_data_failed:
binder_free_buf(target_proc, t->buffer);
err_binder_alloc_buf_failed:
kfree(tcomplete);
- binder_stats.obj_deleted[BINDER_STAT_TRANSACTION_COMPLETE]++;
+ binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
err_alloc_tcomplete_failed:
kfree(t);
- binder_stats.obj_deleted[BINDER_STAT_TRANSACTION]++;
+ binder_stats_deleted(BINDER_STAT_TRANSACTION);
err_alloc_t_failed:
err_bad_call_stack:
err_empty_call_stack:
@@ -2039,7 +2049,7 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
proc->pid, thread->pid);
break;
}
- binder_stats.obj_created[BINDER_STAT_DEATH]++;
+ binder_stats_created(BINDER_STAT_DEATH);
INIT_LIST_HEAD(&death->work.entry);
death->cookie = cookie;
ref->death = death;
@@ -2266,7 +2276,7 @@ retry:
list_del(&w->entry);
kfree(w);
- binder_stats.obj_deleted[BINDER_STAT_TRANSACTION_COMPLETE]++;
+ binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
} break;
case BINDER_WORK_NODE: {
struct binder_node *node = container_of(w, struct binder_node, work);
@@ -2319,7 +2329,7 @@ retry:
node->ptr, node->cookie);
rb_erase(&node->rb_node, &proc->nodes);
kfree(node);
- binder_stats.obj_deleted[BINDER_STAT_NODE]++;
+ binder_stats_deleted(BINDER_STAT_NODE);
} else {
binder_debug(BINDER_DEBUG_INTERNAL_REFS,
"binder: %d:%d node %d u%p c%p state unchanged\n",
@@ -2356,7 +2366,7 @@ retry:
if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION) {
list_del(&w->entry);
kfree(death);
- binder_stats.obj_deleted[BINDER_STAT_DEATH]++;
+ binder_stats_deleted(BINDER_STAT_DEATH);
} else
list_move(&w->entry, &proc->delivered_death);
if (cmd == BR_DEAD_BINDER)
@@ -2433,7 +2443,7 @@ retry:
} else {
t->buffer->transaction = NULL;
kfree(t);
- binder_stats.obj_deleted[BINDER_STAT_TRANSACTION]++;
+ binder_stats_deleted(BINDER_STAT_TRANSACTION);
}
break;
}
@@ -2472,7 +2482,7 @@ static void binder_release_work(struct list_head *list)
} break;
case BINDER_WORK_TRANSACTION_COMPLETE: {
kfree(w);
- binder_stats.obj_deleted[BINDER_STAT_TRANSACTION_COMPLETE]++;
+ binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
} break;
default:
break;
@@ -2502,7 +2512,7 @@ static struct binder_thread *binder_get_thread(struct binder_proc *proc)
thread = kzalloc(sizeof(*thread), GFP_KERNEL);
if (thread == NULL)
return NULL;
- binder_stats.obj_created[BINDER_STAT_THREAD]++;
+ binder_stats_created(BINDER_STAT_THREAD);
thread->proc = proc;
thread->pid = current->pid;
init_waitqueue_head(&thread->wait);
@@ -2553,7 +2563,7 @@ static int binder_free_thread(struct binder_proc *proc,
binder_send_failed_reply(send_reply, BR_DEAD_REPLY);
binder_release_work(&thread->todo);
kfree(thread);
- binder_stats.obj_deleted[BINDER_STAT_THREAD]++;
+ binder_stats_deleted(BINDER_STAT_THREAD);
return active_transactions;
}
@@ -2854,7 +2864,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
init_waitqueue_head(&proc->wait);
proc->default_priority = task_nice(current);
mutex_lock(&binder_lock);
- binder_stats.obj_created[BINDER_STAT_PROC]++;
+ binder_stats_created(BINDER_STAT_PROC);
hlist_add_head(&proc->proc_node, &binder_procs);
proc->pid = current->group_leader->pid;
INIT_LIST_HEAD(&proc->delivered_death);
@@ -2950,7 +2960,7 @@ static void binder_deferred_release(struct binder_proc *proc)
list_del_init(&node->work.entry);
if (hlist_empty(&node->refs)) {
kfree(node);
- binder_stats.obj_deleted[BINDER_STAT_NODE]++;
+ binder_stats_deleted(BINDER_STAT_NODE);
} else {
struct binder_ref *ref;
int death = 0;
@@ -3004,7 +3014,7 @@ static void binder_deferred_release(struct binder_proc *proc)
buffers++;
}
- binder_stats.obj_deleted[BINDER_STAT_PROC]++;
+ binder_stats_deleted(BINDER_STAT_PROC);
page_count = 0;
if (proc->pages) {
--
1.5.4.3
I moved the continual,
if (binder_debug_mask & mask)
printk()
into a single macro so it's all in one place. It could be refined further
from there.
Signed-off-by: Daniel Walker <[email protected]>
---
drivers/staging/android/binder.c | 512 ++++++++++++++++++++------------------
1 files changed, 266 insertions(+), 246 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index c37336d..c4a596f 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -100,6 +100,12 @@ static int binder_set_stop_on_user_error(const char *val,
module_param_call(stop_on_user_error, binder_set_stop_on_user_error,
param_get_int, &binder_stop_on_user_error, S_IWUSR | S_IRUGO);
+#define binder_debug(mask, x...) \
+ do { \
+ if (binder_debug_mask & mask) \
+ printk(KERN_INFO x); \
+ } while (0)
+
#define binder_user_error(x...) \
do { \
if (binder_debug_mask & BINDER_DEBUG_USER_ERROR) \
@@ -468,9 +474,9 @@ static void binder_set_nice(long nice)
return;
}
min_nice = 20 - current->signal->rlim[RLIMIT_NICE].rlim_cur;
- if (binder_debug_mask & BINDER_DEBUG_PRIORITY_CAP)
- printk(KERN_INFO "binder: %d: nice value %ld not allowed use "
- "%ld instead\n", current->pid, nice, min_nice);
+ binder_debug(BINDER_DEBUG_PRIORITY_CAP,
+ "binder: %d: nice value %ld not allowed use "
+ "%ld instead\n", current->pid, nice, min_nice);
set_user_nice(current, min_nice);
if (min_nice < 20)
return;
@@ -500,9 +506,9 @@ static void binder_insert_free_buffer(struct binder_proc *proc,
new_buffer_size = binder_buffer_size(proc, new_buffer);
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: add free buffer, size %zd, "
- "at %p\n", proc->pid, new_buffer_size, new_buffer);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: add free buffer, size %zd, "
+ "at %p\n", proc->pid, new_buffer_size, new_buffer);
while (*p) {
parent = *p;
@@ -579,9 +585,9 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
struct page **page;
struct mm_struct *mm;
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: %s pages %p-%p\n",
- proc->pid, allocate ? "allocate" : "free", start, end);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: %s pages %p-%p\n", proc->pid,
+ allocate ? "allocate" : "free", start, end);
if (end <= start)
return 0;
@@ -696,10 +702,9 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
if (is_async &&
proc->free_async_space < size + sizeof(struct binder_buffer)) {
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_ERR
- "binder: %d: binder_alloc_buf size %zd failed, "
- "no async space left\n", proc->pid, size);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: binder_alloc_buf size %zd"
+ "failed, no async space left\n", proc->pid, size);
return NULL;
}
@@ -727,9 +732,10 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
buffer = rb_entry(best_fit, struct binder_buffer, rb_node);
buffer_size = binder_buffer_size(proc, buffer);
}
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: binder_alloc_buf size %zd got buff"
- "er %p size %zd\n", proc->pid, size, buffer, buffer_size);
+
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: binder_alloc_buf size %zd got buff"
+ "er %p size %zd\n", proc->pid, size, buffer, buffer_size);
has_page_addr =
(void *)(((uintptr_t)buffer->data + buffer_size) & PAGE_MASK);
@@ -756,18 +762,18 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
new_buffer->free = 1;
binder_insert_free_buffer(proc, new_buffer);
}
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: binder_alloc_buf size %zd got "
- "%p\n", proc->pid, size, buffer);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: binder_alloc_buf size %zd got "
+ "%p\n", proc->pid, size, buffer);
buffer->data_size = data_size;
buffer->offsets_size = offsets_size;
buffer->async_transaction = is_async;
if (is_async) {
proc->free_async_space -= size + sizeof(struct binder_buffer);
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC_ASYNC)
- printk(KERN_INFO "binder: %d: binder_alloc_buf size %zd "
- "async free %zd\n", proc->pid, size,
- proc->free_async_space);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC_ASYNC,
+ "binder: %d: binder_alloc_buf size %zd "
+ "async free %zd\n", proc->pid, size,
+ proc->free_async_space);
}
return buffer;
@@ -797,9 +803,9 @@ static void binder_delete_free_buffer(struct binder_proc *proc,
free_page_start = 0;
if (buffer_end_page(prev) == buffer_end_page(buffer))
free_page_end = 0;
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: merge free, buffer %p "
- "share page with %p\n", proc->pid, buffer, prev);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: merge free, buffer %p "
+ "share page with %p\n", proc->pid, buffer, prev);
}
if (!list_is_last(&buffer->entry, &proc->buffers)) {
@@ -810,19 +816,19 @@ static void binder_delete_free_buffer(struct binder_proc *proc,
if (buffer_start_page(next) ==
buffer_start_page(buffer))
free_page_start = 0;
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: merge free, "
- "buffer %p share page with %p\n",
- proc->pid, buffer, prev);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: merge free, buffer"
+ " %p share page with %p\n", proc->pid,
+ buffer, prev);
}
}
list_del(&buffer->entry);
if (free_page_start || free_page_end) {
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: merge free, buffer %p do "
- "not share page%s%s with with %p or %p\n",
- proc->pid, buffer, free_page_start ? "" : " end",
- free_page_end ? "" : " start", prev, next);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: merge free, buffer %p do "
+ "not share page%s%s with with %p or %p\n",
+ proc->pid, buffer, free_page_start ? "" : " end",
+ free_page_end ? "" : " start", prev, next);
binder_update_page_range(proc, 0, free_page_start ?
buffer_start_page(buffer) : buffer_end_page(buffer),
(free_page_end ? buffer_end_page(buffer) :
@@ -839,9 +845,10 @@ static void binder_free_buf(struct binder_proc *proc,
size = ALIGN(buffer->data_size, sizeof(void *)) +
ALIGN(buffer->offsets_size, sizeof(void *));
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO "binder: %d: binder_free_buf %p size %zd buffer"
- "_size %zd\n", proc->pid, buffer, size, buffer_size);
+
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder: %d: binder_free_buf %p size %zd buffer"
+ "_size %zd\n", proc->pid, buffer, size, buffer_size);
BUG_ON(buffer->free);
BUG_ON(size > buffer_size);
@@ -851,10 +858,11 @@ static void binder_free_buf(struct binder_proc *proc,
if (buffer->async_transaction) {
proc->free_async_space += size + sizeof(struct binder_buffer);
- if (binder_debug_mask & BINDER_DEBUG_BUFFER_ALLOC_ASYNC)
- printk(KERN_INFO "binder: %d: binder_free_buf size %zd "
- "async free %zd\n", proc->pid, size,
- proc->free_async_space);
+
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC_ASYNC,
+ "binder: %d: binder_free_buf size %zd "
+ "async free %zd\n", proc->pid, size,
+ proc->free_async_space);
}
binder_update_page_range(proc, 0,
@@ -935,10 +943,10 @@ static struct binder_node *binder_new_node(struct binder_proc *proc,
node->work.type = BINDER_WORK_NODE;
INIT_LIST_HEAD(&node->work.entry);
INIT_LIST_HEAD(&node->async_todo);
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: %d:%d node %d u%p c%p created\n",
- proc->pid, current->pid, node->debug_id,
- node->ptr, node->cookie);
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: %d:%d node %d u%p c%p created\n",
+ proc->pid, current->pid, node->debug_id,
+ node->ptr, node->cookie);
return node;
}
@@ -1003,12 +1011,14 @@ static int binder_dec_node(struct binder_node *node, int strong, int internal)
list_del_init(&node->work.entry);
if (node->proc) {
rb_erase(&node->rb_node, &node->proc->nodes);
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: refless node %d deleted\n", node->debug_id);
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: refless node %d deleted\n",
+ node->debug_id);
} else {
hlist_del(&node->dead_node);
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: dead node %d deleted\n", node->debug_id);
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: dead node %d deleted\n",
+ node->debug_id);
}
kfree(node);
binder_stats.obj_deleted[BINDER_STAT_NODE]++;
@@ -1091,25 +1101,27 @@ static struct binder_ref *binder_get_ref_for_node(struct binder_proc *proc,
rb_insert_color(&new_ref->rb_node_desc, &proc->refs_by_desc);
if (node) {
hlist_add_head(&new_ref->node_entry, &node->refs);
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: %d new ref %d desc %d for "
- "node %d\n", proc->pid, new_ref->debug_id,
- new_ref->desc, node->debug_id);
+
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: %d new ref %d desc %d for "
+ "node %d\n", proc->pid, new_ref->debug_id,
+ new_ref->desc, node->debug_id);
} else {
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: %d new ref %d desc %d for "
- "dead node\n", proc->pid, new_ref->debug_id,
- new_ref->desc);
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: %d new ref %d desc %d for "
+ "dead node\n", proc->pid, new_ref->debug_id,
+ new_ref->desc);
}
return new_ref;
}
static void binder_delete_ref(struct binder_ref *ref)
{
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: %d delete ref %d desc %d for "
- "node %d\n", ref->proc->pid, ref->debug_id,
- ref->desc, ref->node->debug_id);
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: %d delete ref %d desc %d for "
+ "node %d\n", ref->proc->pid, ref->debug_id,
+ ref->desc, ref->node->debug_id);
+
rb_erase(&ref->rb_node_desc, &ref->proc->refs_by_desc);
rb_erase(&ref->rb_node_node, &ref->proc->refs_by_node);
if (ref->strong)
@@ -1117,10 +1129,10 @@ static void binder_delete_ref(struct binder_ref *ref)
hlist_del(&ref->node_entry);
binder_dec_node(ref->node, 0, 1);
if (ref->death) {
- if (binder_debug_mask & BINDER_DEBUG_DEAD_BINDER)
- printk(KERN_INFO "binder: %d delete ref %d desc %d "
- "has death notification\n", ref->proc->pid,
- ref->debug_id, ref->desc);
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "binder: %d delete ref %d desc %d "
+ "has death notification\n", ref->proc->pid,
+ ref->debug_id, ref->desc);
list_del(&ref->death->work.entry);
kfree(ref->death);
binder_stats.obj_deleted[BINDER_STAT_DEATH]++;
@@ -1216,9 +1228,11 @@ static void binder_send_failed_reply(struct binder_transaction *t,
target_thread->return_error = BR_OK;
}
if (target_thread->return_error == BR_OK) {
- if (binder_debug_mask & BINDER_DEBUG_FAILED_TRANSACTION)
- printk(KERN_INFO "binder: send failed reply for transaction %d to %d:%d\n",
- t->debug_id, target_thread->proc->pid, target_thread->pid);
+ binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+ "binder: send failed reply for "
+ "transaction %d to %d:%d\n",
+ t->debug_id, target_thread->proc->pid,
+ target_thread->pid);
binder_pop_transaction(target_thread, t);
target_thread->return_error = error_code;
@@ -1234,22 +1248,22 @@ static void binder_send_failed_reply(struct binder_transaction *t,
} else {
struct binder_transaction *next = t->from_parent;
- if (binder_debug_mask & BINDER_DEBUG_FAILED_TRANSACTION)
- printk(KERN_INFO "binder: send failed reply "
- "for transaction %d, target dead\n",
- t->debug_id);
+ binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+ "binder: send failed reply "
+ "for transaction %d, target dead\n",
+ t->debug_id);
binder_pop_transaction(target_thread, t);
if (next == NULL) {
- if (binder_debug_mask & BINDER_DEBUG_DEAD_BINDER)
- printk(KERN_INFO "binder: reply failed,"
- " no target thread at root\n");
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "binder: reply failed,"
+ " no target thread at root\n");
return;
}
t = next;
- if (binder_debug_mask & BINDER_DEBUG_DEAD_BINDER)
- printk(KERN_INFO "binder: reply failed, no targ"
- "et thread -- retry %d\n", t->debug_id);
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "binder: reply failed, no target "
+ "thread -- retry %d\n", t->debug_id);
}
}
}
@@ -1399,22 +1413,22 @@ static void binder_transaction(struct binder_proc *proc,
t->debug_id = ++binder_last_id;
e->debug_id = t->debug_id;
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION) {
- if (reply)
- printk(KERN_INFO "binder: %d:%d BC_REPLY %d -> %d:%d, "
- "data %p-%p size %zd-%zd\n",
- proc->pid, thread->pid, t->debug_id,
- target_proc->pid, target_thread->pid,
- tr->data.ptr.buffer, tr->data.ptr.offsets,
- tr->data_size, tr->offsets_size);
- else
- printk(KERN_INFO "binder: %d:%d BC_TRANSACTION %d -> "
- "%d - node %d, data %p-%p size %zd-%zd\n",
- proc->pid, thread->pid, t->debug_id,
- target_proc->pid, target_node->debug_id,
- tr->data.ptr.buffer, tr->data.ptr.offsets,
- tr->data_size, tr->offsets_size);
- }
+ if (reply)
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ "binder: %d:%d BC_REPLY %d -> %d:%d, "
+ "data %p-%p size %zd-%zd\n",
+ proc->pid, thread->pid, t->debug_id,
+ target_proc->pid, target_thread->pid,
+ tr->data.ptr.buffer, tr->data.ptr.offsets,
+ tr->data_size, tr->offsets_size);
+ else
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ "binder: %d:%d BC_TRANSACTION %d -> "
+ "%d - node %d, data %p-%p size %zd-%zd\n",
+ proc->pid, thread->pid, t->debug_id,
+ target_proc->pid, target_node->debug_id,
+ tr->data.ptr.buffer, tr->data.ptr.offsets,
+ tr->data_size, tr->offsets_size);
if (!reply && !(tr->flags & TF_ONE_WAY))
t->from = thread;
@@ -1506,9 +1520,11 @@ static void binder_transaction(struct binder_proc *proc,
fp->type = BINDER_TYPE_WEAK_HANDLE;
fp->handle = ref->desc;
binder_inc_ref(ref, fp->type == BINDER_TYPE_HANDLE, &thread->todo);
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO " node %d u%p -> ref %d desc %d\n",
- node->debug_id, node->ptr, ref->debug_id, ref->desc);
+
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " node %d u%p -> ref %d desc %d\n",
+ node->debug_id, node->ptr, ref->debug_id,
+ ref->desc);
} break;
case BINDER_TYPE_HANDLE:
case BINDER_TYPE_WEAK_HANDLE: {
@@ -1529,9 +1545,10 @@ static void binder_transaction(struct binder_proc *proc,
fp->binder = ref->node->ptr;
fp->cookie = ref->node->cookie;
binder_inc_node(ref->node, fp->type == BINDER_TYPE_BINDER, 0, NULL);
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO " ref %d desc %d -> node %d u%p\n",
- ref->debug_id, ref->desc, ref->node->debug_id, ref->node->ptr);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " ref %d desc %d -> node %d u%p\n",
+ ref->debug_id, ref->desc, ref->node->debug_id,
+ ref->node->ptr);
} else {
struct binder_ref *new_ref;
new_ref = binder_get_ref_for_node(target_proc, ref->node);
@@ -1541,9 +1558,10 @@ static void binder_transaction(struct binder_proc *proc,
}
fp->handle = new_ref->desc;
binder_inc_ref(new_ref, fp->type == BINDER_TYPE_HANDLE, NULL);
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO " ref %d desc %d -> ref %d desc %d (node %d)\n",
- ref->debug_id, ref->desc, new_ref->debug_id, new_ref->desc, ref->node->debug_id);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " ref %d desc %d -> ref %d desc %d (node %d)\n",
+ ref->debug_id, ref->desc, new_ref->debug_id,
+ new_ref->desc, ref->node->debug_id);
}
} break;
@@ -1579,8 +1597,8 @@ static void binder_transaction(struct binder_proc *proc,
goto err_get_unused_fd_failed;
}
task_fd_install(target_proc, target_fd, file);
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO " fd %ld -> %d\n", fp->handle, target_fd);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " fd %ld -> %d\n", fp->handle, target_fd);
/* TODO: fput? */
fp->handle = target_fd;
} break;
@@ -1642,11 +1660,10 @@ err_empty_call_stack:
err_dead_binder:
err_invalid_target_handle:
err_no_context_mgr_node:
- if (binder_debug_mask & BINDER_DEBUG_FAILED_TRANSACTION)
- printk(KERN_INFO "binder: %d:%d transaction failed %d, size"
- "%zd-%zd\n",
- proc->pid, thread->pid, return_error,
- tr->data_size, tr->offsets_size);
+ binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+ "binder: %d:%d transaction failed %d, size %zd-%zd\n",
+ proc->pid, thread->pid, return_error,
+ tr->data_size, tr->offsets_size);
{
struct binder_transaction_log_entry *fe;
@@ -1669,10 +1686,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
size_t *offp, *off_end;
int debug_id = buffer->debug_id;
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO "binder: %d buffer release %d, size %zd-%zd, failed at %p\n",
- proc->pid, buffer->debug_id,
- buffer->data_size, buffer->offsets_size, failed_at);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ "binder: %d buffer release %d, size %zd-%zd, failed at %p\n",
+ proc->pid, buffer->debug_id,
+ buffer->data_size, buffer->offsets_size, failed_at);
if (buffer->target_node)
binder_dec_node(buffer->target_node, 1, 0);
@@ -1700,9 +1717,9 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
printk(KERN_ERR "binder: transaction release %d bad node %p\n", debug_id, fp->binder);
break;
}
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO " node %d u%p\n",
- node->debug_id, node->ptr);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " node %d u%p\n",
+ node->debug_id, node->ptr);
binder_dec_node(node, fp->type == BINDER_TYPE_BINDER, 0);
} break;
case BINDER_TYPE_HANDLE:
@@ -1712,15 +1729,15 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
printk(KERN_ERR "binder: transaction release %d bad handle %ld\n", debug_id, fp->handle);
break;
}
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO " ref %d desc %d (node %d)\n",
- ref->debug_id, ref->desc, ref->node->debug_id);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " ref %d desc %d (node %d)\n",
+ ref->debug_id, ref->desc, ref->node->debug_id);
binder_dec_ref(ref, fp->type == BINDER_TYPE_HANDLE);
} break;
case BINDER_TYPE_FD:
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO " fd %ld\n", fp->handle);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ " fd %ld\n", fp->handle);
if (failed_at)
task_close_fd(proc, fp->handle);
break;
@@ -1799,9 +1816,10 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
binder_dec_ref(ref, 0);
break;
}
- if (binder_debug_mask & BINDER_DEBUG_USER_REFS)
- printk(KERN_INFO "binder: %d:%d %s ref %d desc %d s %d w %d for node %d\n",
- proc->pid, thread->pid, debug_string, ref->debug_id, ref->desc, ref->strong, ref->weak, ref->node->debug_id);
+ binder_debug(BINDER_DEBUG_USER_REFS,
+ "binder: %d:%d %s ref %d desc %d s %d w %d for node %d\n",
+ proc->pid, thread->pid, debug_string, ref->debug_id,
+ ref->desc, ref->strong, ref->weak, ref->node->debug_id);
break;
}
case BC_INCREFS_DONE:
@@ -1859,9 +1877,11 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
node->pending_weak_ref = 0;
}
binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
- if (binder_debug_mask & BINDER_DEBUG_USER_REFS)
- printk(KERN_INFO "binder: %d:%d %s node %d ls %d lw %d\n",
- proc->pid, thread->pid, cmd == BC_INCREFS_DONE ? "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE", node->debug_id, node->local_strong_refs, node->local_weak_refs);
+ binder_debug(BINDER_DEBUG_USER_REFS,
+ "binder: %d:%d %s node %d ls %d lw %d\n",
+ proc->pid, thread->pid,
+ cmd == BC_INCREFS_DONE ? "BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
+ node->debug_id, node->local_strong_refs, node->local_weak_refs);
break;
}
case BC_ATTEMPT_ACQUIRE:
@@ -1893,10 +1913,10 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
proc->pid, thread->pid, data_ptr);
break;
}
- if (binder_debug_mask & BINDER_DEBUG_FREE_BUFFER)
- printk(KERN_INFO "binder: %d:%d BC_FREE_BUFFER u%p found buffer %d for %s transaction\n",
- proc->pid, thread->pid, data_ptr, buffer->debug_id,
- buffer->transaction ? "active" : "finished");
+ binder_debug(BINDER_DEBUG_FREE_BUFFER,
+ "binder: %d:%d BC_FREE_BUFFER u%p found buffer %d for %s transaction\n",
+ proc->pid, thread->pid, data_ptr, buffer->debug_id,
+ buffer->transaction ? "active" : "finished");
if (buffer->transaction) {
buffer->transaction->buffer = NULL;
@@ -1926,9 +1946,9 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
}
case BC_REGISTER_LOOPER:
- if (binder_debug_mask & BINDER_DEBUG_THREADS)
- printk(KERN_INFO "binder: %d:%d BC_REGISTER_LOOPER\n",
- proc->pid, thread->pid);
+ binder_debug(BINDER_DEBUG_THREADS,
+ "binder: %d:%d BC_REGISTER_LOOPER\n",
+ proc->pid, thread->pid);
if (thread->looper & BINDER_LOOPER_STATE_ENTERED) {
thread->looper |= BINDER_LOOPER_STATE_INVALID;
binder_user_error("binder: %d:%d ERROR:"
@@ -1948,9 +1968,9 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
thread->looper |= BINDER_LOOPER_STATE_REGISTERED;
break;
case BC_ENTER_LOOPER:
- if (binder_debug_mask & BINDER_DEBUG_THREADS)
- printk(KERN_INFO "binder: %d:%d BC_ENTER_LOOPER\n",
- proc->pid, thread->pid);
+ binder_debug(BINDER_DEBUG_THREADS,
+ "binder: %d:%d BC_ENTER_LOOPER\n",
+ proc->pid, thread->pid);
if (thread->looper & BINDER_LOOPER_STATE_REGISTERED) {
thread->looper |= BINDER_LOOPER_STATE_INVALID;
binder_user_error("binder: %d:%d ERROR:"
@@ -1961,9 +1981,9 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
thread->looper |= BINDER_LOOPER_STATE_ENTERED;
break;
case BC_EXIT_LOOPER:
- if (binder_debug_mask & BINDER_DEBUG_THREADS)
- printk(KERN_INFO "binder: %d:%d BC_EXIT_LOOPER\n",
- proc->pid, thread->pid);
+ binder_debug(BINDER_DEBUG_THREADS,
+ "binder: %d:%d BC_EXIT_LOOPER\n",
+ proc->pid, thread->pid);
thread->looper |= BINDER_LOOPER_STATE_EXITED;
break;
@@ -1992,14 +2012,14 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
break;
}
- if (binder_debug_mask & BINDER_DEBUG_DEATH_NOTIFICATION)
- printk(KERN_INFO "binder: %d:%d %s %p ref %d desc %d s %d w %d for node %d\n",
- proc->pid, thread->pid,
- cmd == BC_REQUEST_DEATH_NOTIFICATION ?
- "BC_REQUEST_DEATH_NOTIFICATION" :
- "BC_CLEAR_DEATH_NOTIFICATION",
- cookie, ref->debug_id, ref->desc,
- ref->strong, ref->weak, ref->node->debug_id);
+ binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
+ "binder: %d:%d %s %p ref %d desc %d s %d w %d for node %d\n",
+ proc->pid, thread->pid,
+ cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+ "BC_REQUEST_DEATH_NOTIFICATION" :
+ "BC_CLEAR_DEATH_NOTIFICATION",
+ cookie, ref->debug_id, ref->desc,
+ ref->strong, ref->weak, ref->node->debug_id);
if (cmd == BC_REQUEST_DEATH_NOTIFICATION) {
if (ref->death) {
@@ -2013,10 +2033,10 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
death = kzalloc(sizeof(*death), GFP_KERNEL);
if (death == NULL) {
thread->return_error = BR_ERROR;
- if (binder_debug_mask & BINDER_DEBUG_FAILED_TRANSACTION)
- printk(KERN_INFO "binder: %d:%d "
- "BC_REQUEST_DEATH_NOTIFICATION failed\n",
- proc->pid, thread->pid);
+ binder_debug(BINDER_DEBUG_FAILED_TRANSACTION,
+ "binder: %d:%d "
+ "BC_REQUEST_DEATH_NOTIFICATION failed\n",
+ proc->pid, thread->pid);
break;
}
binder_stats.obj_created[BINDER_STAT_DEATH]++;
@@ -2082,9 +2102,9 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
break;
}
}
- if (binder_debug_mask & BINDER_DEBUG_DEAD_BINDER)
- printk(KERN_INFO "binder: %d:%d BC_DEAD_BINDER_DONE %p found %p\n",
- proc->pid, thread->pid, cookie, death);
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "binder: %d:%d BC_DEAD_BINDER_DONE %p found %p\n",
+ proc->pid, thread->pid, cookie, death);
if (death == NULL) {
binder_user_error("binder: %d:%d BC_DEAD"
"_BINDER_DONE %p not found\n",
@@ -2240,9 +2260,9 @@ retry:
ptr += sizeof(uint32_t);
binder_stat_br(proc, thread, cmd);
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION_COMPLETE)
- printk(KERN_INFO "binder: %d:%d BR_TRANSACTION_COMPLETE\n",
- proc->pid, thread->pid);
+ binder_debug(BINDER_DEBUG_TRANSACTION_COMPLETE,
+ "binder: %d:%d BR_TRANSACTION_COMPLETE\n",
+ proc->pid, thread->pid);
list_del(&w->entry);
kfree(w);
@@ -2287,22 +2307,24 @@ retry:
ptr += sizeof(void *);
binder_stat_br(proc, thread, cmd);
- if (binder_debug_mask & BINDER_DEBUG_USER_REFS)
- printk(KERN_INFO "binder: %d:%d %s %d u%p c%p\n",
- proc->pid, thread->pid, cmd_name, node->debug_id, node->ptr, node->cookie);
+ binder_debug(BINDER_DEBUG_USER_REFS,
+ "binder: %d:%d %s %d u%p c%p\n",
+ proc->pid, thread->pid, cmd_name, node->debug_id, node->ptr, node->cookie);
} else {
list_del_init(&w->entry);
if (!weak && !strong) {
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: %d:%d node %d u%p c%p deleted\n",
- proc->pid, thread->pid, node->debug_id, node->ptr, node->cookie);
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: %d:%d node %d u%p c%p deleted\n",
+ proc->pid, thread->pid, node->debug_id,
+ node->ptr, node->cookie);
rb_erase(&node->rb_node, &proc->nodes);
kfree(node);
binder_stats.obj_deleted[BINDER_STAT_NODE]++;
} else {
- if (binder_debug_mask & BINDER_DEBUG_INTERNAL_REFS)
- printk(KERN_INFO "binder: %d:%d node %d u%p c%p state unchanged\n",
- proc->pid, thread->pid, node->debug_id, node->ptr, node->cookie);
+ binder_debug(BINDER_DEBUG_INTERNAL_REFS,
+ "binder: %d:%d node %d u%p c%p state unchanged\n",
+ proc->pid, thread->pid, node->debug_id, node->ptr,
+ node->cookie);
}
}
} break;
@@ -2323,13 +2345,13 @@ retry:
if (put_user(death->cookie, (void * __user *)ptr))
return -EFAULT;
ptr += sizeof(void *);
- if (binder_debug_mask & BINDER_DEBUG_DEATH_NOTIFICATION)
- printk(KERN_INFO "binder: %d:%d %s %p\n",
- proc->pid, thread->pid,
- cmd == BR_DEAD_BINDER ?
- "BR_DEAD_BINDER" :
- "BR_CLEAR_DEATH_NOTIFICATION_DONE",
- death->cookie);
+ binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
+ "binder: %d:%d %s %p\n",
+ proc->pid, thread->pid,
+ cmd == BR_DEAD_BINDER ?
+ "BR_DEAD_BINDER" :
+ "BR_CLEAR_DEATH_NOTIFICATION_DONE",
+ death->cookie);
if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION) {
list_del(&w->entry);
@@ -2391,16 +2413,16 @@ retry:
ptr += sizeof(tr);
binder_stat_br(proc, thread, cmd);
- if (binder_debug_mask & BINDER_DEBUG_TRANSACTION)
- printk(KERN_INFO "binder: %d:%d %s %d %d:%d, cmd %d"
- "size %zd-%zd ptr %p-%p\n",
- proc->pid, thread->pid,
- (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" :
- "BR_REPLY",
- t->debug_id, t->from ? t->from->proc->pid : 0,
- t->from ? t->from->pid : 0, cmd,
- t->buffer->data_size, t->buffer->offsets_size,
- tr.data.ptr.buffer, tr.data.ptr.offsets);
+ binder_debug(BINDER_DEBUG_TRANSACTION,
+ "binder: %d:%d %s %d %d:%d, cmd %d"
+ "size %zd-%zd ptr %p-%p\n",
+ proc->pid, thread->pid,
+ (cmd == BR_TRANSACTION) ? "BR_TRANSACTION" :
+ "BR_REPLY",
+ t->debug_id, t->from ? t->from->proc->pid : 0,
+ t->from ? t->from->pid : 0, cmd,
+ t->buffer->data_size, t->buffer->offsets_size,
+ tr.data.ptr.buffer, tr.data.ptr.offsets);
list_del(&t->work.entry);
t->buffer->allow_user_free = 1;
@@ -2425,9 +2447,9 @@ done:
BINDER_LOOPER_STATE_ENTERED)) /* the user-space code fails to */
/*spawn a new thread if we leave this out */) {
proc->requested_threads++;
- if (binder_debug_mask & BINDER_DEBUG_THREADS)
- printk(KERN_INFO "binder: %d:%d BR_SPAWN_LOOPER\n",
- proc->pid, thread->pid);
+ binder_debug(BINDER_DEBUG_THREADS,
+ "binder: %d:%d BR_SPAWN_LOOPER\n",
+ proc->pid, thread->pid);
if (put_user(BR_SPAWN_LOOPER, (uint32_t __user *)buffer))
return -EFAULT;
}
@@ -2507,11 +2529,12 @@ static int binder_free_thread(struct binder_proc *proc,
send_reply = t;
while (t) {
active_transactions++;
- if (binder_debug_mask & BINDER_DEBUG_DEAD_TRANSACTION)
- printk(KERN_INFO "binder: release %d:%d transaction %d "
- "%s, still active\n", proc->pid, thread->pid,
- t->debug_id,
- (t->to_thread == thread) ? "in" : "out");
+ binder_debug(BINDER_DEBUG_DEAD_TRANSACTION,
+ "binder: release %d:%d transaction %d "
+ "%s, still active\n", proc->pid, thread->pid,
+ t->debug_id,
+ (t->to_thread == thread) ? "in" : "out");
+
if (t->to_thread == thread) {
t->to_proc = NULL;
t->to_thread = NULL;
@@ -2598,9 +2621,11 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
ret = -EFAULT;
goto err;
}
- if (binder_debug_mask & BINDER_DEBUG_READ_WRITE)
- printk(KERN_INFO "binder: %d:%d write %ld at %08lx, read %ld at %08lx\n",
- proc->pid, thread->pid, bwr.write_size, bwr.write_buffer, bwr.read_size, bwr.read_buffer);
+ binder_debug(BINDER_DEBUG_READ_WRITE,
+ "binder: %d:%d write %ld at %08lx, read %ld at %08lx\n",
+ proc->pid, thread->pid, bwr.write_size, bwr.write_buffer,
+ bwr.read_size, bwr.read_buffer);
+
if (bwr.write_size > 0) {
ret = binder_thread_write(proc, thread, (void __user *)bwr.write_buffer, bwr.write_size, &bwr.write_consumed);
if (ret < 0) {
@@ -2620,9 +2645,10 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
goto err;
}
}
- if (binder_debug_mask & BINDER_DEBUG_READ_WRITE)
- printk(KERN_INFO "binder: %d:%d wrote %ld of %ld, read return %ld of %ld\n",
- proc->pid, thread->pid, bwr.write_consumed, bwr.write_size, bwr.read_consumed, bwr.read_size);
+ binder_debug(BINDER_DEBUG_READ_WRITE,
+ "binder: %d:%d wrote %ld of %ld, read return %ld of %ld\n",
+ proc->pid, thread->pid, bwr.write_consumed, bwr.write_size,
+ bwr.read_consumed, bwr.read_size);
if (copy_to_user(ubuf, &bwr, sizeof(bwr))) {
ret = -EFAULT;
goto err;
@@ -2663,9 +2689,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
binder_context_mgr_node->has_weak_ref = 1;
break;
case BINDER_THREAD_EXIT:
- if (binder_debug_mask & BINDER_DEBUG_THREADS)
- printk(KERN_INFO "binder: %d:%d exit\n",
- proc->pid, thread->pid);
+ binder_debug(BINDER_DEBUG_THREADS, "binder: %d:%d exit\n",
+ proc->pid, thread->pid);
binder_free_thread(proc, thread);
thread = NULL;
break;
@@ -2697,24 +2722,22 @@ err:
static void binder_vma_open(struct vm_area_struct *vma)
{
struct binder_proc *proc = vma->vm_private_data;
- if (binder_debug_mask & BINDER_DEBUG_OPEN_CLOSE)
- printk(KERN_INFO
- "binder: %d open vm area %lx-%lx (%ld K) vma %lx pagep %lx\n",
- proc->pid, vma->vm_start, vma->vm_end,
- (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags,
- (unsigned long)pgprot_val(vma->vm_page_prot));
+ binder_debug(BINDER_DEBUG_OPEN_CLOSE,
+ "binder: %d open vm area %lx-%lx (%ld K) vma %lx pagep %lx\n",
+ proc->pid, vma->vm_start, vma->vm_end,
+ (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags,
+ (unsigned long)pgprot_val(vma->vm_page_prot));
dump_stack();
}
static void binder_vma_close(struct vm_area_struct *vma)
{
struct binder_proc *proc = vma->vm_private_data;
- if (binder_debug_mask & BINDER_DEBUG_OPEN_CLOSE)
- printk(KERN_INFO
- "binder: %d close vm area %lx-%lx (%ld K) vma %lx pagep %lx\n",
- proc->pid, vma->vm_start, vma->vm_end,
- (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags,
- (unsigned long)pgprot_val(vma->vm_page_prot));
+ binder_debug(BINDER_DEBUG_OPEN_CLOSE,
+ "binder: %d close vm area %lx-%lx (%ld K) vma %lx pagep %lx\n",
+ proc->pid, vma->vm_start, vma->vm_end,
+ (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags,
+ (unsigned long)pgprot_val(vma->vm_page_prot));
proc->vma = NULL;
binder_defer_work(proc, BINDER_DEFERRED_PUT_FILES);
}
@@ -2735,12 +2758,11 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
if ((vma->vm_end - vma->vm_start) > SZ_4M)
vma->vm_end = vma->vm_start + SZ_4M;
- if (binder_debug_mask & BINDER_DEBUG_OPEN_CLOSE)
- printk(KERN_INFO
- "binder_mmap: %d %lx-%lx (%ld K) vma %lx pagep %lx\n",
- proc->pid, vma->vm_start, vma->vm_end,
- (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags,
- (unsigned long)pgprot_val(vma->vm_page_prot));
+ binder_debug(BINDER_DEBUG_OPEN_CLOSE,
+ "binder_mmap: %d %lx-%lx (%ld K) vma %lx pagep %lx\n",
+ proc->pid, vma->vm_start, vma->vm_end,
+ (vma->vm_end - vma->vm_start) / SZ_1K, vma->vm_flags,
+ (unsigned long)pgprot_val(vma->vm_page_prot));
if (vma->vm_flags & FORBIDDEN_MMAP_FLAGS) {
ret = -EPERM;
@@ -2820,9 +2842,8 @@ static int binder_open(struct inode *nodp, struct file *filp)
{
struct binder_proc *proc;
- if (binder_debug_mask & BINDER_DEBUG_OPEN_CLOSE)
- printk(KERN_INFO "binder_open: %d:%d\n",
- current->group_leader->pid, current->pid);
+ binder_debug(BINDER_DEBUG_OPEN_CLOSE, "binder_open: %d:%d\n",
+ current->group_leader->pid, current->pid);
proc = kzalloc(sizeof(*proc), GFP_KERNEL);
if (proc == NULL)
@@ -2875,8 +2896,9 @@ static void binder_deferred_flush(struct binder_proc *proc)
}
wake_up_interruptible_all(&proc->wait);
- if (binder_debug_mask & BINDER_DEBUG_OPEN_CLOSE)
- printk(KERN_INFO "binder_flush: %d woke %d threads\n", proc->pid, wake_count);
+ binder_debug(BINDER_DEBUG_OPEN_CLOSE,
+ "binder_flush: %d woke %d threads\n", proc->pid,
+ wake_count);
}
static int binder_release(struct inode *nodp, struct file *filp)
@@ -2905,8 +2927,9 @@ static void binder_deferred_release(struct binder_proc *proc)
hlist_del(&proc->proc_node);
if (binder_context_mgr_node && binder_context_mgr_node->proc == proc) {
- if (binder_debug_mask & BINDER_DEBUG_DEAD_BINDER)
- printk(KERN_INFO "binder_release: %d context_mgr_node gone\n", proc->pid);
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "binder_release: %d context_mgr_node gone\n",
+ proc->pid);
binder_context_mgr_node = NULL;
}
@@ -2949,10 +2972,10 @@ static void binder_deferred_release(struct binder_proc *proc)
BUG();
}
}
- if (binder_debug_mask & BINDER_DEBUG_DEAD_BINDER)
- printk(KERN_INFO "binder: node %d now dead, "
- "refs %d, death %d\n", node->debug_id,
- incoming_refs, death);
+ binder_debug(BINDER_DEBUG_DEAD_BINDER,
+ "binder: node %d now dead, "
+ "refs %d, death %d\n", node->debug_id,
+ incoming_refs, death);
}
}
outgoing_refs = 0;
@@ -2988,13 +3011,11 @@ static void binder_deferred_release(struct binder_proc *proc)
int i;
for (i = 0; i < proc->buffer_size / PAGE_SIZE; i++) {
if (proc->pages[i]) {
- if (binder_debug_mask &
- BINDER_DEBUG_BUFFER_ALLOC)
- printk(KERN_INFO
- "binder_release: %d: "
- "page %d at %p not freed\n",
- proc->pid, i,
- proc->buffer + i * PAGE_SIZE);
+ binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
+ "binder_release: %d: "
+ "page %d at %p not freed\n",
+ proc->pid, i,
+ proc->buffer + i * PAGE_SIZE);
__free_page(proc->pages[i]);
page_count++;
}
@@ -3005,13 +3026,12 @@ static void binder_deferred_release(struct binder_proc *proc)
put_task_struct(proc->tsk);
- if (binder_debug_mask & BINDER_DEBUG_OPEN_CLOSE)
- printk(KERN_INFO
- "binder_release: %d threads %d, nodes %d (ref %d), "
- "refs %d, active transactions %d, buffers %d, "
- "pages %d\n",
- proc->pid, threads, nodes, incoming_refs, outgoing_refs,
- active_transactions, buffers, page_count);
+ binder_debug(BINDER_DEBUG_OPEN_CLOSE,
+ "binder_release: %d threads %d, nodes %d (ref %d), "
+ "refs %d, active transactions %d, buffers %d, "
+ "pages %d\n",
+ proc->pid, threads, nodes, incoming_refs, outgoing_refs,
+ active_transactions, buffers, page_count);
kfree(proc);
}
--
1.5.4.3
Declare the binder_deferred_state enum, and use the new enum
for one of the binder_defer_work function arguments. This
should keep the argument within the confines of the enum
instead of the whole int range.
Signed-off-by: Daniel Walker <[email protected]>
---
drivers/staging/android/binder.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index ec86808..30a5ea5 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -243,7 +243,7 @@ struct binder_buffer {
uint8_t data[0];
};
-enum {
+enum binder_deferred_state {
BINDER_DEFERRED_PUT_FILES = 0x01,
BINDER_DEFERRED_FLUSH = 0x02,
BINDER_DEFERRED_RELEASE = 0x04,
@@ -326,7 +326,8 @@ struct binder_transaction {
uid_t sender_euid;
};
-static void binder_defer_work(struct binder_proc *proc, int defer);
+static void
+binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer);
/*
* copied from get_unused_fd_flags
@@ -3073,7 +3074,8 @@ static void binder_deferred_func(struct work_struct *work)
}
static DECLARE_WORK(binder_deferred_work, binder_deferred_func);
-static void binder_defer_work(struct binder_proc *proc, int defer)
+static void
+binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer)
{
mutex_lock(&binder_deferred_lock);
proc->deferred_work |= defer;
--
1.5.4.3
Replaced a manual hlist_head declaration with a macro based one.
Also reorganized the globals to be grouped better.
Signed-off-by: Daniel Walker <[email protected]>
---
drivers/staging/android/binder.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 30a5ea5..01711e3 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -31,18 +31,21 @@
#include <linux/sched.h>
#include <linux/uaccess.h>
#include <linux/vmalloc.h>
+
#include "binder.h"
static DEFINE_MUTEX(binder_lock);
+static DEFINE_MUTEX(binder_deferred_lock);
+
static HLIST_HEAD(binder_procs);
+static HLIST_HEAD(binder_deferred_list);
+static HLIST_HEAD(binder_dead_nodes);
+
+static struct proc_dir_entry *binder_proc_dir_entry_root;
+static struct proc_dir_entry *binder_proc_dir_entry_proc;
static struct binder_node *binder_context_mgr_node;
static uid_t binder_context_mgr_uid = -1;
static int binder_last_id;
-static struct proc_dir_entry *binder_proc_dir_entry_root;
-static struct proc_dir_entry *binder_proc_dir_entry_proc;
-static struct hlist_head binder_dead_nodes;
-static HLIST_HEAD(binder_deferred_list);
-static DEFINE_MUTEX(binder_deferred_lock);
static int binder_read_proc_proc(char *page, char **start, off_t off,
int count, int *eof, void *data);
--
1.5.4.3
On 06/12/09 11:51, Daniel Walker wrote:
> Signed-off-by: Daniel Walker<[email protected]>
> ---
> drivers/staging/android/binder.c | 14 ++++++++------
> 1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 17d89a8..c37336d 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -2146,7 +2146,7 @@ static int binder_thread_read(struct binder_proc *proc,
> void __user *end = buffer + size;
>
> int ret = 0;
> - int wait_for_proc_work;
> + int wait_for_proc_work = 0;
>
> if (*consumed == 0) {
> if (put_user(BR_NOOP, (uint32_t __user *)ptr))
> @@ -2155,8 +2155,8 @@ static int binder_thread_read(struct binder_proc *proc,
> }
>
> retry:
> - wait_for_proc_work = thread->transaction_stack == NULL&&
> - list_empty(&thread->todo);
> + if (list_empty(&thread->todo)&& thread->transaction_stack == NULL)
> + wait_for_proc_work = 1;
>
I think the original looks cleaner (in both cases). Assigning a
variable with the result of a boolean expression is perfectly
reasonable. Perhaps change the type of the variable to "bool" to make
it a bit clearer what's going on.
(It would be a different matter if any of the expression had side-effects.)
J
On Tue, 2009-06-16 at 13:46 -0700, Jeremy Fitzhardinge wrote:
> >
> > retry:
> > - wait_for_proc_work = thread->transaction_stack == NULL&&
> > - list_empty(&thread->todo);
> > + if (list_empty(&thread->todo)&& thread->transaction_stack == NULL)
> > + wait_for_proc_work = 1;
> >
>
> I think the original looks cleaner (in both cases). Assigning a
> variable with the result of a boolean expression is perfectly
> reasonable. Perhaps change the type of the variable to "bool" to make
> it a bit clearer what's going on.
I agree it's reasonable in some cases.. The reason I changed this is
because at first glance I didn't know what those lines were suppose to
do. The equals signs all bleed together combined with the length of the
statement makes it not match other similar usage. The if statement just
makes the whole thing explicit.
Not to mention this code is a mess, very dense, and has little or no
comments. Anything that can be done to make the code more clear, seem
like a cleanup to me.
As for using "bool" , AFAIK that's only part of C++ ..
Daniel
On 06/17/09 07:37, Daniel Walker wrote:
> I agree it's reasonable in some cases.. The reason I changed this is
> because at first glance I didn't know what those lines were suppose to
> do. The equals signs all bleed together combined with the length of the
> statement makes it not match other similar usage. The if statement just
> makes the whole thing explicit.
>
I definitely see your point, but the if() statement variant has the
downside of only conditionally assigning the variable, and requiring it
to be initialized separately. I have a general code-cleanup rule to
convert:
foo = false;
if (something_is_true())
foo = true;
to
foo = something_is_true();
Maybe a bit of reformatting and some tactical use of parens would help?
wait_for_proc_work = (!thread->transaction_stack&& list_empty(&thread->todo));
(I'm not normally a fan of NULL-as-false, but it reads OK here.)
> Not to mention this code is a mess, very dense, and has little or no
> comments. Anything that can be done to make the code more clear, seem
> like a cleanup to me.
>
No argument from me. Not to mention that I have no idea from reading
the code what the whole subsystem is for; "Android IPC Subsystem"
doesn't tell me much, other than a gnawing feeling about having yet
another IPC subsystem to deal with.
> As for using "bool" , AFAIK that's only part of C++ ..
>
No, it is also C99, and becoming widely used in the kernel.
J
On Wed, 2009-06-17 at 08:28 -0700, Jeremy Fitzhardinge wrote:
> On 06/17/09 07:37, Daniel Walker wrote:
> > I agree it's reasonable in some cases.. The reason I changed this is
> > because at first glance I didn't know what those lines were suppose to
> > do. The equals signs all bleed together combined with the length of the
> > statement makes it not match other similar usage. The if statement just
> > makes the whole thing explicit.
> >
>
> I definitely see your point, but the if() statement variant has the
> downside of only conditionally assigning the variable, and requiring it
> to be initialized separately. I have a general code-cleanup rule to
> convert:
>
> foo = false;
> if (something_is_true())
> foo = true;
>
> to
>
> foo = something_is_true();
Above seems more like a speed up, rather than a clean up. I would think
it's likely fine for a lot of cases tho.
>
> Maybe a bit of reformatting and some tactical use of parens would help?
>
> wait_for_proc_work = (!thread->transaction_stack&& list_empty(&thread->todo));
>
> (I'm not normally a fan of NULL-as-false, but it reads OK here.)
I'm ok with this change for the first of the two, but the second one is
too long.. However, I reviewed the code a little more and I think the
wait_for_proc_work variable could likely get eliminated in the second
case. There is something racy going on wrt. to the variable in the
second case. So it probably better for me to just drop the second change
in hopes of a more detailed cleanup.
> > Not to mention this code is a mess, very dense, and has little or no
> > comments. Anything that can be done to make the code more clear, seem
> > like a cleanup to me.
> >
>
> No argument from me. Not to mention that I have no idea from reading
> the code what the whole subsystem is for; "Android IPC Subsystem"
> doesn't tell me much, other than a gnawing feeling about having yet
> another IPC subsystem to deal with.
I was hoping Brian could explain this. I also added Arve (the author) to
the CC list. Maybe they can explain the purpose of the subsystem.
> > As for using "bool" , AFAIK that's only part of C++ ..
> >
>
> No, it is also C99, and becoming widely used in the kernel.
Was this a recent change to C99, cause my compiler still doesn't know
about it .. I also see a couple places in the kernel where bool is
getting typedef'ed or somehow declared..
Daniel
On 06/17/09 09:08, Daniel Walker wrote:
> On Wed, 2009-06-17 at 08:28 -0700, Jeremy Fitzhardinge wrote:
>
>> I have a general code-cleanup rule to
>> convert:
>>
>> foo = false;
>> if (something_is_true())
>> foo = true;
>>
>> to
>>
>> foo = something_is_true();
>>
>
> Above seems more like a speed up, rather than a clean up. I would think
> it's likely fine for a lot of cases tho.
>
The compiler should be smart enough to generate identical code in both
cases. I think the second is better because it more directly expresses
what you're trying to do: you have a boolean predicate, and you're
assigning it to a boolean variable. The if() form has the same effect,
but couches it in terms of control flow which is just obfuscation.
> I was hoping Brian could explain this. I also added Arve (the author) to
> the CC list. Maybe they can explain the purpose of the subsystem.
>
Also, what its usermode ABI is, how stable it is, whether its generally
useful, does it have glibc/other library support, etc. Would you ever
want to use this in a non-Android context?
> Was this a recent change to C99, cause my compiler still doesn't know
> about it .. I also see a couple places in the kernel where bool is
> getting typedef'ed or somehow declared..
>
The C99 type has some stupid name like "_Bool", but the kernel typedefs
it to bool everywhere.
J
On Wed, Jun 17, 2009 at 9:31 AM, Jeremy Fitzhardinge<[email protected]> wrote:
> On 06/17/09 09:08, Daniel Walker wrote:
...
> Also, what its usermode ABI is, how stable it is, whether its generally
> useful, does it have glibc/other library support, etc. ?Would you ever want
> to use this in a non-Android context?
You could use this in a non-android context, but the abi is not
stable. There is some documentaion of the current user space api at
http://developer.android.com/reference/android/os/IBinder.html. You
can also find more information at http://www.open-binder.org/ which is
where the api came from.
--
Arve Hj?nnev?g
On Wed, 2009-06-17 at 14:26 -0700, Arve Hjønnevåg wrote:
> On Wed, Jun 17, 2009 at 9:31 AM, Jeremy Fitzhardinge<[email protected]> wrote:
> > On 06/17/09 09:08, Daniel Walker wrote:
> ...
> > Also, what its usermode ABI is, how stable it is, whether its generally
> > useful, does it have glibc/other library support, etc. Would you ever want
> > to use this in a non-Android context?
>
> You could use this in a non-android context, but the abi is not
> stable. There is some documentaion of the current user space api at
> http://developer.android.com/reference/android/os/IBinder.html. You
> can also find more information at http://www.open-binder.org/ which is
> where the api came from.
Why does all this need to be done in the kernel? Couldn't any of the
current IPC mechanisms be re-used to accomplish this?
Daniel
On 06/17/09 14:26, Arve Hj?nnev?g wrote:
> You could use this in a non-android context, but the abi is not
> stable. There is some documentaion of the current user space api at
> http://developer.android.com/reference/android/os/IBinder.html. You
> can also find more information at http://www.open-binder.org/ which is
> where the api came from.
>
So the ancestry is Be -> Palm -> Android/Google -> Linux?
J
>> int ret = 0;
>> - int wait_for_proc_work;
>> + int wait_for_proc_work = 0;
>>
>> if (*consumed == 0) {
>> if (put_user(BR_NOOP, (uint32_t __user *)ptr))
>> @@ -2155,8 +2155,8 @@ static int binder_thread_read(struct binder_proc *proc,
>> }
>>
>> retry:
>> - wait_for_proc_work = thread->transaction_stack == NULL&&
>> - list_empty(&thread->todo);
>> + if (list_empty(&thread->todo)&& thread->transaction_stack == NULL)
>> + wait_for_proc_work = 1;
>>
>
> I think the original looks cleaner (in both cases). Assigning a
> variable with the result of a boolean expression is perfectly
> reasonable. Perhaps change the type of the variable to "bool" to make
> it a bit clearer what's going on.
>
> (It would be a different matter if any of the expression had side-effects.)
Plus you have broken whitespace in there, changed performance
characteristics (list_empty now done first) and with gotos used around
that code I'm not at all sure transformation is equivalent.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri, 2009-06-19 at 16:59 +0200, Pavel Machek wrote:
> >> int ret = 0;
> >> - int wait_for_proc_work;
> >> + int wait_for_proc_work = 0;
> >>
> >> if (*consumed == 0) {
> >> if (put_user(BR_NOOP, (uint32_t __user *)ptr))
> >> @@ -2155,8 +2155,8 @@ static int binder_thread_read(struct binder_proc *proc,
> >> }
> >>
> >> retry:
> >> - wait_for_proc_work = thread->transaction_stack == NULL&&
> >> - list_empty(&thread->todo);
> >> + if (list_empty(&thread->todo)&& thread->transaction_stack == NULL)
> >> + wait_for_proc_work = 1;
> >>
> >
> > I think the original looks cleaner (in both cases). Assigning a
> > variable with the result of a boolean expression is perfectly
> > reasonable. Perhaps change the type of the variable to "bool" to make
> > it a bit clearer what's going on.
> >
> > (It would be a different matter if any of the expression had side-effects.)
>
> Plus you have broken whitespace in there, changed performance
> characteristics (list_empty now done first) and with gotos used around
> that code I'm not at all sure transformation is equivalent.
The broken whitespace was only in replies to the original patch, not
sure how it made it in there. We already resolved most of what your
commenting. Unless you care to comment on a later email.
Daniel
2009/6/17 Daniel Walker <[email protected]>:
> On Wed, 2009-06-17 at 14:26 -0700, Arve Hjønnevåg wrote:
>> On Wed, Jun 17, 2009 at 9:31 AM, Jeremy Fitzhardinge<[email protected]> wrote:
>> > On 06/17/09 09:08, Daniel Walker wrote:
>> ...
>> > Also, what its usermode ABI is, how stable it is, whether its generally
>> > useful, does it have glibc/other library support, etc. Would you ever want
>> > to use this in a non-Android context?
>>
>> You could use this in a non-android context, but the abi is not
>> stable. There is some documentaion of the current user space api at
>> http://developer.android.com/reference/android/os/IBinder.html. You
>> can also find more information at http://www.open-binder.org/ which is
>> where the api came from.
>
> Why does all this need to be done in the kernel? Couldn't any of the
> current IPC mechanisms be re-used to accomplish this?
Arve can probably go into more detail here, but I believe the two
notable properties of the binder that are not present in existing IPC
mechanisms in the kernel (that I'm aware of) are:
- avoiding copies by having the kernel copy from the writer into a
ring buffer in the reader's address space directly (allocating space
if necessary)
- managing the lifespan of proxied remoted userspace objects that can
be shared and passed between processes (upon which the userspace
binder library builds its remote reference counting model)
Brian
On Fri, 2009-06-19 at 12:20 -0700, Brian Swetland wrote:
> 2009/6/17 Daniel Walker <[email protected]>:
> > On Wed, 2009-06-17 at 14:26 -0700, Arve Hjønnevåg wrote:
> >> On Wed, Jun 17, 2009 at 9:31 AM, Jeremy Fitzhardinge<[email protected]> wrote:
> >> > On 06/17/09 09:08, Daniel Walker wrote:
> >> ...
> >> > Also, what its usermode ABI is, how stable it is, whether its generally
> >> > useful, does it have glibc/other library support, etc. Would you ever want
> >> > to use this in a non-Android context?
> >>
> >> You could use this in a non-android context, but the abi is not
> >> stable. There is some documentaion of the current user space api at
> >> http://developer.android.com/reference/android/os/IBinder.html. You
> >> can also find more information at http://www.open-binder.org/ which is
> >> where the api came from.
> >
> > Why does all this need to be done in the kernel? Couldn't any of the
> > current IPC mechanisms be re-used to accomplish this?
>
> Arve can probably go into more detail here, but I believe the two
> notable properties of the binder that are not present in existing IPC
> mechanisms in the kernel (that I'm aware of) are:
> - avoiding copies by having the kernel copy from the writer into a
> ring buffer in the reader's address space directly (allocating space
> if necessary)
This sounds like a performance speed up ..
> - managing the lifespan of proxied remoted userspace objects that can
> be shared and passed between processes (upon which the userspace
> binder library builds its remote reference counting model)
The "managing the lifespan" sounds very much like part of the
description for DBus .. I think the main competing interface would be
DBus. I know it's used in the software for the OpenMoko phone , and I
think the Palm Pre uses it too.
Did Google evaluate DBus at all? Also are there any userspace test cases
that Google used to test the performance of this interface. Or test
cases to compare the binder with something like sockets, or any other
type of IPC?
If Google believes the binder is the right solution for IPC, how was
that conclusion formed?
Daniel
On Fri, Jun 19, 2009 at 3:53 PM, Daniel Walker<[email protected]> wrote:
> On Fri, 2009-06-19 at 12:20 -0700, Brian Swetland wrote:
>> 2009/6/17 Daniel Walker <[email protected]>:
>> > On Wed, 2009-06-17 at 14:26 -0700, Arve Hj?nnev?g wrote:
>> >> On Wed, Jun 17, 2009 at 9:31 AM, Jeremy Fitzhardinge<[email protected]> wrote:
>> >> > On 06/17/09 09:08, Daniel Walker wrote:
>> >> ...
>> >> > Also, what its usermode ABI is, how stable it is, whether its generally
>> >> > useful, does it have glibc/other library support, etc. ?Would you ever want
>> >> > to use this in a non-Android context?
>> >>
>> >> You could use this in a non-android context, but the abi is not
>> >> stable. There is some documentaion of the current user space api at
>> >> http://developer.android.com/reference/android/os/IBinder.html. You
>> >> can also find more information at http://www.open-binder.org/ which is
>> >> where the api came from.
>> >
>> > Why does all this need to be done in the kernel? Couldn't any of the
>> > current IPC mechanisms be re-used to accomplish this?
>>
>> Arve can probably go into more detail here, but I believe the two
>> notable properties of the binder that are not present in existing IPC
>> mechanisms in the kernel (that I'm aware of) are:
>> - avoiding copies by having the kernel copy from the writer into a
>> ring buffer in the reader's address space directly (allocating space
>> if necessary)
>
> This sounds like a performance speed up ..
>
>> - managing the lifespan of proxied remoted userspace objects that can
>> be shared and passed between processes (upon which the userspace
>> binder library builds its remote reference counting model)
>
> The "managing the lifespan" sounds very much like part of the
> description for DBus .. ?I think the main competing interface would be
> DBus. I know it's used in the software for the OpenMoko phone , and I
> think the Palm Pre uses it too.
>
> Did Google evaluate DBus at all?
Some of our user-space code have in the past used or still use dbus,
but as far as I know it has not been seriously considered as a
replacement for the binder.
> Also are there any userspace test cases
> that Google used to test the performance of this interface. Or test
> cases to compare the binder with something like sockets, or any other
> type of IPC?
>
> If Google believes the binder is the right solution for IPC, how was
> that conclusion formed?
>
> Daniel
These are mostly questions for the framework team. The binder driver
is there to support our user space code. At some point we used the
driver from http://www.open-binder.org, but we ran into, and fixed, a lot of
bugs (especially when processes died), so we determined it would be
faster to rewrite the driver from scratch.
--
Arve Hj?nnev?g
On Fri, 2009-06-19 at 17:13 -0700, Arve Hjønnevåg wrote:
>
> > Also are there any userspace test cases
> > that Google used to test the performance of this interface. Or test
> > cases to compare the binder with something like sockets, or any other
> > type of IPC?
> >
> > If Google believes the binder is the right solution for IPC, how was
> > that conclusion formed?
> >
> > Daniel
>
> These are mostly questions for the framework team. The binder driver
> is there to support our user space code. At some point we used the
> driver from http://www.open-binder.org, but we ran into, and fixed, a lot of
> bugs (especially when processes died), so we determined it would be
> faster to rewrite the driver from scratch.
Most of these questions related to the fact that I don't think an
interface like this just slips into the kernel as a driver. Since it's
IPC, it's totally generic, and it's not part of a standard (i.e. POSIX),
we need to have some better and more specific information about it (or
atleast I do).
If for instance the main reason for Google using this interface is cause
a large number of android people once worked at Palm or BeOS, that's not
reason enough for it to go into the kernel. Or if this binder interface
really fits well with Java or C++ people and they just love it, that's
not really acceptable either..
Daniel
2009/6/20 Arve Hjønnevåg <[email protected]>:
>>
>> Did Google evaluate DBus at all?
>
> Some of our user-space code have in the past used or still use dbus,
> but as far as I know it has not been seriously considered as a
> replacement for the binder.
>
>> Also are there any userspace test cases
>> that Google used to test the performance of this interface. Or test
>> cases to compare the binder with something like sockets, or any other
>> type of IPC?
>>
>> If Google believes the binder is the right solution for IPC, how was
>> that conclusion formed?
>>
>> Daniel
>
> These are mostly questions for the framework team. The binder driver
> is there to support our user space code. At some point we used the
> driver from http://www.open-binder.org, but we ran into, and fixed, a lot of
> bugs (especially when processes died), so we determined it would be
> faster to rewrite the driver from scratch.
Ok, The binder driver is there to support our user space code as you explained.
Currently, Android platform selected both binder and Dbus for IPC mecanism as
http://blogfiles13.naver.net/data44/2009/6/20/108/binder-dbus-android-invain.png
and
android bluetooth architecture
diagram(http://sites.google.com/a/android.com/opensource/projects/bluetooth-faq).
Why did Google select the two mechanism(both Binder and Dbus) for IPC
in androd software stack?
And, What is differenece about major roles between Binder and Dbus in
Android OpenSource Project(AOSOP)?
* Binder as IPC
- http://www.open-binder.org
-Don't worry about processes or IPC Because of distributed architecture.
-Provides resource management between processes.
-Handle on an object in another proces. ( lightweight sharing between
processes ???)
-Powerful facilities for doing multithreaded programming with the Binder.
* Dbus as IPC
- http://www.freedesktop.org/software/dbus/
- System for sending messages between applications. (Systemwide
message-bus service) -
- For example , Broadcast signal and System/Session Bus for tasks in
Application Framework.
--
Regards,
GeunSik Lim ( Samsung Electronics )
Blog : http://blog.naver.com/invain/
e-Mail: [email protected]
[email protected] , [email protected]
On Fri, Jun 19, 2009 at 05:49:00PM -0700, Daniel Walker wrote:
> Most of these questions related to the fact that I don't think an
> interface like this just slips into the kernel as a driver. Since it's
> IPC, it's totally generic, and it's not part of a standard (i.e. POSIX),
> we need to have some better and more specific information about it (or
> atleast I do).
It's in the crap^H^H^H^Hstaging tree and might for all practivcal
purposes not exist. The binders interface as implemented currently with
it's fdtable munging and messing around with the user virtual address
space has exactly zero chance to go properly upstream.
Hi Daniel,
> > > Also are there any userspace test cases
> > > that Google used to test the performance of this interface. Or test
> > > cases to compare the binder with something like sockets, or any other
> > > type of IPC?
> > >
> > > If Google believes the binder is the right solution for IPC, how was
> > > that conclusion formed?
> > >
> > > Daniel
> >
> > These are mostly questions for the framework team. The binder driver
> > is there to support our user space code. At some point we used the
> > driver from http://www.open-binder.org, but we ran into, and fixed, a lot of
> > bugs (especially when processes died), so we determined it would be
> > faster to rewrite the driver from scratch.
>
> Most of these questions related to the fact that I don't think an
> interface like this just slips into the kernel as a driver. Since it's
> IPC, it's totally generic, and it's not part of a standard (i.e. POSIX),
> we need to have some better and more specific information about it (or
> atleast I do).
at this point of time we are using D-Bus quite successfully for mostly
every userspace application. And embedded devices like Nokia 810 or even
the new Palm Pre seems to be very happy with it. Even the Android
platform contains D-Bus support (even though limited for BlueZ).
The only downside with D-Bus right now is that you can't really copy
large amount of data over its IPC. And that is purely for performance
reason and memory constraints. However it is actually an implementation
detail since D-Bus as of now uses Unix sockets underneath. Switching to
shared memory or implementing dbus-daemon as AF_DBUS inside the kernel
would solve these.
For the problem of not being able to pass file descriptors over D-Bus we
have patches that are currently under review and are most likely to get
merged for the next release. This would also remove the limitation of
the direct data communication since you could just give the file
descriptor of the data plane to the other process.
> If for instance the main reason for Google using this interface is cause
> a large number of android people once worked at Palm or BeOS, that's not
> reason enough for it to go into the kernel. Or if this binder interface
> really fits well with Java or C++ people and they just love it, that's
> not really acceptable either..
It is a Google only thing. Everybody else uses D-Bus and is actually
happy with it.
Regards
Marcel
On Fri, 2009-06-19 at 17:13 -0700, Arve Hjønnevåg wrote:
>
> These are mostly questions for the framework team. The binder driver
> is there to support our user space code. At some point we used the
> driver from http://www.open-binder.org, but we ran into, and fixed, a lot of
> bugs (especially when processes died), so we determined it would be
> faster to rewrite the driver from scratch.
>
Is there someone in your framework team that would be willing to respond
to these questions ? (I know you added hackbod to the CC, but they
aren't responding..)
Daniel
2009/6/24 Daniel Walker <[email protected]>:
> On Fri, 2009-06-19 at 17:13 -0700, Arve Hjønnevåg wrote:
>>
>> These are mostly questions for the framework team. The binder driver
>> is there to support our user space code. At some point we used the
>> driver from http://www.open-binder.org, but we ran into, and fixed, a lot of
>> bugs (especially when processes died), so we determined it would be
>> faster to rewrite the driver from scratch.
>
> Is there someone in your framework team that would be willing to respond
> to these questions ? (I know you added hackbod to the CC, but they
> aren't responding..)
I guess it depends on the questions -- for good or ill, it's what is
in use and replacing it with a different mechanism would be a pretty
huge effort that there's unlikely to be time for in the near future.
Quite a bit of the userspace framework depends on the binder handling
the remote reference counting, object lifespan, etc, stuff and it's
subtle and hairy to debug. Moving to a different transport is
possible (it's just software, as they say), but there's a lot of code
that depends on the behavior that exists today.
If the binder is absolutely unacceptable as something to be included
in the linux kernel (that's a message that I seem to be getting here),
I think the best thing for us to do is maintain it in our tree for now
and focus on stuff that is far less controversial.
For example the msm7k SoC code may need some various cleanup, but I
think at the end of the day adding support for a new ARM based SoC and
peripherals is not going to be that contentious. The
wakelock/suspendblock stuff is a bit further out there, but there
seemed to be some good progress on getting it reviewed and revised on
the linux-pm list, last I saw.
Brian
On Wed, 2009-06-24 at 15:14 -0700, Brian Swetland wrote:
>
> I guess it depends on the questions -- for good or ill, it's what is
> in use and replacing it with a different mechanism would be a pretty
> huge effort that there's unlikely to be time for in the near future.
> Quite a bit of the userspace framework depends on the binder handling
> the remote reference counting, object lifespan, etc, stuff and it's
> subtle and hairy to debug. Moving to a different transport is
> possible (it's just software, as they say), but there's a lot of code
> that depends on the behavior that exists today.
I'm really interested in how binder was picked. It seems like a dead
technology (hasn't been touched in 3 years according to open-binder.org)
I think the best option is to write a layer over D-Bus that implements
the binder API. I don't think that would be too difficult since D-Bus
does IPC, binder does IPC, and the differences should be worked out with
a layer between the two. D-Bus seems like the best solution since it has
the best community backing..
> If the binder is absolutely unacceptable as something to be included
> in the linux kernel (that's a message that I seem to be getting here),
> I think the best thing for us to do is maintain it in our tree for now
> and focus on stuff that is far less controversial.
Yes... I'm refraining from doing more cleanups because it seems like a
lost cause.
> For example the msm7k SoC code may need some various cleanup, but I
> think at the end of the day adding support for a new ARM based SoC and
> peripherals is not going to be that contentious. The
> wakelock/suspendblock stuff is a bit further out there, but there
> seemed to be some good progress on getting it reviewed and revised on
> the linux-pm list, last I saw.
Do you have a msm branch someplace that is strictly msm support with
absolutely no generic changes mixed in?
The only thing in staging that is less controversial from my perspective
is ram_console.c that could actually be cleaned up and possibly merged.
Daniel
On Wed, Jun 24, 2009 at 3:49 PM, Daniel Walker<[email protected]> wrote:
>> For example the msm7k SoC code may need some various cleanup, but I
>> think at the end of the day adding support for a new ARM based SoC and
>> peripherals is not going to be that contentious. The
>> wakelock/suspendblock stuff is a bit further out there, but there
>> seemed to be some good progress on getting it reviewed and revised on
>> the linux-pm list, last I saw.
>
> Do you have a msm branch someplace that is strictly msm support with
> absolutely no generic changes mixed in?
Unfortunately, no. However, the generic changes tend to be
self-contained (binder, logger, etc) and not necessary for core msm7k
support. The one set of changes that does touch both generic and
platform code is the wakelock/suspendblock stuff, which some of the
drivers make use of, but that's usually not very invasive.
http://android.git.kernel.org/?p=kernel/msm.git;a=shortlog;h=refs/heads/android-msm-2.6.29
is the most up to date msm7k tree.
We'll be rebasing on a newer kernel baseline in the near future (.29
is obviously not the cutting edge, but is what's being supported for
the upcoming donut release of the android platform).
Brian
On Wed, 2009-06-24 at 16:05 -0700, Brian Swetland wrote:
> On Wed, Jun 24, 2009 at 3:49 PM, Daniel Walker<[email protected]> wrote:
> >> For example the msm7k SoC code may need some various cleanup, but I
> >> think at the end of the day adding support for a new ARM based SoC and
> >> peripherals is not going to be that contentious. The
> >> wakelock/suspendblock stuff is a bit further out there, but there
> >> seemed to be some good progress on getting it reviewed and revised on
> >> the linux-pm list, last I saw.
> >
> > Do you have a msm branch someplace that is strictly msm support with
> > absolutely no generic changes mixed in?
>
> Unfortunately, no. However, the generic changes tend to be
> self-contained (binder, logger, etc) and not necessary for core msm7k
> support. The one set of changes that does touch both generic and
> platform code is the wakelock/suspendblock stuff, which some of the
> drivers make use of, but that's usually not very invasive.
>
> http://android.git.kernel.org/?p=kernel/msm.git;a=shortlog;h=refs/heads/android-msm-2.6.29
> is the most up to date msm7k tree.
Having a tree with strictly msm changes is kind of a minimum requirement
for mainline. It would be good to split your tree into branches with
specific functionality. For instance one branch with just msm, one
branch with just wakelocks, and one branch with driver changes (or one
branch per driver change). Then you can merge all those branches
together which would make your unified kernel.
Maintaining in git isn't my specialty, but the above is what I've seen
in other trees.
Daniel
On Wed, Jun 24, 2009 at 4:29 PM, Daniel Walker<[email protected]> wrote:
>> Unfortunately, no. However, the generic changes tend to be
>> self-contained (binder, logger, etc) and not necessary for core msm7k
>> support. The one set of changes that does touch both generic and
>> platform code is the wakelock/suspendblock stuff, which some of the
>> drivers make use of, but that's usually not very invasive.
>>
>> http://android.git.kernel.org/?p=kernel/msm.git;a=shortlog;h=refs/heads/android-msm-2.6.29
>> is the most up to date msm7k tree.
>
> Having a tree with strictly msm changes is kind of a minimum requirement
> for mainline. It would be good to split your tree into branches with
> specific functionality. For instance one branch with just msm, one
> branch with just wakelocks, and one branch with driver changes (or one
> branch per driver change). Then you can merge all those branches
> together which would make your unified kernel.
For stuff going upstream in the past (earlier contributions via
lakml), I've generally pulled out series of patches for review and
built up a for-rmk branch to pull from or the like.
We tend to rebase onto a kernel release (last time was 2.6.29) and
work on that towards a platform release, and simultaneously we can be
feeding patches upstream (we should be doing more of this, obviously).
When we rebase up to the next release we snap to, we pick up anything
that's already gone upstream, and in theory, over time the delta
between our tree and mainline shrinks. This was the case when we
moved to .29 from .27 (as a bunch of msm7k patches had gone into .28).
Brian
2009/6/25 Brian Swetland <[email protected]>:
> 2009/6/24 Daniel Walker <[email protected]>:
>> On Fri, 2009-06-19 at 17:13 -0700, Arve Hj?nnev?g wrote:
>>>
>>> These are mostly questions for the framework team. The binder driver
>>> is there to support our user space code. At some point we used the
>>> driver from http://www.open-binder.org, but we ran into, and fixed, a lot of
>>> bugs (especially when processes died), so we determined it would be
>>> faster to rewrite the driver from scratch.
>>
>> Is there someone in your framework team that would be willing to respond
>> to these questions ? (I know you added hackbod to the CC, but they
>> aren't responding..)
>
> I guess it depends on the questions -- for good or ill, it's what is
> in use and replacing it with a different mechanism would be a pretty
> huge effort that there's unlikely to be time for in the near future.
> Quite a bit of the userspace framework depends on the binder handling
> the remote reference counting, object lifespan, etc, stuff and it's
> subtle and hairy to debug. ?Moving to a different transport is
> possible (it's just software, as they say), but there's a lot of code
> that depends on the behavior that exists today.
What is nice about binder is that is steps in an fills the gap for rapid
buffer-passing IPC, is it really not possible to find common ground
with a D-Bus in-kernel IPC driver now that you're anyway using both
mechanisms and benefiting to both ends?
What I really want to know, is how this relates to the vmsplice() and
other zero-copy buffer passing schemes already in the kernel. I was
sort of dreaming that D-Bus and other IPC could be accelerated on
top of that.
Yours,
Linus Walleij
On Thu, 2009-06-25 at 02:01 +0200, Linus Walleij wrote:
> What I really want to know, is how this relates to the vmsplice() and
> other zero-copy buffer passing schemes already in the kernel. I was
> sort of dreaming that D-Bus and other IPC could be accelerated on
> top of that.
Marcel had mentioned earlier in this thread that D-Bus could be
accelerated with shared memory or moving the dbus-daemon into the
kernel. splice() and vmplice() seem like fairly robust system calls. I
would think they could be used also ..
Daniel
2009/6/19 Daniel Walker <[email protected]>
> Most of these questions related to the fact that I don't think an
> interface like this just slips into the kernel as a driver. Since it's
> IPC, it's totally generic, and it's not part of a standard (i.e. POSIX),
> we need to have some better and more specific information about it (or
> atleast I do).
Hi, sorry I have been slow to respond.? I can give a summary of how
binder is used in the Android platform and the associated feature set.
I won't try to address other options, especially D-Bus, because
honestly I haven't been following it for the last 3 or so years so
don't really know its current state of art.
In the Android platform, the binder is used for nearly everything that
happens across processes in the core platform.? Some examples of this,
illustrating key features are:
- The window manager and clients talk with each other through Binder.
When a client starts up, it does a binder IPC into the window manager
to create a new binder connection dedicated to that client.? This is a
common use of the capability model of the binder, where secure
connections are given to clients which they can use for communication
with the system.
- The window manager and lower-level surface compositor talk with each
other through Binder.? There is as simple binder-based API that is
used to allocate a surface for a window.? This takes advantage of the
Binder's fd passing and object identity facilities to allow the
surface compositor to allocate area in a shared heap it manages: the
window manager makes this request on behalf of a client application,
and then passes that binder object over to the client process (it will
retrieve the associated fd and map it for each unique heap it
receives) for it to draw directly into the associated surface memory.
The binder's object identity rules (an object has a single identity as
it travels across processes, no matter how many times it does so or
where it goes) are very convenient for managing this.
- Separate components, like the window manager or surface flinger, may
be switched between running in the same process or different processes
with no change to their code.? For example, in the current android
platform these two components run in the same process, but we also
have had run them in other processes and would like to do so on
higher-end systems where there is more memory.? This is not strictly a
feature of the kernel part of the binder, but the IPC semantics it
provides greatly ease its implementation: dispatching transactions to
thread pools, synchronous calls with recursion across processes, etc.
- The activity (or really application/process) manager also uses the
binder for launching and managing components in a process.? For
applications, it creates a simple binder object for use as a "token"
for the application.? It gives this token to both the application and
the window manager, and the application gives its token to the window
manager when it adds windows.? Because the binder maintains object
identity, this model is used extensively in the system for security:
you can hand someone a token, and then can hand that token to others,
and you can always check whenever you get a token exactly who it was
originally given to without any way for clients to spoof it.? So the
activity manager can say to the window manager, "all of this token's
windows should be hidden," and the window manager can absolutely
identify which windows came from that application through the token
the app supplied with them.
- The fundamentals of Android's security are a combination of
uid-based permissions and binder capabilities.? Some capabilities are
direct (I give you access to my interface that you can call on), some
are indirect (I give you a binder object as a token that you can
compare against other tokens you receive to validate who it is).? For
permissions, every incoming binder transaction has associated with it
the uid of the initator, which is used in numerous places where we
want to only allow specific uids to access specific features.? For
example, there are APIs on the window manager to inject high-level
input events into the system, and the implementation of those methods
checks the calling uid to see if it is an application that has been
granted the permission to do this.
- The binder natively supports one-way and two-way calls.? Its two-way
calls are used extensively by all of the system services for incoming
IPCs for better multi-threading: they are dispatched directly from a
thread pool and the services acquire specific locks as needed to
protect their state (rather than serializing all calls through one
thread).? More traditional one-way/async calls are used for
communicating back with applications (or really for any service to
send commands to a higher-level part of the system).
- Many of the system services of course want to clean up state they
have associated with a client process.? For example, if an application
process goes away, all of its windows should be removed.? This is made
easy by the binder's "link to death" facility, which allows a process
to get a callback when another process hosting a binder object goes
away.? For example, the window manager links to the death of a
window's callback interface, and other services have clients send a
binder object token just to be able to find out when its process dies.
The driver provides this facility by telling a process about the
death of any objects it is watching.
- The Input Method Manager is probably one of the better
representative examples of how the binder facilities are used in the
system: it is a relatively small component, but makes extensive use of
binder object identities, capabilities, death links, and other
features to arbitrate between N applications and M IMEs securely
interacting with each other in a controlled way.? A taste of this can
be seen in the "Security" section of
http://developer.android.com/reference/android/view/inputmethod/InputMethodManager.html
.? One particular feature it relies on is allowing an application to
hand it a binder object for an interface (here an InputConnection),
which it can then send to an IME running in another process.? That IME
can now make direct calls on the InputConnection for just that
application (it has been granted that capability) without having to go
through the Input Method Manager intermediary process.
One part of the binder protocol that is really nice but doesn't yet
have a user space implementation is weak references.? This allows a
process to maintain knowledge of a remote object, without forcing it
to stay around.? At any point it can try to promote that to a strong
reference (to actively call on the object), which will either succeed
or fail based on whether the original object is still around or is not
around because all of the strong references (either in-proc or remote)
are gone.? We never re-implemented the user space code for this
because we didn't do weak references in the Java layer, but for native
C and C++ code it is a very nice facility for managing object
lifetimes.
For a rough idea of the scope of the binder's use in Android, here is
a list of the basic system services that are implemented on top of it:
package manager, telephony manager, app widgets, audio services,
search manager, location manager, notification manager, accessibility
manager, connectivity manager, wifi manager, input method manager,
clipboard, status bar, window manager, sensor service, alarm manager,
content service, activity manager, power manager, surface compositor.
> If for instance the main reason for Google using this interface is cause
> a large number of android people once worked at Palm or BeOS, that's not
> reason enough for it to go into the kernel. Or if this binder interface
> really fits well with Java or C++ people and they just love it, that's
> not really acceptable either..
It is true that a lot of the ideas of the binder came from previous
work on BeOS and Palm's Cobalt. However, that is mostly inspiration:
we started with the Open Binder code for very intial bringup, but
entirely rewrote both the user space and driver code to address our
needs for Android and to better fit with the Linux-centric design of
the platform.
I'm not sure what the relevance is of Java or C++ people liking it.
Does this mean that the important thing is that C people love it and
other languages don't matter? :) Anyway whether or not you "love" it
I don't think is a matter of programming language but just design
style, personal preference, and who knows what else. It has been
extremely useful in our implementation of Android, as can be seen in
just how much of the system sits on top of it, but that's all.
Finally as far as someone else's comment of Open Binder being dead --
well it's an interesting situation. That particular code is no longer
being developed, but basically the active development switched over to
the fork/rewrite of it we have now in Android. You could maybe say
that Open Binder was a research project, and Android is the shipping
implementation. Though really, the main difference between them is
that Android has a much simpler user-space implementation (because we
didn't need the full features of Open Binder); there isn't any reason
the full Open Binder environment couldn't be put back on top of the
current binder. The binder shell is certainly a fun toy. :) See
http://www.open-binder.org/docs/html/BinderShellTutorial.html for
example. But a lot of the stuff there is just not hugely interesting
for Linux/Android.
--
Dianne Hackborn
Android framework engineer
[email protected]
On Wed, 24 Jun 2009 17:20:19 -0700
Daniel Walker <[email protected]> wrote:
> On Thu, 2009-06-25 at 02:01 +0200, Linus Walleij wrote:
>
> > What I really want to know, is how this relates to the vmsplice() and
> > other zero-copy buffer passing schemes already in the kernel. I was
> > sort of dreaming that D-Bus and other IPC could be accelerated on
> > top of that.
>
> Marcel had mentioned earlier in this thread that D-Bus could be
> accelerated with shared memory or moving the dbus-daemon into the
> kernel. splice() and vmplice() seem like fairly robust system calls. I
> would think they could be used also ..
Except for very large amounts of data what makes you think zero copy
buffer passing will be fast ? TLB shootdowns are expensive and they scale
horribly badly with threaded apps on multiprocessor systems ?
Hi Alan,
> > > What I really want to know, is how this relates to the vmsplice() and
> > > other zero-copy buffer passing schemes already in the kernel. I was
> > > sort of dreaming that D-Bus and other IPC could be accelerated on
> > > top of that.
> >
> > Marcel had mentioned earlier in this thread that D-Bus could be
> > accelerated with shared memory or moving the dbus-daemon into the
> > kernel. splice() and vmplice() seem like fairly robust system calls. I
> > would think they could be used also ..
>
> Except for very large amounts of data what makes you think zero copy
> buffer passing will be fast ? TLB shootdowns are expensive and they scale
> horribly badly with threaded apps on multiprocessor systems ?
there is always the problem if we have really stupidly written apps that
just copy megabyte of data from one app to the other. These just need
fixing and Lennart posted patches to integrate file descriptor passing
into the D-Bus protocol which will make it more versatile. And for most
cases it will be just good enough to move file descriptors around. Then
having the possibility to pass bigger data blobs without too many
penalties would be an extra bonus.
Regards
Marcel
Hi Dianne,
> > Most of these questions related to the fact that I don't think an
> > interface like this just slips into the kernel as a driver. Since it's
> > IPC, it's totally generic, and it's not part of a standard (i.e. POSIX),
> > we need to have some better and more specific information about it (or
> > atleast I do).
>
> Hi, sorry I have been slow to respond. I can give a summary of how
> binder is used in the Android platform and the associated feature set.
> I won't try to address other options, especially D-Bus, because
> honestly I haven't been following it for the last 3 or so years so
> don't really know its current state of art.
let see how we can compare it to D-Bus right now.
> In the Android platform, the binder is used for nearly everything that
> happens across processes in the core platform. Some examples of this,
> illustrating key features are:
>
> - The window manager and clients talk with each other through Binder.
> When a client starts up, it does a binder IPC into the window manager
> to create a new binder connection dedicated to that client. This is a
> common use of the capability model of the binder, where secure
> connections are given to clients which they can use for communication
> with the system.
That is exactly what D-Bus is used for by any other Linux system. And
embedded examples are the Nokia N810 and Palm Pre. To name only two
since there are more.
> - The window manager and lower-level surface compositor talk with each
> other through Binder. There is as simple binder-based API that is
> used to allocate a surface for a window. This takes advantage of the
> Binder's fd passing and object identity facilities to allow the
> surface compositor to allocate area in a shared heap it manages: the
> window manager makes this request on behalf of a client application,
> and then passes that binder object over to the client process (it will
> retrieve the associated fd and map it for each unique heap it
> receives) for it to draw directly into the associated surface memory.
> The binder's object identity rules (an object has a single identity as
> it travels across processes, no matter how many times it does so or
> where it goes) are very convenient for managing this.
Patches for D-Bus with file descriptor passing have been posted. So that
feature is available.
> - Separate components, like the window manager or surface flinger, may
> be switched between running in the same process or different processes
> with no change to their code. For example, in the current android
> platform these two components run in the same process, but we also
> have had run them in other processes and would like to do so on
> higher-end systems where there is more memory. This is not strictly a
> feature of the kernel part of the binder, but the IPC semantics it
> provides greatly ease its implementation: dispatching transactions to
> thread pools, synchronous calls with recursion across processes, etc.
D-Bus provides namespacing based on service name, interface names and
object paths. So you can combine applications into one binary if you
want to.
> - The activity (or really application/process) manager also uses the
> binder for launching and managing components in a process. For
> applications, it creates a simple binder object for use as a "token"
> for the application. It gives this token to both the application and
> the window manager, and the application gives its token to the window
> manager when it adds windows. Because the binder maintains object
> identity, this model is used extensively in the system for security:
> you can hand someone a token, and then can hand that token to others,
> and you can always check whenever you get a token exactly who it was
> originally given to without any way for clients to spoof it. So the
> activity manager can say to the window manager, "all of this token's
> windows should be hidden," and the window manager can absolutely
> identify which windows came from that application through the token
> the app supplied with them.
D-Bus has unique bus names for all application connecting to the bus. It
can be used to detect startup and termination of apps. The Bluetooth
stack for example makes massive use of this to cleanup after broken UI
application.
Also D-Bus supports system and session bus activation. So it can start
applications and daemons.
> - The fundamentals of Android's security are a combination of
> uid-based permissions and binder capabilities. Some capabilities are
> direct (I give you access to my interface that you can call on), some
> are indirect (I give you a binder object as a token that you can
> compare against other tokens you receive to validate who it is). For
> permissions, every incoming binder transaction has associated with it
> the uid of the initator, which is used in numerous places where we
> want to only allow specific uids to access specific features. For
> example, there are APIs on the window manager to inject high-level
> input events into the system, and the implementation of those methods
> checks the calling uid to see if it is an application that has been
> granted the permission to do this.
D-Bus integrates with SELinux which is way superior than any UID based
approach anyway. It also has its own authentication scheme and with the
help of PolicyKit system wide access policies can be implemented and
extended at runtime by privileged users.
> - The binder natively supports one-way and two-way calls. Its two-way
> calls are used extensively by all of the system services for incoming
> IPCs for better multi-threading: they are dispatched directly from a
> thread pool and the services acquire specific locks as needed to
> protect their state (rather than serializing all calls through one
> thread). More traditional one-way/async calls are used for
> communicating back with applications (or really for any service to
> send commands to a higher-level part of the system).
I have no idea on how to compare this and what kind of advantage this
should be.
Within D-Bus every message is async and some of them are method call
with a reply message or errors or signals.
> - Many of the system services of course want to clean up state they
> have associated with a client process. For example, if an application
> process goes away, all of its windows should be removed. This is made
> easy by the binder's "link to death" facility, which allows a process
> to get a callback when another process hosting a binder object goes
> away. For example, the window manager links to the death of a
> window's callback interface, and other services have clients send a
> binder object token just to be able to find out when its process dies.
> The driver provides this facility by telling a process about the
> death of any objects it is watching.
That can be done with D-Bus via the NameOwnerChanged signal and the
unique bus name. As mentioned above the Bluetooth subsystem makes
massive use of this feature.
> - The Input Method Manager is probably one of the better
> representative examples of how the binder facilities are used in the
> system: it is a relatively small component, but makes extensive use of
> binder object identities, capabilities, death links, and other
> features to arbitrate between N applications and M IMEs securely
> interacting with each other in a controlled way. A taste of this can
> be seen in the "Security" section of
> http://developer.android.com/reference/android/view/inputmethod/InputMethodManager.html
> . One particular feature it relies on is allowing an application to
> hand it a binder object for an interface (here an InputConnection),
> which it can then send to an IME running in another process. That IME
> can now make direct calls on the InputConnection for just that
> application (it has been granted that capability) without having to go
> through the Input Method Manager intermediary process.
The Linux desktop systems use D-Bus a lot. Some use more features than
other. Feel free to look at it.
> One part of the binder protocol that is really nice but doesn't yet
> have a user space implementation is weak references. This allows a
> process to maintain knowledge of a remote object, without forcing it
> to stay around. At any point it can try to promote that to a strong
> reference (to actively call on the object), which will either succeed
> or fail based on whether the original object is still around or is not
> around because all of the strong references (either in-proc or remote)
> are gone. We never re-implemented the user space code for this
> because we didn't do weak references in the Java layer, but for native
> C and C++ code it is a very nice facility for managing object
> lifetimes.
I am not sure what this is good for. Especially since you didn't expose
it in the Java layer anyway.
D-Bus has system and session activation which can be used to start/stop
application based on service names. A D-Bus service name can also be
pending and waiting for the previous owner to terminate and then it will
take over. Some sort of failsafe if you want.
> For a rough idea of the scope of the binder's use in Android, here is
> a list of the basic system services that are implemented on top of it:
> package manager, telephony manager, app widgets, audio services,
> search manager, location manager, notification manager, accessibility
> manager, connectivity manager, wifi manager, input method manager,
> clipboard, status bar, window manager, sensor service, alarm manager,
> content service, activity manager, power manager, surface compositor.
I am not doing the Linux list since that would take me longer than a day
to compile and would forgot some of the apps ;)
> > If for instance the main reason for Google using this interface is cause
> > a large number of android people once worked at Palm or BeOS, that's not
> > reason enough for it to go into the kernel. Or if this binder interface
> > really fits well with Java or C++ people and they just love it, that's
> > not really acceptable either..
>
> It is true that a lot of the ideas of the binder came from previous
> work on BeOS and Palm's Cobalt. However, that is mostly inspiration:
> we started with the Open Binder code for very intial bringup, but
> entirely rewrote both the user space and driver code to address our
> needs for Android and to better fit with the Linux-centric design of
> the platform.
>
> I'm not sure what the relevance is of Java or C++ people liking it.
> Does this mean that the important thing is that C people love it and
> other languages don't matter? :) Anyway whether or not you "love" it
> I don't think is a matter of programming language but just design
> style, personal preference, and who knows what else. It has been
> extremely useful in our implementation of Android, as can be seen in
> just how much of the system sits on top of it, but that's all.
>
> Finally as far as someone else's comment of Open Binder being dead --
> well it's an interesting situation. That particular code is no longer
> being developed, but basically the active development switched over to
> the fork/rewrite of it we have now in Android. You could maybe say
> that Open Binder was a research project, and Android is the shipping
> implementation. Though really, the main difference between them is
> that Android has a much simpler user-space implementation (because we
> didn't need the full features of Open Binder); there isn't any reason
> the full Open Binder environment couldn't be put back on top of the
> current binder. The binder shell is certainly a fun toy. :) See
> http://www.open-binder.org/docs/html/BinderShellTutorial.html for
> example. But a lot of the stuff there is just not hugely interesting
> for Linux/Android.
The thing that I don't understand is why you bothered to re-write Binder
and not just use D-Bus. Especially since D-Bus is part of Android
anyway, because of BlueZ.
To me it seems everything that you want from Binder is already possible
with D-Bus. Seems the only missing piece is to create dbus-daemon as a
kernel subsystem so it is available for all userspace processes from the
beginning (including init/upstart).
Regards
Marcel
> To me it seems everything that you want from Binder is already possible
> with D-Bus. Seems the only missing piece is to create dbus-daemon as a
> kernel subsystem so it is available for all userspace processes from the
> beginning (including init/upstart).
You don't even need that - you can start dbus immediately as part of init
if it makes you happy.
On Wed, 2009-06-24 at 21:09 -0700, Dianne Hackborn wrote:
> 2009/6/19 Daniel Walker <[email protected]>
> > Most of these questions related to the fact that I don't think an
> > interface like this just slips into the kernel as a driver. Since it's
> > IPC, it's totally generic, and it's not part of a standard (i.e. POSIX),
> > we need to have some better and more specific information about it (or
> > atleast I do).
>
> Hi, sorry I have been slow to respond. I can give a summary of how
> binder is used in the Android platform and the associated feature set.
> I won't try to address other options, especially D-Bus, because
> honestly I haven't been following it for the last 3 or so years so
> don't really know its current state of art.
Thank your for the extensive response. I'm sure it will be helpful in
educating us as to the usage of the interface..
Was dbus initially investigated by the Android project as something that
could be used for IPC? Which other mechanisms had been investigated for
IPC beyond binder?
> I'm not sure what the relevance is of Java or C++ people liking it.
> Does this mean that the important thing is that C people love it and
> other languages don't matter? :) Anyway whether or not you "love" it
> I don't think is a matter of programming language but just design
> style, personal preference, and who knows what else. It has been
> extremely useful in our implementation of Android, as can be seen in
> just how much of the system sits on top of it, but that's all.
The important thing is that it's in the kernel for a better reason than
simply satisfying a certain language group ..
I think the biggest issue I have with the binder implementation is that
it's doing far too much in the kernel, it's not just IPC. It's also
thread management, memory management, and lots of other stuff that I
haven't figured out yet .. A lot of it can already be done in userspace.
Daniel
> 2009/6/19 Daniel Walker <[email protected]>
>> Most of these questions related to the fact that I don't think an
>> interface like this just slips into the kernel as a driver. Since it's
>> IPC, it's totally generic, and it's not part of a standard (i.e. POSIX),
>> we need to have some better and more specific information about it (or
>> atleast I do).
>
> Hi, sorry I have been slow to respond. I can give a summary of how
> binder is used in the Android platform and the associated feature set.
> I won't try to address other options, especially D-Bus, because
> honestly I haven't been following it for the last 3 or so years so
> don't really know its current state of art.
> Dianne Hackborn
Um. You means that Android team selected Openbinder of beOS from Palm
for IPC mechanism in Android software stack 3 years ago.
At that time,
Framework team used Binder(= modified Openbinder core only) just
because D-bus is a premature for android opensource project like belows.
- dbus-0.1.tar.gz (12-Jan-2005 17:20 , 309K)
After all,
Current android software stack consists of two IPC equipments like
Binder and D-bus without especial goal ( ? ) .
> I think the biggest issue I have with the binder implementation is that
> it's doing far too much in the kernel, it's not just IPC. It's also
> thread management, memory management, and lots of other stuff that I
> haven't figured out yet .. A lot of it can already be done in userspace.
> Daniel
I agree with your opinions.
I want android framework team to improve IPC from two(Binder and D-ubs)
to one in the future.
D-bus is stable and mature for IPC currently as we all know.
This is used in the linux based distributions like Fedora, Ubuntu,
Suse successfully. D-bus also is connecting Udev(Userspace Device)
and sysfs(system filesystem)
well in embedded linux products as Nokie released commericial N8XX products.
I think that In theory as well as in practice, the idea of two IPCs was unsound.
Thks,
--
Regards,
GeunSik Lim ( Samsung Electronics )
Blog : http://blog.naver.com/invain/
e-Mail: [email protected]
[email protected] , [email protected]
