[PATCH 0/4] Pull request for 'ipg-fixes' branch

[PATCH 0/4] Pull request for 'ipg-fixes' branch

[Francois Romieu]

[-- Attachment #1: Type: text/plain, Size: 5177 bytes --]

Please pull from branch 'ipg-fixes' in repository

git://git.kernel.org/pub/scm/linux/kernel/git/romieu/netdev-2.6.git ipg-fixes

to get the changes below.

I have tested the driver with a PIV HT based motherboard. The network
controller is connected to a fast ethernet switch (yeah, I'm cheap).
A second host is performing two loop of scp in both direction for a
400Mb file. The files are sha1sumed after each scp. I have added a
'ping -q -f -l16' from the computer under test after some time.

After 35 copies from the computer under test and 28 copies to it
(the ping eats a bit):
             total       used       free     shared    buffers     cached
Mem:       1019040    1003860      15180          0      20556     936792
-/+ buffers/cache:      46512     972528
Swap:      2031608          0    2031608

Before:
             total       used       free     shared    buffers     cached
Mem:       1019040     572036     447004          0      14988     525924
-/+ buffers/cache:      31124     987916
Swap:      2031608          0    2031608

/proc/slabinfo before and after the test are attached.

The driver is still a POMS but it seems better now.

I will not be available to work further on this issue before sunday.
I'd appreciate being Cced though.

Distance from 'net-2.6/master' (27d1cba21fcc50c37eef5042c6be9fa7135e88fc)
-------------------------------------------------------------------------

286c83ce6e8263a5c4c55a57b4c1040800de0171
d42f3afc953f9c99ffe84667a3ecf0d3b69f3d64
358bf4b8e8cbde5d6411b219e93a61728c892685
a58cceed4464ba8ae94294184c15f43e92a5de89

Diffstat
--------

 drivers/net/ipg.c |   36 ++++++++++++------------------------
 1 files changed, 12 insertions(+), 24 deletions(-)

Shortlog
--------

Francois Romieu (4):
      ipg: balance locking in irq handler
      ipg: plug Tx completion leak
      ipg: fix queue stop condition in the xmit handler
      ipg: fix Tx completion irq request

Patch
-----

diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
index dbd23bb..50f0c17 100644
--- a/drivers/net/ipg.c
+++ b/drivers/net/ipg.c
@@ -857,21 +857,14 @@ static void init_tfdlist(struct net_device *dev)
 static void ipg_nic_txfree(struct net_device *dev)
 {
 	struct ipg_nic_private *sp = netdev_priv(dev);
-	void __iomem *ioaddr = sp->ioaddr;
-	unsigned int curr;
-	u64 txd_map;
-	unsigned int released, pending;
-
-	txd_map = (u64)sp->txd_map;
-	curr = ipg_r32(TFD_LIST_PTR_0) -
-		do_div(txd_map, sizeof(struct ipg_tx)) - 1;
+	unsigned int released, pending, dirty;
 
 	IPG_DEBUG_MSG("_nic_txfree\n");
 
 	pending = sp->tx_current - sp->tx_dirty;
+	dirty = sp->tx_dirty % IPG_TFDLIST_LENGTH;
 
 	for (released = 0; released < pending; released++) {
-		unsigned int dirty = sp->tx_dirty % IPG_TFDLIST_LENGTH;
 		struct sk_buff *skb = sp->TxBuff[dirty];
 		struct ipg_tx *txfd = sp->txd + dirty;
 
@@ -882,11 +875,8 @@ static void ipg_nic_txfree(struct net_device *dev)
 		 * If the TFDDone bit is set, free the associated
 		 * buffer.
 		 */
-		if (dirty == curr)
-			break;
-
-		/* Setup TFDDONE for compatible issue. */
-		txfd->tfc |= cpu_to_le64(IPG_TFC_TFDDONE);
+		if (!(txfd->tfc & cpu_to_le64(IPG_TFC_TFDDONE)))
+                        break;
 
 		/* Free the transmit buffer. */
 		if (skb) {
@@ -898,6 +888,7 @@ static void ipg_nic_txfree(struct net_device *dev)
 
 			sp->TxBuff[dirty] = NULL;
 		}
+		dirty = (dirty + 1) % IPG_TFDLIST_LENGTH;
 	}
 
 	sp->tx_dirty += released;
@@ -1630,6 +1621,8 @@ static irqreturn_t ipg_interrupt_handler(int irq, void *dev_inst)
 #ifdef JUMBO_FRAME
 	ipg_nic_rxrestore(dev);
 #endif
+	spin_lock(&sp->lock);
+
 	/* Get interrupt source information, and acknowledge
 	 * some (i.e. TxDMAComplete, RxDMAComplete, RxEarly,
 	 * IntRequested, MacControlFrame, LinkEvent) interrupts
@@ -1647,9 +1640,7 @@ static irqreturn_t ipg_interrupt_handler(int irq, void *dev_inst)
 	handled = 1;
 
 	if (unlikely(!netif_running(dev)))
-		goto out;
-
-	spin_lock(&sp->lock);
+		goto out_unlock;
 
 	/* If RFDListEnd interrupt, restore all used RFDs. */
 	if (status & IPG_IS_RFD_LIST_END) {
@@ -1733,9 +1724,9 @@ out_enable:
 	ipg_w16(IPG_IE_TX_DMA_COMPLETE | IPG_IE_RX_DMA_COMPLETE |
 		IPG_IE_HOST_ERROR | IPG_IE_INT_REQUESTED | IPG_IE_TX_COMPLETE |
 		IPG_IE_LINK_EVENT | IPG_IE_UPDATE_STATS, INT_ENABLE);
-
+out_unlock:
 	spin_unlock(&sp->lock);
-out:
+
 	return IRQ_RETVAL(handled);
 }
 
@@ -1943,10 +1934,7 @@ static int ipg_nic_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	 */
 	if (sp->tenmbpsmode)
 		txfd->tfc |= cpu_to_le64(IPG_TFC_TXINDICATE);
-	else if (!((sp->tx_current - sp->tx_dirty + 1) >
-	    IPG_FRAMESBETWEENTXDMACOMPLETES)) {
-		txfd->tfc |= cpu_to_le64(IPG_TFC_TXDMAINDICATE);
-	}
+	txfd->tfc |= cpu_to_le64(IPG_TFC_TXDMAINDICATE);
 	/* Based on compilation option, determine if FCS is to be
 	 * appended to transmit frame by IPG.
 	 */
@@ -2003,7 +1991,7 @@ static int ipg_nic_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	ipg_w32(IPG_DC_TX_DMA_POLL_NOW, DMA_CTRL);
 
 	if (sp->tx_current == (sp->tx_dirty + IPG_TFDLIST_LENGTH))
-		netif_wake_queue(dev);
+		netif_stop_queue(dev);
 
 	spin_unlock_irqrestore(&sp->lock, flags);
 
-- 
Ueimor

[-- Attachment #2: slab.tgz --]
[-- Type: application/x-gzip, Size: 9304 bytes --]

[PATCH 1/4] ipg: balance locking in irq handler

[Francois Romieu]

Spotted-by: <linux@horizon.com>
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ipg.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
index dbd23bb..cd1650e 100644
--- a/drivers/net/ipg.c
+++ b/drivers/net/ipg.c
@@ -1630,6 +1630,8 @@ static irqreturn_t ipg_interrupt_handler(int irq, void *dev_inst)
 #ifdef JUMBO_FRAME
 	ipg_nic_rxrestore(dev);
 #endif
+	spin_lock(&sp->lock);
+
 	/* Get interrupt source information, and acknowledge
 	 * some (i.e. TxDMAComplete, RxDMAComplete, RxEarly,
 	 * IntRequested, MacControlFrame, LinkEvent) interrupts
@@ -1647,9 +1649,7 @@ static irqreturn_t ipg_interrupt_handler(int irq, void *dev_inst)
 	handled = 1;
 
 	if (unlikely(!netif_running(dev)))
-		goto out;
-
-	spin_lock(&sp->lock);
+		goto out_unlock;
 
 	/* If RFDListEnd interrupt, restore all used RFDs. */
 	if (status & IPG_IS_RFD_LIST_END) {
@@ -1733,9 +1733,9 @@ out_enable:
 	ipg_w16(IPG_IE_TX_DMA_COMPLETE | IPG_IE_RX_DMA_COMPLETE |
 		IPG_IE_HOST_ERROR | IPG_IE_INT_REQUESTED | IPG_IE_TX_COMPLETE |
 		IPG_IE_LINK_EVENT | IPG_IE_UPDATE_STATS, INT_ENABLE);
-
+out_unlock:
 	spin_unlock(&sp->lock);
-out:
+
 	return IRQ_RETVAL(handled);
 }
 
-- 
1.5.3.3

[PATCH 2/4] ipg: plug Tx completion leak

[Francois Romieu]

The Tx skb release could not free more than one skb per call.
Add it to the fact that the xmit handler does not check for
a queue full condition and you have a recipe to leak quickly.

Let's release every pending Tx descriptor which has been given
back to the host CPU by the network controller. The xmit handler
suggests that it is done through the IPG_TFC_TFDDONE bit.

Remove the former "curr" computing: it does not produce anything
usable in its current form.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ipg.c |   19 +++++--------------
 1 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
index cd1650e..9752902 100644
--- a/drivers/net/ipg.c
+++ b/drivers/net/ipg.c
@@ -857,21 +857,14 @@ static void init_tfdlist(struct net_device *dev)
 static void ipg_nic_txfree(struct net_device *dev)
 {
 	struct ipg_nic_private *sp = netdev_priv(dev);
-	void __iomem *ioaddr = sp->ioaddr;
-	unsigned int curr;
-	u64 txd_map;
-	unsigned int released, pending;
-
-	txd_map = (u64)sp->txd_map;
-	curr = ipg_r32(TFD_LIST_PTR_0) -
-		do_div(txd_map, sizeof(struct ipg_tx)) - 1;
+	unsigned int released, pending, dirty;
 
 	IPG_DEBUG_MSG("_nic_txfree\n");
 
 	pending = sp->tx_current - sp->tx_dirty;
+	dirty = sp->tx_dirty % IPG_TFDLIST_LENGTH;
 
 	for (released = 0; released < pending; released++) {
-		unsigned int dirty = sp->tx_dirty % IPG_TFDLIST_LENGTH;
 		struct sk_buff *skb = sp->TxBuff[dirty];
 		struct ipg_tx *txfd = sp->txd + dirty;
 
@@ -882,11 +875,8 @@ static void ipg_nic_txfree(struct net_device *dev)
 		 * If the TFDDone bit is set, free the associated
 		 * buffer.
 		 */
-		if (dirty == curr)
-			break;
-
-		/* Setup TFDDONE for compatible issue. */
-		txfd->tfc |= cpu_to_le64(IPG_TFC_TFDDONE);
+		if (!(txfd->tfc & cpu_to_le64(IPG_TFC_TFDDONE)))
+                        break;
 
 		/* Free the transmit buffer. */
 		if (skb) {
@@ -898,6 +888,7 @@ static void ipg_nic_txfree(struct net_device *dev)
 
 			sp->TxBuff[dirty] = NULL;
 		}
+		dirty = (dirty + 1) % IPG_TFDLIST_LENGTH;
 	}
 
 	sp->tx_dirty += released;
-- 
1.5.3.3

[PATCH 3/4] ipg: fix queue stop condition in the xmit handler

[Francois Romieu]

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ipg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
index 9752902..b234b29 100644
--- a/drivers/net/ipg.c
+++ b/drivers/net/ipg.c
@@ -1994,7 +1994,7 @@ static int ipg_nic_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	ipg_w32(IPG_DC_TX_DMA_POLL_NOW, DMA_CTRL);
 
 	if (sp->tx_current == (sp->tx_dirty + IPG_TFDLIST_LENGTH))
-		netif_wake_queue(dev);
+		netif_stop_queue(dev);
 
 	spin_unlock_irqrestore(&sp->lock, flags);
 
-- 
1.5.3.3

[PATCH 4/4] ipg: fix Tx completion irq request

[Francois Romieu]

The current logic will only request an ack for the first pending
packet. No irq is triggered as soon as the CPU submits a few
packets a bit quickly.  Let's request an irq for every packet
instead.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ipg.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
index b234b29..50f0c17 100644
--- a/drivers/net/ipg.c
+++ b/drivers/net/ipg.c
@@ -1934,10 +1934,7 @@ static int ipg_nic_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	 */
 	if (sp->tenmbpsmode)
 		txfd->tfc |= cpu_to_le64(IPG_TFC_TXINDICATE);
-	else if (!((sp->tx_current - sp->tx_dirty + 1) >
-	    IPG_FRAMESBETWEENTXDMACOMPLETES)) {
-		txfd->tfc |= cpu_to_le64(IPG_TFC_TXDMAINDICATE);
-	}
+	txfd->tfc |= cpu_to_le64(IPG_TFC_TXDMAINDICATE);
 	/* Based on compilation option, determine if FCS is to be
 	 * appended to transmit frame by IPG.
 	 */
-- 
1.5.3.3

Re: [PATCH 0/4] Pull request for 'ipg-fixes' branch

[linux]

Thank you very much, this appears to work.

> The driver is still a POMS but it seems better now.

I notice that the vendor-supplied driver doesn't have these bugs.
Now, it does have a bug in that it doesn't have an "is this
interrupt for me?" test at all (and always returns "I handled it"),
but the bypass and its locking screwups are a later addition.

The same with the sp->rx_current bugs.  The original loop which used
rx_current as the loop iteration variable wasn't great style, precisely
because it hides the interaction that someone's "optimization" broke,
but I don't want to blame the vendor for things they didn't do.

Would you be interested in some cleanup patches?  In particular, I think I
can get rid of tx->lock entirely, or at least take it off the fast path.
All it's protecting is the write to sp->tx_current, and a few judicious
memory barriers can deal with that.


(Oh, another BUG: the sp->ResetCurrentTFD logic in hard_start_xmit is
just plain broken.  It writes the new data to entry 0, then increments
sp->tx_current just like usual.  THAT isn't in the vendor driver that
I see, either.)

Re: [PATCH 0/4] Pull request for 'ipg-fixes' branch

[Francois Romieu]

linux@horizon.com <linux@horizon.com> :
[...]
> I notice that the vendor-supplied driver doesn't have these bugs.

The M in POMS stands for "my".

[...]
> Would you be interested in some cleanup patches ?

Yes.

> In particular, I think I can get rid of tx->lock entirely, or at least
> take it off the fast path. All it's protecting is the write to
> sp->tx_current, and a few judicious memory barriers can deal with that.

I have done a kind of memory barrier trick for the r8169 in the past but
it is not clear that I would do it again. Today I would argue more strongly
in direction of similar locking amongst different drivers. The tg3 driver
is a good model imho.

Anyway you have been here for some time so I see no reason to kill any
different/new locking scheme you could come with.

Off until sunday.

-- 
Ueimor

Re: [PATCH 0/4] Pull request for 'ipg-fixes' branch

[Jeff Garzik]

Francois Romieu wrote:
> Please pull from branch 'ipg-fixes' in repository
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/romieu/netdev-2.6.git ipg-fixes
> 
> to get the changes below.
> 
> I have tested the driver with a PIV HT based motherboard. The network
> controller is connected to a fast ethernet switch (yeah, I'm cheap).
> A second host is performing two loop of scp in both direction for a
> 400Mb file. The files are sha1sumed after each scp. I have added a
> 'ping -q -f -l16' from the computer under test after some time.
> 
> After 35 copies from the computer under test and 28 copies to it
> (the ping eats a bit):
>              total       used       free     shared    buffers     cached
> Mem:       1019040    1003860      15180          0      20556     936792
> -/+ buffers/cache:      46512     972528
> Swap:      2031608          0    2031608
> 
> Before:
>              total       used       free     shared    buffers     cached
> Mem:       1019040     572036     447004          0      14988     525924
> -/+ buffers/cache:      31124     987916
> Swap:      2031608          0    2031608
> 
> /proc/slabinfo before and after the test are attached.
> 
> The driver is still a POMS but it seems better now.
> 
> I will not be available to work further on this issue before sunday.
> I'd appreciate being Cced though.
> 
> Distance from 'net-2.6/master' (27d1cba21fcc50c37eef5042c6be9fa7135e88fc)
> -------------------------------------------------------------------------
> 
> 286c83ce6e8263a5c4c55a57b4c1040800de0171
> d42f3afc953f9c99ffe84667a3ecf0d3b69f3d64
> 358bf4b8e8cbde5d6411b219e93a61728c892685
> a58cceed4464ba8ae94294184c15f43e92a5de89
> 
> Diffstat
> --------
> 
>  drivers/net/ipg.c |   36 ++++++++++++------------------------
>  1 files changed, 12 insertions(+), 24 deletions(-)

hrm... tried to pull this, but received non-ipg stuff too

Re: [PATCH 0/4] Pull request for 'ipg-fixes' branch

[Francois Romieu]

Jeff Garzik <jeff@garzik.org> :
> Francois Romieu wrote:
[...]
>> Distance from 'net-2.6/master' (27d1cba21fcc50c37eef5042c6be9fa7135e88fc)
>> -------------------------------------------------------------------------
[...]
> hrm... tried to pull this, but received non-ipg stuff too

It is based on one of davem's branches (see the 'distance' line above).

Do you want a branch against your upstream-fixes ?

-- 
Ueimor

Re: [PATCH 0/4] Pull request for 'ipg-fixes' branch

[Francois Romieu]

Francois Romieu <romieu@fr.zoreil.com> :
> Jeff Garzik <jeff@garzik.org> :
> > Francois Romieu wrote:
> [...]
> >> Distance from 'net-2.6/master' (27d1cba21fcc50c37eef5042c6be9fa7135e88fc)
> >> -------------------------------------------------------------------------
> [...]
> > hrm... tried to pull this, but received non-ipg stuff too
> 
> It is based on one of davem's branches (see the 'distance' line above).
> 
> Do you want a branch against your upstream-fixes ?

It is available at:

git://git.kernel.org/pub/scm/linux/kernel/git/romieu/netdev-2.6.git upstream-jeff

Distance from 'upstream-linus' (cb8da8a38015ded1df319a39b7298e69f89036ac)
-------------------------------------------------------------------------

f1f443e6276aae3862a970b141102c51f5e25e4b
b76c7331afe11544063d2c3390de8c9a7ba6db83
98ae57a0dada63ab9b811e89a4b70ca4e4da7497
184c6cf5e4b8e659095fd9e2b54ef37f131b5810

It is up to you and davem to decide who wants to pull it.

-- 
Ueimor