All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Siddharth Gupta <sidgup@codeaurora.org>
Cc: ohad@wizery.com, linux-remoteproc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, psodagud@codeaurora.org
Subject: Re: [PATCH] remoteproc: core: Invoke subdev callbacks in list order
Date: Mon, 24 May 2021 22:03:00 -0500	[thread overview]
Message-ID: <YKxo5BXwEPjS5K4d@yoga> (raw)
In-Reply-To: <1621292893-18827-1-git-send-email-sidgup@codeaurora.org>

On Mon 17 May 18:08 CDT 2021, Siddharth Gupta wrote:

> Subdevices at the beginning of the subdev list should have
> higher priority than those at the end of the list. Reverse
> traversal of the list causes priority inversion, which can
> impact the performance of the device.
> 

The subdev lists layers of the communication onion, we bring them up
inside out and we take them down outside in.

This stems from the primary idea that we want to be able to shut things
down cleanly (in the case of a stop) and we pass the "crashed" flag to
indicate to each recipient during "stop" that it may not rely on the
response of a lower layer.

As such, I don't think it's right to say that we have a priority
inversion.

> For example a device adds the glink, sysmon and ssr subdevs
> to its list. During a crash the ssr notification would go
> before the glink and sysmon notifications. This can cause a
> degraded response when a client driver waits for a response
> from the crashed rproc.
> 

In general the design is such that components are not expected to
communicate with the crashed remote when "crashed" is set, this avoids
the single-remote crash.

The case where this isn't holding up is when two remote processors
crashes simultaneously, in which case e.g. sysmon has been seen hitting
its timeout waiting for an ack from a dead remoteproc - but I was under
the impression that this window shrunk dramatically as a side effect of
us fixing the notification ordering.

> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 626a6b90f..ac8fc42 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1167,7 +1167,7 @@ static int rproc_handle_resources(struct rproc *rproc,
>  
>  static int rproc_prepare_subdevices(struct rproc *rproc)
>  {
> -	struct rproc_subdev *subdev;
> +	struct rproc_subdev *subdev, *itr;
>  	int ret;
>  
>  	list_for_each_entry(subdev, &rproc->subdevs, node) {
> @@ -1181,9 +1181,11 @@ static int rproc_prepare_subdevices(struct rproc *rproc)
>  	return 0;
>  
>  unroll_preparation:
> -	list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node) {
> -		if (subdev->unprepare)
> -			subdev->unprepare(subdev);
> +	list_for_each_entry(itr, &rproc->subdevs, node) {
> +		if (itr == subdev)
> +			break;
> +		if (itr->unprepare)
> +			itr->unprepare(subdev);
>  	}
>  
>  	return ret;
> @@ -1191,7 +1193,7 @@ static int rproc_prepare_subdevices(struct rproc *rproc)
>  
>  static int rproc_start_subdevices(struct rproc *rproc)
>  {
> -	struct rproc_subdev *subdev;
> +	struct rproc_subdev *subdev, *itr;
>  	int ret;
>  
>  	list_for_each_entry(subdev, &rproc->subdevs, node) {
> @@ -1205,9 +1207,11 @@ static int rproc_start_subdevices(struct rproc *rproc)
>  	return 0;
>  
>  unroll_registration:
> -	list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node) {
> -		if (subdev->stop)
> -			subdev->stop(subdev, true);
> +	list_for_each_entry(itr, &rproc->subdevs, node) {
> +		if (itr == subdev)
> +			break;
> +		if (itr->stop)
> +			itr->stop(itr, true);
>  	}
>  
>  	return ret;
> @@ -1217,7 +1221,7 @@ static void rproc_stop_subdevices(struct rproc *rproc, bool crashed)
>  {
>  	struct rproc_subdev *subdev;
>  
> -	list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
> +	list_for_each_entry(subdev, &rproc->subdevs, node) {

I presume this is the case you actually care about, can you help me
understand if you changed the others for consistence or if there's some
flow of events where that might be necessary.

Regards,
Bjorn

>  		if (subdev->stop)
>  			subdev->stop(subdev, crashed);
>  	}
> @@ -1227,7 +1231,7 @@ static void rproc_unprepare_subdevices(struct rproc *rproc)
>  {
>  	struct rproc_subdev *subdev;
>  
> -	list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
> +	list_for_each_entry(subdev, &rproc->subdevs, node) {
>  		if (subdev->unprepare)
>  			subdev->unprepare(subdev);
>  	}
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Siddharth Gupta <sidgup@codeaurora.org>
Cc: ohad@wizery.com, linux-remoteproc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, psodagud@codeaurora.org
Subject: Re: [PATCH] remoteproc: core: Invoke subdev callbacks in list order
Date: Mon, 24 May 2021 22:03:00 -0500	[thread overview]
Message-ID: <YKxo5BXwEPjS5K4d@yoga> (raw)
In-Reply-To: <1621292893-18827-1-git-send-email-sidgup@codeaurora.org>

On Mon 17 May 18:08 CDT 2021, Siddharth Gupta wrote:

> Subdevices at the beginning of the subdev list should have
> higher priority than those at the end of the list. Reverse
> traversal of the list causes priority inversion, which can
> impact the performance of the device.
> 

The subdev lists layers of the communication onion, we bring them up
inside out and we take them down outside in.

This stems from the primary idea that we want to be able to shut things
down cleanly (in the case of a stop) and we pass the "crashed" flag to
indicate to each recipient during "stop" that it may not rely on the
response of a lower layer.

As such, I don't think it's right to say that we have a priority
inversion.

> For example a device adds the glink, sysmon and ssr subdevs
> to its list. During a crash the ssr notification would go
> before the glink and sysmon notifications. This can cause a
> degraded response when a client driver waits for a response
> from the crashed rproc.
> 

In general the design is such that components are not expected to
communicate with the crashed remote when "crashed" is set, this avoids
the single-remote crash.

The case where this isn't holding up is when two remote processors
crashes simultaneously, in which case e.g. sysmon has been seen hitting
its timeout waiting for an ack from a dead remoteproc - but I was under
the impression that this window shrunk dramatically as a side effect of
us fixing the notification ordering.

> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 626a6b90f..ac8fc42 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1167,7 +1167,7 @@ static int rproc_handle_resources(struct rproc *rproc,
>  
>  static int rproc_prepare_subdevices(struct rproc *rproc)
>  {
> -	struct rproc_subdev *subdev;
> +	struct rproc_subdev *subdev, *itr;
>  	int ret;
>  
>  	list_for_each_entry(subdev, &rproc->subdevs, node) {
> @@ -1181,9 +1181,11 @@ static int rproc_prepare_subdevices(struct rproc *rproc)
>  	return 0;
>  
>  unroll_preparation:
> -	list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node) {
> -		if (subdev->unprepare)
> -			subdev->unprepare(subdev);
> +	list_for_each_entry(itr, &rproc->subdevs, node) {
> +		if (itr == subdev)
> +			break;
> +		if (itr->unprepare)
> +			itr->unprepare(subdev);
>  	}
>  
>  	return ret;
> @@ -1191,7 +1193,7 @@ static int rproc_prepare_subdevices(struct rproc *rproc)
>  
>  static int rproc_start_subdevices(struct rproc *rproc)
>  {
> -	struct rproc_subdev *subdev;
> +	struct rproc_subdev *subdev, *itr;
>  	int ret;
>  
>  	list_for_each_entry(subdev, &rproc->subdevs, node) {
> @@ -1205,9 +1207,11 @@ static int rproc_start_subdevices(struct rproc *rproc)
>  	return 0;
>  
>  unroll_registration:
> -	list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node) {
> -		if (subdev->stop)
> -			subdev->stop(subdev, true);
> +	list_for_each_entry(itr, &rproc->subdevs, node) {
> +		if (itr == subdev)
> +			break;
> +		if (itr->stop)
> +			itr->stop(itr, true);
>  	}
>  
>  	return ret;
> @@ -1217,7 +1221,7 @@ static void rproc_stop_subdevices(struct rproc *rproc, bool crashed)
>  {
>  	struct rproc_subdev *subdev;
>  
> -	list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
> +	list_for_each_entry(subdev, &rproc->subdevs, node) {

I presume this is the case you actually care about, can you help me
understand if you changed the others for consistence or if there's some
flow of events where that might be necessary.

Regards,
Bjorn

>  		if (subdev->stop)
>  			subdev->stop(subdev, crashed);
>  	}
> @@ -1227,7 +1231,7 @@ static void rproc_unprepare_subdevices(struct rproc *rproc)
>  {
>  	struct rproc_subdev *subdev;
>  
> -	list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
> +	list_for_each_entry(subdev, &rproc->subdevs, node) {
>  		if (subdev->unprepare)
>  			subdev->unprepare(subdev);
>  	}
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-05-25  3:03 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-17 23:08 [PATCH] remoteproc: core: Invoke subdev callbacks in list order Siddharth Gupta
2021-05-25  3:03 ` Bjorn Andersson [this message]
2021-05-25  3:03   ` Bjorn Andersson
2021-05-25 19:48   ` Siddharth Gupta
2021-05-26  0:37     ` Bjorn Andersson
2021-05-26  0:37       ` Bjorn Andersson
2021-05-26  1:16       ` Siddharth Gupta
2021-05-26  3:00         ` Bjorn Andersson
2021-05-26  3:00           ` Bjorn Andersson
2021-05-26  0:00 Saravana Kannan
2021-05-26  0:41 ` Siddharth Gupta
     [not found] <CAGETcx8ykYhBzkqZT+5G9oz2MOiHaSy4F3JoHudgK9WFnmRjbw@mail.gmail.com>
2021-05-26  0:59 ` Bjorn Andersson
2021-05-26  0:59   ` Bjorn Andersson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YKxo5BXwEPjS5K4d@yoga \
    --to=bjorn.andersson@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=ohad@wizery.com \
    --cc=psodagud@codeaurora.org \
    --cc=sidgup@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.