linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn
@ 2022-05-26 22:00 Li Li
  2022-05-26 22:00 ` [RESEND PATCH v3 1/1] " Li Li
       [not found] ` <YpBmmVa+09ZhP9LH@kroah.com>
  0 siblings, 2 replies; 5+ messages in thread
From: Li Li @ 2022-05-26 22:00 UTC (permalink / raw)
  To: dualli, tkjos, gregkh, christian, arve, devel, linux-kernel,
	maco, hridya, surenb
  Cc: joel, kernel-team

From: Li Li <dualli@google.com>

Resend [Patch v3] with cover letter in case my previous email failed
to reach the maillist (no comments for 2 weeks).

The previous comments of the old patch can be found at the following link:
https://lore.kernel.org/lkml/CANBPYPjkNWsO94nuG1TkR1DgK2W2kBxiJTriyVB7S3czHTZ1Yg@mail.gmail.com/

I copy and paste the key information here for your convenience.

* Question #1

Note, your subject does not say what TF_UPDATE_TXN is, so it's a bit
hard to determine what is happening here.  Can you clean that up a bit
and sumarize what this new addition does?
How was this tested?

* Answer #1 ===

A more descriptive summary has been added to the new version of patch.

*  Question #2

How was this tested?

* Answer #2

Old kernel: without this TF_UPDATE_TXN patch
New kernel: with this TF_UPDATE_TXN patch
Old apps: without setting TF_UPDATE_TXN
New apps: if (flags & TF_ONE_WAY) flags |= TF_UPDATE_TXN;

1. Compatibility: New kernel + Old apps, to verify the original
behavior doesn't change;

2. Compatibility: Old kernel + New apps, to verify the original
behavior doesn't change;

3. Unit test: New kernel + New apps, to verify the outdated oneway
binder transaction is actually superseded by the latest one (by
enabling BINDER_DEBUG logs);

4. Stress test: New kernel + New apps sending oneway binder
transactions repeatedly, to verify the size of the available async
binder buffer over time, and if the transactions fail as before
(due to async buffer running out).

* Question #3

Did checkpatch pass this?  Please always use --strict and fix up all the
issues that it reports as this is not a normal kernel coding style.

* Answer #3

Yes, the latest version has passed "./scripts/checkpatch.pl --strict"

* Changelog

v3:
  - Add this changelog required by "The canonical patch format"
v2:
  - Fix alignment warnings reported by checkpatch --strict
  - Add descriptive summary in patch subject

