[PATCH v3 6/6] net: sh_eth: use NAPI

[PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

This patch modifies the driver to use NAPI.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 about v3:
  - fix the condition to call the napi_complete()
  - call napi_disable/enable in the sh_eth_set_ringparam()

 drivers/net/ethernet/renesas/sh_eth.c |   85 +++++++++++++++++++++++----------
 drivers/net/ethernet/renesas/sh_eth.h |    3 +
 2 files changed, 62 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index c64a31c..3ee170a 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1035,7 +1035,7 @@ static int sh_eth_txfree(struct net_device *ndev)
 }

 /* Packet receive function */
-static int sh_eth_rx(struct net_device *ndev)
+static int sh_eth_rx(struct net_device *ndev, int *work, int budget)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
 	struct sh_eth_rxdesc *rxdesc;
@@ -1047,7 +1047,8 @@ static int sh_eth_rx(struct net_device *ndev)
 	u32 desc_status;

 	rxdesc = &mdp->rx_ring[entry];
-	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT))) {
+	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT)) &&
+	       *work < budget) {
 		desc_status = edmac_to_cpu(mdp, rxdesc->status);
 		pkt_len = rxdesc->frame_length;

@@ -1087,13 +1088,17 @@ static int sh_eth_rx(struct net_device *ndev)
 				skb_reserve(skb, NET_IP_ALIGN);
 			skb_put(skb, pkt_len);
 			skb->protocol = eth_type_trans(skb, ndev);
-			netif_rx(skb);
-			ndev->stats.rx_packets++;
-			ndev->stats.rx_bytes += pkt_len;
+			if (netif_receive_skb(skb) = NET_RX_DROP) {
+				ndev->stats.rx_dropped++;
+			} else {
+				ndev->stats.rx_packets++;
+				ndev->stats.rx_bytes += pkt_len;
+			}
 		}
 		rxdesc->status |= cpu_to_edmac(mdp, RD_RACT);
 		entry = (++mdp->cur_rx) % mdp->num_rx_ring;
 		rxdesc = &mdp->rx_ring[entry];
+		(*work)++;
 	}

 	/* Refill the Rx ring buffers. */
@@ -1125,7 +1130,7 @@ static int sh_eth_rx(struct net_device *ndev)

 	/* Restart Rx engine if stopped. */
 	/* If we don't need to check status, don't. -KDU */
-	if (!(sh_eth_read(ndev, EDRRR) & EDRRR_R)) {
+	if (*work < budget && !(sh_eth_read(ndev, EDRRR) & EDRRR_R)) {
 		/* fix the values for the next receiving */
 		mdp->cur_rx = mdp->dirty_rx = (sh_eth_read(ndev, RDFAR) -
 					       sh_eth_read(ndev, RDLAR)) >> 4;
@@ -1281,38 +1286,57 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev)

 	/* Get interrpt stat */
 	intr_status = sh_eth_read(ndev, EESR);
-	/* Clear interrupt */
 	if (intr_status & (EESR_FRC | EESR_RMAF | EESR_RRF |
 			EESR_RTLF | EESR_RTSF | EESR_PRE | EESR_CERF |
 			cd->tx_check | cd->eesr_err_check)) {
-		sh_eth_write(ndev, intr_status, EESR);
+		if (napi_schedule_prep(&mdp->napi)) {
+			/* Disable interrupts of the channel */
+			sh_eth_write(ndev, 0, EESIPR);
+			__napi_schedule(&mdp->napi);
+		}
 		ret = IRQ_HANDLED;
-	} else
-		goto other_irq;
-
-	if (intr_status & (EESR_FRC | /* Frame recv*/
-			EESR_RMAF | /* Multi cast address recv*/
-			EESR_RRF  | /* Bit frame recv */
-			EESR_RTLF | /* Long frame recv*/
-			EESR_RTSF | /* short frame recv */
-			EESR_PRE  | /* PHY-LSI recv error */
-			EESR_CERF)){ /* recv frame CRC error */
-		sh_eth_rx(ndev);
 	}

-	/* Tx Check */
-	if (intr_status & cd->tx_check) {
-		sh_eth_txfree(ndev);
+	spin_unlock(&mdp->lock);
+
+	return ret;
+}
+
+static int sh_eth_poll(struct napi_struct *napi, int budget)
+{
+	struct sh_eth_private *mdp = container_of(napi, struct sh_eth_private,
+						  napi);
+	struct net_device *ndev = mdp->ndev;
+	struct sh_eth_cpu_data *cd = mdp->cd;
+	int work_done = 0, txfree_num;
+	u32 intr_status = sh_eth_read(ndev, EESR);
+
+	/* Clear interrupt flags */
+	sh_eth_write(ndev, intr_status, EESR);
+
+	/* check txdesc */
+	txfree_num = sh_eth_txfree(ndev);
+	if (txfree_num)
 		netif_wake_queue(ndev);
-	}

+	/* check rxdesc */
+	sh_eth_rx(ndev, &work_done, budget);
+
+	/* check error flags */
 	if (intr_status & cd->eesr_err_check)
 		sh_eth_error(ndev, intr_status);

-other_irq:
-	spin_unlock(&mdp->lock);
+	/* get current interrupt flags */
+	intr_status = sh_eth_read(ndev, EESR);

-	return ret;
+	/* check whether this driver should call napi_complete() */
+	if (work_done < budget) {
+		napi_complete(napi);
+		/* Enable all interrupts */
+		sh_eth_write(ndev, cd->eesipr_value, EESIPR);
+	}
+
+	return work_done;
 }

 /* PHY state control function */
