Re: [PATCH V6 00/20] net: ks8851: Unify KS8851 SPI and MLL drivers

On 5/17/20 9:13 AM, Lukas Wunner wrote:

On Sat, May 16, 2020 at 07:02:25PM -0700, David Miller wrote:

quoted

quoted

The KS8851SNL/SNLI and KS8851-16MLL/MLLI/MLLU are very much the same pieces
of silicon, except the former has an SPI interface, while the later has a
parallel bus interface. Thus far, Linux has two separate drivers for each
and they are diverging considerably.

This series unifies them into a single driver with small SPI and parallel
bus specific parts. The approach here is to first separate out the SPI
specific parts into a separate file, then add parallel bus accessors in
another separate file and then finally remove the old parallel bus driver.
The reason for replacing the old parallel bus driver is because the SPI
bus driver is much higher quality.

What strikes me in these changes is all of the new indirect jumps in
the fast paths of TX and RX packet processing.  It's just too much for
my eyes. :-)

Especially in the presence of Spectre mitigations, these costs are
quite non-trivial.

Seriously, I would recommend that instead of having these small
indirect helpers, just inline the differences into two instances of
the RX interrupt and the TX handler.

I agree.

I do not.

However in terms of performance there's a bigger problem:

Previously ks8851.c (SPI driver) had 8-bit and 32-bit register accessors.
The present series drops them and performs a 32-bit access as two 16-bit
accesses and an 8-bit access as one 16-bit access because that's what
ks8851_mll.c (16-bit parallel bus driver) does.  That has a real,
measurable performance impact because in the case of 8-bit accesses,
another 8 bits need to be transferred over the SPI bus, and in the case
of 32-bit accesses, *two* SPI transfers need to be performed.

The 8-bit and 32-bit accesses happen in ks8851_rx_pkts(), i.e. in the
RX hotpath.  I've provided numbers for the performance impact and even
a patch to solve them but it was dismissed and not included in the
present series:

https://lore.kernel.org/netdev/20200420140700.6632hztejwcgjwsf@wunner.de/ (local)

The reason given for the dismissal was that I had performed the measurements
on 4.19 which is allegedly "long dead" (in Andrew Lunn's words).
However I can assure you that performing two SPI transfers has not
magically become as fast as performing one SPI transfer since 4.19.
So the argument is nonsense.

I invested time and even obtained the SPI variant of the card to perform
actual comparative measurements on linux-next both on the SPI and
parallel variant with iperf, both for latency and throughput, and I do
not observe this problem.

A month ago, I even provided you a branch with all the patches and the
DT patch for RPi3 (the platform you claim to use for these tests, so I
used the same) so you can perform the same test as I did, with the same
hardware and the same software. So it should have been trivial to
reproduce the tests I did and their results.

Nevertheless I was going to repeat the performance measurements on a
recent kernel but haven't gotten around to that yet because the
measurements need to be performed with CONFIG_PREEMPT_RT_FULL to
be reliable (a vanilla kernel is too jittery), so I have to create
a new branch with RT patches on the test machine, which is fairly
involved and time consuming.

I think it's fair that the two drivers are unified, but the performance
for the SPI variant shouldn't be unnecessarily diminished in the process.

Could it be that your problem is related to this huge out-of-tree patch
you use then ?

-- 
Best regards,
Marek Vasut