Li Li (1):
  Binder: add TF_UPDATE_TXN to replace outdated txn

 drivers/android/binder.c            | 85 ++++++++++++++++++++++++++++-
 drivers/android/binder_trace.h      |  4 ++
 include/uapi/linux/android/binder.h |  1 +
 3 files changed, 87 insertions(+), 3 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RESEND PATCH v3 1/1] Binder: add TF_UPDATE_TXN to replace outdated txn
  2022-05-26 22:00 [RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn Li Li
@ 2022-05-26 22:00 ` Li Li
  2022-06-21 17:11   ` Todd Kjos
       [not found] ` <YpBmmVa+09ZhP9LH@kroah.com>
  1 sibling, 1 reply; 5+ messages in thread
From: Li Li @ 2022-05-26 22:00 UTC (permalink / raw)
  To: dualli, tkjos, gregkh, christian, arve, devel, linux-kernel,
	maco, hridya, surenb
  Cc: joel, kernel-team

From: Li Li <dualli@google.com>

When the target process is busy, incoming oneway transactions are
queued in the async_todo list. If the clients continue sending extra
oneway transactions while the target process is frozen, this queue can
become too large to accommodate new transactions. That's why binder
driver introduced ONEWAY_SPAM_DETECTION to detect this situation. It's
helpful to debug the async binder buffer exhausting issue, but the
issue itself isn't solved directly.

In real cases applications are designed to send oneway transactions
repeatedly, delivering updated inforamtion to the target process.
Typical examples are Wi-Fi signal strength and some real time sensor
data. Even if the apps might only care about the lastet information,
all outdated oneway transactions are still accumulated there until the
frozen process is thawed later. For this kind of situations, there's
no existing method to skip those outdated transactions and deliver the
latest one only.

This patch introduces a new transaction flag TF_UPDATE_TXN. To use it,
use apps can set this new flag along with TF_ONE_WAY. When such an
oneway transaction is to be queued into the async_todo list of a frozen
process, binder driver will check if any previous pending transactions
can be superseded by comparing their code, flags and target node. If
such an outdated pending transaction is found, the latest transaction
will supersede that outdated one. This effectively prevents the async
binder buffer running out and saves unnecessary binder read workloads.

Signed-off-by: Li Li <dualli@google.com>
---
v3:
  - Add this changelog required by "The canonical patch format"
v2:
  - Fix alignment warnings reported by checkpatch --strict
  - Add descriptive summary in patch subject

 drivers/android/binder.c            | 85 ++++++++++++++++++++++++++++-
 drivers/android/binder_trace.h      |  4 ++
 include/uapi/linux/android/binder.h |  1 +
 3 files changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f3b639e89dd8..bb968cf2f9ec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2594,6 +2594,56 @@ static int binder_fixup_parent(struct list_head *pf_head,
 	return binder_add_fixup(pf_head, buffer_offset, bp->buffer, 0);
 }
 
+/**
+ * binder_can_update_transaction() - Can a txn be superseded by an updated one?
+ * @t1: the pending async txn in the frozen process
+ * @t2: the new async txn to supersede the outdated pending one
+ *
+ * Return:  true if t2 can supersede t1
+ *          false if t2 can not supersede t1
+ */
+static bool binder_can_update_transaction(struct binder_transaction *t1,
+					  struct binder_transaction *t2)
+{
+	if ((t1->flags & t2->flags & (TF_ONE_WAY | TF_UPDATE_TXN)) !=
+	    (TF_ONE_WAY | TF_UPDATE_TXN) || !t1->to_proc || !t2->to_proc)
+		return false;
+	if (t1->to_proc->tsk == t2->to_proc->tsk && t1->code == t2->code &&
+	    t1->flags == t2->flags && t1->buffer->pid == t2->buffer->pid &&
+	    t1->buffer->target_node->ptr == t2->buffer->target_node->ptr &&
+	    t1->buffer->target_node->cookie == t2->buffer->target_node->cookie)
+		return true;
+	return false;
+}
+
+/**
+ * binder_find_outdated_transaction_ilocked() - Find the outdated transaction
+ * @t:		 new async transaction
+ * @target_list: list to find outdated transaction
+ *
+ * Return: the outdated transaction if found
+ *         NULL if no outdated transacton can be found
+ *
+ * Requires the proc->inner_lock to be held.
+ */
+static struct binder_transaction *
+binder_find_outdated_transaction_ilocked(struct binder_transaction *t,
+					 struct list_head *target_list)
+{
+	struct binder_work *w;
+
+	list_for_each_entry(w, target_list, entry) {
+		struct binder_transaction *t_queued;
+
+		if (w->type != BINDER_WORK_TRANSACTION)
+			continue;
+		t_queued = container_of(w, struct binder_transaction, work);
+		if (binder_can_update_transaction(t_queued, t))
+			return t_queued;
+	}
+	return NULL;
+}
+
 /**
  * binder_proc_transaction() - sends a transaction to a process and wakes it up
  * @t:		transaction to send
@@ -2619,6 +2669,7 @@ static int binder_proc_transaction(struct binder_transaction *t,
 	struct binder_node *node = t->buffer->target_node;
 	bool oneway = !!(t->flags & TF_ONE_WAY);
 	bool pending_async = false;
+	struct binder_transaction *t_outdated = NULL;
 
 	BUG_ON(!node);
 	binder_node_lock(node);
@@ -2646,12 +2697,24 @@ static int binder_proc_transaction(struct binder_transaction *t,
 	if (!thread && !pending_async)
 		thread = binder_select_thread_ilocked(proc);
 
-	if (thread)
+	if (thread) {
 		binder_enqueue_thread_work_ilocked(thread, &t->work);
-	else if (!pending_async)
+	} else if (!pending_async) {
 		binder_enqueue_work_ilocked(&t->work, &proc->todo);
-	else
+	} else {
+		if ((t->flags & TF_UPDATE_TXN) && proc->is_frozen) {
+			t_outdated = binder_find_outdated_transaction_ilocked(t,
+									      &node->async_todo);
+			if (t_outdated) {
+				binder_debug(BINDER_DEBUG_TRANSACTION,
+					     "txn %d supersedes %d\n",
+					     t->debug_id, t_outdated->debug_id);
+				list_del_init(&t_outdated->work.entry);
+				proc->outstanding_txns--;
+			}
+		}
 		binder_enqueue_work_ilocked(&t->work, &node->async_todo);
+	}
 
 	if (!pending_async)
 		binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
@@ -2660,6 +2723,22 @@ static int binder_proc_transaction(struct binder_transaction *t,
 	binder_inner_proc_unlock(proc);
 	binder_node_unlock(node);
 
+	/*
+	 * To reduce potential contention, free the outdated transaction and
+	 * buffer after releasing the locks.
+	 */
+	if (t_outdated) {
+		struct binder_buffer *buffer = t_outdated->buffer;
+
+		t_outdated->buffer = NULL;
+		buffer->transaction = NULL;
+		trace_binder_transaction_update_buffer_release(buffer);
+		binder_transaction_buffer_release(proc, NULL, buffer, 0, 0);
+		binder_alloc_free_buf(&proc->alloc, buffer);
+		kfree(t_outdated);
+		binder_stats_deleted(BINDER_STAT_TRANSACTION);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
index 8eeccdc64724..8cc07e6a4273 100644
--- a/drivers/android/binder_trace.h
+++ b/drivers/android/binder_trace.h
@@ -311,6 +311,10 @@ DEFINE_EVENT(binder_buffer_class, binder_transaction_failed_buffer_release,
 	TP_PROTO(struct binder_buffer *buffer),
 	TP_ARGS(buffer));
 
+DEFINE_EVENT(binder_buffer_class, binder_transaction_update_buffer_release,
+	     TP_PROTO(struct binder_buffer *buffer),
+	     TP_ARGS(buffer));
+
 TRACE_EVENT(binder_update_page_range,
 	TP_PROTO(struct binder_alloc *alloc, bool allocate,
 		 void __user *start, void __user *end),
diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 11157fae8a8e..0b291b1d29d1 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -271,6 +271,7 @@ enum transaction_flags {
 	TF_STATUS_CODE	= 0x08,	/* contents are a 32-bit status code */
 	TF_ACCEPT_FDS	= 0x10,	/* allow replies with file descriptors */
 	TF_CLEAR_BUF	= 0x20,	/* clear buffer on txn complete */
+	TF_UPDATE_TXN	= 0x40,	/* update the outdated pending async txn */
 };
 
 struct binder_transaction_data {
-- 
2.36.1.124.g0e6072fb45-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn
       [not found] ` <YpBmmVa+09ZhP9LH@kroah.com>
@ 2022-06-15 18:05   ` Li Li
  2022-06-16  5:58     ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Li Li @ 2022-06-15 18:05 UTC (permalink / raw)
  To: Greg KH, Li Li, Todd Kjos, Christian Brauner,
	Arve Hjønnevåg, open list:ANDROID DRIVERS, LKML,
	Martijn Coenen, Hridya Valsaraju, Suren Baghdasaryan,
	Joel Fernandes (Google),
	Android Kernel Team

On Thu, May 26, 2022 at 10:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 26, 2022 at 03:00:17PM -0700, Li Li wrote:
> > From: Li Li <dualli@google.com>
> >
> > Resend [Patch v3] with cover letter in case my previous email failed
> > to reach the maillist (no comments for 2 weeks).
> >
> > The previous comments of the old patch can be found at the following link:
> > https://lore.kernel.org/lkml/CANBPYPjkNWsO94nuG1TkR1DgK2W2kBxiJTriyVB7S3czHTZ1Yg@mail.gmail.com/
> >
> > I copy and paste the key information here for your convenience.
> >
> > * Question #1
> >
> > Note, your subject does not say what TF_UPDATE_TXN is, so it's a bit
> > hard to determine what is happening here.  Can you clean that up a bit
> > and sumarize what this new addition does?
> > How was this tested?
> >
> > * Answer #1 ===
> >
> > A more descriptive summary has been added to the new version of patch.
> >
> > *  Question #2
> >
> > How was this tested?
> >
> > * Answer #2
> >
> > Old kernel: without this TF_UPDATE_TXN patch
> > New kernel: with this TF_UPDATE_TXN patch
> > Old apps: without setting TF_UPDATE_TXN
> > New apps: if (flags & TF_ONE_WAY) flags |= TF_UPDATE_TXN;
> >
> > 1. Compatibility: New kernel + Old apps, to verify the original
> > behavior doesn't change;
> >
> > 2. Compatibility: Old kernel + New apps, to verify the original
> > behavior doesn't change;
> >
> > 3. Unit test: New kernel + New apps, to verify the outdated oneway
> > binder transaction is actually superseded by the latest one (by
> > enabling BINDER_DEBUG logs);
> >
> > 4. Stress test: New kernel + New apps sending oneway binder
> > transactions repeatedly, to verify the size of the available async
> > binder buffer over time, and if the transactions fail as before
> > (due to async buffer running out).
> >
> > * Question #3
> >
> > Did checkpatch pass this?  Please always use --strict and fix up all the
> > issues that it reports as this is not a normal kernel coding style.
> >
> > * Answer #3
> >
> > Yes, the latest version has passed "./scripts/checkpatch.pl --strict"
> >
> > * Changelog
> >
> > v3:
> >   - Add this changelog required by "The canonical patch format"
> > v2:
> >   - Fix alignment warnings reported by checkpatch --strict
> >   - Add descriptive summary in patch subject
> >
> > Li Li (1):
> >   Binder: add TF_UPDATE_TXN to replace outdated txn
> >
> >  drivers/android/binder.c            | 85 ++++++++++++++++++++++++++++-
> >  drivers/android/binder_trace.h      |  4 ++
> >  include/uapi/linux/android/binder.h |  1 +
> >  3 files changed, 87 insertions(+), 3 deletions(-)
> >
> > --
> > 2.36.1.124.g0e6072fb45-goog
> >
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>
>
> Hi,
>
> This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> You have sent him a patch that has triggered this response.
>
> Right now, the development tree you have sent a patch for is "closed"
> due to the timing of the merge window.  Don't worry, the patch(es) you
> have sent are not lost, and will be looked at after the merge window is
> over (after the -rc1 kernel is released by Linus).
>
> So thank you for your patience and your patches will be reviewed at this
> later time, you do not have to do anything further, this is just a short
> note to let you know the patch status and so you don't worry they didn't
> make it through.

Hi Greg and all reviewers,

The rc-1 has been released for some days. Do I need to resend the patch
v3 [1] again to the maillist? Please let me know what I should do next to
have it reviewed. Thanks!

[1]:
[RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn
https://lore.kernel.org/lkml/20220526220018.3334775-1-dualli@chromium.org/

>
> thanks,
>
> greg k-h's patch email bot

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn
  2022-06-15 18:05   ` [RESEND PATCH v3 0/1] " Li Li
@ 2022-06-16  5:58     ` Greg KH
  0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2022-06-16  5:58 UTC (permalink / raw)
  To: Li Li
  Cc: Li Li, Todd Kjos, Christian Brauner, Arve Hjønnevåg,
	open list:ANDROID DRIVERS, LKML, Martijn Coenen,
	Hridya Valsaraju, Suren Baghdasaryan, Joel Fernandes (Google),
	Android Kernel Team

On Wed, Jun 15, 2022 at 11:05:23AM -0700, Li Li wrote:
> On Thu, May 26, 2022 at 10:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, May 26, 2022 at 03:00:17PM -0700, Li Li wrote:
> > > From: Li Li <dualli@google.com>
> > >
> > > Resend [Patch v3] with cover letter in case my previous email failed
> > > to reach the maillist (no comments for 2 weeks).
> > >
> > > The previous comments of the old patch can be found at the following link:
> > > https://lore.kernel.org/lkml/CANBPYPjkNWsO94nuG1TkR1DgK2W2kBxiJTriyVB7S3czHTZ1Yg@mail.gmail.com/
> > >
> > > I copy and paste the key information here for your convenience.
> > >
> > > * Question #1
> > >
> > > Note, your subject does not say what TF_UPDATE_TXN is, so it's a bit
> > > hard to determine what is happening here.  Can you clean that up a bit
> > > and sumarize what this new addition does?
> > > How was this tested?
> > >
> > > * Answer #1 ===
> > >
> > > A more descriptive summary has been added to the new version of patch.
> > >
> > > *  Question #2
> > >
> > > How was this tested?
> > >
> > > * Answer #2
> > >
> > > Old kernel: without this TF_UPDATE_TXN patch
> > > New kernel: with this TF_UPDATE_TXN patch
> > > Old apps: without setting TF_UPDATE_TXN
> > > New apps: if (flags & TF_ONE_WAY) flags |= TF_UPDATE_TXN;
> > >
> > > 1. Compatibility: New kernel + Old apps, to verify the original
> > > behavior doesn't change;
> > >
> > > 2. Compatibility: Old kernel + New apps, to verify the original
> > > behavior doesn't change;
> > >
> > > 3. Unit test: New kernel + New apps, to verify the outdated oneway
> > > binder transaction is actually superseded by the latest one (by
> > > enabling BINDER_DEBUG logs);
> > >
> > > 4. Stress test: New kernel + New apps sending oneway binder
> > > transactions repeatedly, to verify the size of the available async
> > > binder buffer over time, and if the transactions fail as before
> > > (due to async buffer running out).
> > >
> > > * Question #3
> > >
> > > Did checkpatch pass this?  Please always use --strict and fix up all the
> > > issues that it reports as this is not a normal kernel coding style.
> > >
> > > * Answer #3
> > >
> > > Yes, the latest version has passed "./scripts/checkpatch.pl --strict"
> > >
> > > * Changelog
> > >
> > > v3:
> > >   - Add this changelog required by "The canonical patch format"
> > > v2:
> > >   - Fix alignment warnings reported by checkpatch --strict
> > >   - Add descriptive summary in patch subject
> > >
> > > Li Li (1):
> > >   Binder: add TF_UPDATE_TXN to replace outdated txn
> > >
> > >  drivers/android/binder.c            | 85 ++++++++++++++++++++++++++++-
> > >  drivers/android/binder_trace.h      |  4 ++
> > >  include/uapi/linux/android/binder.h |  1 +
> > >  3 files changed, 87 insertions(+), 3 deletions(-)
> > >
> > > --
> > > 2.36.1.124.g0e6072fb45-goog
> > >
> > > _______________________________________________
> > > devel mailing list
> > > devel@linuxdriverproject.org
> > > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> >
> >
> > Hi,
> >
> > This is the friendly semi-automated patch-bot of Greg Kroah-Hartman.
> > You have sent him a patch that has triggered this response.
> >
> > Right now, the development tree you have sent a patch for is "closed"
> > due to the timing of the merge window.  Don't worry, the patch(es) you
> > have sent are not lost, and will be looked at after the merge window is
> > over (after the -rc1 kernel is released by Linus).
> >
> > So thank you for your patience and your patches will be reviewed at this
> > later time, you do not have to do anything further, this is just a short
> > note to let you know the patch status and so you don't worry they didn't
> > make it through.
> 
> Hi Greg and all reviewers,
> 
> The rc-1 has been released for some days. Do I need to resend the patch
> v3 [1] again to the maillist? Please let me know what I should do next to
> have it reviewed. Thanks!

If it still applies, no need to resend.

I'm waiting for the other binder maintainers to review it before doing
anything with it.

thanks

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RESEND PATCH v3 1/1] Binder: add TF_UPDATE_TXN to replace outdated txn
  2022-05-26 22:00 ` [RESEND PATCH v3 1/1] " Li Li
@ 2022-06-21 17:11   ` Todd Kjos
  0 siblings, 0 replies; 5+ messages in thread
From: Todd Kjos @ 2022-06-21 17:11 UTC (permalink / raw)
  To: Li Li
  Cc: dualli, gregkh, christian, arve, devel, linux-kernel, maco,
	hridya, surenb, joel, kernel-team

On Thu, May 26, 2022 at 3:00 PM Li Li <dualli@chromium.org> wrote:
>
> From: Li Li <dualli@google.com>
>
> When the target process is busy, incoming oneway transactions are
> queued in the async_todo list. If the clients continue sending extra
> oneway transactions while the target process is frozen, this queue can
> become too large to accommodate new transactions. That's why binder
> driver introduced ONEWAY_SPAM_DETECTION to detect this situation. It's
> helpful to debug the async binder buffer exhausting issue, but the
> issue itself isn't solved directly.
>
> In real cases applications are designed to send oneway transactions
> repeatedly, delivering updated inforamtion to the target process.
> Typical examples are Wi-Fi signal strength and some real time sensor
> data. Even if the apps might only care about the lastet information,
> all outdated oneway transactions are still accumulated there until the
> frozen process is thawed later. For this kind of situations, there's
> no existing method to skip those outdated transactions and deliver the
> latest one only.
>
> This patch introduces a new transaction flag TF_UPDATE_TXN. To use it,
> use apps can set this new flag along with TF_ONE_WAY. When such an
> oneway transaction is to be queued into the async_todo list of a frozen
> process, binder driver will check if any previous pending transactions
> can be superseded by comparing their code, flags and target node. If
> such an outdated pending transaction is found, the latest transaction
> will supersede that outdated one. This effectively prevents the async
> binder buffer running out and saves unnecessary binder read workloads.
>
> Signed-off-by: Li Li <dualli@google.com>

Acked-by: Todd Kjos <tkjos@google.com>

> ---
> v3:
>   - Add this changelog required by "The canonical patch format"
> v2:
>   - Fix alignment warnings reported by checkpatch --strict
>   - Add descriptive summary in patch subject
>
>  drivers/android/binder.c            | 85 ++++++++++++++++++++++++++++-
>  drivers/android/binder_trace.h      |  4 ++
>  include/uapi/linux/android/binder.h |  1 +
>  3 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f3b639e89dd8..bb968cf2f9ec 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2594,6 +2594,56 @@ static int binder_fixup_parent(struct list_head *pf_head,
>         return binder_add_fixup(pf_head, buffer_offset, bp->buffer, 0);
>  }
>
> +/**
> + * binder_can_update_transaction() - Can a txn be superseded by an updated one?
> + * @t1: the pending async txn in the frozen process
> + * @t2: the new async txn to supersede the outdated pending one
> + *
> + * Return:  true if t2 can supersede t1
> + *          false if t2 can not supersede t1
> + */
> +static bool binder_can_update_transaction(struct binder_transaction *t1,
> +                                         struct binder_transaction *t2)
> +{
> +       if ((t1->flags & t2->flags & (TF_ONE_WAY | TF_UPDATE_TXN)) !=
> +           (TF_ONE_WAY | TF_UPDATE_TXN) || !t1->to_proc || !t2->to_proc)
> +               return false;
> +       if (t1->to_proc->tsk == t2->to_proc->tsk && t1->code == t2->code &&
> +           t1->flags == t2->flags && t1->buffer->pid == t2->buffer->pid &&
> +           t1->buffer->target_node->ptr == t2->buffer->target_node->ptr &&
> +           t1->buffer->target_node->cookie == t2->buffer->target_node->cookie)
> +               return true;
> +       return false;
> +}
> +
> +/**
> + * binder_find_outdated_transaction_ilocked() - Find the outdated transaction
> + * @t:          new async transaction
> + * @target_list: list to find outdated transaction
> + *
> + * Return: the outdated transaction if found
> + *         NULL if no outdated transacton can be found
> + *
> + * Requires the proc->inner_lock to be held.
> + */
> +static struct binder_transaction *
> +binder_find_outdated_transaction_ilocked(struct binder_transaction *t,
> +                                        struct list_head *target_list)
> +{
> +       struct binder_work *w;
> +
> +       list_for_each_entry(w, target_list, entry) {
> +               struct binder_transaction *t_queued;
> +
> +               if (w->type != BINDER_WORK_TRANSACTION)
> +                       continue;
> +               t_queued = container_of(w, struct binder_transaction, work);
> +               if (binder_can_update_transaction(t_queued, t))
> +                       return t_queued;
> +       }
> +       return NULL;
> +}
> +
>  /**
>   * binder_proc_transaction() - sends a transaction to a process and wakes it up
>   * @t:         transaction to send
> @@ -2619,6 +2669,7 @@ static int binder_proc_transaction(struct binder_transaction *t,
>         struct binder_node *node = t->buffer->target_node;
>         bool oneway = !!(t->flags & TF_ONE_WAY);
>         bool pending_async = false;
> +       struct binder_transaction *t_outdated = NULL;
>
>         BUG_ON(!node);
>         binder_node_lock(node);
> @@ -2646,12 +2697,24 @@ static int binder_proc_transaction(struct binder_transaction *t,
>         if (!thread && !pending_async)
>                 thread = binder_select_thread_ilocked(proc);
>
> -       if (thread)
> +       if (thread) {
>                 binder_enqueue_thread_work_ilocked(thread, &t->work);
> -       else if (!pending_async)
> +       } else if (!pending_async) {
>                 binder_enqueue_work_ilocked(&t->work, &proc->todo);
> -       else
> +       } else {
> +               if ((t->flags & TF_UPDATE_TXN) && proc->is_frozen) {
> +                       t_outdated = binder_find_outdated_transaction_ilocked(t,
> +                                                                             &node->async_todo);
> +                       if (t_outdated) {
> +                               binder_debug(BINDER_DEBUG_TRANSACTION,
> +                                            "txn %d supersedes %d\n",
> +                                            t->debug_id, t_outdated->debug_id);
> +                               list_del_init(&t_outdated->work.entry);
> +                               proc->outstanding_txns--;
> +                       }
> +               }
>                 binder_enqueue_work_ilocked(&t->work, &node->async_todo);
> +       }
>
>         if (!pending_async)
>                 binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
> @@ -2660,6 +2723,22 @@ static int binder_proc_transaction(struct binder_transaction *t,
>         binder_inner_proc_unlock(proc);
>         binder_node_unlock(node);
>
> +       /*
> +        * To reduce potential contention, free the outdated transaction and
> +        * buffer after releasing the locks.
> +        */
> +       if (t_outdated) {
> +               struct binder_buffer *buffer = t_outdated->buffer;
> +
> +               t_outdated->buffer = NULL;
> +               buffer->transaction = NULL;
> +               trace_binder_transaction_update_buffer_release(buffer);
> +               binder_transaction_buffer_release(proc, NULL, buffer, 0, 0);
> +               binder_alloc_free_buf(&proc->alloc, buffer);
> +               kfree(t_outdated);
> +               binder_stats_deleted(BINDER_STAT_TRANSACTION);
> +       }
> +
>         return 0;
>  }
>
> diff --git a/drivers/android/binder_trace.h b/drivers/android/binder_trace.h
> index 8eeccdc64724..8cc07e6a4273 100644
> --- a/drivers/android/binder_trace.h
> +++ b/drivers/android/binder_trace.h
> @@ -311,6 +311,10 @@ DEFINE_EVENT(binder_buffer_class, binder_transaction_failed_buffer_release,
>         TP_PROTO(struct binder_buffer *buffer),
>         TP_ARGS(buffer));
>
> +DEFINE_EVENT(binder_buffer_class, binder_transaction_update_buffer_release,
> +            TP_PROTO(struct binder_buffer *buffer),
> +            TP_ARGS(buffer));
> +
>  TRACE_EVENT(binder_update_page_range,
>         TP_PROTO(struct binder_alloc *alloc, bool allocate,
>                  void __user *start, void __user *end),
> diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> index 11157fae8a8e..0b291b1d29d1 100644
> --- a/include/uapi/linux/android/binder.h
> +++ b/include/uapi/linux/android/binder.h
> @@ -271,6 +271,7 @@ enum transaction_flags {
>         TF_STATUS_CODE  = 0x08, /* contents are a 32-bit status code */
>         TF_ACCEPT_FDS   = 0x10, /* allow replies with file descriptors */
>         TF_CLEAR_BUF    = 0x20, /* clear buffer on txn complete */
> +       TF_UPDATE_TXN   = 0x40, /* update the outdated pending async txn */
>  };
>
>  struct binder_transaction_data {
> --
> 2.36.1.124.g0e6072fb45-goog
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-06-21 17:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-26 22:00 [RESEND PATCH v3 0/1] Binder: add TF_UPDATE_TXN to replace outdated txn Li Li
2022-05-26 22:00 ` [RESEND PATCH v3 1/1] " Li Li
2022-06-21 17:11   ` Todd Kjos
     [not found] ` <YpBmmVa+09ZhP9LH@kroah.com>
2022-06-15 18:05   ` [RESEND PATCH v3 0/1] " Li Li
2022-06-16  5:58     ` Greg KH

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).