@@ -1545,6 +1569,7 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
 		/* Stop the chip's Tx and Rx processes. */
 		sh_eth_write(ndev, 0, EDTRR);
 		sh_eth_write(ndev, 0, EDRRR);
+		napi_disable(&mdp->napi);
 		synchronize_irq(ndev->irq);
 	}

@@ -1569,6 +1594,7 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
 	}

 	if (netif_running(ndev)) {
+		napi_enable(&mdp->napi);
 		sh_eth_write(ndev, mdp->cd->eesipr_value, EESIPR);
 		/* Setting the Rx mode will start the Rx process. */
 		sh_eth_write(ndev, EDRRR_R, EDRRR);
@@ -1600,6 +1626,8 @@ static int sh_eth_open(struct net_device *ndev)

 	pm_runtime_get_sync(&mdp->pdev->dev);

+	napi_enable(&mdp->napi);
+
 	ret = request_irq(ndev->irq, sh_eth_interrupt,
 #if defined(CONFIG_CPU_SUBTYPE_SH7763) || \
 	defined(CONFIG_CPU_SUBTYPE_SH7764) || \
@@ -1739,6 +1767,8 @@ static int sh_eth_close(struct net_device *ndev)
 		phy_disconnect(mdp->phydev);
 	}

+	napi_disable(&mdp->napi);
+
 	free_irq(ndev->irq, ndev);

 	/* Free all the skbuffs in the Rx queue. */
@@ -2368,6 +2398,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 #endif
 	sh_eth_set_default_cpu_data(mdp->cd);

+	mdp->ndev = ndev;
+	netif_napi_add(ndev, &mdp->napi, sh_eth_poll, SH_ETH_NAPI_WEIGHT);
+
 	/* set function */
 	ndev->netdev_ops = &sh_eth_netdev_ops;
 	SET_ETHTOOL_OPS(ndev, &sh_eth_ethtool_ops);
diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
index f1dbc27..93dad7b 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -35,6 +35,7 @@
 #define PKT_BUF_SZ		1538
 #define SH_ETH_TSU_TIMEOUT_MS	500
 #define SH_ETH_TSU_CAM_ENTRIES	32
+#define SH_ETH_NAPI_WEIGHT	32

 enum {
 	/* E-DMAC registers */
@@ -728,6 +729,8 @@ struct sh_eth_private {
 	int duplex;
 	int port;		/* for TSU */
 	int vlan_num_ids;	/* for VLAN tag filter */
+	struct napi_struct napi;
+	struct net_device *ndev;

 	unsigned no_ether_link:1;
 	unsigned ether_link_active_low:1;
-- 
1.7.1

[PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

This patch modifies the driver to use NAPI.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 about v3:
  - fix the condition to call the napi_complete()
  - call napi_disable/enable in the sh_eth_set_ringparam()

 drivers/net/ethernet/renesas/sh_eth.c |   85 +++++++++++++++++++++++----------
 drivers/net/ethernet/renesas/sh_eth.h |    3 +
 2 files changed, 62 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index c64a31c..3ee170a 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1035,7 +1035,7 @@ static int sh_eth_txfree(struct net_device *ndev)
 }

 /* Packet receive function */
-static int sh_eth_rx(struct net_device *ndev)
+static int sh_eth_rx(struct net_device *ndev, int *work, int budget)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
 	struct sh_eth_rxdesc *rxdesc;
@@ -1047,7 +1047,8 @@ static int sh_eth_rx(struct net_device *ndev)
 	u32 desc_status;

 	rxdesc = &mdp->rx_ring[entry];
-	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT))) {
+	while (!(rxdesc->status & cpu_to_edmac(mdp, RD_RACT)) &&
+	       *work < budget) {
 		desc_status = edmac_to_cpu(mdp, rxdesc->status);
 		pkt_len = rxdesc->frame_length;

@@ -1087,13 +1088,17 @@ static int sh_eth_rx(struct net_device *ndev)
 				skb_reserve(skb, NET_IP_ALIGN);
 			skb_put(skb, pkt_len);
 			skb->protocol = eth_type_trans(skb, ndev);
-			netif_rx(skb);
-			ndev->stats.rx_packets++;
-			ndev->stats.rx_bytes += pkt_len;
+			if (netif_receive_skb(skb) == NET_RX_DROP) {
+				ndev->stats.rx_dropped++;
+			} else {
+				ndev->stats.rx_packets++;
+				ndev->stats.rx_bytes += pkt_len;
+			}
 		}
 		rxdesc->status |= cpu_to_edmac(mdp, RD_RACT);
 		entry = (++mdp->cur_rx) % mdp->num_rx_ring;
 		rxdesc = &mdp->rx_ring[entry];
+		(*work)++;
 	}

 	/* Refill the Rx ring buffers. */
