Re: Depreciated spi_master.transfer and "prepared spi messages" for an optimized pipelined-SPI-DMA-driver

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5439 bytes --]

On Sun, Nov 10, 2013 at 11:54:16AM +0100, Martin Sperl wrote:
> On 09.11.2013, at 19:30, Mark Brown wrote:

> > What I'm missing with this example is the benefit of having an API for
> > pre-cooked messages, how it will deliver a performance improvement?
> > Flashing should be an infrequent operation and I'd expect it to involve
> > little reuse of messages which I'd expect to be the main thing that we
> > could gain from the API change.  I'd also not expect the controller
> > specific work to be especially expensive.

> It will deliver a performance improvement by not having to create identical 
> structures or running sanity thest every time you transmit the prepared message.

But is that a 0.01% improvement or a 50% improvement and is that because
we're pushing a silly implementation into a corner or because there's
actually substantial work that needs doing there?

If the error checking and so on is really that expensive this sounds
like we need to fix that anyway, even if it's just quick things like
making some of the validation debug only or pushing some of the work
out to the message update functions.

The messages and transfers ought to be totally reusable already, the
only bit that should need to be recreated is any data the driver creates
internally, most likely that's only going to be data for DMA.  We do
already have an interface for drivers to do the DMA mapping though it's
not very well loved at the minute...

> And for the DMA chains I have to create DMA chains that are of a length 10-20 
> - depending on the number of transfers.

What's the expense here - the mapping or something else?  This does also
seem like a lot of scatter/gather, why is the data so spread out and is
there room to do better in the client driver?

We ought to modernise this interface to be more in tune with what
dmaengine is doing anyway (probably taking scatterlists for example) -
you were also talking about DMA a lot in some of your earlier messages
and it is where I'd expect the cost to be so I'm wondering if this isn't
actually more to do with this interface being poor and if we don't need
dmaengine changes too to realise the benefits.

> I would say any little less CPU utilization helps if it is easy to implement.

I'm not clear on the easy to implement bit here (though obviously there
haven't been patches posted for review yet), especially the interaction
with DMA if that's a part of this, or what exactly you're expecting to
be allowed to happen to the message while it's been partially initalised
by the driver (I wouldn't use the term prepared for what you're
suggesting by the way, I'd expect preparation to be interacting with the
hardware) since that will affect how widely clients could take advantage
of this.

The other bit of easy to implement that concerns me is making sure that
it's easy to implement for the stack as a whole - one of the big
problems with the existing stack is that there is too little factoring
out of code into the core so we end up with individual drivers
implementing good ideas that are widely useful and most drivers being
basic so drivers are more complex to review and the benefits are not
widely distributed.  This makes me worry about an API that just
delegates everything to the driver and which requires both master and
client drivers to specifically support the feature.

> > You've been doing a bunch of other performance improvements as well,
> > it's not clear to me how much of this is coming from the API change and
> > how much of this is due to changes which can also be done by improving
> > the implementation and without requiring drivers to be specifically
> > updated to take advantage of it. 

> I have told you, that I have created a module parameter which enables/disables 
> the prepare code.

Right, and what I'm saying is that I'm not clear what's being done in
that code and hence why moving it makes a difference.

> >> Does this answer your question and convince you of this being realistic?

> > It's still not clear to me exactly how this works and hence if the
> > benefit comes from the API change itself.

> There is no change in API - there is only an optional extension...

Adding new interfaces is a change too, on the driver side as well.

> Would these be good enough scenarios for you to finally convince you if the
> numbers are favorable?

These seem rather artificial and I'm not sure they'd tell us much about
why the effects were being observed.  What would be more interesting
would be seeing where the time is being spent at the minute for real
loads (for example by using trace points like those that are currently
being added in mainline for examining the behaviour of the message pump)
- if we really are spending a noticeable proportion of the time in setup
then that's obviously a good case for optimising it but right now I
don't feel I understand which bits are slow enough to register in the
context of the overall transaction and hence if the suggested approach
is the best way to optimise them.

An implementation that avoided the need to have the master drivers
updated in order to get benefits would be a lot more interesting since
that would make this much more generally useful, and of course concrete
patches would be good too, but the main thing I'm missing is clarity on
how the gains are being made.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]