p54spi: Remove FIXME in op_stop
A message that is not in the archive (sent off-list or not yet received): 20111116235120.4c60c066@milhouse
Christian Lamparter
Acked-byQuoted 4 lines · Michael Büsch
Don't use the interruptible variant of
mutex_lock(). It doesn't really need to be interruptible. This avoids nasty error handling.Signed-off-by Michael Buesch <[email protected]>
BTW: I always wondered if it would make sense to have a cached rx skb ready in p54spi_rx(). This way we don't have to do DMA onto the stack [which is really ugly and possibly illegal] and might even get a better rx performance. I could write the code but as you know I don't have the hardware to test it.
Regards,
Christian
Quoted 6 lines · On Thu, 17 Nov 2011 00:12:03 +0100 Christian Lamparter…
BTW: I always wondered if it would make sense to have a cached rx skb ready in
p54spi_rx(). This way we don't have to do DMA onto the stack [which is really ugly and possibly illegal] and might even get a better rx performance. I could write the code but as you know I don't have the hardware to test it.
I'll test it, if you can come up with a patch.
Signature · 1 line
Greetings, Michael.
RFC 1/1 p54spi: don't DMA onto the stack
Quoted 10 lines · On Thursday 17 November 2011 00:15:42 Michael Büsch wrote:
Quoted 6 lines · On Thu, 17 Nov 2011 00:12:03 +0100 Christian Lamparter…
BTW: I always wondered if it would make sense to have a cached rx skb ready in
p54spi_rx(). This way we don't have to do DMA onto the stack [which is really ugly and possibly illegal] and might even get a better rx performance. I could write the code but as you know I don't have the hardware to test it.I'll test it, if you can come up with a patch.
---
[RFC] p54spi: don't DMA onto the stack
DMA transfers should not be done onto the kernel stack.
---
drivers/net/wireless/p54/p54spi.c +64 -23
@@ -339,22 +339,55 @@ static void p54spi_int_ready(struct p54s_priv *priv) } } +static int p54spi_alloc_rx_skb (struct p54s_priv *priv) +{ +if (priv->rx_cache !=NULL ) +return 0 ; + +/* +* Add extra space for spi rx header and reserve some space since the +* firmware may insert up to 4 padding bytes after the lmac header, +* but it does not amend the size of SPI data transfer. Such packets +* has correct data size in header, thus referencing past the end of +* allocated skb. Reserve extra 4 bytes for this case. +*/ +#define RX_EXTRA_SPACE (sizeof(__le16) + sizeof(struct p54_rx_data) + 4) + + priv->rx_cache =dev_alloc_skb (priv->common.rx_mtu +RX_EXTRA_SPACE ); +if (!priv->rx_cache) +return -ENOMEM ; + +/* reserve head space for spi transfer length. */ +skb_reserve (priv->rx_cache,2 ); +return 0 ; +} +static int p54spi_rx (struct p54s_priv *priv) {struct sk_buff *skb; +__le16 *rx_head; +int err;u16 len; -u16 rx_head[2 ]; -#define READAHEAD_SZ (sizeof(rx_head)-sizeof(u16)) + +#define READAHEAD (sizeof(__le16))if (p54spi_wakeup (priv) <0 )return -EBUSY ; -/* Read data size and first data word in one SPI transaction + err =p54spi_alloc_rx_skb (priv); +if (err) +return err; + +/* +* Read data size and first data word in one SPI transaction * This is workaround for firmware/DMA bug, * when first data word gets lost under high load. */ -p54spi_spi_read (priv,SPI_ADRS_DMA_DATA , rx_head,sizeof (rx_head)); - len = rx_head[0 ]; + skb = priv->rx_cache; + rx_head = (__le16 *)(unsigned long )(priv->rx_cache->data - +sizeof (__le16)); +p54spi_spi_read (priv,SPI_ADRS_DMA_DATA , rx_head,4 ); + len =le16_to_cpu (rx_head[0 ]);if (len ==0 ) {p54spi_sleep (priv);
@@ -362,36 +395,41 @@ static int p54spi_rx(struct p54s_priv *priv)return 0 ; } -/* Firmware may insert up to 4 padding bytes after the lmac header, -* but it does not amend the size of SPI data transfer. -* Such packets has correct data size in header, thus referencing -* past the end of allocated skb. Reserve extra 4 bytes for this case */ - skb =dev_alloc_skb (len +4 ); -if (!skb) { +if (len >= (RX_EXTRA_SPACE + priv->common.rx_mtu)) {p54spi_sleep (priv); -dev_err (&priv->spi->dev,"could not alloc skb" ); -return -ENOMEM ; +dev_err (&priv->spi->dev,"rx request larger than max rx mtu\n" ); +return 0 ; } -if (len <=READAHEAD_SZ ) { -memcpy (skb_put (skb, len), rx_head +1 , len); +if (len <=READAHEAD ) { +skb_put (skb, len); }else { -memcpy (skb_put (skb,READAHEAD_SZ ), rx_head +1 ,READAHEAD_SZ ); +skb_put (skb,READAHEAD );p54spi_spi_read (priv,SPI_ADRS_DMA_DATA , -skb_put (skb, len -READAHEAD_SZ ), - len -READAHEAD_SZ ); +skb_put (skb, len -READAHEAD ), + len -READAHEAD ); }p54spi_sleep (priv); -/* Put additional bytes to compensate for the possible -* alignment-caused truncation */ +/* +* Put additional bytes to compensate for the possible +* alignment-caused truncation +*/ skb_put (skb,4 ); -if (p54_rx (priv->hw, skb) ==0 ) -dev_kfree_skb (skb); +if (p54_rx (priv->hw, skb) ==0 ) { +/* skb was not used up, can be recycled */ +skb_reset_tail_pointer (skb); +skb_trim (skb,0 ); + }else { +/* get next skb ready */ + priv->rx_cache =NULL ; +return p54spi_alloc_rx_skb (priv); + }return 0 ; } - +#undef RX_EXTRA_SPACE +#undef READAHEADstatic irqreturn_t p54spi_interrupt (int irq,void *config) {
@@ -666,6 +704,8 @@ static int __devinit p54spi_probe(struct spi_device *spi)if (ret) goto err_free_common; +p54spi_alloc_rx_skb (priv); + ret =p54_register_common (hw, &priv->spi->dev);if (ret) goto err_free_common;
@@ -691,6 +731,7 @@ static int __devexit p54spi_remove(struct spi_device *spi)mutex_destroy (&priv->mutex); +kfree_skb (priv->rx_cache);p54_free_common (priv->hw);return 0 ;
drivers/net/wireless/p54/p54spi.h +3 -1
@@ -25,7 +25,7 @@#include <linux/mutex.h> #include <linux/list.h> #include <net/mac80211.h> - +#include <linux/skbuff.h> #include "p54.h" /* Bit 15 is read/write bit; ON = READ, OFF = WRITE */
@@ -118,6 +118,8 @@ struct p54s_priv {/* protected by tx_lock */ struct list_head tx_pending; +struct sk_buff *rx_cache; +enum fw_state fw_state;const struct firmware *firmware; };
Re: [RFC] p54spi: don't DMA onto the stack
Quoted 10 lines · On Thursday 17 November 2011 00:15:42 Michael Büsch wrote:
Quoted 9 lines
Quoted 6 lines · On Thu, 17 Nov 2011 00:12:03 +0100 Christian Lamparter…
BTW: I always wondered if it would make sense to have a cached rx skb ready in
p54spi_rx(). This way we don't have to do DMA onto the stack [which is really ugly and possibly illegal] and might even get a better rx performance. I could write the code but as you know I don't have the hardware to test it.
Quoted 5 lines
Quoted 1 line
I'll test it, if you can come up with a patch.
---
[RFC] p54spi: don't DMA onto the stackDMA transfers should not be done onto the kernel stack.
What about p54spi_read32, it does the same thing?
I have tested this patch, it works, no measurable rx speed boost though (~6.1Mbit/sec in iperf as either server or client).
[...snip...]
drivers/net/wireless/p54/p54spi.c static int p54spi_rx(struct p54s_priv *priv) · line 411
Quoted 13 lines
drivers/net/wireless/p54/p54spi.c +5
-5-if (len <=READAHEAD_SZ ) { -memcpy (skb_put (skb, len), rx_head +1 , len); +if (len <=READAHEAD ) { +skb_put (skb, len); }else { -memcpy (skb_put (skb,READAHEAD_SZ ), rx_head +1 ,READAHEAD_SZ ); +skb_put (skb,READAHEAD );p54spi_spi_read (priv,SPI_ADRS_DMA_DATA , -skb_put (skb, len -READAHEAD_SZ ), - len -READAHEAD_SZ ); +skb_put (skb, len -READAHEAD ), + len -READAHEAD ); }
I have also tested this patch without this (READAHEAD_SZ) kludge. It appears to work now.
Thanks.
-- Max
Re: [RFC] p54spi: don't DMA onto the stack
Quoted 10 lines · On Saturday 19 November 2011 23:15:34 Max Filippov wrote:
Quoted 10 lines · On Thursday 17 November 2011 00:15:42 Michael Büsch wrote:
Quoted 9 lines
Quoted 6 lines · On Thu, 17 Nov 2011 00:12:03 +0100 Christian Lamparter…
BTW: I always wondered if it would make sense to have a cached rx skb ready in
p54spi_rx(). This way we don't have to do DMA onto the stack [which is really ugly and possibly illegal] and might even get a better rx performance. I could write the code but as you know I don't have the hardware to test it.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'll test it, if you can come up with a patch.
---
[RFC] p54spi: don't DMA onto the stackDMA transfers should not be done onto the kernel stack.
What about
p54spi_read32, it does the same thing?
AFAIK no, p54spi_read32 and p54spi_write16/32 uses PIO.
Of course, I don't know 100% just the docs from johannes' says so :-D.
Quoted 2 lines
I have tested this patch, it works, no measurable rx speed boost though (~6.1Mbit/sec in iperf as either server or client).
I guess that number comes from unicast plain udp testing, right. Do you know if the performance can be improved by setting the mtu to 2274 [ifconfig wlanX mtu 2274] on both client and AP/server?
Quoted 8 lines
Quoted 8 lines
(quoted diff) +3
-3- if (len <= READAHEAD_SZ) { - memcpy(skb_put(skb, len), rx_head + 1, len); + if (len <= READAHEAD) { + skb_put(skb, len); } else { - memcpy(skb_put(skb, READAHEAD_SZ), rx_head + 1, READAHEAD_SZ); + skb_put(skb, READAHEAD); p54spi_spi_read(priv, SPI_ADRS_DMA_DATA,
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +2
-2- skb_put(skb, len - READAHEAD_SZ), - len - READAHEAD_SZ); + skb_put(skb, len - READAHEAD), + len - READAHEAD); }I have also tested this patch without this (
READAHEAD_SZ) kludge. It appears to work now.
well, there's one more thing: what happens when there's just a single read. .e.g.:
---
drivers/net/wireless/p54/p54spi.c +61 -27
@@ -339,22 +339,56 @@ static void p54spi_int_ready(struct p54s_priv *priv) } } +static int p54spi_alloc_rx_skb (struct p54s_priv *priv) +{ +if (priv->rx_cache !=NULL ) +return 0 ; + +/* +* Add extra space for spi rx header and reserve some space since the +* firmware may insert up to 4 padding bytes after the lmac header, +* but it does not amend the size of SPI data transfer. Such packets +* has correct data size in header, thus referencing past the end of +* allocated skb. Reserve extra 4 bytes for this case +*/ +#define RX_EXTRA_SPACE (sizeof(__le16) + sizeof(struct p54_rx_data) + 4) + + priv->rx_cache =dev_alloc_skb (priv->common.rx_mtu +RX_EXTRA_SPACE ); +if (!priv->rx_cache) +return -ENOMEM ; + +/* reserve space for spi transfer size */ +skb_reserve (priv->rx_cache,2 ); +return 0 ; +} +static int p54spi_rx (struct p54s_priv *priv) {struct sk_buff *skb; +__le16 *rx_head; +int err;u16 len; -u16 rx_head[2 ]; -#define READAHEAD_SZ (sizeof(rx_head)-sizeof(u16)) + +#define READAHEAD (sizeof(__le16))if (p54spi_wakeup (priv) <0 )return -EBUSY ; -/* Read data size and first data word in one SPI transaction + err =p54spi_alloc_rx_skb (priv); +if (err) +return err; + +/* +* Read data size and first data word in one SPI transaction * This is workaround for firmware/DMA bug, * when first data word gets lost under high load. */ -p54spi_spi_read (priv,SPI_ADRS_DMA_DATA , rx_head,sizeof (rx_head)); - len = rx_head[0 ]; + skb = priv->rx_cache; + rx_head = (__le16 *)(unsigned long )(priv->rx_cache->data - +sizeof (__le16)); +p54spi_spi_read (priv,SPI_ADRS_DMA_DATA , rx_head,RX_EXTRA_SPACE + + priv->common.rx_mtu); + len =le16_to_cpu (rx_head[0 ]);if (len ==0 ) {p54spi_sleep (priv);
@@ -362,36 +396,33 @@ static int p54spi_rx(struct p54s_priv *priv)return 0 ; } -/* Firmware may insert up to 4 padding bytes after the lmac header, -* but it does not amend the size of SPI data transfer. -* Such packets has correct data size in header, thus referencing -* past the end of allocated skb. Reserve extra 4 bytes for this case */ - skb =dev_alloc_skb (len +4 ); -if (!skb) { +if (len >= (RX_EXTRA_SPACE + priv->common.rx_mtu -4 )) {p54spi_sleep (priv); -dev_err (&priv->spi->dev,"could not alloc skb" ); -return -ENOMEM ; +dev_err (&priv->spi->dev,"rx request larger than max rx mtu\n" ); +return 0 ; } -if (len <=READAHEAD_SZ ) { -memcpy (skb_put (skb, len), rx_head +1 , len); - }else { -memcpy (skb_put (skb,READAHEAD_SZ ), rx_head +1 ,READAHEAD_SZ ); -p54spi_spi_read (priv,SPI_ADRS_DMA_DATA , -skb_put (skb, len -READAHEAD_SZ ), - len -READAHEAD_SZ ); - } +/* +* Put additional bytes to compensate for the possible +* alignment-caused truncation. +*/ +skb_put (skb, len +4 );p54spi_sleep (priv); -/* Put additional bytes to compensate for the possible -* alignment-caused truncation */ -skb_put (skb,4 ); -if (p54_rx (priv->hw, skb) ==0 ) -dev_kfree_skb (skb); +if (p54_rx (priv->hw, skb) ==0 ) { +/* skb was not used up, can be recycled */ +skb_reset_tail_pointer (skb); +skb_trim (skb,0 ); + }else { +/* get next skb ready */ + priv->rx_cache =NULL ; +return p54spi_alloc_rx_skb (priv); + }return 0 ; } - +#undef RX_EXTRA_SPACE +#undef READAHEADstatic irqreturn_t p54spi_interrupt (int irq,void *config) {
@@ -666,6 +697,8 @@ static int __devinit p54spi_probe(struct spi_device *spi)if (ret) goto err_free_common; +p54spi_alloc_rx_skb (priv); + ret =p54_register_common (hw, &priv->spi->dev);if (ret) goto err_free_common;
@@ -691,6 +724,7 @@ static int __devexit p54spi_remove(struct spi_device *spi)mutex_destroy (&priv->mutex); +kfree_skb (priv->rx_cache);p54_free_common (priv->hw);return 0 ;
Re: [RFC] p54spi: don't DMA onto the stack
Quoted 11 lines · On Saturday 19 November 2011 23:15:34 Max Filippov wrote:
Quoted 10 lines
Quoted 10 lines · On Thursday 17 November 2011 00:15:42 Michael Büsch wrote:
Quoted 9 lines
Quoted 6 lines · On Thu, 17 Nov 2011 00:12:03 +0100 Christian Lamparter…
BTW: I always wondered if it would make sense to have a cached rx skb ready in
p54spi_rx(). This way we don't have to do DMA onto the stack [which is really ugly and possibly illegal] and might even get a better rx performance. I could write the code but as you know I don't have the hardware to test it.
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'll test it, if you can come up with a patch.
---
[RFC] p54spi: don't DMA onto the stackDMA transfers should not be done onto the kernel stack.
What about
p54spi_read32, it does the same thing?AFAIK no,
p54spi_read32and p54spi_write16/32 uses PIO.
Initial p54spi_rx transfer with the kludge in place is 4 bytes long as well.
Quoted 1 line
Of course, I don't know 100% just the docs from johannes' says so :-D.
That's right, drivers/spi/omap2_mcspi.c says that:
/* use PIO for small transfers, avoiding DMA setup/teardown overhead and
* cache operations; better heuristics consider wordsize and bitrate. */
#define DMA_MIN_BYTES 160
Quoted 5 lines
Quoted 2 lines
I have tested this patch, it works, no measurable rx speed boost though (~6.1Mbit/sec in iperf as either server or client).
I guess that number comes from unicast plain udp testing, right. Do you know if the performance can be improved by setting the mtu to 2274 [ifconfig wlanX mtu 2274] on both client and AP/server?
~6.7Mbit/sec
Quoted 16 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +3
-3- if (len <= READAHEAD_SZ) { - memcpy(skb_put(skb, len), rx_head + 1, len); + if (len <= READAHEAD) { + skb_put(skb, len); } else { - memcpy(skb_put(skb, READAHEAD_SZ), rx_head + 1, READAHEAD_SZ); + skb_put(skb, READAHEAD); p54spi_spi_read(priv, SPI_ADRS_DMA_DATA,Quoted 8 lines
drivers/net/wireless/p54/p54spi.c static int p54spi_rx(struct p54s_priv *priv) · line 423
Quoted 5 lines
drivers/net/wireless/p54/p54spi.c +2
-2-skb_put (skb, len -READAHEAD_SZ ), - len - READAHEAD_SZ); +skb_put (skb, len -READAHEAD ), + len - READAHEAD); }I have also tested this patch without this (
READAHEAD_SZ) kludge. It appears to work now.
Quoted 2 lines
well, there's one more thing: what happens when there's just a single read. .e.g.:
[...snip...]
Highly unstable link and lots of "rx request of zero bytes" in the dmesg log.
Thanks.
-- Max
Re: [RFC] p54spi: don't DMA onto the stack
Quoted 9 lines · On Sunday 20 November 2011 01:48:55 Max Filippov wrote:
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
[RFC] p54spi: don't DMA onto the stack
DMA transfers should not be done onto the kernel stack.
What about
p54spi_read32, it does the same thing?AFAIK no,
p54spi_read32and p54spi_write16/32 uses PIO.Initial
p54spi_rxtransfer with the kludge in place is 4 bytes long as well.
Quoted 8 lines
Quoted 1 line
Of course, I don't know 100% just the docs from johannes' says so :-D.
That's right,
drivers/spi/omap2_mcspi.csays that:/* use PIO for small transfers, avoiding DMA setup/teardown overhead and
* cache operations; better heuristics consider wordsize and bitrate. */
#defineDMA_MIN_BYTES160
so omap2_mcspi.c might paper of a bug right here and nobody never noticed it. Of course, if we had bothered to read Documentation/spi/spi-summary in the first place then we might not need a paper bag now...
qoute: "
- I/O buffers use the usual Linux rules, and must be DMA-safe. You'd normally allocate them from the heap or free page pool. Don't use the stack, or anything that's declared "static".
- The spi_message and spi_transfer metadata used to glue those I/O buffers into a group of protocol transactions. These can be allocated anywhere it's convenient, including as part of other allocate-once driver data structures. Zero-init these.
"
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Review
Quoted 13 lines
(quoted diff) +5
-5- if (len <= READAHEAD_SZ) { - memcpy(skb_put(skb, len), rx_head + 1, len); + if (len <= READAHEAD) { + skb_put(skb, len); } else { - memcpy(skb_put(skb, READAHEAD_SZ), rx_head + 1, READAHEAD_SZ); + skb_put(skb, READAHEAD); p54spi_spi_read(priv, SPI_ADRS_DMA_DATA, - skb_put(skb, len - READAHEAD_SZ), - len - READAHEAD_SZ); + skb_put(skb, len - READAHEAD), + len - READAHEAD); }I have also tested this patch without this (
READAHEAD_SZ) kludge. It appears to work now.
Quoted 6 lines
Quoted 2 lines
well, there's one more thing: what happens when there's just a single read. .e.g.:
[...snip...]
Highly unstable link and lots of "rx request of zero bytes" in the dmesg log.
Ok, that's a dead end then. BTW, what's your opinion on the subject. Should we alloc a bufffer on demand or have one which is "big enough" always around?
Regards,
Christian
Re: [RFC] p54spi: don't DMA onto the stack
Quoted 3 lines
Ok, that's a dead end then. BTW, what's your opinion on the subject. Should we alloc a bufffer on demand or have one which is "big enough" always around?
The latter seems to me a bit better w.r.t. amount of work on the fast path (:
Thanks.
-- Max