@@ -1125,7 +1130,7 @@ static int sh_eth_rx(struct net_device *ndev)

 	/* Restart Rx engine if stopped. */
 	/* If we don't need to check status, don't. -KDU */
-	if (!(sh_eth_read(ndev, EDRRR) & EDRRR_R)) {
+	if (*work < budget && !(sh_eth_read(ndev, EDRRR) & EDRRR_R)) {
 		/* fix the values for the next receiving */
 		mdp->cur_rx = mdp->dirty_rx = (sh_eth_read(ndev, RDFAR) -
 					       sh_eth_read(ndev, RDLAR)) >> 4;
@@ -1281,38 +1286,57 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev)

 	/* Get interrpt stat */
 	intr_status = sh_eth_read(ndev, EESR);
-	/* Clear interrupt */
 	if (intr_status & (EESR_FRC | EESR_RMAF | EESR_RRF |
 			EESR_RTLF | EESR_RTSF | EESR_PRE | EESR_CERF |
 			cd->tx_check | cd->eesr_err_check)) {
-		sh_eth_write(ndev, intr_status, EESR);
+		if (napi_schedule_prep(&mdp->napi)) {
+			/* Disable interrupts of the channel */
+			sh_eth_write(ndev, 0, EESIPR);
+			__napi_schedule(&mdp->napi);
+		}
 		ret = IRQ_HANDLED;
-	} else
-		goto other_irq;
-
-	if (intr_status & (EESR_FRC | /* Frame recv*/
-			EESR_RMAF | /* Multi cast address recv*/
-			EESR_RRF  | /* Bit frame recv */
-			EESR_RTLF | /* Long frame recv*/
-			EESR_RTSF | /* short frame recv */
-			EESR_PRE  | /* PHY-LSI recv error */
-			EESR_CERF)){ /* recv frame CRC error */
-		sh_eth_rx(ndev);
 	}

-	/* Tx Check */
-	if (intr_status & cd->tx_check) {
-		sh_eth_txfree(ndev);
+	spin_unlock(&mdp->lock);
+
+	return ret;
+}
+
+static int sh_eth_poll(struct napi_struct *napi, int budget)
+{
+	struct sh_eth_private *mdp = container_of(napi, struct sh_eth_private,
+						  napi);
+	struct net_device *ndev = mdp->ndev;
+	struct sh_eth_cpu_data *cd = mdp->cd;
+	int work_done = 0, txfree_num;
+	u32 intr_status = sh_eth_read(ndev, EESR);
+
+	/* Clear interrupt flags */
+	sh_eth_write(ndev, intr_status, EESR);
+
+	/* check txdesc */
+	txfree_num = sh_eth_txfree(ndev);
+	if (txfree_num)
 		netif_wake_queue(ndev);
-	}

+	/* check rxdesc */
+	sh_eth_rx(ndev, &work_done, budget);
+
+	/* check error flags */
 	if (intr_status & cd->eesr_err_check)
 		sh_eth_error(ndev, intr_status);

-other_irq:
-	spin_unlock(&mdp->lock);
+	/* get current interrupt flags */
+	intr_status = sh_eth_read(ndev, EESR);

-	return ret;
+	/* check whether this driver should call napi_complete() */
+	if (work_done < budget) {
+		napi_complete(napi);
+		/* Enable all interrupts */
+		sh_eth_write(ndev, cd->eesipr_value, EESIPR);
+	}
+
+	return work_done;
 }

 /* PHY state control function */
