* [PATCH v1 0/1] binder: return pending info for frozen async txns @ 2022-11-03 19:05 Li Li 2022-11-03 19:05 ` [PATCH v1 1/1] " Li Li 0 siblings, 1 reply; 4+ messages in thread From: Li Li @ 2022-11-03 19:05 UTC (permalink / raw) To: dualli, gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb, arnd, masahiroy, devel, linux-kernel, hridya, smoreland Cc: kernel-team From: Li Li <dualli@google.com> User applications need to know if their binder transactions reach a frozen process or not. For sync binder calls, Linux kernel already has a dedicated return value BR_FROZEN_REPLY, indicating this sync binder transaction will be rejected (similar to BR_DEAD_REPLY) as the target process is frozen. But for async binder calls, the user space application doesn't have a way to know if the target process is frozen. This patch add a new return value, BR_TRANSACTION_PENDING, to fix this issue. Similar to BR_TRANSACTION_COMPLETE, it means the async binder transaction has been put in the queue of the target process, but it's waiting for the target process to be unfrozen. v1: checkpatch.pl --strict passed Li Li (1): binder: return pending info for frozen async txns drivers/android/binder.c | 23 ++++++++++++++++++++--- drivers/android/binder_internal.h | 3 ++- include/uapi/linux/android/binder.h | 7 ++++++- 3 files changed, 28 insertions(+), 5 deletions(-) -- 2.38.1.431.g37b22c650d-goog ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v1 1/1] binder: return pending info for frozen async txns 2022-11-03 19:05 [PATCH v1 0/1] binder: return pending info for frozen async txns Li Li @ 2022-11-03 19:05 ` Li Li 2022-11-09 22:43 ` Carlos Llamas 0 siblings, 1 reply; 4+ messages in thread From: Li Li @ 2022-11-03 19:05 UTC (permalink / raw) To: dualli, gregkh, arve, tkjos, maco, joel, brauner, cmllamas, surenb, arnd, masahiroy, devel, linux-kernel, hridya, smoreland Cc: kernel-team From: Li Li <dualli@google.com> An async transaction to a frozen process will still be successsfully put in the queue. But this pending async transaction won't be processed until the target process is unfrozen at an unspecified time in the future. Pass this important information back to the user space caller by returning BR_TRANSACTION_PENDING. Signed-off-by: Li Li <dualli@google.com> --- drivers/android/binder.c | 23 ++++++++++++++++++++--- drivers/android/binder_internal.h | 3 ++- include/uapi/linux/android/binder.h | 7 ++++++- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 880224ec6abb..a097b89f6a7a 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2728,7 +2728,10 @@ binder_find_outdated_transaction_ilocked(struct binder_transaction *t, * * Return: 0 if the transaction was successfully queued * BR_DEAD_REPLY if the target process or thread is dead - * BR_FROZEN_REPLY if the target process or thread is frozen + * BR_FROZEN_REPLY if the target process or thread is frozen and + * the sync transaction was rejected + * BR_TRANSACTION_PENDING if the target process is frozen and the + * async transaction was successfully queued */ static int binder_proc_transaction(struct binder_transaction *t, struct binder_proc *proc, @@ -2807,6 +2810,9 @@ static int binder_proc_transaction(struct binder_transaction *t, binder_stats_deleted(BINDER_STAT_TRANSACTION); } + if (oneway && proc->is_frozen) + return BR_TRANSACTION_PENDING; + return 0; } @@ -3607,9 +3613,16 @@ static void binder_transaction(struct binder_proc *proc, } else { BUG_ON(target_node == NULL); BUG_ON(t->buffer->async_transaction != 1); - binder_enqueue_thread_work(thread, tcomplete); return_error = binder_proc_transaction(t, target_proc, NULL); - if (return_error) + /* + * Let the caller know its async transaction reaches a frozen + * process and is put in a pending queue, waiting for the target + * process to be unfrozen. + */ + if (return_error == BR_TRANSACTION_PENDING) + tcomplete->type = BINDER_WORK_TRANSACTION_PENDING; + binder_enqueue_thread_work(thread, tcomplete); + if (return_error && return_error != BR_TRANSACTION_PENDING) goto err_dead_proc_or_thread; } if (target_thread) @@ -4440,10 +4453,13 @@ static int binder_thread_read(struct binder_proc *proc, binder_stat_br(proc, thread, cmd); } break; case BINDER_WORK_TRANSACTION_COMPLETE: + case BINDER_WORK_TRANSACTION_PENDING: case BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT: { if (proc->oneway_spam_detection_enabled && w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT) cmd = BR_ONEWAY_SPAM_SUSPECT; + else if (w->type == BINDER_WORK_TRANSACTION_PENDING) + cmd = BR_TRANSACTION_PENDING; else cmd = BR_TRANSACTION_COMPLETE; binder_inner_proc_unlock(proc); @@ -6170,6 +6186,7 @@ static const char * const binder_return_strings[] = { "BR_FAILED_REPLY", "BR_FROZEN_REPLY", "BR_ONEWAY_SPAM_SUSPECT", + "BR_TRANSACTION_PENDING" }; static const char * const binder_command_strings[] = { diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h index abe19d88c6ec..6c51325a826f 100644 --- a/drivers/android/binder_internal.h +++ b/drivers/android/binder_internal.h @@ -133,7 +133,7 @@ enum binder_stat_types { }; struct binder_stats { - atomic_t br[_IOC_NR(BR_ONEWAY_SPAM_SUSPECT) + 1]; + atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING) + 1]; atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1]; atomic_t obj_created[BINDER_STAT_COUNT]; atomic_t obj_deleted[BINDER_STAT_COUNT]; @@ -152,6 +152,7 @@ struct binder_work { enum binder_work_type { BINDER_WORK_TRANSACTION = 1, BINDER_WORK_TRANSACTION_COMPLETE, + BINDER_WORK_TRANSACTION_PENDING, BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT, BINDER_WORK_RETURN_ERROR, BINDER_WORK_NODE, diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h index e72e4de8f452..c21b3b3eb4e4 100644 --- a/include/uapi/linux/android/binder.h +++ b/include/uapi/linux/android/binder.h @@ -450,7 +450,7 @@ enum binder_driver_return_protocol { BR_FROZEN_REPLY = _IO('r', 18), /* - * The target of the last transaction (either a bcTRANSACTION or + * The target of the last sync transaction (either a bcTRANSACTION or * a bcATTEMPT_ACQUIRE) is frozen. No parameters. */ @@ -460,6 +460,11 @@ enum binder_driver_return_protocol { * asynchronous transaction makes the allocated async buffer size exceed * detection threshold. No parameters. */ + + BR_TRANSACTION_PENDING = _IO('r', 20), + /* + * The target of the last async transaction is frozen. No parameters. + */ }; enum binder_driver_command_protocol { -- 2.38.1.431.g37b22c650d-goog ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] binder: return pending info for frozen async txns 2022-11-03 19:05 ` [PATCH v1 1/1] " Li Li @ 2022-11-09 22:43 ` Carlos Llamas 2022-11-10 19:53 ` Li Li 0 siblings, 1 reply; 4+ messages in thread From: Carlos Llamas @ 2022-11-09 22:43 UTC (permalink / raw) To: Li Li Cc: dualli, gregkh, arve, tkjos, maco, joel, brauner, surenb, arnd, masahiroy, devel, linux-kernel, hridya, smoreland, kernel-team On Thu, Nov 03, 2022 at 12:05:49PM -0700, Li Li wrote: > From: Li Li <dualli@google.com> > > An async transaction to a frozen process will still be successsfully nit: sucessfully typo > put in the queue. But this pending async transaction won't be processed > until the target process is unfrozen at an unspecified time in the > future. Pass this important information back to the user space caller > by returning BR_TRANSACTION_PENDING. > > Signed-off-by: Li Li <dualli@google.com> > --- > drivers/android/binder.c | 23 ++++++++++++++++++++--- > drivers/android/binder_internal.h | 3 ++- > include/uapi/linux/android/binder.h | 7 ++++++- > 3 files changed, 28 insertions(+), 5 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 880224ec6abb..a097b89f6a7a 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2728,7 +2728,10 @@ binder_find_outdated_transaction_ilocked(struct binder_transaction *t, > * > * Return: 0 if the transaction was successfully queued > * BR_DEAD_REPLY if the target process or thread is dead > - * BR_FROZEN_REPLY if the target process or thread is frozen > + * BR_FROZEN_REPLY if the target process or thread is frozen and > + * the sync transaction was rejected > + * BR_TRANSACTION_PENDING if the target process is frozen and the > + * async transaction was successfully queued > */ > static int binder_proc_transaction(struct binder_transaction *t, > struct binder_proc *proc, > @@ -2807,6 +2810,9 @@ static int binder_proc_transaction(struct binder_transaction *t, > binder_stats_deleted(BINDER_STAT_TRANSACTION); > } > > + if (oneway && proc->is_frozen) Do you need the inner lock here for proc->is_frozen? > + return BR_TRANSACTION_PENDING; > + > return 0; > } > > @@ -3607,9 +3613,16 @@ static void binder_transaction(struct binder_proc *proc, > } else { > BUG_ON(target_node == NULL); > BUG_ON(t->buffer->async_transaction != 1); > - binder_enqueue_thread_work(thread, tcomplete); > return_error = binder_proc_transaction(t, target_proc, NULL); > - if (return_error) > + /* > + * Let the caller know its async transaction reaches a frozen nit: I believe you meant s/its/when or similar? > + * process and is put in a pending queue, waiting for the target > + * process to be unfrozen. > + */ > + if (return_error == BR_TRANSACTION_PENDING) > + tcomplete->type = BINDER_WORK_TRANSACTION_PENDING; > + binder_enqueue_thread_work(thread, tcomplete); I wonder if switching the order of queuing the tcomplete here and waking up the target thread inside binder_proc_transaction() will have any performance implications if this task gets scheduled out. > + if (return_error && return_error != BR_TRANSACTION_PENDING) > goto err_dead_proc_or_thread; > } > if (target_thread) > @@ -4440,10 +4453,13 @@ static int binder_thread_read(struct binder_proc *proc, > binder_stat_br(proc, thread, cmd); > } break; > case BINDER_WORK_TRANSACTION_COMPLETE: > + case BINDER_WORK_TRANSACTION_PENDING: > case BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT: { > if (proc->oneway_spam_detection_enabled && > w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT) > cmd = BR_ONEWAY_SPAM_SUSPECT; > + else if (w->type == BINDER_WORK_TRANSACTION_PENDING) > + cmd = BR_TRANSACTION_PENDING; > else > cmd = BR_TRANSACTION_COMPLETE; > binder_inner_proc_unlock(proc); > @@ -6170,6 +6186,7 @@ static const char * const binder_return_strings[] = { > "BR_FAILED_REPLY", > "BR_FROZEN_REPLY", > "BR_ONEWAY_SPAM_SUSPECT", > + "BR_TRANSACTION_PENDING" > }; > > static const char * const binder_command_strings[] = { > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h > index abe19d88c6ec..6c51325a826f 100644 > --- a/drivers/android/binder_internal.h > +++ b/drivers/android/binder_internal.h > @@ -133,7 +133,7 @@ enum binder_stat_types { > }; > > struct binder_stats { > - atomic_t br[_IOC_NR(BR_ONEWAY_SPAM_SUSPECT) + 1]; > + atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING) + 1]; > atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1]; > atomic_t obj_created[BINDER_STAT_COUNT]; > atomic_t obj_deleted[BINDER_STAT_COUNT]; > @@ -152,6 +152,7 @@ struct binder_work { > enum binder_work_type { > BINDER_WORK_TRANSACTION = 1, > BINDER_WORK_TRANSACTION_COMPLETE, > + BINDER_WORK_TRANSACTION_PENDING, > BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT, > BINDER_WORK_RETURN_ERROR, > BINDER_WORK_NODE, > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h > index e72e4de8f452..c21b3b3eb4e4 100644 > --- a/include/uapi/linux/android/binder.h > +++ b/include/uapi/linux/android/binder.h > @@ -450,7 +450,7 @@ enum binder_driver_return_protocol { > > BR_FROZEN_REPLY = _IO('r', 18), > /* > - * The target of the last transaction (either a bcTRANSACTION or > + * The target of the last sync transaction (either a bcTRANSACTION or > * a bcATTEMPT_ACQUIRE) is frozen. No parameters. > */ > > @@ -460,6 +460,11 @@ enum binder_driver_return_protocol { > * asynchronous transaction makes the allocated async buffer size exceed > * detection threshold. No parameters. > */ > + > + BR_TRANSACTION_PENDING = _IO('r', 20), > + /* > + * The target of the last async transaction is frozen. No parameters. > + */ > }; > > enum binder_driver_command_protocol { > -- > 2.38.1.431.g37b22c650d-goog > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/1] binder: return pending info for frozen async txns 2022-11-09 22:43 ` Carlos Llamas @ 2022-11-10 19:53 ` Li Li 0 siblings, 0 replies; 4+ messages in thread From: Li Li @ 2022-11-10 19:53 UTC (permalink / raw) To: Carlos Llamas Cc: dualli, gregkh, arve, tkjos, maco, joel, brauner, surenb, arnd, masahiroy, devel, linux-kernel, hridya, smoreland, kernel-team On Wed, Nov 9, 2022 at 2:43 PM Carlos Llamas <cmllamas@google.com> wrote: > > On Thu, Nov 03, 2022 at 12:05:49PM -0700, Li Li wrote: > > From: Li Li <dualli@google.com> > > > > An async transaction to a frozen process will still be successsfully > > nit: sucessfully typo Nice catch! Will remove the extra 's' in v2. > > > put in the queue. But this pending async transaction won't be processed > > until the target process is unfrozen at an unspecified time in the > > future. Pass this important information back to the user space caller > > by returning BR_TRANSACTION_PENDING. > > > > Signed-off-by: Li Li <dualli@google.com> > > --- > > drivers/android/binder.c | 23 ++++++++++++++++++++--- > > drivers/android/binder_internal.h | 3 ++- > > include/uapi/linux/android/binder.h | 7 ++++++- > > 3 files changed, 28 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > index 880224ec6abb..a097b89f6a7a 100644 > > --- a/drivers/android/binder.c > > +++ b/drivers/android/binder.c > > @@ -2728,7 +2728,10 @@ binder_find_outdated_transaction_ilocked(struct binder_transaction *t, > > * > > * Return: 0 if the transaction was successfully queued > > * BR_DEAD_REPLY if the target process or thread is dead > > - * BR_FROZEN_REPLY if the target process or thread is frozen > > + * BR_FROZEN_REPLY if the target process or thread is frozen and > > + * the sync transaction was rejected > > + * BR_TRANSACTION_PENDING if the target process is frozen and the > > + * async transaction was successfully queued > > */ > > static int binder_proc_transaction(struct binder_transaction *t, > > struct binder_proc *proc, > > @@ -2807,6 +2810,9 @@ static int binder_proc_transaction(struct binder_transaction *t, > > binder_stats_deleted(BINDER_STAT_TRANSACTION); > > } > > > > + if (oneway && proc->is_frozen) > > Do you need the inner lock here for proc->is_frozen? You're right. I'll add a new local variable and set it between the existing binder_inner_proc_lock() and binder_inner_proc_unlock(). > > > + return BR_TRANSACTION_PENDING; > > + > > return 0; > > } > > > > @@ -3607,9 +3613,16 @@ static void binder_transaction(struct binder_proc *proc, > > } else { > > BUG_ON(target_node == NULL); > > BUG_ON(t->buffer->async_transaction != 1); > > - binder_enqueue_thread_work(thread, tcomplete); > > return_error = binder_proc_transaction(t, target_proc, NULL); > > - if (return_error) > > + /* > > + * Let the caller know its async transaction reaches a frozen > > nit: I believe you meant s/its/when or similar? Will change it in v2. Thanks! > > > + * process and is put in a pending queue, waiting for the target > > + * process to be unfrozen. > > + */ > > + if (return_error == BR_TRANSACTION_PENDING) > > + tcomplete->type = BINDER_WORK_TRANSACTION_PENDING; > > + binder_enqueue_thread_work(thread, tcomplete); > > I wonder if switching the order of queuing the tcomplete here and waking > up the target thread inside binder_proc_transaction() will have any > performance implications if this task gets scheduled out. Eventually the sender has to wait this whole function finish and then call another ioctl to actually read tcomplete back. If this task gets cheduled out, it won't have a chance to perform that reading operation even without this change. So there's no difference. > > > + if (return_error && return_error != BR_TRANSACTION_PENDING) > > goto err_dead_proc_or_thread; > > } > > if (target_thread) > > @@ -4440,10 +4453,13 @@ static int binder_thread_read(struct binder_proc *proc, > > binder_stat_br(proc, thread, cmd); > > } break; > > case BINDER_WORK_TRANSACTION_COMPLETE: > > + case BINDER_WORK_TRANSACTION_PENDING: > > case BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT: { > > if (proc->oneway_spam_detection_enabled && > > w->type == BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT) > > cmd = BR_ONEWAY_SPAM_SUSPECT; > > + else if (w->type == BINDER_WORK_TRANSACTION_PENDING) > > + cmd = BR_TRANSACTION_PENDING; > > else > > cmd = BR_TRANSACTION_COMPLETE; > > binder_inner_proc_unlock(proc); > > @@ -6170,6 +6186,7 @@ static const char * const binder_return_strings[] = { > > "BR_FAILED_REPLY", > > "BR_FROZEN_REPLY", > > "BR_ONEWAY_SPAM_SUSPECT", > > + "BR_TRANSACTION_PENDING" > > }; > > > > static const char * const binder_command_strings[] = { > > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h > > index abe19d88c6ec..6c51325a826f 100644 > > --- a/drivers/android/binder_internal.h > > +++ b/drivers/android/binder_internal.h > > @@ -133,7 +133,7 @@ enum binder_stat_types { > > }; > > > > struct binder_stats { > > - atomic_t br[_IOC_NR(BR_ONEWAY_SPAM_SUSPECT) + 1]; > > + atomic_t br[_IOC_NR(BR_TRANSACTION_PENDING) + 1]; > > atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1]; > > atomic_t obj_created[BINDER_STAT_COUNT]; > > atomic_t obj_deleted[BINDER_STAT_COUNT]; > > @@ -152,6 +152,7 @@ struct binder_work { > > enum binder_work_type { > > BINDER_WORK_TRANSACTION = 1, > > BINDER_WORK_TRANSACTION_COMPLETE, > > + BINDER_WORK_TRANSACTION_PENDING, > > BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT, > > BINDER_WORK_RETURN_ERROR, > > BINDER_WORK_NODE, > > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h > > index e72e4de8f452..c21b3b3eb4e4 100644 > > --- a/include/uapi/linux/android/binder.h > > +++ b/include/uapi/linux/android/binder.h > > @@ -450,7 +450,7 @@ enum binder_driver_return_protocol { > > > > BR_FROZEN_REPLY = _IO('r', 18), > > /* > > - * The target of the last transaction (either a bcTRANSACTION or > > + * The target of the last sync transaction (either a bcTRANSACTION or > > * a bcATTEMPT_ACQUIRE) is frozen. No parameters. > > */ > > > > @@ -460,6 +460,11 @@ enum binder_driver_return_protocol { > > * asynchronous transaction makes the allocated async buffer size exceed > > * detection threshold. No parameters. > > */ > > + > > + BR_TRANSACTION_PENDING = _IO('r', 20), > > + /* > > + * The target of the last async transaction is frozen. No parameters. > > + */ > > }; > > > > enum binder_driver_command_protocol { > > -- > > 2.38.1.431.g37b22c650d-goog > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-10 19:53 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-03 19:05 [PATCH v1 0/1] binder: return pending info for frozen async txns Li Li 2022-11-03 19:05 ` [PATCH v1 1/1] " Li Li 2022-11-09 22:43 ` Carlos Llamas 2022-11-10 19:53 ` Li Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).