Date:   Thu, 19 May 2022 10:21:25 -0700
From:   Li Li <dualli@...omium.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     dualli@...gle.com, tkjos@...gle.com, christian@...uner.io,
        arve@...roid.com, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, maco@...gle.com, hridya@...gle.com,
        surenb@...gle.com, joel@...lfernandes.org, kernel-team@...roid.com
Subject: Re: [PATCH v1] Binder: add TF_UPDATE_TXN

On Thu, May 19, 2022 at 8:50 AM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Wed, May 18, 2022 at 05:06:23PM -0700, Li Li wrote:
> > From: Li Li <dualli@...gle.com>
>
> 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?

Sure, I'll add a brief summary there.

>
> >
> > 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@...gle.com>
> > ---
> >  drivers/android/binder.c            | 90 ++++++++++++++++++++++++++++-
> >  drivers/android/binder_trace.h      |  4 ++
> >  include/uapi/linux/android/binder.h |  1 +
>
> How was this tested?

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

>
> >  3 files changed, 92 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index f3b639e89dd8..153486a32d69 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -2594,6 +2594,60 @@ 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 == NULL || t2->to_proc == NULL)
> > +             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)
>
> 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,
> sorry.

It passed checkpatch but --strict does suggest I adjust the logical ops.
I'll update it in v2. Thanks for reminding me about using --strict.

Thanks,
Li

Powered by blists - more mailing lists