@@ -1545,6 +1569,7 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
 		/* Stop the chip's Tx and Rx processes. */
 		sh_eth_write(ndev, 0, EDTRR);
 		sh_eth_write(ndev, 0, EDRRR);
+		napi_disable(&mdp->napi);
 		synchronize_irq(ndev->irq);
 	}

@@ -1569,6 +1594,7 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
 	}

 	if (netif_running(ndev)) {
+		napi_enable(&mdp->napi);
 		sh_eth_write(ndev, mdp->cd->eesipr_value, EESIPR);
 		/* Setting the Rx mode will start the Rx process. */
 		sh_eth_write(ndev, EDRRR_R, EDRRR);
@@ -1600,6 +1626,8 @@ static int sh_eth_open(struct net_device *ndev)

 	pm_runtime_get_sync(&mdp->pdev->dev);

+	napi_enable(&mdp->napi);
+
 	ret = request_irq(ndev->irq, sh_eth_interrupt,
 #if defined(CONFIG_CPU_SUBTYPE_SH7763) || \
 	defined(CONFIG_CPU_SUBTYPE_SH7764) || \
@@ -1739,6 +1767,8 @@ static int sh_eth_close(struct net_device *ndev)
 		phy_disconnect(mdp->phydev);
 	}

+	napi_disable(&mdp->napi);
+
 	free_irq(ndev->irq, ndev);

 	/* Free all the skbuffs in the Rx queue. */
@@ -2368,6 +2398,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 #endif
 	sh_eth_set_default_cpu_data(mdp->cd);

+	mdp->ndev = ndev;
+	netif_napi_add(ndev, &mdp->napi, sh_eth_poll, SH_ETH_NAPI_WEIGHT);
+
 	/* set function */
 	ndev->netdev_ops = &sh_eth_netdev_ops;
 	SET_ETHTOOL_OPS(ndev, &sh_eth_ethtool_ops);
diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
index f1dbc27..93dad7b 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -35,6 +35,7 @@
 #define PKT_BUF_SZ		1538
 #define SH_ETH_TSU_TIMEOUT_MS	500
 #define SH_ETH_TSU_CAM_ENTRIES	32
+#define SH_ETH_NAPI_WEIGHT	32

 enum {
 	/* E-DMAC registers */
@@ -728,6 +729,8 @@ struct sh_eth_private {
 	int duplex;
 	int port;		/* for TSU */
 	int vlan_num_ids;	/* for VLAN tag filter */
+	struct napi_struct napi;
+	struct net_device *ndev;

 	unsigned no_ether_link:1;
 	unsigned ether_link_active_low:1;
-- 
1.7.1

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[David Miller]

From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
Date: Mon, 14 May 2012 15:47:24 +0900

> This patch modifies the driver to use NAPI.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

I think your TX path is still extremely racey.

No locks are held here, so you tell me what happens if we execute:

> +	/* check txdesc */
> +	txfree_num = sh_eth_txfree(ndev);
> +	if (txfree_num)

and at this exact moment the queue was in fact already awake and
another thread of control transmits packets, and this action fills up
the TX queue and stops the queue.

>  		netif_wake_queue(ndev);

This will erroneously wake the queue and trigger the debugging
message in your TX function.

You need strict synchronization between your TX queueing and TX
liberation flows.  So that queue stop and wake are only performed
at the correct moment.

In fact, looking at how the mdp->lock is used in your TX routine, it
seems to protect absolutely against nothing.

Please read the TX flow of drivers/net/ethernet/broadcom/tg3.c to see
how to do this correctly, and lock free, in a NAPI driver.

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[David Miller]

From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
Date: Mon, 14 May 2012 15:47:24 +0900

> This patch modifies the driver to use NAPI.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

I think your TX path is still extremely racey.

No locks are held here, so you tell me what happens if we execute:

> +	/* check txdesc */
> +	txfree_num = sh_eth_txfree(ndev);
> +	if (txfree_num)

and at this exact moment the queue was in fact already awake and
another thread of control transmits packets, and this action fills up
the TX queue and stops the queue.

>  		netif_wake_queue(ndev);

This will erroneously wake the queue and trigger the debugging
message in your TX function.

You need strict synchronization between your TX queueing and TX
liberation flows.  So that queue stop and wake are only performed
at the correct moment.

In fact, looking at how the mdp->lock is used in your TX routine, it
seems to protect absolutely against nothing.

Please read the TX flow of drivers/net/ethernet/broadcom/tg3.c to see
how to do this correctly, and lock free, in a NAPI driver.

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/15 7:50, David Miller wrote:
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> Date: Mon, 14 May 2012 15:47:24 +0900
> 
>> This patch modifies the driver to use NAPI.
>>
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> I think your TX path is still extremely racey.
> 
> No locks are held here, so you tell me what happens if we execute:
> 
>> +	/* check txdesc */
>> +	txfree_num = sh_eth_txfree(ndev);
>> +	if (txfree_num)
> 

Thank you for your review.
In the sh_eth_txfree(), it check the tx descriptors. If the tx descriptor
is completed, it calls dev_kfree_skb_irq(), and it returns value 1 or more.
So, the sh_eth_poll() calls netif_wake_queue() if the dev_kfree_skb_irq() is
called.

> and at this exact moment the queue was in fact already awake and
> another thread of control transmits packets, and this action fills up
> the TX queue and stops the queue.
> 
>>  		netif_wake_queue(ndev);
> 
> This will erroneously wake the queue and trigger the debugging
> message in your TX function.
> 
> You need strict synchronization between your TX queueing and TX
> liberation flows.  So that queue stop and wake are only performed
> at the correct moment.

I will add netif_queue_stopped() in the sh_eth_poll().

> In fact, looking at how the mdp->lock is used in your TX routine, it
> seems to protect absolutely against nothing.

I wlll remove the mdp->lock in the sh_eth_start_xmit().

> Please read the TX flow of drivers/net/ethernet/broadcom/tg3.c to see
> how to do this correctly, and lock free, in a NAPI driver.
> 

Thank you for your suggestion.
I will add netif_tx_lock/unlock before and after netif_wake_queue().

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/15 7:50, David Miller wrote:
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> Date: Mon, 14 May 2012 15:47:24 +0900
> 
>> This patch modifies the driver to use NAPI.
>>
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> I think your TX path is still extremely racey.
> 
> No locks are held here, so you tell me what happens if we execute:
> 
>> +	/* check txdesc */
>> +	txfree_num = sh_eth_txfree(ndev);
>> +	if (txfree_num)
> 

Thank you for your review.
In the sh_eth_txfree(), it check the tx descriptors. If the tx descriptor
is completed, it calls dev_kfree_skb_irq(), and it returns value 1 or more.
So, the sh_eth_poll() calls netif_wake_queue() if the dev_kfree_skb_irq() is
called.

> and at this exact moment the queue was in fact already awake and
> another thread of control transmits packets, and this action fills up
> the TX queue and stops the queue.
> 
>>  		netif_wake_queue(ndev);
> 
> This will erroneously wake the queue and trigger the debugging
> message in your TX function.
> 
> You need strict synchronization between your TX queueing and TX
> liberation flows.  So that queue stop and wake are only performed
> at the correct moment.

I will add netif_queue_stopped() in the sh_eth_poll().

> In fact, looking at how the mdp->lock is used in your TX routine, it
> seems to protect absolutely against nothing.

I wlll remove the mdp->lock in the sh_eth_start_xmit().

> Please read the TX flow of drivers/net/ethernet/broadcom/tg3.c to see
> how to do this correctly, and lock free, in a NAPI driver.
> 

Thank you for your suggestion.
I will add netif_tx_lock/unlock before and after netif_wake_queue().

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[David Miller]

From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
Date: Tue, 15 May 2012 13:47:44 +0900

> 2012/05/15 7:50, David Miller wrote:
>> You need strict synchronization between your TX queueing and TX
>> liberation flows.  So that queue stop and wake are only performed
>> at the correct moment.
> 
> I will add netif_queue_stopped() in the sh_eth_poll().

That doesn't fix the bug.  What if someone transmits a packet and
fills the TX queue between the netif_queue_stopped() test and the
call to netif_wake_queue()?

Adding another test doesn't create the necessary synchronization.

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[David Miller]

From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
Date: Tue, 15 May 2012 13:47:44 +0900

> 2012/05/15 7:50, David Miller wrote:
>> You need strict synchronization between your TX queueing and TX
>> liberation flows.  So that queue stop and wake are only performed
>> at the correct moment.
> 
> I will add netif_queue_stopped() in the sh_eth_poll().

That doesn't fix the bug.  What if someone transmits a packet and
fills the TX queue between the netif_queue_stopped() test and the
call to netif_wake_queue()?

Adding another test doesn't create the necessary synchronization.

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/15 14:07, David Miller wrote:
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> Date: Tue, 15 May 2012 13:47:44 +0900
> 
>> 2012/05/15 7:50, David Miller wrote:
>>> You need strict synchronization between your TX queueing and TX
>>> liberation flows.  So that queue stop and wake are only performed
>>> at the correct moment.
>>
>> I will add netif_queue_stopped() in the sh_eth_poll().
> 
> That doesn't fix the bug.  What if someone transmits a packet and
> fills the TX queue between the netif_queue_stopped() test and the
> call to netif_wake_queue()?
> 
> Adding another test doesn't create the necessary synchronization.
> 

Thank you for the reply again.
I will modify the code as the following. Is it correct?

	if (txfree_num) {
		netif_tx_lock(ndev);
		if (netif_queue_stopped(ndev))
			netif_wake_queue(ndev);
		netif_tx_unlock(ndev);
	}

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/15 14:07, David Miller wrote:
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> Date: Tue, 15 May 2012 13:47:44 +0900
> 
>> 2012/05/15 7:50, David Miller wrote:
>>> You need strict synchronization between your TX queueing and TX
>>> liberation flows.  So that queue stop and wake are only performed
>>> at the correct moment.
>>
>> I will add netif_queue_stopped() in the sh_eth_poll().
> 
> That doesn't fix the bug.  What if someone transmits a packet and
> fills the TX queue between the netif_queue_stopped() test and the
> call to netif_wake_queue()?
> 
> Adding another test doesn't create the necessary synchronization.
> 

Thank you for the reply again.
I will modify the code as the following. Is it correct?

	if (txfree_num) {
		netif_tx_lock(ndev);
		if (netif_queue_stopped(ndev))
			netif_wake_queue(ndev);
		netif_tx_unlock(ndev);
	}

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Francois Romieu]

Shimoda, Yoshihiro <yoshihiro.shimoda.uh@renesas.com> :
[...]
> I will modify the code as the following. Is it correct?

No.

Please take a look at tg3, especially the ...mb() calls and rework the
queue disabling part in the xmit handler. Btw sh_eth_txfree has no
business being called from the xmit and poll handlers at the same time.

There is no memory barrier in the xmit handler. It it not clear if the
poll thread always sees tx ring status and cur_tx updates in the same
order or not. If not, the driver may unmap before the device actually
accesses skb buffer.

-- 
Ueimor

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Francois Romieu]

Shimoda, Yoshihiro <yoshihiro.shimoda.uh@renesas.com> :
[...]
> I will modify the code as the following. Is it correct?

No.

Please take a look at tg3, especially the ...mb() calls and rework the
queue disabling part in the xmit handler. Btw sh_eth_txfree has no
business being called from the xmit and poll handlers at the same time.

There is no memory barrier in the xmit handler. It it not clear if the
poll thread always sees tx ring status and cur_tx updates in the same
order or not. If not, the driver may unmap before the device actually
accesses skb buffer.

-- 
Ueimor

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[David Miller]

From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
Date: Tue, 15 May 2012 18:46:25 +0900

> 2012/05/15 14:07, David Miller wrote:
>> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
>> Date: Tue, 15 May 2012 13:47:44 +0900
>> 
>>> 2012/05/15 7:50, David Miller wrote:
>>>> You need strict synchronization between your TX queueing and TX
>>>> liberation flows.  So that queue stop and wake are only performed
>>>> at the correct moment.
>>>
>>> I will add netif_queue_stopped() in the sh_eth_poll().
>> 
>> That doesn't fix the bug.  What if someone transmits a packet and
>> fills the TX queue between the netif_queue_stopped() test and the
>> call to netif_wake_queue()?
>> 
>> Adding another test doesn't create the necessary synchronization.
>> 
> 
> Thank you for the reply again.
> I will modify the code as the following. Is it correct?
> 
> 	if (txfree_num) {
> 		netif_tx_lock(ndev);
> 		if (netif_queue_stopped(ndev))
> 			netif_wake_queue(ndev);
> 		netif_tx_unlock(ndev);
> 	}

Yes, and then you don't need that private lock in the start_xmit()
method at all, since that method runs with the tx_lock held.

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[David Miller]

From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
Date: Tue, 15 May 2012 18:46:25 +0900

> 2012/05/15 14:07, David Miller wrote:
>> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
>> Date: Tue, 15 May 2012 13:47:44 +0900
>> 
>>> 2012/05/15 7:50, David Miller wrote:
>>>> You need strict synchronization between your TX queueing and TX
>>>> liberation flows.  So that queue stop and wake are only performed
>>>> at the correct moment.
>>>
>>> I will add netif_queue_stopped() in the sh_eth_poll().
>> 
>> That doesn't fix the bug.  What if someone transmits a packet and
>> fills the TX queue between the netif_queue_stopped() test and the
>> call to netif_wake_queue()?
>> 
>> Adding another test doesn't create the necessary synchronization.
>> 
> 
> Thank you for the reply again.
> I will modify the code as the following. Is it correct?
> 
> 	if (txfree_num) {
> 		netif_tx_lock(ndev);
> 		if (netif_queue_stopped(ndev))
> 			netif_wake_queue(ndev);
> 		netif_tx_unlock(ndev);
> 	}

Yes, and then you don't need that private lock in the start_xmit()
method at all, since that method runs with the tx_lock held.

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Francois Romieu]

