Patchwork sky2: safer transmit ring cleaning (v4)

login
register
mail settings
Submitter Mike McCormack
Date Jan. 14, 2010, 1:19 p.m.
Message ID <4B4F19EB.5080207@ring3k.org>
Download mbox | patch
Permalink /patch/42880/
State RFC
Delegated to: David Miller
Headers show

Comments

Mike McCormack - Jan. 14, 2010, 1:19 p.m.
Jarek Poplawski wrote:
> On Thu, Jan 14, 2010 at 03:20:09AM -0800, David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Thu, 14 Jan 2010 11:16:36 +0000
>>
>>> So, now I really ;-) agree with David: this needs a proper fix.
>> Now Jarek, do you now see my dirty little secret?
>>
>> When people disagree with me, I just silently sit around waiting for
>> them to eventually change their mind.
>>
>> Isn't it brilliant? -)
> 
> If it were that easy... (it never works for me :-()
> 
> Probably, there is something more... ;-)

Here's what was sitting in my tree...


Subject: [PATCH] sky2: Don't detach device when restarting

Block the tx queue from transmitting when restarting
 rather than trying to take the device offline.

Signed-off-by: Mike McCormack <mikem@ring3k.org>
---
 drivers/net/sky2.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

-- 1.5.6.5 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Breuer - Jan. 14, 2010, 3:43 p.m.
On 1/14/2010 8:19 AM, Mike McCormack wrote:
> Here's what was sitting in my tree...
> ...
>   err_out:
> @@ -1596,6 +1598,8 @@ static inline int tx_inuse(const struct sky2_port *sky2)
>   /* Number of list elements available for next tx */
>   static inline int tx_avail(const struct sky2_port *sky2)
>   {
> +	if (unlikely(!sky2->tx_ring))
> +		return 0;
>   	return sky2->tx_pending - tx_inuse(sky2);
>   }
>    
This hunk (patch) conflicts with the v4 patch Stephen sent out last 
night. He added an smp_mb in front of the return. I'm going to give this 
a go with the smb_mb before the unlikely test.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Breuer - Jan. 14, 2010, 4:46 p.m.
On 1/14/2010 8:19 AM, Mike McCormack wrote:
> Jarek Poplawski wrote:
>    
>> On Thu, Jan 14, 2010 at 03:20:09AM -0800, David Miller wrote:
>>      
>>> From: Jarek Poplawski<jarkao2@gmail.com>
>>> Date: Thu, 14 Jan 2010 11:16:36 +0000
>>>
>>>        
>>>> So, now I really ;-) agree with David: this needs a proper fix.
>>>>          
>>> Now Jarek, do you now see my dirty little secret?
>>>
>>> When people disagree with me, I just silently sit around waiting for
>>> them to eventually change their mind.
>>>
>>> Isn't it brilliant? -)
>>>        
>> If it were that easy... (it never works for me :-()
>>
>> Probably, there is something more... ;-)
>>      
> Here's what was sitting in my tree...
>
>
> Subject: [PATCH] sky2: Don't detach device when restarting
>
> Block the tx queue from transmitting when restarting
>   rather than trying to take the device offline.
>
> Signed-off-by: Mike McCormack<mikem@ring3k.org>
> ---
>   drivers/net/sky2.c |   13 +++++++++----
>   1 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index d76d907..061f6f2 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -1580,6 +1580,8 @@ static int sky2_up(struct net_device *dev)
>   	if (netif_msg_ifup(sky2))
>   		printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
>
> +	netif_start_queue(dev);
> +
>   	return 0;
>
>   err_out:
> @@ -1596,6 +1598,8 @@ static inline int tx_inuse(const struct sky2_port *sky2)
>   /* Number of list elements available for next tx */
>   static inline int tx_avail(const struct sky2_port *sky2)
>   {
> +	if (unlikely(!sky2->tx_ring))
> +		return 0;
>   	return sky2->tx_pending - tx_inuse(sky2);
>   }
>
> @@ -1925,7 +1929,9 @@ static int sky2_down(struct net_device *dev)
>   	sky2_read32(hw, B0_IMSK);
>
>   	synchronize_irq(hw->pdev->irq);
> -	napi_synchronize(&hw->napi);
> +	netif_tx_lock_bh(dev);
> +	napi_disable(&hw->napi);
> +	netif_stop_queue(dev);
>
>   	spin_lock_bh(&sky2->phy_lock);
>   	sky2_phy_power_down(hw, port);
> @@ -1939,6 +1945,8 @@ static int sky2_down(struct net_device *dev)
>   	sky2_rx_clean(sky2);
>
>   	sky2_free_buffers(sky2);
> +	napi_enable(&hw->napi);
> +	netif_tx_unlock_bh(dev);
>
>   	return 0;
>   }
> @@ -3177,9 +3185,6 @@ static void sky2_reset(struct sky2_hw *hw)
>   static void sky2_detach(struct net_device *dev)
>   {
>   	if (netif_running(dev)) {
> -		netif_tx_lock_bh(dev);
> -		netif_device_detach(dev);	/* stop txq */
> -		netif_tx_unlock_bh(dev);
>   		sky2_down(dev);
>   	}
>   }
> -- 1.5.6.5
>    
Ok - no obvious difference with this patch + Stephen's.

