| Message ID | 20190224162311.23899-3-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] spi: bcm2835: add driver stats to debugfs | expand |
Hi Martin, > kernel@martin.sperl.org hat am 24. Februar 2019 um 17:23 geschrieben: > > > From: Martin Sperl <kernel@martin.sperl.org> > > Allow setting the length of the transfer at which dma is used by > setting a module parameter. please provide the motivation of this change. Regards Stefan
On Sun, Feb 24, 2019 at 04:23:11PM +0000, kernel@martin.sperl.org wrote: > +/* define dma min number of bytes to use in dma mode with value validation */ > +static int dma_min_bytes_limit_set(const char *val, > + const struct kernel_param *kp) > +{ > + unsigned int v; > + > + if (kstrtouint(val, 10, &v)) > + return -EINVAL; > + /* value needs to be a multiple of 4 */ > + if (v % 4) { > + pr_err("dma_min_bytes_limit needs to be a multiple of 4\n"); > + return -EINVAL; > + } Transfers don't need to be a multiple of 4 to be eligible for DMA, so this check can be dropped. Thanks, Lukas
> On 24.02.2019, at 20:10, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > Hi Martin, > >> kernel@martin.sperl.org hat am 24. Februar 2019 um 17:23 geschrieben: >> >> >> From: Martin Sperl <kernel@martin.sperl.org> >> >> Allow setting the length of the transfer at which dma is used by >> setting a module parameter. > > please provide the motivation of this change. As we provide control over the selection of polling vs. interrupt mode we should - for consistency - also provide control over selection of DMA mode. DMA mapping is quite expensive and with higher SPI clock speeds it may be more economic CPU wise to run in polling mode instead of DMA mode. Also DMA mode has one specific difference to Polling mode: there is no idle clock cycle between bytes transferred. This may have negative impact when transferring lots of bytes to some mcus without SPI buffers at the fastest possible clock speed, where it helps when there is a gap after each byte. Martin
> On 22.03.2019, at 13:36, Lukas Wunner <lukas@wunner.de> wrote: > > On Sun, Feb 24, 2019 at 04:23:11PM +0000, kernel@martin.sperl.org wrote: >> +/* define dma min number of bytes to use in dma mode with value validation */ >> +static int dma_min_bytes_limit_set(const char *val, >> + const struct kernel_param *kp) >> +{ >> + unsigned int v; >> + >> + if (kstrtouint(val, 10, &v)) >> + return -EINVAL; >> + /* value needs to be a multiple of 4 */ >> + if (v % 4) { >> + pr_err("dma_min_bytes_limit needs to be a multiple of 4\n"); >> + return -EINVAL; >> + } > > Transfers don't need to be a multiple of 4 to be eligible for DMA, > so this check can be dropped. I definitely did not want to write a custom module argument parser but if i remember correctly there is one limitation on the transmission path where you would hit some inefficiencies in the DMA code when you run transfers that are not a multiple of 4 - especially for short transfers. That is why it was implemented. But I can leave this precaution out. Martin
On Sun, Mar 24, 2019 at 09:52:15AM +0100, kernel@martin.sperl.org wrote: > > On 24.02.2019, at 20:10, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > kernel@martin.sperl.org hat am 24. Februar 2019 um 17:23 geschrieben: > > > Allow setting the length of the transfer at which dma is used by > > > setting a module parameter. > > > > please provide the motivation of this change. > > As we provide control over the selection of polling vs. interrupt mode > we should - for consistency - also provide control over selection of > DMA mode. > > DMA mapping is quite expensive and with higher SPI clock speeds it > may be more economic CPU wise to run in polling mode instead of DMA > mode. The problem is that making the DMA minimum length configurable by itself impacts performance because a memory read is necessary to retrieve the limit, instead of a hardcoded immediate in the machine code. Ultimately this feature is only of interest to developers optimizing the code, not really to end users. > Also DMA mode has one specific difference to Polling mode: > there is no idle clock cycle between bytes transferred. Seriously? If that's true it should be documented in the driver. That seems like a major advantage of DMA mode. > This may have negative impact when transferring lots of bytes to > some mcus without SPI buffers at the fastest possible clock speed, > where it helps when there is a gap after each byte. Hm, wouldn't a slower SPI clock speed achieve the same? As a general remark, the interrupt mode is currently suboptimal because when the TX FIFO becomes empty, there's a latency until it is filled again. Instead, we should try to keep it non-empty at all times. This can be achieved with the RXR interrupt: It signals that >= 48 bytes are in the RX FIFO, so in theory if we receive that interrupt, we could write 48 bytes to the TX FIFO. The problem is, this clashes with your algorithm which tries to stuff as many bytes as possible in the TX FIFO. Only if we give that FIFO stuffing algorithm up do we know for sure that 48 bytes are free in the TX FIFO. Also, both poll mode and interrupt mode could be sped up by switching to pseudo-DMA mode, as I've done in 3bd7f6589f67, i.e. switch to DMA mode but access the chip with programmed I/O. That way, the number of MMIO accesses would be reduced by a factor of 4. So if the TX FIFO is empty, perform 16 writes to fill it. Write another 12 dwords once RXR is signaled. Read 16 dwords upon RXF or 12 dwords upon RXR. This would make the time spent in the IRQ handler super short, but at the expense of receiving more interrupts. Poll mode could function the same and precalculate the time it takes for the TX FIFO to empty or the RX FIFO to become filled, and usleep_range() as long to yield the CPU to other tasks. Again, this means more wakeups for the thread. I'm not sure which one is the lesser evil but your FIFO stuffing algorithm forces us to leave optimization potential on the table and that bothers me. Let me know what you think. Thanks, Lukas
On Sun, Mar 24, 2019 at 09:58:27AM +0100, kernel@martin.sperl.org wrote: > On 22.03.2019, at 13:36, Lukas Wunner <lukas@wunner.de> wrote: > > On Sun, Feb 24, 2019 at 04:23:11PM +0000, kernel@martin.sperl.org wrote: > > > +/* define dma min number of bytes to use in dma mode with value validation */ > > > +static int dma_min_bytes_limit_set(const char *val, > > > + const struct kernel_param *kp) > > > +{ > > > + unsigned int v; > > > + > > > + if (kstrtouint(val, 10, &v)) > > > + return -EINVAL; > > > + /* value needs to be a multiple of 4 */ > > > + if (v % 4) { > > > + pr_err("dma_min_bytes_limit needs to be a multiple of 4\n"); > > > + return -EINVAL; > > > + } > > > > Transfers don't need to be a multiple of 4 to be eligible for DMA, > > so this check can be dropped. > > I definitely did not want to write a custom module argument parser > but if i remember correctly there is one limitation on the transmission path > where you would hit some inefficiencies in the DMA code when you run > transfers that are not a multiple of 4 - especially for short transfers. No, the *length* of a transfer in DMA mode doesn't need to be a multiple of 4. You just write the length to the DLEN register and the chip counts that down to zero while clocking out bytes. Once it reaches zero, it stops clocking out bytes. Because the FIFO is accessed with 32-bit width in DMA mode, you'll leave a few extra bytes behind in the TX FIFO if DLEN is not a multiple of 4, so you have to clear the TX FIFO before the next transfer is commenced. But the driver does all that. The inefficiency you're referring to only occurs if you have a transfer that spans multiple non-contiguous pages and in the first page, it starts at an offset that's not a multiple of 4. In that case we transfer the first few bytes of the first page via programmed I/O such that the offset in the first page becomes a multiple of 4 and then we can switch to transferring by DMA. We handle that just fine since 3bd7f6589f67. Note, most clients transfer bytes from a kmalloc'ed allocation and those are always contiguous in memory, so the above is completely irrelevant for them. It's only relevant for vmalloc'ed allocations, which are probably rare in SPI client drivers. Thanks, Lukas
Hi Lukas! > On 24.03.2019, at 11:15, Lukas Wunner <lukas@wunner.de> wrote: > > On Sun, Mar 24, 2019 at 09:52:15AM +0100, kernel@martin.sperl.org wrote: >>> On 24.02.2019, at 20:10, Stefan Wahren <stefan.wahren@i2se.com> wrote: >>>> kernel@martin.sperl.org hat am 24. Februar 2019 um 17:23 geschrieben: >>>> Allow setting the length of the transfer at which dma is used by >>>> setting a module parameter. >>> >>> please provide the motivation of this change. >> >> As we provide control over the selection of polling vs. interrupt mode >> we should - for consistency - also provide control over selection of >> DMA mode. >> >> DMA mapping is quite expensive and with higher SPI clock speeds it >> may be more economic CPU wise to run in polling mode instead of DMA >> mode. > > The problem is that making the DMA minimum length configurable > by itself impacts performance because a memory read is necessary > to retrieve the limit, instead of a hardcoded immediate in the > machine code. Ultimately this feature is only of interest to > developers optimizing the code, not really to end users. The host path is IMO not so hot that one additional memory read is that expensive. For all practical purposes - at least with DMA or IRQ driven mode - your “hot" code path will (typically) no longer be in cache by the time that the code executes again, as other processes/threads have probably been scheduled on the cpu and the cache lines for the code will be gone anyway. > >> Also DMA mode has one specific difference to Polling mode: >> there is no idle clock cycle between bytes transferred. > > Seriously? If that's true it should be documented in the driver. > That seems like a major advantage of DMA mode. It is true - not being documented explicitly is also true (I believe it was mentioned in a cover letter). But it is NOT always an advantage as said. Also I find it surprising that you have not looked at spi traces on a logic-analyzer where this is immediately visible. > >> This may have negative impact when transferring lots of bytes to >> some mcus without SPI buffers at the fastest possible clock speed, >> where it helps when there is a gap after each byte. > > Hm, wouldn't a slower SPI clock speed achieve the same? Yes, it would, but then you would need to make the SPI clock cycle possibly 3-4 times as long to communicate with an atmega MCU in slave mode, which is essentially wasting possible transfer rates. > > As a general remark, the interrupt mode is currently suboptimal > because when the TX FIFO becomes empty, there's a latency until > it is filled again. Instead, we should try to keep it non-empty > at all times. This can be achieved with the RXR interrupt: > It signals that >= 48 bytes are in the RX FIFO, so in theory if > we receive that interrupt, we could write 48 bytes to the TX FIFO. > > The problem is, this clashes with your algorithm which tries to > stuff as many bytes as possible in the TX FIFO. Only if we give > that FIFO stuffing algorithm up do we know for sure that 48 bytes > are free in the TX FIFO. > > Also, both poll mode and interrupt mode could be sped up by > switching to pseudo-DMA mode, as I've done in 3bd7f6589f67, > i.e. switch to DMA mode but access the chip with programmed I/O. > That way, the number of MMIO accesses would be reduced by a > factor of 4. So if the TX FIFO is empty, perform 16 writes > to fill it. Write another 12 dwords once RXR is signaled. > Read 16 dwords upon RXF or 12 dwords upon RXR. How would you speed up poll mode really? It is polling and consuming CPU cycles anyway! > > This would make the time spent in the IRQ handler super short, > but at the expense of receiving more interrupts. In my experience minimizing interrupts should be the main goal because this adds long “stalls” at high SPI clock speeds. I do not have the exact numbers now, but there is a latency that typically produces gaps in the order of 2-10us (depending on clock frequencies) So my original modifications of the driver is focused on avoiding those in the first place. Also on single core machines (like the original RPI or RPIzero) every interrupt means interruption of normal processes for quite some time. So this should be avoided in the first place. And that is where the actual limit of 96 bytes for DMA came from! At 96 (= 2 * 48) bytes we would run 2 interrupts in interrupt mode and also 2 interrupts in DMA mode (one for RX one for TX) > > Poll mode could function the same and precalculate the time it > takes for the TX FIFO to empty or the RX FIFO to become filled, > and usleep_range() as long to yield the CPU to other tasks. > Again, this means more wakeups for the thread. I'm not sure > which one is the lesser evil but your FIFO stuffing algorithm > forces us to leave optimization potential on the table and that > bothers me. Poll mode is wasting time to be as efficient as possible from the thru-put perspective by trying to avoid unnecessary interrupts and thread wakeup latencies, which are not guaranteed to happen as fast as possible. Right now the implementation is wasting a bit more time filling the fifo but if that was faster, then you would still be reading the status register in a tight loop, which does not make a huge difference on the internal BUS - it is a transfer. This is especially efficient when running the spi pump “inline" in spi_sync - no wakeups, no interrupts no context switches… That is the only way to get SPI transfer latencies down to ~6us between 2 transfers (that is the time between stop the clock on the first transfer and start the clock for the second transfer. Otherwise you can get long latencies in the order of 1ms, which is prohibitively long and just wastes resources on scheduling, context switches,… At one point I was thinking if a “shared” polling infrastructure (where multiple busses could poll in a single shared tread) would help. But that resulted in realization that for this to work we would need to schedule at least 2 thread-wakeups: one to the “shared” polling thread (even if filling the FIFO could get doe before scheduling the wakeup) and another one to wake spi_sync again. Assuming that the polling thread is already running, it would mean that the wakeup is not needed really. But this would still leave us with one thread-wakeup. In general principle there are other low hanging fruit that would improve CPU efficiency much more! E.g: unnecessary wakeups of the spi pump thread when running in sync mode where we consume about 30% of a single CPU just for these wakeups when running 22k spi messages/second. In total this resulted in 130% CPU utilization in 2 threads plus sometimes async scheduling... If you want to give optimizing things a try (e.g: pseudo DMA mode), then please go head and post patches and I will try to give it a try when triggering those. Finally: maybe this “use DMA/polling” decission settings should be handled by spi: core instead so that these can get set in the device tree on a per spi device not as a general bus policy. But I guess we have now moved from discussing a specific patch to a discussion of “visions” for individual drivers and the spi-core. Martin
On Sun, Mar 24, 2019 at 12:23:39PM +0100, kernel@martin.sperl.org wrote: > On 24.03.2019, at 11:15, Lukas Wunner <lukas@wunner.de> wrote: > > The problem is that making the DMA minimum length configurable > > by itself impacts performance because a memory read is necessary > > to retrieve the limit, instead of a hardcoded immediate in the > > machine code. Ultimately this feature is only of interest to > > developers optimizing the code, not really to end users. > > The host path is IMO not so hot that one additional memory read is > that expensive. As it stands, spi-bcm2835.c is fairly CPU-intensive. We should try to reduce the load, not add more. > > > This may have negative impact when transferring lots of bytes to > > > some mcus without SPI buffers at the fastest possible clock speed, > > > where it helps when there is a gap after each byte. > > > > Hm, wouldn't a slower SPI clock speed achieve the same? > > Yes, it would, but then you would need to make the SPI clock > cycle possibly 3-4 times as long to communicate with an atmega MCU > in slave mode, which is essentially wasting possible transfer rates. I don't quite follow, what's the downside of a slower transfer rate with 8 cycles/byte if the effective speed is the same as a higher transfer rate with 9 cycles/byte? > > Poll mode could function the same and precalculate the time it > > takes for the TX FIFO to empty or the RX FIFO to become filled, > > and usleep_range() as long to yield the CPU to other tasks. > > How would you speed up poll mode really? It is polling and > consuming CPU cycles anyway! No, usleep_range() either yields the CPU to another task or allows the CPU to be put in a low power state. > In my experience minimizing interrupts should be the main goal > because this adds long ???stalls??? at high SPI clock speeds. > I do not have the exact numbers now, but there is a latency that > typically produces gaps in the order of 2-10us (depending on > clock frequencies) Okay. Then the goal should probably be to reduce the cost of DMA and use DMA for anything that requires more than 1 or 2 interrupts. > If you want to give optimizing things a try (e.g: pseudo DMA > mode), then please go head and post patches and I will try > to give it a try when triggering those. Could you tell me a little more about your use cases so that I can keep them in mind when working on optimizations? What are the SPI slaves you're using, how many bytes are you typically transferring, do you use TX-only / RX-only transfers, have you enabled CONFIG_PREEMPT_RT_FULL? In our case it's mainly the ks8851 Ethernet chip and the hi3110 CAN chip that we're concerned with. The ks8851 usually transfers either very small buffers (around 5 bytes) to access registers, or relatively large buffers (around 1000 bytes) with packet data. The latter is always either RX-only or TX-only. The hi3110 also uses typically small buffers, smaller than the 64 byte FIFO length. The problem is that the number of transfers in particular with the ks8851 is staggering. We're talking gigabytes transferred per day here. So we really feel the pain if cost per transfer goes up. Thanks, Lukas
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index dcf922ca2603..ef4b3468a97d 100644 --- a/drivers/spi/spi-bcm2835.c +++ b/drivers/spi/spi-bcm2835.c @@ -31,6 +31,7 @@ #include <linux/io.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/moduleparam.h> #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> @@ -44,6 +45,39 @@ module_param(polling_limit_us, uint, 0664); MODULE_PARM_DESC(polling_limit_us, "time in us to run a transfer in polling mode\n"); +/* define dma min number of bytes to use in dma mode with value validation */ +static int dma_min_bytes_limit_set(const char *val, + const struct kernel_param *kp) +{ + unsigned int v; + + if (kstrtouint(val, 10, &v)) + return -EINVAL; + /* value needs to be a multiple of 4 */ + if (v % 4) { + pr_err("dma_min_bytes_limit needs to be a multiple of 4\n"); + return -EINVAL; + } + /* value needs to be at least 6 - so actually 8 - rational below */ + if (v < 6) { + pr_err("dma_min_bytes_limit needs to be at least 8\n"); + return -EINVAL; + } + + return param_set_uint(val, kp); +} + +static const struct kernel_param_ops dma_min_bytes_limit_ops = { + .set = dma_min_bytes_limit_set, + .get = param_get_int, +}; + +unsigned int dma_min_bytes_limit = 96; +module_param_cb(dma_min_bytes_limit, &dma_min_bytes_limit_ops, + &dma_min_bytes_limit, 0664); +MODULE_PARM_DESC(dma_min_bytes_limit, + "minimum number of bytes to run a transfer in dma mode\n"); + /* SPI register offsets */ #define BCM2835_SPI_CS 0x00 #define BCM2835_SPI_FIFO 0x04 @@ -80,7 +114,6 @@ MODULE_PARM_DESC(polling_limit_us, #define BCM2835_SPI_FIFO_SIZE 64 #define BCM2835_SPI_FIFO_SIZE_3_4 48 -#define BCM2835_SPI_DMA_MIN_LENGTH 96 #define BCM2835_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_CS_HIGH \ | SPI_NO_CS | SPI_3WIRE) @@ -447,7 +480,8 @@ static int bcm2835_spi_transfer_one_irq(struct spi_master *master, * if the length of the first is *exactly* 1. * * At most 6 bytes are written and at most 3 bytes read. Do we know the - * transfer has this many bytes? Yes, see BCM2835_SPI_DMA_MIN_LENGTH. + * transfer has this many bytes? Yes, see validation in + * dma_min_bytes_limit_set. * * The FIFO is normally accessed with 8-bit width by the CPU and 32-bit width * by the DMA engine. Toggling the DMA Enable flag in the CS register switches @@ -690,7 +724,7 @@ static bool bcm2835_spi_can_dma(struct spi_master *master, struct spi_transfer *tfr) { /* we start DMA efforts only on bigger transfers */ - if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH) + if (tfr->len < dma_min_bytes_limit) return false; /* BCM2835_SPI_DLEN has defined a max transfer size as