Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 May 2022 17:50:46 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Li Li <dualli@chromium.org>
Cc:     dualli@google.com, tkjos@google.com, christian@brauner.io,
        arve@android.com, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org, maco@google.com, hridya@google.com,
        surenb@google.com, joel@joelfernandes.org, kernel-team@android.com
Subject: Re: [PATCH v1] Binder: add TF_UPDATE_TXN
Message-ID: <YoZnVrU8Dih+urv6@kroah.com>
References: <20220519000623.1715899-1-dualli@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220519000623.1715899-1-dualli@chromium.org>
Precedence: bulk

On Wed, May 18, 2022 at 05:06:23PM -0700, Li Li wrote:
> From: Li Li <dualli@google.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?

> 
> 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>
> ---
>  drivers/android/binder.c            | 90 ++++++++++++++++++++++++++++-
>  drivers/android/binder_trace.h      |  4 ++
>  include/uapi/linux/android/binder.h |  1 +

How was this tested? 

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

thanks,

greg k-h