Re: High CPU load when using MAX14830 SPI UART controller

From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
To: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org, "Jan Kundrát" <jan.kundrat@cesnet.cz>,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	nicolas.ferre@microchip.com
Subject: Re: High CPU load when using MAX14830 SPI UART controller
Date: Tue, 21 Jul 2020 15:39:44 +0200	[thread overview]
Message-ID: <20200721153944.3c331415@windsurf.home> (raw)
In-Reply-To: <20200520112659.GB4823@sirena.org.uk>

Hello Mark,

Sorry for the very long delay in getting back to you on this. I'm
finally back on this topic.

On Wed, 20 May 2020 12:26:59 +0100
Mark Brown <broonie@kernel.org> wrote:

> Right.  You'd need to rearrange it to go through the entire message
> setting up DMA mappings that will be needed then had everything off to
> interrupt context or have extra complexity and optionally go back up to
> task context if there's anything complicated to handle.

This overall requires some fairly significant surgery in the spi-atmel
driver, which has code paths for PIO, peripheral DMA and dmaengine DMA,
all of which need to be adjusted.

> > I'm not sure what you mean by "it might want to consider error
> > checking". Could you explain?  
> 
> It wasn't checking the return values of SPI API calls.

Right, but this is unlikely to be the source of the CPU consumption
issue we're seeing.

> > Mark: in this situation where a threaded interrupt handler in max310x
> > is doing SPI transfers, are those SPI transfers then offloaded to the
> > SPI workqueue, causing another context switch, or are they done
> > directly within the context of the interrupt thread ? I see
> > __spi_sync() has some logic to push out the messages from the calling
> > context, I guess as opposed to offloading them to the SPI workqueue ?  
> 
> It should be doing transfers in calling context if the controller is
> idle, the SPI thread should only be used when the controller is already
> busy or to clean up when the controller goes idle (which will
> unfortunately happen rather a lot in your use case).
> 
> Actually looking at the code in __spi_pump_messages() again I think that
> in the case where we don't have any cleanup to do we should be able to
> avoid kicking the thread for that which should help a bit for spi-atmel.
> Can you give the patch below a go (compile tested only, not even tried
> to boot)?

I gave it a try, and unfortunately there is no difference. But it is
not too surprising, as top shows something like this:

   80     2 root     SW       0   0%  24% [irq/50-spi1.3]
   57     2 root     IW       0   0%   2% [kworker/0:1-eve]

So it's not the kthread of the SPI subsystem that is consuming most of
the CPU time, but really the threaded handler of the MAX14830 driver.
What your patch does is avoid offloading to the SPI subsystem kthread
some cleanup work when it can be done in the current context, if I
understand correctly.

> You can generally get a good idea of what's going on with regard to
> context switching at the SPI level from the SPI tracepoints, and about
> any latencies in there too.

I'll have a look there, but I don't really have any latency issue,
rather a CPU consumption issue, which is quite different.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com