linux-kernel - Re: [PATCH V1 2/6] rpmsg: glink: Deny intent request if reusable intent fits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YWmayBrhjGB+lvbU@builder.lan>
Date:   Fri, 15 Oct 2021 10:14:16 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Deepak Kumar Singh <deesin@...eaurora.org>
Cc:     clew@...eaurora.org, mathieu.poirier@...aro.org,
        linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V1 2/6] rpmsg: glink: Deny intent request if reusable
 intent fits

On Thu 30 Jul 00:18 CDT 2020, Deepak Kumar Singh wrote:

> From: Chris Lew <clew@...eaurora.org>
> 
> In high traffic scenarios a remote may request extra intents to send
> data faster. If the work thread that handles these intent requests is
> starved of cpu time, then these requests can build up. Some remote
> procs may not be able to handle this burst of built up intent requests.
> 
> In order to prevent intent build up, deny intent requests that can be
> fulfilled by default intents that are reusable.
> 
> Signed-off-by: Chris Lew <clew@...eaurora.org>
> Signed-off-by: Deepak Kumar Singh <deesin@...eaurora.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 2668c66..df3c608 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -734,9 +734,11 @@ static void qcom_glink_handle_rx_done(struct qcom_glink *glink,
>  static void qcom_glink_handle_intent_req(struct qcom_glink *glink,
>  					 u32 cid, size_t size)
>  {
> -	struct glink_core_rx_intent *intent;
> +	struct glink_core_rx_intent *intent = NULL;
> +	struct glink_core_rx_intent *tmp;
>  	struct glink_channel *channel;
>  	unsigned long flags;
> +	int iid;
>  
>  	spin_lock_irqsave(&glink->idr_lock, flags);
>  	channel = idr_find(&glink->rcids, cid);
> @@ -747,6 +749,19 @@ static void qcom_glink_handle_intent_req(struct qcom_glink *glink,
>  		return;
>  	}
>  
> +	spin_lock_irqsave(&channel->intent_lock, flags);
> +	idr_for_each_entry(&channel->liids, tmp, iid) {
> +		if (tmp->size >= size && tmp->reuse) {
> +			intent = tmp;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&channel->intent_lock, flags);
> +	if (intent) {
> +		qcom_glink_send_intent_req_ack(glink, channel, !!intent);

This looks reasonable, but you know that !!intent is true in this block,
and looking more at it I think the end result would look better as:

	/* Try to find a reusable intent to fit the request */
	spin_lock()
	intent = findit();
	spin_unlock()

	/* Allocate a new intent if none was found */
	if (!intent) {
		intent = qcom_glink_alloc_intent(glink, channel, size, false);
		if (intent)
			qcom_glink_advertise_intent(glink, channel, intent);
	}

	qcom_glink_send_intent_req_ack(glink, channel, !!intent);

Regards,
Bjorn

> +		return;
> +	}
> +
>  	intent = qcom_glink_alloc_intent(glink, channel, size, false);
>  	if (intent)
>  		qcom_glink_advertise_intent(glink, channel, intent);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>