* [PATCH net-next 1/3] macb: add RM9200's interrupt flag TBRE
2020-10-11 9:09 [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91 Willy Tarreau
@ 2020-10-11 9:09 ` Willy Tarreau
2020-10-11 9:09 ` [PATCH net-next 2/3] macb: prepare at91 to use a 2-frame TX queue Willy Tarreau
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Willy Tarreau @ 2020-10-11 9:09 UTC (permalink / raw)
To: Nicolas Ferre, Claudiu Beznea
Cc: netdev, David Miller, Jakub Kicinski, Willy Tarreau, Daniel Palmer
Transmit Buffer Register Empty replaces TXERR on RM9200 and signals the
sender may try to send again becase the last queued frame is no longer
in queue (being transmitted or already transmitted).
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Daniel Palmer <daniel@0x0f.com>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/cadence/macb.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 4f1b41569260..49d347429de8 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -365,6 +365,8 @@
#define MACB_ISR_RLE_SIZE 1
#define MACB_TXERR_OFFSET 6 /* EN TX frame corrupt from error interrupt */
#define MACB_TXERR_SIZE 1
+#define MACB_RM9200_TBRE_OFFSET 6 /* EN may send new frame interrupt (RM9200) */
+#define MACB_RM9200_TBRE_SIZE 1
#define MACB_TCOMP_OFFSET 7 /* Enable transmit complete interrupt */
#define MACB_TCOMP_SIZE 1
#define MACB_ISR_LINK_OFFSET 9 /* Enable link change interrupt */
--
2.28.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH net-next 2/3] macb: prepare at91 to use a 2-frame TX queue
2020-10-11 9:09 [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91 Willy Tarreau
2020-10-11 9:09 ` [PATCH net-next 1/3] macb: add RM9200's interrupt flag TBRE Willy Tarreau
@ 2020-10-11 9:09 ` Willy Tarreau
2020-10-11 9:09 ` [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200 Willy Tarreau
2020-10-14 0:03 ` [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91 Jakub Kicinski
3 siblings, 0 replies; 12+ messages in thread
From: Willy Tarreau @ 2020-10-11 9:09 UTC (permalink / raw)
To: Nicolas Ferre, Claudiu Beznea
Cc: netdev, David Miller, Jakub Kicinski, Willy Tarreau, Daniel Palmer
The RM9200 supports one frame being sent while another one is waiting in
queue. This avoids the dead time that follows the emission of a frame
and which prevents one from reaching line speed.
Right now the driver supports only a single skb, so we'll first replace
the rm9200-specific skb info with an array of two macb_tx_skb (already
used by other drivers). This patch only moves the skb_length to
txq[0].size and skb_physaddr to skb[0].mapping but doesn't perform any
other change. It already uses [desc] in order to minimize future changes.
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Daniel Palmer <daniel@0x0f.com>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/cadence/macb.h | 6 ++---
drivers/net/ethernet/cadence/macb_main.c | 28 ++++++++++++++----------
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 49d347429de8..e87db95fb0f6 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1206,10 +1206,8 @@ struct macb {
phy_interface_t phy_interface;
- /* AT91RM9200 transmit */
- struct sk_buff *skb; /* holds skb until xmit interrupt completes */
- dma_addr_t skb_physaddr; /* phys addr from pci_map_single */
- int skb_length; /* saved skb length for pci_unmap_single */
+ /* AT91RM9200 transmit queue (1 on wire + 1 queued) */
+ struct macb_tx_skb rm9200_txq[2];
unsigned int max_tx_length;
u64 ethtool_stats[GEM_STATS_LEN + QUEUE_STATS_LEN * MACB_MAX_QUEUES];
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 9179f7b0b900..ca6e5456906a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3996,14 +3996,16 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
struct macb *lp = netdev_priv(dev);
if (macb_readl(lp, TSR) & MACB_BIT(RM9200_BNQ)) {
+ int desc = 0;
+
netif_stop_queue(dev);
/* Store packet information (to free when Tx completed) */
- lp->skb = skb;
- lp->skb_length = skb->len;
- lp->skb_physaddr = dma_map_single(&lp->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE);
- if (dma_mapping_error(&lp->pdev->dev, lp->skb_physaddr)) {
+ lp->rm9200_txq[desc].skb = skb;
+ lp->rm9200_txq[desc].size = skb->len;
+ lp->rm9200_txq[desc].mapping = dma_map_single(&lp->pdev->dev, skb->data,
+ skb->len, DMA_TO_DEVICE);
+ if (dma_mapping_error(&lp->pdev->dev, lp->rm9200_txq[desc].mapping)) {
dev_kfree_skb_any(skb);
dev->stats.tx_dropped++;
netdev_err(dev, "%s: DMA mapping error\n", __func__);
@@ -4011,7 +4013,7 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
}
/* Set address of the data in the Transmit Address register */
- macb_writel(lp, TAR, lp->skb_physaddr);
+ macb_writel(lp, TAR, lp->rm9200_txq[desc].mapping);
/* Set length of the packet in the Transmit Control register */
macb_writel(lp, TCR, skb->len);
@@ -4074,6 +4076,7 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
struct net_device *dev = dev_id;
struct macb *lp = netdev_priv(dev);
u32 intstatus, ctl;
+ unsigned int desc;
/* MAC Interrupt Status register indicates what interrupts are pending.
* It is automatically cleared once read.
@@ -4090,13 +4093,14 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
if (intstatus & (MACB_BIT(ISR_TUND) | MACB_BIT(ISR_RLE)))
dev->stats.tx_errors++;
- if (lp->skb) {
- dev_consume_skb_irq(lp->skb);
- lp->skb = NULL;
- dma_unmap_single(&lp->pdev->dev, lp->skb_physaddr,
- lp->skb_length, DMA_TO_DEVICE);
+ desc = 0;
+ if (lp->rm9200_txq[desc].skb) {
+ dev_consume_skb_irq(lp->rm9200_txq[desc].skb);
+ lp->rm9200_txq[desc].skb = NULL;
+ dma_unmap_single(&lp->pdev->dev, lp->rm9200_txq[desc].mapping,
+ lp->rm9200_txq[desc].size, DMA_TO_DEVICE);
dev->stats.tx_packets++;
- dev->stats.tx_bytes += lp->skb_length;
+ dev->stats.tx_bytes += lp->rm9200_txq[desc].size;
}
netif_wake_queue(dev);
}
--
2.28.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200
2020-10-11 9:09 [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91 Willy Tarreau
2020-10-11 9:09 ` [PATCH net-next 1/3] macb: add RM9200's interrupt flag TBRE Willy Tarreau
2020-10-11 9:09 ` [PATCH net-next 2/3] macb: prepare at91 to use a 2-frame TX queue Willy Tarreau
@ 2020-10-11 9:09 ` Willy Tarreau
2020-10-14 16:08 ` Claudiu.Beznea
2020-10-14 0:03 ` [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91 Jakub Kicinski
3 siblings, 1 reply; 12+ messages in thread
From: Willy Tarreau @ 2020-10-11 9:09 UTC (permalink / raw)
To: Nicolas Ferre, Claudiu Beznea
Cc: netdev, David Miller, Jakub Kicinski, Willy Tarreau, Daniel Palmer
The at91rm9200 variant used by a few chips including the MSC313 supports
two Tx descriptors (one frame being serialized and another one queued).
However the driver only implemented a single one, which adds a dead time
after each transfer to receive and process the interrupt and wake the
queue up, preventing from reaching line rate.
This patch implements a very basic 2-deep queue to address this limitation.
The tests run on a Breadbee board equipped with an MSC313E show that at
1 GHz, HTTP traffic on medium-sized objects (45kB) was limited to exactly
50 Mbps before this patch, and jumped to 76 Mbps with this patch. And tests
on a single TCP stream with an MTU of 576 jump from 10kpps to 15kpps. With
1500 byte packets it's now possible to reach line rate versus 75 Mbps
before.
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Daniel Palmer <daniel@0x0f.com>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/cadence/macb.h | 2 ++
drivers/net/ethernet/cadence/macb_main.c | 46 +++++++++++++++++++-----
2 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index e87db95fb0f6..f8133003981f 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1208,6 +1208,8 @@ struct macb {
/* AT91RM9200 transmit queue (1 on wire + 1 queued) */
struct macb_tx_skb rm9200_txq[2];
+ unsigned int rm9200_tx_tail;
+ unsigned int rm9200_tx_len;
unsigned int max_tx_length;
u64 ethtool_stats[GEM_STATS_LEN + QUEUE_STATS_LEN * MACB_MAX_QUEUES];
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ca6e5456906a..6ff8e4b0b95d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3909,6 +3909,7 @@ static int at91ether_start(struct macb *lp)
MACB_BIT(ISR_TUND) |
MACB_BIT(ISR_RLE) |
MACB_BIT(TCOMP) |
+ MACB_BIT(RM9200_TBRE) |
MACB_BIT(ISR_ROVR) |
MACB_BIT(HRESP));
@@ -3925,6 +3926,7 @@ static void at91ether_stop(struct macb *lp)
MACB_BIT(ISR_TUND) |
MACB_BIT(ISR_RLE) |
MACB_BIT(TCOMP) |
+ MACB_BIT(RM9200_TBRE) |
MACB_BIT(ISR_ROVR) |
MACB_BIT(HRESP));
@@ -3994,11 +3996,10 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct macb *lp = netdev_priv(dev);
+ unsigned long flags;
- if (macb_readl(lp, TSR) & MACB_BIT(RM9200_BNQ)) {
- int desc = 0;
-
- netif_stop_queue(dev);
+ if (lp->rm9200_tx_len < 2) {
+ int desc = lp->rm9200_tx_tail;
/* Store packet information (to free when Tx completed) */
lp->rm9200_txq[desc].skb = skb;
@@ -4012,6 +4013,15 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
return NETDEV_TX_OK;
}
+ spin_lock_irqsave(&lp->lock, flags);
+
+ lp->rm9200_tx_tail = (desc + 1) & 1;
+ lp->rm9200_tx_len++;
+ if (lp->rm9200_tx_len > 1)
+ netif_stop_queue(dev);
+
+ spin_unlock_irqrestore(&lp->lock, flags);
+
/* Set address of the data in the Transmit Address register */
macb_writel(lp, TAR, lp->rm9200_txq[desc].mapping);
/* Set length of the packet in the Transmit Control register */
@@ -4077,6 +4087,8 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
struct macb *lp = netdev_priv(dev);
u32 intstatus, ctl;
unsigned int desc;
+ unsigned int qlen;
+ u32 tsr;
/* MAC Interrupt Status register indicates what interrupts are pending.
* It is automatically cleared once read.
@@ -4088,21 +4100,39 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
at91ether_rx(dev);
/* Transmit complete */
- if (intstatus & MACB_BIT(TCOMP)) {
+ if (intstatus & (MACB_BIT(TCOMP) | MACB_BIT(RM9200_TBRE))) {
/* The TCOM bit is set even if the transmission failed */
if (intstatus & (MACB_BIT(ISR_TUND) | MACB_BIT(ISR_RLE)))
dev->stats.tx_errors++;
- desc = 0;
- if (lp->rm9200_txq[desc].skb) {
+ spin_lock(&lp->lock);
+
+ tsr = macb_readl(lp, TSR);
+
+ /* we have three possibilities here:
+ * - all pending packets transmitted (TGO, implies BNQ)
+ * - only first packet transmitted (!TGO && BNQ)
+ * - two frames pending (!TGO && !BNQ)
+ * Note that TGO ("transmit go") is called "IDLE" on RM9200.
+ */
+ qlen = (tsr & MACB_BIT(TGO)) ? 0 :
+ (tsr & MACB_BIT(RM9200_BNQ)) ? 1 : 2;
+
+ while (lp->rm9200_tx_len > qlen) {
+ desc = (lp->rm9200_tx_tail - lp->rm9200_tx_len) & 1;
dev_consume_skb_irq(lp->rm9200_txq[desc].skb);
lp->rm9200_txq[desc].skb = NULL;
dma_unmap_single(&lp->pdev->dev, lp->rm9200_txq[desc].mapping,
lp->rm9200_txq[desc].size, DMA_TO_DEVICE);
dev->stats.tx_packets++;
dev->stats.tx_bytes += lp->rm9200_txq[desc].size;
+ lp->rm9200_tx_len--;
}
- netif_wake_queue(dev);
+
+ if (lp->rm9200_tx_len < 2 && netif_queue_stopped(dev))
+ netif_wake_queue(dev);
+
+ spin_unlock(&lp->lock);
}
/* Work-around for EMAC Errata section 41.3.1 */
--
2.28.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200
2020-10-11 9:09 ` [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200 Willy Tarreau
@ 2020-10-14 16:08 ` Claudiu.Beznea
2020-10-14 16:27 ` Willy Tarreau
0 siblings, 1 reply; 12+ messages in thread
From: Claudiu.Beznea @ 2020-10-14 16:08 UTC (permalink / raw)
To: w, Nicolas.Ferre; +Cc: netdev, davem, kuba, daniel
Hi Willy,
On 11.10.2020 12:09, Willy Tarreau wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> The at91rm9200 variant used by a few chips including the MSC313 supports
> two Tx descriptors (one frame being serialized and another one queued).
> However the driver only implemented a single one, which adds a dead time
> after each transfer to receive and process the interrupt and wake the
> queue up, preventing from reaching line rate.
>
> This patch implements a very basic 2-deep queue to address this limitation.
> The tests run on a Breadbee board equipped with an MSC313E show that at
> 1 GHz, HTTP traffic on medium-sized objects (45kB) was limited to exactly
> 50 Mbps before this patch, and jumped to 76 Mbps with this patch. And tests
> on a single TCP stream with an MTU of 576 jump from 10kpps to 15kpps. With
> 1500 byte packets it's now possible to reach line rate versus 75 Mbps
> before.
>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Daniel Palmer <daniel@0x0f.com>
> Signed-off-by: Willy Tarreau <w@1wt.eu>
> ---
> drivers/net/ethernet/cadence/macb.h | 2 ++
> drivers/net/ethernet/cadence/macb_main.c | 46 +++++++++++++++++++-----
> 2 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index e87db95fb0f6..f8133003981f 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1208,6 +1208,8 @@ struct macb {
>
> /* AT91RM9200 transmit queue (1 on wire + 1 queued) */
> struct macb_tx_skb rm9200_txq[2];
> + unsigned int rm9200_tx_tail;
> + unsigned int rm9200_tx_len;
> unsigned int max_tx_length;
>
> u64 ethtool_stats[GEM_STATS_LEN + QUEUE_STATS_LEN * MACB_MAX_QUEUES];
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ca6e5456906a..6ff8e4b0b95d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3909,6 +3909,7 @@ static int at91ether_start(struct macb *lp)
> MACB_BIT(ISR_TUND) |
> MACB_BIT(ISR_RLE) |
> MACB_BIT(TCOMP) |
> + MACB_BIT(RM9200_TBRE) |
> MACB_BIT(ISR_ROVR) |
> MACB_BIT(HRESP));
>
> @@ -3925,6 +3926,7 @@ static void at91ether_stop(struct macb *lp)
> MACB_BIT(ISR_TUND) |
> MACB_BIT(ISR_RLE) |
> MACB_BIT(TCOMP) |
> + MACB_BIT(RM9200_TBRE) |
> MACB_BIT(ISR_ROVR) |
> MACB_BIT(HRESP));
>
> @@ -3994,11 +3996,10 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> struct macb *lp = netdev_priv(dev);
> + unsigned long flags;
>
> - if (macb_readl(lp, TSR) & MACB_BIT(RM9200_BNQ)) {
> - int desc = 0;
> -
> - netif_stop_queue(dev);
> + if (lp->rm9200_tx_len < 2) {
> + int desc = lp->rm9200_tx_tail;
I think you also want to protect these reads with spin_lock() to avoid
concurrency with the interrupt handler.
>
> /* Store packet information (to free when Tx completed) */
> lp->rm9200_txq[desc].skb = skb;
> @@ -4012,6 +4013,15 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
> return NETDEV_TX_OK;
> }
>
> + spin_lock_irqsave(&lp->lock, flags);
> +
> + lp->rm9200_tx_tail = (desc + 1) & 1;
> + lp->rm9200_tx_len++;
> + if (lp->rm9200_tx_len > 1)
> + netif_stop_queue(dev);
> +
> + spin_unlock_irqrestore(&lp->lock, flags);
> +
> /* Set address of the data in the Transmit Address register */
> macb_writel(lp, TAR, lp->rm9200_txq[desc].mapping);
> /* Set length of the packet in the Transmit Control register */
> @@ -4077,6 +4087,8 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
> struct macb *lp = netdev_priv(dev);
> u32 intstatus, ctl;
> unsigned int desc;
> + unsigned int qlen;
> + u32 tsr;
>
> /* MAC Interrupt Status register indicates what interrupts are pending.
> * It is automatically cleared once read.
> @@ -4088,21 +4100,39 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
> at91ether_rx(dev);
>
> /* Transmit complete */
> - if (intstatus & MACB_BIT(TCOMP)) {
> + if (intstatus & (MACB_BIT(TCOMP) | MACB_BIT(RM9200_TBRE))) {
> /* The TCOM bit is set even if the transmission failed */
> if (intstatus & (MACB_BIT(ISR_TUND) | MACB_BIT(ISR_RLE)))
> dev->stats.tx_errors++;
>
> - desc = 0;
> - if (lp->rm9200_txq[desc].skb) {
> + spin_lock(&lp->lock);
Also, this lock could be moved before while, below, as you want to protect
the rm9200_tx_len and rm9200_tx_tails members of lp as I understand.
Thank you,
Claudiu Beznea
> +
> + tsr = macb_readl(lp, TSR);
> +
> + /* we have three possibilities here:
> + * - all pending packets transmitted (TGO, implies BNQ)
> + * - only first packet transmitted (!TGO && BNQ)
> + * - two frames pending (!TGO && !BNQ)
> + * Note that TGO ("transmit go") is called "IDLE" on RM9200.
> + */
> + qlen = (tsr & MACB_BIT(TGO)) ? 0 :
> + (tsr & MACB_BIT(RM9200_BNQ)) ? 1 : 2;
> +> + while (lp->rm9200_tx_len > qlen) {
> + desc = (lp->rm9200_tx_tail - lp->rm9200_tx_len) & 1;
> dev_consume_skb_irq(lp->rm9200_txq[desc].skb);
> lp->rm9200_txq[desc].skb = NULL;
> dma_unmap_single(&lp->pdev->dev, lp->rm9200_txq[desc].mapping,
> lp->rm9200_txq[desc].size, DMA_TO_DEVICE);
> dev->stats.tx_packets++;
> dev->stats.tx_bytes += lp->rm9200_txq[desc].size;
> + lp->rm9200_tx_len--;
> }
> - netif_wake_queue(dev);
> +
> + if (lp->rm9200_tx_len < 2 && netif_queue_stopped(dev))
> + netif_wake_queue(dev);
> +
> + spin_unlock(&lp->lock);
> }
>
> /* Work-around for EMAC Errata section 41.3.1 */
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200
2020-10-14 16:08 ` Claudiu.Beznea
@ 2020-10-14 16:27 ` Willy Tarreau
2020-10-21 9:02 ` Claudiu.Beznea
0 siblings, 1 reply; 12+ messages in thread
From: Willy Tarreau @ 2020-10-14 16:27 UTC (permalink / raw)
To: Claudiu.Beznea; +Cc: Nicolas.Ferre, netdev, davem, kuba, daniel
Hi Claudiu,
first, thanks for your feedback!
On Wed, Oct 14, 2020 at 04:08:00PM +0000, Claudiu.Beznea@microchip.com wrote:
> > @@ -3994,11 +3996,10 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> > {
> > struct macb *lp = netdev_priv(dev);
> > + unsigned long flags;
> >
> > - if (macb_readl(lp, TSR) & MACB_BIT(RM9200_BNQ)) {
> > - int desc = 0;
> > -
> > - netif_stop_queue(dev);
> > + if (lp->rm9200_tx_len < 2) {
> > + int desc = lp->rm9200_tx_tail;
>
> I think you also want to protect these reads with spin_lock() to avoid
> concurrency with the interrupt handler.
I don't think it's needed because the condition doesn't change below
us as the interrupt handler only decrements. However I could use a
READ_ONCE to make things cleaner. And in practice this test was kept
to keep some sanity checks but it never fails, as if the queue length
reaches 2, the queue is stopped (and I never got the device busy message
either before nor after the patch).
> > /* Store packet information (to free when Tx completed) */
> > lp->rm9200_txq[desc].skb = skb;
> > @@ -4012,6 +4013,15 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
> > return NETDEV_TX_OK;
> > }
> >
> > + spin_lock_irqsave(&lp->lock, flags);
> > +
> > + lp->rm9200_tx_tail = (desc + 1) & 1;
> > + lp->rm9200_tx_len++;
> > + if (lp->rm9200_tx_len > 1)
> > + netif_stop_queue(dev);
This is where we guarantee that we won't call start_xmit() again with
rm9200_tx_len >= 2.
> > @@ -4088,21 +4100,39 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
> > at91ether_rx(dev);
> >
> > /* Transmit complete */
> > - if (intstatus & MACB_BIT(TCOMP)) {
> > + if (intstatus & (MACB_BIT(TCOMP) | MACB_BIT(RM9200_TBRE))) {
> > /* The TCOM bit is set even if the transmission failed */
> > if (intstatus & (MACB_BIT(ISR_TUND) | MACB_BIT(ISR_RLE)))
> > dev->stats.tx_errors++;
> >
> > - desc = 0;
> > - if (lp->rm9200_txq[desc].skb) {
> > + spin_lock(&lp->lock);
>
> Also, this lock could be moved before while, below, as you want to protect
> the rm9200_tx_len and rm9200_tx_tails members of lp as I understand.
Sure, but it actually *is* before the while(). I'm sorry if that was not
visible from the context of the diff. The while is just a few lins below,
thus rm9200_tx_len and rm9200_tx_tail are properly protected. Do not
hesitate to tell me if something is not clear or if I'm wrong!
Thanks!
Willy
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200
2020-10-14 16:27 ` Willy Tarreau
@ 2020-10-21 9:02 ` Claudiu.Beznea
0 siblings, 0 replies; 12+ messages in thread
From: Claudiu.Beznea @ 2020-10-21 9:02 UTC (permalink / raw)
To: w; +Cc: Nicolas.Ferre, netdev, davem, kuba, daniel
Hi Willy,
On 14.10.2020 19:27, Willy Tarreau wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Claudiu,
>
> first, thanks for your feedback!
>
> On Wed, Oct 14, 2020 at 04:08:00PM +0000, Claudiu.Beznea@microchip.com wrote:
>>> @@ -3994,11 +3996,10 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
>>> struct net_device *dev)
>>> {
>>> struct macb *lp = netdev_priv(dev);
>>> + unsigned long flags;
>>>
>>> - if (macb_readl(lp, TSR) & MACB_BIT(RM9200_BNQ)) {
>>> - int desc = 0;
>>> -
>>> - netif_stop_queue(dev);
>>> + if (lp->rm9200_tx_len < 2) {
>>> + int desc = lp->rm9200_tx_tail;
>>
>> I think you also want to protect these reads with spin_lock() to avoid
>> concurrency with the interrupt handler.
>
> I don't think it's needed because the condition doesn't change below
> us as the interrupt handler only decrements. However I could use a
> READ_ONCE to make things cleaner. And in practice this test was kept
> to keep some sanity checks but it never fails, as if the queue length
> reaches 2, the queue is stopped (and I never got the device busy message
> either before nor after the patch).
>
>>> /* Store packet information (to free when Tx completed) */
>>> lp->rm9200_txq[desc].skb = skb;
>>> @@ -4012,6 +4013,15 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
>>> return NETDEV_TX_OK;
>>> }
>>>
>>> + spin_lock_irqsave(&lp->lock, flags);
>>> +
>>> + lp->rm9200_tx_tail = (desc + 1) & 1;
>>> + lp->rm9200_tx_len++;
>>> + if (lp->rm9200_tx_len > 1)
>>> + netif_stop_queue(dev);
>
> This is where we guarantee that we won't call start_xmit() again with
> rm9200_tx_len >= 2.
I see it!
>
>>> @@ -4088,21 +4100,39 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
>>> at91ether_rx(dev);
>>>
>>> /* Transmit complete */
>>> - if (intstatus & MACB_BIT(TCOMP)) {
>>> + if (intstatus & (MACB_BIT(TCOMP) | MACB_BIT(RM9200_TBRE))) {
>>> /* The TCOM bit is set even if the transmission failed */
>>> if (intstatus & (MACB_BIT(ISR_TUND) | MACB_BIT(ISR_RLE)))
>>> dev->stats.tx_errors++;
>>>
>>> - desc = 0;
>>> - if (lp->rm9200_txq[desc].skb) {
>>> + spin_lock(&lp->lock);
>>
>> Also, this lock could be moved before while, below, as you want to protect
>> the rm9200_tx_len and rm9200_tx_tails members of lp as I understand.
>
> Sure, but it actually *is* before the while(). I'm sorry if that was not
> visible from the context of the diff. The while is just a few lins below,
> thus rm9200_tx_len and rm9200_tx_tail are properly protected. Do not
> hesitate to tell me if something is not clear or if I'm wrong!
What I was trying to say is that since for this version of IP TSR is only
read once, here, in interrupt context and the idea of the lock is to
protect the lp->rm9200_tx_tail and lp->rm9200_tx_len, the spin_lock() call
could be moved just before while:
+ spin_lock(&lp->lock);
+
+ tsr = macb_readl(lp, TSR);
+
+ /* we have three possibilities here:
+ * - all pending packets transmitted (TGO, implies BNQ)
+ * - only first packet transmitted (!TGO && BNQ)
+ * - two frames pending (!TGO && !BNQ)
+ * Note that TGO ("transmit go") is called "IDLE" on RM9200.
+ */
+ qlen = (tsr & MACB_BIT(TGO)) ? 0 :
+ (tsr & MACB_BIT(RM9200_BNQ)) ? 1 : 2;
+
here
+ while (lp->rm9200_tx_len > qlen) {
Let me know if I'm missing something.
Thank you,
Claudiu
>
> Thanks!
> Willy
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91
2020-10-11 9:09 [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91 Willy Tarreau
` (2 preceding siblings ...)
2020-10-11 9:09 ` [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200 Willy Tarreau
@ 2020-10-14 0:03 ` Jakub Kicinski
2020-10-14 3:06 ` Willy Tarreau
3 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2020-10-14 0:03 UTC (permalink / raw)
To: Willy Tarreau; +Cc: Nicolas Ferre, Claudiu Beznea, netdev, David Miller
On Sun, 11 Oct 2020 11:09:41 +0200 Willy Tarreau wrote:
> while running some tests on my Breadbee board, I noticed poor network
> Tx performance. I had a look at the driver (macb, at91ether variant)
> and noticed that at91ether_start_xmit() immediately stops the queue
> after sending a frame and waits for the interrupt to restart the queue,
> causing a dead time after each packet is sent.
>
> The AT91RM9200 datasheet states that the controller supports two frames,
> one being sent and the other one being queued, so I performed minimal
> changes to support this. The transmit performance on my board has
> increased by 50% on medium-sized packets (HTTP traffic), and with large
> packets I can now reach line rate.
>
> Since this driver is shared by various platforms, I tried my best to
> isolate and limit the changes as much as possible and I think it's pretty
> reasonable as-is. I've run extensive tests and couldn't meet any
> unexpected situation (no stall, overflow nor lockup).
>
> There are 3 patches in this series. The first one adds the missing
> interrupt flag for RM9200 (TBRE, indicating the tx buffer is willing
> to take a new packet). The second one replaces the single skb with a
> 2-array and uses only index 0. It does no other change, this is just
> to prepare the code for the third one. The third one implements the
> queue. Packets are added at the tail of the queue, the queue is
> stopped at 2 packets and the interrupt releases 0, 1 or 2 depending
> on what the transmit status register reports.
LGTM. There's always a chance that this will make other
designs explode, but short of someone from Cadence giving
us a timely review we have only one way to find that out.. :)
Applied, thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91
2020-10-14 0:03 ` [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91 Jakub Kicinski
@ 2020-10-14 3:06 ` Willy Tarreau
2020-10-14 3:13 ` Jakub Kicinski
2020-11-13 10:03 ` Alexandre Belloni
0 siblings, 2 replies; 12+ messages in thread
From: Willy Tarreau @ 2020-10-14 3:06 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Nicolas Ferre, Claudiu Beznea, netdev, David Miller
On Tue, Oct 13, 2020 at 05:03:58PM -0700, Jakub Kicinski wrote:
> On Sun, 11 Oct 2020 11:09:41 +0200 Willy Tarreau wrote:
> > while running some tests on my Breadbee board, I noticed poor network
> > Tx performance. I had a look at the driver (macb, at91ether variant)
> > and noticed that at91ether_start_xmit() immediately stops the queue
> > after sending a frame and waits for the interrupt to restart the queue,
> > causing a dead time after each packet is sent.
> >
> > The AT91RM9200 datasheet states that the controller supports two frames,
> > one being sent and the other one being queued, so I performed minimal
> > changes to support this. The transmit performance on my board has
> > increased by 50% on medium-sized packets (HTTP traffic), and with large
> > packets I can now reach line rate.
> >
> > Since this driver is shared by various platforms, I tried my best to
> > isolate and limit the changes as much as possible and I think it's pretty
> > reasonable as-is. I've run extensive tests and couldn't meet any
> > unexpected situation (no stall, overflow nor lockup).
> >
> > There are 3 patches in this series. The first one adds the missing
> > interrupt flag for RM9200 (TBRE, indicating the tx buffer is willing
> > to take a new packet). The second one replaces the single skb with a
> > 2-array and uses only index 0. It does no other change, this is just
> > to prepare the code for the third one. The third one implements the
> > queue. Packets are added at the tail of the queue, the queue is
> > stopped at 2 packets and the interrupt releases 0, 1 or 2 depending
> > on what the transmit status register reports.
>
> LGTM. There's always a chance that this will make other
> designs explode, but short of someone from Cadence giving
> us a timely review we have only one way to find that out.. :)
Not that much in fact, given that the at91ether_* functions are only
used by AT91RM9200 (whose datasheet I used to do this) and Mstar which
I used for the tests. I initially wanted to get my old SAM9G20 board
to boot until I noticed that it doesn't even use the same set of
functions, so the potential victims are extremely limited :-)
> Applied, thanks!
Thank you!
Willy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91
2020-10-14 3:06 ` Willy Tarreau
@ 2020-10-14 3:13 ` Jakub Kicinski
2020-11-13 10:03 ` Alexandre Belloni
1 sibling, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2020-10-14 3:13 UTC (permalink / raw)
To: Willy Tarreau; +Cc: Nicolas Ferre, Claudiu Beznea, netdev, David Miller
On Wed, 14 Oct 2020 05:06:30 +0200 Willy Tarreau wrote:
> > LGTM. There's always a chance that this will make other
> > designs explode, but short of someone from Cadence giving
> > us a timely review we have only one way to find that out.. :)
>
> Not that much in fact, given that the at91ether_* functions are only
> used by AT91RM9200 (whose datasheet I used to do this) and Mstar which
> I used for the tests. I initially wanted to get my old SAM9G20 board
> to boot until I noticed that it doesn't even use the same set of
> functions, so the potential victims are extremely limited :-)
GTK :)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91
2020-10-14 3:06 ` Willy Tarreau
2020-10-14 3:13 ` Jakub Kicinski
@ 2020-11-13 10:03 ` Alexandre Belloni
2020-11-13 19:44 ` Willy Tarreau
1 sibling, 1 reply; 12+ messages in thread
From: Alexandre Belloni @ 2020-11-13 10:03 UTC (permalink / raw)
To: Willy Tarreau
Cc: Jakub Kicinski, Nicolas Ferre, Claudiu Beznea, netdev, David Miller
On 14/10/2020 05:06:30+0200, Willy Tarreau wrote:
> On Tue, Oct 13, 2020 at 05:03:58PM -0700, Jakub Kicinski wrote:
> > On Sun, 11 Oct 2020 11:09:41 +0200 Willy Tarreau wrote:
> > > while running some tests on my Breadbee board, I noticed poor network
> > > Tx performance. I had a look at the driver (macb, at91ether variant)
> > > and noticed that at91ether_start_xmit() immediately stops the queue
> > > after sending a frame and waits for the interrupt to restart the queue,
> > > causing a dead time after each packet is sent.
> > >
> > > The AT91RM9200 datasheet states that the controller supports two frames,
> > > one being sent and the other one being queued, so I performed minimal
> > > changes to support this. The transmit performance on my board has
> > > increased by 50% on medium-sized packets (HTTP traffic), and with large
> > > packets I can now reach line rate.
> > >
> > > Since this driver is shared by various platforms, I tried my best to
> > > isolate and limit the changes as much as possible and I think it's pretty
> > > reasonable as-is. I've run extensive tests and couldn't meet any
> > > unexpected situation (no stall, overflow nor lockup).
> > >
> > > There are 3 patches in this series. The first one adds the missing
> > > interrupt flag for RM9200 (TBRE, indicating the tx buffer is willing
> > > to take a new packet). The second one replaces the single skb with a
> > > 2-array and uses only index 0. It does no other change, this is just
> > > to prepare the code for the third one. The third one implements the
> > > queue. Packets are added at the tail of the queue, the queue is
> > > stopped at 2 packets and the interrupt releases 0, 1 or 2 depending
> > > on what the transmit status register reports.
> >
> > LGTM. There's always a chance that this will make other
> > designs explode, but short of someone from Cadence giving
> > us a timely review we have only one way to find that out.. :)
>
> Not that much in fact, given that the at91ether_* functions are only
> used by AT91RM9200 (whose datasheet I used to do this) and Mstar which
> I used for the tests. I initially wanted to get my old SAM9G20 board
> to boot until I noticed that it doesn't even use the same set of
> functions, so the potential victims are extremely limited :-)
>
I think I'm the only one booting recent linux kernels on at91rm9200 and
I'm currently stuck home while the board is at the office. I'll try to
test as soon as possible, which may not be before 2021... At least I'll
know who is the culprit ;)
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/3] macb: support the 2-deep Tx queue on at91
2020-11-13 10:03 ` Alexandre Belloni
@ 2020-11-13 19:44 ` Willy Tarreau
0 siblings, 0 replies; 12+ messages in thread
From: Willy Tarreau @ 2020-11-13 19:44 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Jakub Kicinski, Nicolas Ferre, Claudiu Beznea, netdev, David Miller
Hi Alexandre!
On Fri, Nov 13, 2020 at 11:03:59AM +0100, Alexandre Belloni wrote:
> I think I'm the only one booting recent linux kernels on at91rm9200 and
> I'm currently stuck home while the board is at the office. I'll try to
> test as soon as possible, which may not be before 2021... At least I'll
> know who is the culprit ;)
Oh that's great. I have a SAMG20 based one, which uses slightly different
registers and supports a tx ring, so initially I thought that I couldn't
test it there. But a friend of mine who wrote the drivers for FreeBSD
told me that the original driver still worked for the SAMG20, and I
suspect that despite not being mentioned in the datasheet, the more
recent chip still supports the old behavior, at least to ease the
transistion for their customers. So eventually I'll try it too.
In all transparency, I must tell you that I recently noticed an issue
when facing intense bidirectional traffic, eventually the chip would
report a TX overrun and would stop sending. I finally attributed this
to the unmerged changes needed for the MStar chip, which uses two 16
bit I/O accesses instead of a single 32-bit one, and which apparently
doesn't like being interrupted between the two when writing to the TAR
or TLEN registers. It took me the whole week-end to figure the root
cause so now I need to remove all my debugging code, address the issue
and test again. If I can't manage to fix this and you can't find a
moment to test on your board, I'll propose to revert my patch to stay
on the safe side, as I want at least one implementation to be 100%
reliable with it.
Cheers,
Willy
^ permalink raw reply [flat|nested] 12+ messages in thread