David Miller <davem@davemloft.net> :
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
[...]
> > I will modify the code as the following. Is it correct?
> > 
> > 	if (txfree_num) {
> > 		netif_tx_lock(ndev);
> > 		if (netif_queue_stopped(ndev))
> > 			netif_wake_queue(ndev);
> > 		netif_tx_unlock(ndev);
> > 	}
> 
> Yes, and then you don't need that private lock in the start_xmit()
> method at all, since that method runs with the tx_lock held.

I agree that the lock should go but:
1. something must be done to prevent sh_eth_txfree() being called
   at the same time from the xmit and poll handlers
2. while netif_tx locking above provides an implicit memory barrier,
   there won't be one in sh_eth_start_xmit once the lock is removed.
   mdp->dirty_tx changes may thus go unnoticed in sh_eth_start_xmit

-- 
Ueimor

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Francois Romieu]

David Miller <davem@davemloft.net> :
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
[...]
> > I will modify the code as the following. Is it correct?
> > 
> > 	if (txfree_num) {
> > 		netif_tx_lock(ndev);
> > 		if (netif_queue_stopped(ndev))
> > 			netif_wake_queue(ndev);
> > 		netif_tx_unlock(ndev);
> > 	}
> 
> Yes, and then you don't need that private lock in the start_xmit()
> method at all, since that method runs with the tx_lock held.

