* [PATCH 1/6] staging: android: binder: Remove some funny && usage @ 2009-06-12 18:51 Daniel Walker 2009-06-12 18:51 ` [PATCH 2/6] staging: android: binder: move debugging mask into a macro Daniel Walker 2009-06-16 20:46 ` [PATCH 1/6] staging: android: binder: Remove some funny && usage Jeremy Fitzhardinge 0 siblings, 2 replies; 38+ messages in thread From: Daniel Walker @ 2009-06-12 18:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Brian Swetland, linux-kernel, Daniel Walker 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 <dwalker@fifo99.com> --- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 2/6] staging: android: binder: move debugging mask into a macro 2009-06-12 18:51 [PATCH 1/6] staging: android: binder: Remove some funny && usage Daniel Walker @ 2009-06-12 18:51 ` Daniel Walker 2009-06-12 18:51 ` [PATCH 3/6] staging: android: binder: remove a predefine Daniel Walker 2009-06-16 20:46 ` [PATCH 1/6] staging: android: binder: Remove some funny && usage Jeremy Fitzhardinge 1 sibling, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-12 18:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Brian Swetland, linux-kernel, Daniel Walker 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 <dwalker@fifo99.com> --- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 3/6] staging: android: binder: remove a predefine 2009-06-12 18:51 ` [PATCH 2/6] staging: android: binder: move debugging mask into a macro Daniel Walker @ 2009-06-12 18:51 ` Daniel Walker 2009-06-12 18:51 ` [PATCH 4/6] staging: android: binder: add enum usage in function arguments Daniel Walker 0 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-12 18:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Brian Swetland, linux-kernel, Daniel Walker I removed the binder_transaction_buffer_release predefine, and put the actual function in place of it. Signed-off-by: Daniel Walker <dwalker@fifo99.com> --- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 4/6] staging: android: binder: add enum usage in function arguments 2009-06-12 18:51 ` [PATCH 3/6] staging: android: binder: remove a predefine Daniel Walker @ 2009-06-12 18:51 ` Daniel Walker 2009-06-12 18:51 ` [PATCH 5/6] staging: android: binder: global variable cleanup Daniel Walker 0 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-12 18:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Brian Swetland, linux-kernel, Daniel Walker 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 <dwalker@fifo99.com> --- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 5/6] staging: android: binder: global variable cleanup. 2009-06-12 18:51 ` [PATCH 4/6] staging: android: binder: add enum usage in function arguments Daniel Walker @ 2009-06-12 18:51 ` Daniel Walker 2009-06-12 18:51 ` [PATCH 6/6] staging: android: binder: clean up for all the stat statments Daniel Walker 0 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-12 18:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Brian Swetland, linux-kernel, Daniel Walker Replaced a manual hlist_head declaration with a macro based one. Also reorganized the globals to be grouped better. Signed-off-by: Daniel Walker <dwalker@fifo99.com> --- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 6/6] staging: android: binder: clean up for all the stat statments 2009-06-12 18:51 ` [PATCH 5/6] staging: android: binder: global variable cleanup Daniel Walker @ 2009-06-12 18:51 ` Daniel Walker 0 siblings, 0 replies; 38+ messages in thread From: Daniel Walker @ 2009-06-12 18:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Brian Swetland, linux-kernel, Daniel Walker 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 <dwalker@fifo99.com> --- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-12 18:51 [PATCH 1/6] staging: android: binder: Remove some funny && usage Daniel Walker 2009-06-12 18:51 ` [PATCH 2/6] staging: android: binder: move debugging mask into a macro Daniel Walker @ 2009-06-16 20:46 ` Jeremy Fitzhardinge 2009-06-17 14:37 ` Daniel Walker 2009-06-19 14:59 ` Pavel Machek 1 sibling, 2 replies; 38+ messages in thread From: Jeremy Fitzhardinge @ 2009-06-16 20:46 UTC (permalink / raw) To: Daniel Walker; +Cc: Greg Kroah-Hartman, Brian Swetland, linux-kernel On 06/12/09 11:51, Daniel Walker wrote: > Signed-off-by: Daniel Walker<dwalker@fifo99.com> > --- > 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-16 20:46 ` [PATCH 1/6] staging: android: binder: Remove some funny && usage Jeremy Fitzhardinge @ 2009-06-17 14:37 ` Daniel Walker 2009-06-17 15:28 ` Jeremy Fitzhardinge 2009-06-19 14:59 ` Pavel Machek 1 sibling, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-17 14:37 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: Greg Kroah-Hartman, Brian Swetland, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-17 14:37 ` Daniel Walker @ 2009-06-17 15:28 ` Jeremy Fitzhardinge 2009-06-17 16:08 ` Daniel Walker 0 siblings, 1 reply; 38+ messages in thread From: Jeremy Fitzhardinge @ 2009-06-17 15:28 UTC (permalink / raw) To: Daniel Walker; +Cc: Greg Kroah-Hartman, Brian Swetland, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-17 15:28 ` Jeremy Fitzhardinge @ 2009-06-17 16:08 ` Daniel Walker 2009-06-17 16:31 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-17 16:08 UTC (permalink / raw) To: Jeremy Fitzhardinge Cc: Greg Kroah-Hartman, Brian Swetland, arve, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-17 16:08 ` Daniel Walker @ 2009-06-17 16:31 ` Jeremy Fitzhardinge 2009-06-17 21:26 ` Arve Hjønnevåg 0 siblings, 1 reply; 38+ messages in thread From: Jeremy Fitzhardinge @ 2009-06-17 16:31 UTC (permalink / raw) To: Daniel Walker; +Cc: Greg Kroah-Hartman, Brian Swetland, arve, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-17 16:31 ` Jeremy Fitzhardinge @ 2009-06-17 21:26 ` Arve Hjønnevåg 2009-06-17 21:31 ` Daniel Walker 2009-06-17 21:38 ` Jeremy Fitzhardinge 0 siblings, 2 replies; 38+ messages in thread From: Arve Hjønnevåg @ 2009-06-17 21:26 UTC (permalink / raw) To: Jeremy Fitzhardinge Cc: Daniel Walker, Greg Kroah-Hartman, Brian Swetland, linux-kernel On Wed, Jun 17, 2009 at 9:31 AM, Jeremy Fitzhardinge<jeremy@goop.org> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-17 21:26 ` Arve Hjønnevåg @ 2009-06-17 21:31 ` Daniel Walker 2009-06-19 19:20 ` Brian Swetland 2009-06-17 21:38 ` Jeremy Fitzhardinge 1 sibling, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-17 21:31 UTC (permalink / raw) To: Arve Hjønnevåg Cc: Jeremy Fitzhardinge, Greg Kroah-Hartman, Brian Swetland, linux-kernel 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<jeremy@goop.org> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-17 21:31 ` Daniel Walker @ 2009-06-19 19:20 ` Brian Swetland 2009-06-19 22:53 ` Daniel Walker 0 siblings, 1 reply; 38+ messages in thread From: Brian Swetland @ 2009-06-19 19:20 UTC (permalink / raw) To: Daniel Walker Cc: Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel 2009/6/17 Daniel Walker <dwalker@fifo99.com>: > 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<jeremy@goop.org> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-19 19:20 ` Brian Swetland @ 2009-06-19 22:53 ` Daniel Walker 2009-06-20 0:13 ` Arve Hjønnevåg 0 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-19 22:53 UTC (permalink / raw) To: Brian Swetland Cc: Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel On Fri, 2009-06-19 at 12:20 -0700, Brian Swetland wrote: > 2009/6/17 Daniel Walker <dwalker@fifo99.com>: > > 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<jeremy@goop.org> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-19 22:53 ` Daniel Walker @ 2009-06-20 0:13 ` Arve Hjønnevåg 2009-06-20 0:49 ` Daniel Walker ` (2 more replies) 0 siblings, 3 replies; 38+ messages in thread From: Arve Hjønnevåg @ 2009-06-20 0:13 UTC (permalink / raw) To: Daniel Walker Cc: Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod On Fri, Jun 19, 2009 at 3:53 PM, Daniel Walker<dwalker@fifo99.com> wrote: > On Fri, 2009-06-19 at 12:20 -0700, Brian Swetland wrote: >> 2009/6/17 Daniel Walker <dwalker@fifo99.com>: >> > 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<jeremy@goop.org> 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 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-20 0:13 ` Arve Hjønnevåg @ 2009-06-20 0:49 ` Daniel Walker 2009-06-20 18:48 ` Christoph Hellwig ` (2 more replies) 2009-06-20 1:26 ` GeunSik Lim 2009-06-24 13:13 ` Daniel Walker 2 siblings, 3 replies; 38+ messages in thread From: Daniel Walker @ 2009-06-20 0:49 UTC (permalink / raw) To: Arve Hjønnevåg Cc: Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 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 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-20 0:49 ` Daniel Walker @ 2009-06-20 18:48 ` Christoph Hellwig 2009-06-21 12:09 ` Marcel Holtmann 2009-06-25 4:09 ` Dianne Hackborn 2 siblings, 0 replies; 38+ messages in thread From: Christoph Hellwig @ 2009-06-20 18:48 UTC (permalink / raw) To: Daniel Walker Cc: Arve Hj?nnev?g, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-20 0:49 ` Daniel Walker 2009-06-20 18:48 ` Christoph Hellwig @ 2009-06-21 12:09 ` Marcel Holtmann 2009-06-25 4:09 ` Dianne Hackborn 2 siblings, 0 replies; 38+ messages in thread From: Marcel Holtmann @ 2009-06-21 12:09 UTC (permalink / raw) To: Daniel Walker Cc: Arve Hjønnevåg, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 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 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-20 0:49 ` Daniel Walker 2009-06-20 18:48 ` Christoph Hellwig 2009-06-21 12:09 ` Marcel Holtmann @ 2009-06-25 4:09 ` Dianne Hackborn 2009-06-25 10:14 ` Marcel Holtmann 2009-06-25 13:24 ` Daniel Walker 2 siblings, 2 replies; 38+ messages in thread From: Dianne Hackborn @ 2009-06-25 4:09 UTC (permalink / raw) To: Daniel Walker Cc: Arve Hjønnevåg, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel 2009/6/19 Daniel Walker <dwalker@fifo99.com> > 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 hackbod@android.com ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-25 4:09 ` Dianne Hackborn @ 2009-06-25 10:14 ` Marcel Holtmann 2009-06-25 11:34 ` Alan Cox 2009-06-25 13:24 ` Daniel Walker 1 sibling, 1 reply; 38+ messages in thread From: Marcel Holtmann @ 2009-06-25 10:14 UTC (permalink / raw) To: Dianne Hackborn Cc: Daniel Walker, Arve Hjønnevåg, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-25 10:14 ` Marcel Holtmann @ 2009-06-25 11:34 ` Alan Cox 0 siblings, 0 replies; 38+ messages in thread From: Alan Cox @ 2009-06-25 11:34 UTC (permalink / raw) To: Marcel Holtmann Cc: Dianne Hackborn, Daniel Walker, Arve Hjønnevåg, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel > 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-25 4:09 ` Dianne Hackborn 2009-06-25 10:14 ` Marcel Holtmann @ 2009-06-25 13:24 ` Daniel Walker 2009-06-27 2:20 ` GeunSik Lim 1 sibling, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-25 13:24 UTC (permalink / raw) To: Dianne Hackborn Cc: Arve Hjønnevåg, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel On Wed, 2009-06-24 at 21:09 -0700, Dianne Hackborn wrote: > 2009/6/19 Daniel Walker <dwalker@fifo99.com> > > 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-25 13:24 ` Daniel Walker @ 2009-06-27 2:20 ` GeunSik Lim 0 siblings, 0 replies; 38+ messages in thread From: GeunSik Lim @ 2009-06-27 2:20 UTC (permalink / raw) To: Daniel Walker Cc: Dianne Hackborn, Arve Hjønnevåg, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel > 2009/6/19 Daniel Walker <dwalker@fifo99.com> >> 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: geunsik.lim@samsung.com leemgs@gmail.com , leemgs1@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-20 0:13 ` Arve Hjønnevåg 2009-06-20 0:49 ` Daniel Walker @ 2009-06-20 1:26 ` GeunSik Lim 2009-06-24 13:13 ` Daniel Walker 2 siblings, 0 replies; 38+ messages in thread From: GeunSik Lim @ 2009-06-20 1:26 UTC (permalink / raw) To: Arve Hjønnevåg Cc: Daniel Walker, Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 2009/6/20 Arve Hjønnevåg <arve@android.com>: >> >> 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 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: geunsik.lim@samsung.com leemgs@gmail.com , leemgs1@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-20 0:13 ` Arve Hjønnevåg 2009-06-20 0:49 ` Daniel Walker 2009-06-20 1:26 ` GeunSik Lim @ 2009-06-24 13:13 ` Daniel Walker 2009-06-24 22:14 ` Brian Swetland 2 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-24 13:13 UTC (permalink / raw) To: Arve Hjønnevåg Cc: Brian Swetland, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 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 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-24 13:13 ` Daniel Walker @ 2009-06-24 22:14 ` Brian Swetland 2009-06-24 22:49 ` Daniel Walker 2009-06-25 0:01 ` Linus Walleij 0 siblings, 2 replies; 38+ messages in thread From: Brian Swetland @ 2009-06-24 22:14 UTC (permalink / raw) To: Daniel Walker Cc: Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 2009/6/24 Daniel Walker <dwalker@fifo99.com>: > 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 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-24 22:14 ` Brian Swetland @ 2009-06-24 22:49 ` Daniel Walker 2009-06-24 23:05 ` Brian Swetland 2009-06-25 0:01 ` Linus Walleij 1 sibling, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-24 22:49 UTC (permalink / raw) To: Brian Swetland Cc: Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-24 22:49 ` Daniel Walker @ 2009-06-24 23:05 ` Brian Swetland 2009-06-24 23:29 ` Daniel Walker 0 siblings, 1 reply; 38+ messages in thread From: Brian Swetland @ 2009-06-24 23:05 UTC (permalink / raw) To: Daniel Walker Cc: Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod On Wed, Jun 24, 2009 at 3:49 PM, Daniel Walker<dwalker@fifo99.com> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-24 23:05 ` Brian Swetland @ 2009-06-24 23:29 ` Daniel Walker 2009-06-24 23:37 ` Brian Swetland 0 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-24 23:29 UTC (permalink / raw) To: Brian Swetland Cc: Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod On Wed, 2009-06-24 at 16:05 -0700, Brian Swetland wrote: > On Wed, Jun 24, 2009 at 3:49 PM, Daniel Walker<dwalker@fifo99.com> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-24 23:29 ` Daniel Walker @ 2009-06-24 23:37 ` Brian Swetland 0 siblings, 0 replies; 38+ messages in thread From: Brian Swetland @ 2009-06-24 23:37 UTC (permalink / raw) To: Daniel Walker Cc: Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod On Wed, Jun 24, 2009 at 4:29 PM, Daniel Walker<dwalker@fifo99.com> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-24 22:14 ` Brian Swetland 2009-06-24 22:49 ` Daniel Walker @ 2009-06-25 0:01 ` Linus Walleij 2009-06-25 0:20 ` Daniel Walker 1 sibling, 1 reply; 38+ messages in thread From: Linus Walleij @ 2009-06-25 0:01 UTC (permalink / raw) To: Brian Swetland Cc: Daniel Walker, Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 2009/6/25 Brian Swetland <swetland@google.com>: > 2009/6/24 Daniel Walker <dwalker@fifo99.com>: >> 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 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-25 0:01 ` Linus Walleij @ 2009-06-25 0:20 ` Daniel Walker 2009-06-25 8:15 ` Alan Cox 0 siblings, 1 reply; 38+ messages in thread From: Daniel Walker @ 2009-06-25 0:20 UTC (permalink / raw) To: Linus Walleij Cc: Brian Swetland, Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod, Marcel Holtmann 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-25 0:20 ` Daniel Walker @ 2009-06-25 8:15 ` Alan Cox 2009-06-25 9:56 ` Marcel Holtmann 0 siblings, 1 reply; 38+ messages in thread From: Alan Cox @ 2009-06-25 8:15 UTC (permalink / raw) To: Daniel Walker Cc: Linus Walleij, Brian Swetland, Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod, Marcel Holtmann On Wed, 24 Jun 2009 17:20:19 -0700 Daniel Walker <dwalker@fifo99.com> 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 ? ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-25 8:15 ` Alan Cox @ 2009-06-25 9:56 ` Marcel Holtmann 0 siblings, 0 replies; 38+ messages in thread From: Marcel Holtmann @ 2009-06-25 9:56 UTC (permalink / raw) To: Alan Cox Cc: Daniel Walker, Linus Walleij, Brian Swetland, Arve Hjønnevåg, Jeremy Fitzhardinge, Greg Kroah-Hartman, linux-kernel, hackbod 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-17 21:26 ` Arve Hjønnevåg 2009-06-17 21:31 ` Daniel Walker @ 2009-06-17 21:38 ` Jeremy Fitzhardinge 1 sibling, 0 replies; 38+ messages in thread From: Jeremy Fitzhardinge @ 2009-06-17 21:38 UTC (permalink / raw) To: Arve Hjønnevåg Cc: Daniel Walker, Greg Kroah-Hartman, Brian Swetland, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-16 20:46 ` [PATCH 1/6] staging: android: binder: Remove some funny && usage Jeremy Fitzhardinge 2009-06-17 14:37 ` Daniel Walker @ 2009-06-19 14:59 ` Pavel Machek 2009-06-19 15:08 ` Daniel Walker 1 sibling, 1 reply; 38+ messages in thread From: Pavel Machek @ 2009-06-19 14:59 UTC (permalink / raw) To: Jeremy Fitzhardinge Cc: Daniel Walker, Greg Kroah-Hartman, Brian Swetland, linux-kernel >> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage 2009-06-19 14:59 ` Pavel Machek @ 2009-06-19 15:08 ` Daniel Walker 0 siblings, 0 replies; 38+ messages in thread From: Daniel Walker @ 2009-06-19 15:08 UTC (permalink / raw) To: Pavel Machek Cc: Jeremy Fitzhardinge, Greg Kroah-Hartman, Brian Swetland, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2009-06-27 2:20 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-06-12 18:51 [PATCH 1/6] staging: android: binder: Remove some funny && usage Daniel Walker 2009-06-12 18:51 ` [PATCH 2/6] staging: android: binder: move debugging mask into a macro Daniel Walker 2009-06-12 18:51 ` [PATCH 3/6] staging: android: binder: remove a predefine Daniel Walker 2009-06-12 18:51 ` [PATCH 4/6] staging: android: binder: add enum usage in function arguments Daniel Walker 2009-06-12 18:51 ` [PATCH 5/6] staging: android: binder: global variable cleanup Daniel Walker 2009-06-12 18:51 ` [PATCH 6/6] staging: android: binder: clean up for all the stat statments Daniel Walker 2009-06-16 20:46 ` [PATCH 1/6] staging: android: binder: Remove some funny && usage Jeremy Fitzhardinge 2009-06-17 14:37 ` Daniel Walker 2009-06-17 15:28 ` Jeremy Fitzhardinge 2009-06-17 16:08 ` Daniel Walker 2009-06-17 16:31 ` Jeremy Fitzhardinge 2009-06-17 21:26 ` Arve Hjønnevåg 2009-06-17 21:31 ` Daniel Walker 2009-06-19 19:20 ` Brian Swetland 2009-06-19 22:53 ` Daniel Walker 2009-06-20 0:13 ` Arve Hjønnevåg 2009-06-20 0:49 ` Daniel Walker 2009-06-20 18:48 ` Christoph Hellwig 2009-06-21 12:09 ` Marcel Holtmann 2009-06-25 4:09 ` Dianne Hackborn 2009-06-25 10:14 ` Marcel Holtmann 2009-06-25 11:34 ` Alan Cox 2009-06-25 13:24 ` Daniel Walker 2009-06-27 2:20 ` GeunSik Lim 2009-06-20 1:26 ` GeunSik Lim 2009-06-24 13:13 ` Daniel Walker 2009-06-24 22:14 ` Brian Swetland 2009-06-24 22:49 ` Daniel Walker 2009-06-24 23:05 ` Brian Swetland 2009-06-24 23:29 ` Daniel Walker 2009-06-24 23:37 ` Brian Swetland 2009-06-25 0:01 ` Linus Walleij 2009-06-25 0:20 ` Daniel Walker 2009-06-25 8:15 ` Alan Cox 2009-06-25 9:56 ` Marcel Holtmann 2009-06-17 21:38 ` Jeremy Fitzhardinge 2009-06-19 14:59 ` Pavel Machek 2009-06-19 15:08 ` Daniel Walker
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.