I still see:

No reported errors; decent throughput; earlier issues with IRQ resolved.
But... still seeing DHCP DISCOVER/OFFER (no REQUEST/ACK) while under 
load. I can try to sniff this... but given that it's under load, I'd 
probably have to filter out non DHCP stuff and might miss whatever is 
really going on. Again - main point here is that absent these patches I 
don't see this issue. It's also possible that these two patches allow 
the load to be heavier thus actually causing a different problem.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - Jan. 14, 2010, 5:51 p.m.
On Thu, 14 Jan 2010 22:19:39 +0900
Mike McCormack <mikem@ring3k.org> wrote:

>  /* Number of list elements available for next tx */
>  static inline int tx_avail(const struct sky2_port *sky2)
>  {
> +	if (unlikely(!sky2->tx_ring))
> +		return 0;
>  	return sky2->tx_pending - tx_inuse(sky2);
>  }
>

Breaks in detach case

> @@ -1925,7 +1929,9 @@ static int sky2_down(struct net_device *dev)
>  	sky2_read32(hw, B0_IMSK);
>  
>  	synchronize_irq(hw->pdev->irq);
> -	napi_synchronize(&hw->napi);
> +	netif_tx_lock_bh(dev);
> +	napi_disable(&hw->napi);
> +	netif_stop_queue(dev);
>  
>  	spin_lock_bh(&sky2->phy_lock);
>  	sky2_phy_power_down(hw, port);
> @@ -1939,6 +1945,8 @@ static int sky2_down(struct net_device *dev)
>  	sky2_rx_clean(sky2);
>  
>  	sky2_free_buffers(sky2);
> +	napi_enable(&hw->napi);
> +	netif_tx_unlock_bh(dev);
>  
>  	return 0;

Breaks on dual ported boards
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index d76d907..061f6f2 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1580,6 +1580,8 @@  static int sky2_up(struct net_device *dev)
 	if (netif_msg_ifup(sky2))
 		printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
 
+	netif_start_queue(dev);
+
 	return 0;
 
 err_out:
@@ -1596,6 +1598,8 @@  static inline int tx_inuse(const struct sky2_port *sky2)
 /* Number of list elements available for next tx */
 static inline int tx_avail(const struct sky2_port *sky2)
 {
+	if (unlikely(!sky2->tx_ring))
+		return 0;
 	return sky2->tx_pending - tx_inuse(sky2);
 }
 
@@ -1925,7 +1929,9 @@  static int sky2_down(struct net_device *dev)
 	sky2_read32(hw, B0_IMSK);
 
 	synchronize_irq(hw->pdev->irq);
-	napi_synchronize(&hw->napi);
+	netif_tx_lock_bh(dev);
+	napi_disable(&hw->napi);
+	netif_stop_queue(dev);
 
 	spin_lock_bh(&sky2->phy_lock);
 	sky2_phy_power_down(hw, port);
@@ -1939,6 +1945,8 @@  static int sky2_down(struct net_device *dev)
 	sky2_rx_clean(sky2);
 
 	sky2_free_buffers(sky2);
+	napi_enable(&hw->napi);
+	netif_tx_unlock_bh(dev);
 
 	return 0;
 }
@@ -3177,9 +3185,6 @@  static void sky2_reset(struct sky2_hw *hw)
 static void sky2_detach(struct net_device *dev)
 {
 	if (netif_running(dev)) {
-		netif_tx_lock_bh(dev);
-		netif_device_detach(dev);	/* stop txq */
-		netif_tx_unlock_bh(dev);
 		sky2_down(dev);
 	}
 }