I agree that the lock should go but:
1. something must be done to prevent sh_eth_txfree() being called
   at the same time from the xmit and poll handlers
2. while netif_tx locking above provides an implicit memory barrier,
   there won't be one in sh_eth_start_xmit once the lock is removed.
   mdp->dirty_tx changes may thus go unnoticed in sh_eth_start_xmit

-- 
Ueimor

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/16 2:05, David Miller wrote:
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> Date: Tue, 15 May 2012 18:46:25 +0900
> 
>> 2012/05/15 14:07, David Miller wrote:
>>> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
>>> Date: Tue, 15 May 2012 13:47:44 +0900
>>>
>>>> 2012/05/15 7:50, David Miller wrote:
>>>>> You need strict synchronization between your TX queueing and TX
>>>>> liberation flows.  So that queue stop and wake are only performed
>>>>> at the correct moment.
>>>>
>>>> I will add netif_queue_stopped() in the sh_eth_poll().
>>>
>>> That doesn't fix the bug.  What if someone transmits a packet and
>>> fills the TX queue between the netif_queue_stopped() test and the
>>> call to netif_wake_queue()?
>>>
>>> Adding another test doesn't create the necessary synchronization.
>>>
>>
>> Thank you for the reply again.
>> I will modify the code as the following. Is it correct?
>>
>> 	if (txfree_num) {
>> 		netif_tx_lock(ndev);
>> 		if (netif_queue_stopped(ndev))
>> 			netif_wake_queue(ndev);
>> 		netif_tx_unlock(ndev);
>> 	}
> 
> Yes, and then you don't need that private lock in the start_xmit()
> method at all, since that method runs with the tx_lock held.
> 

