[1/4] spi: For controllers that need realtime always use the pump thread
diff mbox series

Message ID 20190510223437.84368-2-dianders@chromium.org
State New, archived
Headers show
Series
  • spi: A better solution for cros_ec_spi reliability
Related show

Commit Message

Doug Anderson May 10, 2019, 10:34 p.m. UTC
If a controller specifies that it needs high priority for sending
messages we should always schedule our transfers on the thread.  If we
don't do this we'll do the transfer in the caller's context which
might not be very high priority.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/spi/spi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Guenter Roeck May 11, 2019, 12:24 a.m. UTC | #1
From: Douglas Anderson <dianders@chromium.org>
Date: Fri, May 10, 2019 at 3:35 PM
To: Mark Brown, Benson Leung, Enric Balletbo i Serra
Cc: <linux-rockchip@lists.infradead.org>, <drinkcat@chromium.org>,
Guenter Roeck, <briannorris@chromium.org>, <mka@chromium.org>, Douglas
Anderson, <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>

> If a controller specifies that it needs high priority for sending
> messages we should always schedule our transfers on the thread.  If we
> don't do this we'll do the transfer in the caller's context which
> might not be very high priority.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
>  drivers/spi/spi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 8eb7460dd744..0597f7086de3 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1230,8 +1230,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>                 return;
>         }
>
> -       /* If another context is idling the device then defer */
> -       if (ctlr->idling) {
> +       /*
> +        * If another context is idling the device then defer.
> +        * If we are high priority then the thread should do the transfer.
> +        */
> +       if (ctlr->idling || (ctlr->rt && !in_kthread)) {
>                 kthread_queue_work(&ctlr->kworker, &ctlr->pump_messages);
>                 spin_unlock_irqrestore(&ctlr->queue_lock, flags);
>                 return;
> --
> 2.21.0.1020.gf2820cf01a-goog
>
Mark Brown May 12, 2019, 7:33 a.m. UTC | #2
On Fri, May 10, 2019 at 03:34:34PM -0700, Douglas Anderson wrote:
> If a controller specifies that it needs high priority for sending
> messages we should always schedule our transfers on the thread.  If we
> don't do this we'll do the transfer in the caller's context which
> might not be very high priority.

If performance is important you probably also want to avoid the context
thrashing - executing in the calling context is generally a substantial
performance boost.  I can see this causing problems further down the
line when someone else turns up with a different requirement, perhaps in
an application where the caller does actually have a raised priority
themselves and just wanted to make sure that the thread wasn't lower
than they are.  I guess it'd be nice if we could check what priority the
calling thread has and make a decision based on that but there don't
seem to be any facilities for doing that which I can see right now.
Doug Anderson May 13, 2019, 8:24 p.m. UTC | #3
Hi,

On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:

> On Fri, May 10, 2019 at 03:34:34PM -0700, Douglas Anderson wrote:
> > If a controller specifies that it needs high priority for sending
> > messages we should always schedule our transfers on the thread.  If we
> > don't do this we'll do the transfer in the caller's context which
> > might not be very high priority.
>
> If performance is important you probably also want to avoid the context
> thrashing - executing in the calling context is generally a substantial
> performance boost.  I can see this causing problems further down the
> line when someone else turns up with a different requirement, perhaps in
> an application where the caller does actually have a raised priority
> themselves and just wanted to make sure that the thread wasn't lower
> than they are.  I guess it'd be nice if we could check what priority the
> calling thread has and make a decision based on that but there don't
> seem to be any facilities for doing that which I can see right now.

In my case performance is 2nd place to a transfer not getting
interrupted once started (so we don't break the 8ms rule of the EC).
My solution in v2 of my series is to take out the forcing in the case
that the controller wanted "rt" priority and then to add "force" to
the parameter name.  If someone wants rt priority for the thread but
doesn't want to force all transfers to the thread we can later add a
different parameter for that?

-Doug
Mark Brown May 14, 2019, 9:30 a.m. UTC | #4
On Mon, May 13, 2019 at 01:24:57PM -0700, Doug Anderson wrote:
> On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:

> > If performance is important you probably also want to avoid the context
> > thrashing - executing in the calling context is generally a substantial
> > performance boost.  I can see this causing problems further down the
> > line when someone else turns up with a different requirement, perhaps in
> > an application where the caller does actually have a raised priority
> > themselves and just wanted to make sure that the thread wasn't lower
> > than they are.  I guess it'd be nice if we could check what priority the
> > calling thread has and make a decision based on that but there don't
> > seem to be any facilities for doing that which I can see right now.

> In my case performance is 2nd place to a transfer not getting
> interrupted once started (so we don't break the 8ms rule of the EC).

That's great but other users do care very much about performance and are
also interested in both priority control and avoiding context thrashing.

> My solution in v2 of my series is to take out the forcing in the case
> that the controller wanted "rt" priority and then to add "force" to
> the parameter name.  If someone wants rt priority for the thread but
> doesn't want to force all transfers to the thread we can later add a
> different parameter for that?

I think that's going to be the common case for this.  Forcing context
thrashing is really not something anyone else is asking for.
Doug Anderson May 14, 2019, 2:42 p.m. UTC | #5
Hi,

On Tue, May 14, 2019 at 2:30 AM Mark Brown <broonie@kernel.org> wrote:

> On Mon, May 13, 2019 at 01:24:57PM -0700, Doug Anderson wrote:
> > On Sun, May 12, 2019 at 10:05 AM Mark Brown <broonie@kernel.org> wrote:
>
> > In my case performance is 2nd place to a transfer not getting
> > interrupted once started (so we don't break the 8ms rule of the EC).
>
> That's great but other users do care very much about performance and are
> also interested in both priority control and avoiding context thrashing.
>
> > My solution in v2 of my series is to take out the forcing in the case
> > that the controller wanted "rt" priority and then to add "force" to
> > the parameter name.  If someone wants rt priority for the thread but
> > doesn't want to force all transfers to the thread we can later add a
> > different parameter for that?
>
> I think that's going to be the common case for this.  Forcing context
> thrashing is really not something anyone else is asking for.

OK, that's fair.  Even if nobody else is asking for it, the solution
I've coded up for v2 still allows cros_ec to use the SPI core's thread
in a pretty clean way and saves a bunch of code in cros_ec.  It
shouldn't penalize any other SPI users.

...but I guess you're saying that you don't want to guarantee that the
SPI core will happen to have this thread sitting around in the future
so you'd rather add the extra complexity to cros_ec so the core can
evolve more freely?

-Doug
Mark Brown May 14, 2019, 3:06 p.m. UTC | #6
On Tue, May 14, 2019 at 07:42:38AM -0700, Doug Anderson wrote:

> ...but I guess you're saying that you don't want to guarantee that the
> SPI core will happen to have this thread sitting around in the future
> so you'd rather add the extra complexity to cros_ec so the core can
> evolve more freely?

We need something to support spi_async() but what you're asking for is
fairly specific implementation details about how things are currently
structured, and we do need to be able to continue to make improvements
for users who are interested in performance.  Ensuring that the calling
context is also less likely to be preempted is going to make it much
less likely that any other work is going to cause some timing change
that creates problems for you.

Patch
diff mbox series

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8eb7460dd744..0597f7086de3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1230,8 +1230,11 @@  static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
 		return;
 	}
 
-	/* If another context is idling the device then defer */
-	if (ctlr->idling) {
+	/*
+	 * If another context is idling the device then defer.
+	 * If we are high priority then the thread should do the transfer.
+	 */
+	if (ctlr->idling || (ctlr->rt && !in_kthread)) {
 		kthread_queue_work(&ctlr->kworker, &ctlr->pump_messages);
 		spin_unlock_irqrestore(&ctlr->queue_lock, flags);
 		return;