Patchwork [net-next,4/7] net/mlx4_en: Set carrier to off when a port is stopped

login
register
mail settings
Submitter Amir Vadai
Date Jan. 16, 2013, 3:42 p.m.
Message ID <1358350935-3281-5-git-send-email-amirv@mellanox.com>
Download mbox | patch
Permalink /patch/212565/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Amir Vadai - Jan. 16, 2013, 3:42 p.m.
From: Eugenia Emantayev <eugenia@mellanox.com>

Under heavy CPU load changing ring size/mtu/etc. could result in transmit
timeout, since stop-start port might take more than 10 sec. Set
netif_carrier_off to prevent tx queue transmit timeout.

Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Ben Hutchings - Jan. 16, 2013, 4 p.m.
On Wed, 2013-01-16 at 17:42 +0200, Amir Vadai wrote:
> From: Eugenia Emantayev <eugenia@mellanox.com>
> 
> Under heavy CPU load changing ring size/mtu/etc. could result in transmit
> timeout, since stop-start port might take more than 10 sec. Set
> netif_carrier_off to prevent tx queue transmit timeout.

A spurious link change can restart L3 auto-configuration (DHCP, SLAAC,
etc.)  netif_device_detach() also inhibits the watchdog and doesn't have
that problem.

Ben.

> Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 805e242..108c4cf 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -1212,6 +1212,7 @@ void mlx4_en_stop_port(struct net_device *dev)
>  
>  	/* Synchronize with tx routine */
>  	netif_tx_lock_bh(dev);
> +	netif_carrier_off(dev);
>  	netif_tx_stop_all_queues(dev);
>  	netif_tx_unlock_bh(dev);
>
Amir Vadai - Jan. 17, 2013, 7:04 a.m.
On 16/01/2013 18:00, Ben Hutchings wrote:
> On Wed, 2013-01-16 at 17:42 +0200, Amir Vadai wrote:
>> >From: Eugenia Emantayev<eugenia@mellanox.com>
>> >
>> >Under heavy CPU load changing ring size/mtu/etc. could result in transmit
>> >timeout, since stop-start port might take more than 10 sec. Set
>> >netif_carrier_off to prevent tx queue transmit timeout.
> A spurious link change can restart L3 auto-configuration (DHCP, SLAAC,
> etc.)  netif_device_detach() also inhibits the watchdog and doesn't have
> that problem.
>
> Ben.
>

Thanks, Ben.

I will change it to netif_device_attach().
Since this will need some testing, will resend the patchset without this 
patch for now.

Amir.
--
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
Amir Vadai - Jan. 28, 2013, 12:07 p.m.
On 16/01/2013 18:00, Ben Hutchings wrote:
> On Wed, 2013-01-16 at 17:42 +0200, Amir Vadai wrote:
>> From: Eugenia Emantayev <eugenia@mellanox.com>
>>
>> Under heavy CPU load changing ring size/mtu/etc. could result in transmit
>> timeout, since stop-start port might take more than 10 sec. Set
>> netif_carrier_off to prevent tx queue transmit timeout.
>
> A spurious link change can restart L3 auto-configuration (DHCP, SLAAC,
> etc.)  netif_device_detach() also inhibits the watchdog and doesn't have
> that problem.
>
> Ben.
>

But after calling netif_device_detach(), device present bit is cleared 
and therefore won't be able to access the device. This means, that after 
'ifconfig down', won't be able to do 'ifconfig up'.

Amir


>> Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> index 805e242..108c4cf 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> @@ -1212,6 +1212,7 @@ void mlx4_en_stop_port(struct net_device *dev)
>>
>>   	/* Synchronize with tx routine */
>>   	netif_tx_lock_bh(dev);
>> +	netif_carrier_off(dev);
>>   	netif_tx_stop_all_queues(dev);
>>   	netif_tx_unlock_bh(dev);
>>
>

--
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
Ben Hutchings - Jan. 28, 2013, 12:53 p.m.
On Mon, 2013-01-28 at 14:07 +0200, Amir Vadai wrote:
> On 16/01/2013 18:00, Ben Hutchings wrote:
> > On Wed, 2013-01-16 at 17:42 +0200, Amir Vadai wrote:
> >> From: Eugenia Emantayev <eugenia@mellanox.com>
> >>
> >> Under heavy CPU load changing ring size/mtu/etc. could result in transmit
> >> timeout, since stop-start port might take more than 10 sec. Set
> >> netif_carrier_off to prevent tx queue transmit timeout.
> >
> > A spurious link change can restart L3 auto-configuration (DHCP, SLAAC,
> > etc.)  netif_device_detach() also inhibits the watchdog and doesn't have
> > that problem.
> >
> > Ben.
> >
> 
> But after calling netif_device_detach(), device present bit is cleared 
> and therefore won't be able to access the device. This means, that after 
> 'ifconfig down', won't be able to do 'ifconfig up'.

Indeed, so don't do it in ndo_stop!  Your commit message referred to
changing ring size and MTU.

Ben.

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 805e242..108c4cf 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1212,6 +1212,7 @@  void mlx4_en_stop_port(struct net_device *dev)
 
 	/* Synchronize with tx routine */
 	netif_tx_lock_bh(dev);
+	netif_carrier_off(dev);
 	netif_tx_stop_all_queues(dev);
 	netif_tx_unlock_bh(dev);