Thank you for the reply. I will also modify the start_xmit().

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/16 2:05, David Miller wrote:
> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> Date: Tue, 15 May 2012 18:46:25 +0900
> 
>> 2012/05/15 14:07, David Miller wrote:
>>> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
>>> Date: Tue, 15 May 2012 13:47:44 +0900
>>>
>>>> 2012/05/15 7:50, David Miller wrote:
>>>>> You need strict synchronization between your TX queueing and TX
>>>>> liberation flows.  So that queue stop and wake are only performed
>>>>> at the correct moment.
>>>>
>>>> I will add netif_queue_stopped() in the sh_eth_poll().
>>>
>>> That doesn't fix the bug.  What if someone transmits a packet and
>>> fills the TX queue between the netif_queue_stopped() test and the
>>> call to netif_wake_queue()?
>>>
>>> Adding another test doesn't create the necessary synchronization.
>>>
>>
>> Thank you for the reply again.
>> I will modify the code as the following. Is it correct?
>>
>> 	if (txfree_num) {
>> 		netif_tx_lock(ndev);
>> 		if (netif_queue_stopped(ndev))
>> 			netif_wake_queue(ndev);
>> 		netif_tx_unlock(ndev);
>> 	}
> 
> Yes, and then you don't need that private lock in the start_xmit()
> method at all, since that method runs with the tx_lock held.
> 

Thank you for the reply. I will also modify the start_xmit().

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/16 3:29, Francois Romieu wrote:
> David Miller <davem@davemloft.net> :
>> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> [...]
>>> I will modify the code as the following. Is it correct?
>>>
>>> 	if (txfree_num) {
>>> 		netif_tx_lock(ndev);
>>> 		if (netif_queue_stopped(ndev))
>>> 			netif_wake_queue(ndev);
>>> 		netif_tx_unlock(ndev);
>>> 	}
>>
>> Yes, and then you don't need that private lock in the start_xmit()
>> method at all, since that method runs with the tx_lock held.
> 
> I agree that the lock should go but:
> 1. something must be done to prevent sh_eth_txfree() being called
>    at the same time from the xmit and poll handlers
> 2. while netif_tx locking above provides an implicit memory barrier,
>    there won't be one in sh_eth_start_xmit once the lock is removed.
>    mdp->dirty_tx changes may thus go unnoticed in sh_eth_start_xmit
> 

Thank you for the review.

I checked tg3.c, and I found a patch to fix the race condition:
commit ID 1b2a72050 : [TG3]: Fix tx race condition

I will check the patch, and I will write such a patch for the sh_eth
driver later.

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3 6/6] net: sh_eth: use NAPI

[Shimoda, Yoshihiro]

2012/05/16 3:29, Francois Romieu wrote:
> David Miller <davem@davemloft.net> :
>> From: "Shimoda, Yoshihiro" <yoshihiro.shimoda.uh@renesas.com>
> [...]
>>> I will modify the code as the following. Is it correct?
>>>
>>> 	if (txfree_num) {
>>> 		netif_tx_lock(ndev);
>>> 		if (netif_queue_stopped(ndev))
>>> 			netif_wake_queue(ndev);
>>> 		netif_tx_unlock(ndev);
>>> 	}
>>
>> Yes, and then you don't need that private lock in the start_xmit()
>> method at all, since that method runs with the tx_lock held.
> 
> I agree that the lock should go but:
> 1. something must be done to prevent sh_eth_txfree() being called
>    at the same time from the xmit and poll handlers
> 2. while netif_tx locking above provides an implicit memory barrier,
>    there won't be one in sh_eth_start_xmit once the lock is removed.
>    mdp->dirty_tx changes may thus go unnoticed in sh_eth_start_xmit
> 

Thank you for the review.

I checked tg3.c, and I found a patch to fix the race condition:
commit ID 1b2a72050 : [TG3]: Fix tx race condition

I will check the patch, and I will write such a patch for the sh_eth
driver later.

Best regards,
Yoshihiro Shimoda