[Patch 1/2] s2io: add dynamic LRO disable support

[Patch 1/2] s2io: add dynamic LRO disable support

[Amerigo Wang]

This patch adds dynamic LRO diable support for s2io net driver.

I don't have s2io card, so only did compiling test. Anyone who wants
to test this is more than welcome.

This is based on Neil's initial work.

Signed-off-by: WANG Cong <amwang@redhat.com>
Signed-off-by: Neil Horman <nhorman@redhat.com>
Acked-by: Neil Horman <nhorman@redhat.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>


---
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 668327c..9db5df7 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -6684,6 +6684,35 @@ static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data)
 
 	return 0;
 }
+static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
+{
+	struct s2io_nic *sp = netdev_priv(dev);
+	int rc = 0;
+	int changed = 0;
+
+	if (data & ETH_FLAG_LRO) {
+		if (lro_enable) {
+			if (!(dev->features & NETIF_F_LRO)) {
+				dev->features |= NETIF_F_LRO;
+				changed = 1;
+			}
+		} else
+			rc = -EINVAL;
+	} else if (dev->features & NETIF_F_LRO) {
+		dev->features &= ~NETIF_F_LRO;
+		changed = 1;
+	}
+
+	if (changed && netif_running(dev)) {
+		s2io_stop_all_tx_queue(sp);
+		s2io_card_down(sp);
+		sp->lro = dev->features & NETIF_F_LRO;
+		rc = s2io_card_up(sp);
+		s2io_start_all_tx_queue(sp);
+	}
+
+	return rc;
+}
 
 static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_settings = s2io_ethtool_gset,
@@ -6701,6 +6730,8 @@ static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_rx_csum = s2io_ethtool_get_rx_csum,
 	.set_rx_csum = s2io_ethtool_set_rx_csum,
 	.set_tx_csum = s2io_ethtool_op_set_tx_csum,
+	.set_flags = s2io_ethtool_set_flags,
+	.get_flags = ethtool_op_get_flags,
 	.set_sg = ethtool_op_set_sg,
 	.get_tso = s2io_ethtool_op_get_tso,
 	.set_tso = s2io_ethtool_op_set_tso,

[Patch 2/2] mlx4: add dynamic LRO disable support

[Amerigo Wang]

This patch adds dynamic LRO diable support for mlx4 net driver.
It also fixes a bug of mlx4, which checks NETIF_F_LRO flag in rx
path without rtnl lock.

I don't have mlx4 card, so only did compiling test. Anyone who wants
to test this is more than welcome.

This is based on Neil's initial work too.

Signed-off-by: WANG Cong <amwang@redhat.com>
Signed-off-by: Neil Horman <nhorman@redhat.com>
Acked-by: Neil Horman <nhorman@redhat.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>


---
diff --git a/drivers/net/mlx4/en_ethtool.c b/drivers/net/mlx4/en_ethtool.c
index d5afd03..46cd049 100644
--- a/drivers/net/mlx4/en_ethtool.c
+++ b/drivers/net/mlx4/en_ethtool.c
@@ -387,6 +387,39 @@ static void mlx4_en_get_ringparam(struct net_device *dev,
 	param->tx_pending = mdev->profile.prof[priv->port].tx_ring_size;
 }
 
+static int mlx4_ethtool_op_set_flags(struct net_device *dev, u32 data)
+{
+	struct mlx4_en_priv *priv = netdev_priv(dev);
+	struct mlx4_en_dev *mdev = priv->mdev;
+	int rc = 0;
+	int changed = 0;
+	static int old_num_lro;
+
+	if (data & ETH_FLAG_LRO) {
+		if (!(dev->features & NETIF_F_LRO)) {
+			dev->features |= NETIF_F_LRO;
+			changed = 1;
+			mdev->profile.num_lro = old_num_lro;
+		}
+	} else if (dev->features & NETIF_F_LRO) {
+		dev->features &= ~NETIF_F_LRO;
+		changed = 1;
+		old_num_lro = mdev->profile.num_lro;
+		mdev->profile.num_lro = 0;
+	}
+
+	if (changed && netif_running(dev)) {
+		mutex_lock(&mdev->state_lock);
+		mlx4_en_stop_port(dev);
+		rc = mlx4_en_start_port(dev);
+		if (rc)
+			en_err(priv, "Failed to restart port\n");
+		mutex_unlock(&mdev->state_lock);
+	}
+
+	return rc;
+}
+
 const struct ethtool_ops mlx4_en_ethtool_ops = {
 	.get_drvinfo = mlx4_en_get_drvinfo,
 	.get_settings = mlx4_en_get_settings,
@@ -415,7 +448,7 @@ const struct ethtool_ops mlx4_en_ethtool_ops = {
 	.get_ringparam = mlx4_en_get_ringparam,
 	.set_ringparam = mlx4_en_set_ringparam,
 	.get_flags = ethtool_op_get_flags,
-	.set_flags = ethtool_op_set_flags,
+	.set_flags = mlx4_ethtool_op_set_flags,
 };
 
 
diff --git a/drivers/net/mlx4/en_rx.c b/drivers/net/mlx4/en_rx.c
index 8e2fcb7..10f50ef 100644
--- a/drivers/net/mlx4/en_rx.c
+++ b/drivers/net/mlx4/en_rx.c
@@ -609,7 +609,7 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 				 * - without IP options
 				 * - not an IP fragment */
 				if (mlx4_en_can_lro(cqe->status) &&
-				    dev->features & NETIF_F_LRO) {
+				    priv->mdev->profile.num_lro) {
 
 					nr = mlx4_en_complete_rx_desc(
 						priv, rx_desc,

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Ben Hutchings]

On Wed, 2010-06-02 at 23:39 -0400, Amerigo Wang wrote:
> This patch adds dynamic LRO diable support for mlx4 net driver.
> It also fixes a bug of mlx4, which checks NETIF_F_LRO flag in rx
> path without rtnl lock.
[...]

Is that flag test actually unsafe - and if so, how is testing num_lro
any better?  Perhaps access to net_device::features should be wrapped
with ACCESS_ONCE() to ensure that reads and writes are atomic.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [Patch 1/2] s2io: add dynamic LRO disable support

[Michal Schmidt]

[adding Ram Vepa to CC]

On Wed, 2 Jun 2010 23:38:59 -0400 Amerigo Wang wrote:
> 
> This patch adds dynamic LRO diable support for s2io net driver.
> 
> I don't have s2io card, so only did compiling test. Anyone who wants
> to test this is more than welcome.
> 
> This is based on Neil's initial work.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Signed-off-by: Neil Horman <nhorman@redhat.com>
> Acked-by: Neil Horman <nhorman@redhat.com>
> Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
> 
> 
> ---
> diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
> index 668327c..9db5df7 100644
> --- a/drivers/net/s2io.c
> +++ b/drivers/net/s2io.c
> @@ -6684,6 +6684,35 @@ static int s2io_ethtool_op_set_tso(struct
> net_device *dev, u32 data) 
>  	return 0;
>  }
> +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
> +{
> +	struct s2io_nic *sp = netdev_priv(dev);
> +	int rc = 0;
> +	int changed = 0;
> +
> +	if (data & ETH_FLAG_LRO) {
> +		if (lro_enable) {
> +			if (!(dev->features & NETIF_F_LRO)) {
> +				dev->features |= NETIF_F_LRO;
> +				changed = 1;
> +			}
> +		} else
> +			rc = -EINVAL;
> +	} else if (dev->features & NETIF_F_LRO) {
> +		dev->features &= ~NETIF_F_LRO;
> +		changed = 1;
> +	}
> +
> +	if (changed && netif_running(dev)) {
> +		s2io_stop_all_tx_queue(sp);
> +		s2io_card_down(sp);
> +		sp->lro = dev->features & NETIF_F_LRO;
> +		rc = s2io_card_up(sp);
> +		s2io_start_all_tx_queue(sp);
> +	}
> +
> +	return rc;
> +}
>  
>  static const struct ethtool_ops netdev_ethtool_ops = {
>  	.get_settings = s2io_ethtool_gset,
> @@ -6701,6 +6730,8 @@ static const struct ethtool_ops
> netdev_ethtool_ops = { .get_rx_csum = s2io_ethtool_get_rx_csum,
>  	.set_rx_csum = s2io_ethtool_set_rx_csum,
>  	.set_tx_csum = s2io_ethtool_op_set_tx_csum,
> +	.set_flags = s2io_ethtool_set_flags,
> +	.get_flags = ethtool_op_get_flags,
>  	.set_sg = ethtool_op_set_sg,
>  	.get_tso = s2io_ethtool_op_get_tso,
>  	.set_tso = s2io_ethtool_op_set_tso,
> --
> 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

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Cong Wang]

On 06/03/10 20:37, Ben Hutchings wrote:
> On Wed, 2010-06-02 at 23:39 -0400, Amerigo Wang wrote:
>> This patch adds dynamic LRO diable support for mlx4 net driver.
>> It also fixes a bug of mlx4, which checks NETIF_F_LRO flag in rx
>> path without rtnl lock.
> [...]
>
> Is that flag test actually unsafe - and if so, how is testing num_lro
> any better?  Perhaps access to net_device::features should be wrapped
> with ACCESS_ONCE() to ensure that reads and writes are atomic.
>

At least, I don't find there is any race with 'num_lro', thus
no lock is needed.

Thanks.

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Ben Hutchings]

On Fri, 2010-06-04 at 09:56 +0800, Cong Wang wrote:
> On 06/03/10 20:37, Ben Hutchings wrote:
> > On Wed, 2010-06-02 at 23:39 -0400, Amerigo Wang wrote:
> >> This patch adds dynamic LRO diable support for mlx4 net driver.
> >> It also fixes a bug of mlx4, which checks NETIF_F_LRO flag in rx
> >> path without rtnl lock.
> > [...]
> >
> > Is that flag test actually unsafe - and if so, how is testing num_lro
> > any better?  Perhaps access to net_device::features should be wrapped
> > with ACCESS_ONCE() to ensure that reads and writes are atomic.
> >
> 
> At least, I don't find there is any race with 'num_lro', thus
> no lock is needed.

In both cases there is a race condition but it is harmless so long as
the read and the write are atomic.  There is a general assumption in
networking code that this is the case for int and long.  Personally I
would prefer to see this made explicit using ACCESS_ONCE(), but I don't
see any specific problem in mlx4 (not that I'm familiar with this driver
either).

Now that I look at the patch again, I see you're using a static (i.e.
global) variable to 'back up' the non-zero (enabled) value of num_lro.
This is introducing a bug!  The correct value is apparently set in
mlx4_en_get_profile(); you would need to replicate that.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [Patch 1/2] s2io: add dynamic LRO disable support

[Ramkrishna Vepa]

> +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
> +{
> +	struct s2io_nic *sp = netdev_priv(dev);
> +	int rc = 0;
> +	int changed = 0;
> +
> +	if (data & ETH_FLAG_LRO) {
> +		if (lro_enable) {
> +			if (!(dev->features & NETIF_F_LRO)) {
> +				dev->features |= NETIF_F_LRO;
> +				changed = 1;
> +			}
> +		} else
> +			rc = -EINVAL;
> +	} else if (dev->features & NETIF_F_LRO) {
> +		dev->features &= ~NETIF_F_LRO;
> +		changed = 1;
> +	}
> +
> +	if (changed && netif_running(dev)) {
> +		s2io_stop_all_tx_queue(sp);
> +		s2io_card_down(sp);
> +		sp->lro = dev->features & NETIF_F_LRO;
> +		rc = s2io_card_up(sp);
In s2io_card_up, update ring->lro too as it is used in the fast path -
		struct ring_info *ring = &mac_control->rings[i];
 
 		ring->mtu = dev->mtu;
+		ring->lro = sp->lro;

> +		s2io_start_all_tx_queue(sp);

The following line in init_shared_mem() is redundant and can be removed.
-	ring->lro = lro_enable;

Thanks,
Ram

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Cong Wang]

On 06/04/10 22:25, Ben Hutchings wrote:
> On Fri, 2010-06-04 at 09:56 +0800, Cong Wang wrote:
>> On 06/03/10 20:37, Ben Hutchings wrote:
>>> On Wed, 2010-06-02 at 23:39 -0400, Amerigo Wang wrote:
>>>> This patch adds dynamic LRO diable support for mlx4 net driver.
>>>> It also fixes a bug of mlx4, which checks NETIF_F_LRO flag in rx
>>>> path without rtnl lock.
>>> [...]
>>>
>>> Is that flag test actually unsafe - and if so, how is testing num_lro
>>> any better?  Perhaps access to net_device::features should be wrapped
>>> with ACCESS_ONCE() to ensure that reads and writes are atomic.
>>>
>>
>> At least, I don't find there is any race with 'num_lro', thus
>> no lock is needed.
>
> In both cases there is a race condition but it is harmless so long as
> the read and the write are atomic.  There is a general assumption in
> networking code that this is the case for int and long.  Personally I
> would prefer to see this made explicit using ACCESS_ONCE(), but I don't
> see any specific problem in mlx4 (not that I'm familiar with this driver
> either).

Hmm, right, it seems mlx4_en_add() is async too.
I will pick your suggestion.


>
> Now that I look at the patch again, I see you're using a static (i.e.
> global) variable to 'back up' the non-zero (enabled) value of num_lro.
> This is introducing a bug!  The correct value is apparently set in
> mlx4_en_get_profile(); you would need to replicate that.
>

Oh, probably, but unfortunately 'num_lro' is static so only visible
in en_main.c.

Thanks!

Re: [Patch 1/2] s2io: add dynamic LRO disable support

[Cong Wang]

On 06/05/10 16:53, Ramkrishna Vepa wrote:
>> +static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
>> +{
>> +	struct s2io_nic *sp = netdev_priv(dev);
>> +	int rc = 0;
>> +	int changed = 0;
>> +
>> +	if (data&  ETH_FLAG_LRO) {
>> +		if (lro_enable) {
>> +			if (!(dev->features&  NETIF_F_LRO)) {
>> +				dev->features |= NETIF_F_LRO;
>> +				changed = 1;
>> +			}
>> +		} else
>> +			rc = -EINVAL;
>> +	} else if (dev->features&  NETIF_F_LRO) {
>> +		dev->features&= ~NETIF_F_LRO;
>> +		changed = 1;
>> +	}
>> +
>> +	if (changed&&  netif_running(dev)) {
>> +		s2io_stop_all_tx_queue(sp);
>> +		s2io_card_down(sp);
>> +		sp->lro = dev->features&  NETIF_F_LRO;
>> +		rc = s2io_card_up(sp);
> In s2io_card_up, update ring->lro too as it is used in the fast path -
> 		struct ring_info *ring =&mac_control->rings[i];
>
>   		ring->mtu = dev->mtu;
> +		ring->lro = sp->lro;
>

Yeah, I missed this important part.


>> +		s2io_start_all_tx_queue(sp);
>
> The following line in init_shared_mem() is redundant and can be removed.
> -	ring->lro = lro_enable;
>

Okay.

I will send an updated version soon.

Thanks a lot!

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Stanislaw Gruszka]

On Mon, 07 Jun 2010 16:51:49 +0800
Cong Wang <amwang@redhat.com> wrote:

> > Now that I look at the patch again, I see you're using a static (i.e.
> > global) variable to 'back up' the non-zero (enabled) value of num_lro.
> > This is introducing a bug!  The correct value is apparently set in
> > mlx4_en_get_profile(); you would need to replicate that.
> >
> 
> Oh, probably, but unfortunately 'num_lro' is static so only visible
> in en_main.c.

So just remove "static" and make it global :-)

Stanislaw

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Cong Wang]

On 06/07/10 19:00, Stanislaw Gruszka wrote:
> On Mon, 07 Jun 2010 16:51:49 +0800
> Cong Wang<amwang@redhat.com>  wrote:
>
>>> Now that I look at the patch again, I see you're using a static (i.e.
>>> global) variable to 'back up' the non-zero (enabled) value of num_lro.
>>> This is introducing a bug!  The correct value is apparently set in
>>> mlx4_en_get_profile(); you would need to replicate that.
>>>
>>
>> Oh, probably, but unfortunately 'num_lro' is static so only visible
>> in en_main.c.
>
> So just remove "static" and make it global :-)
>

Not that easy, it is defined with a macro which is also used
by other parameters. :-/

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Cong Wang]

On 06/04/10 22:25, Ben Hutchings wrote:
> On Fri, 2010-06-04 at 09:56 +0800, Cong Wang wrote:
>> On 06/03/10 20:37, Ben Hutchings wrote:
>>> On Wed, 2010-06-02 at 23:39 -0400, Amerigo Wang wrote:
>>>> This patch adds dynamic LRO diable support for mlx4 net driver.
>>>> It also fixes a bug of mlx4, which checks NETIF_F_LRO flag in rx
>>>> path without rtnl lock.
>>> [...]
>>>
>>> Is that flag test actually unsafe - and if so, how is testing num_lro
>>> any better?  Perhaps access to net_device::features should be wrapped
>>> with ACCESS_ONCE() to ensure that reads and writes are atomic.
>>>
>>
>> At least, I don't find there is any race with 'num_lro', thus
>> no lock is needed.
>
> In both cases there is a race condition but it is harmless so long as
> the read and the write are atomic.  There is a general assumption in
> networking code that this is the case for int and long.  Personally I
> would prefer to see this made explicit using ACCESS_ONCE(), but I don't
> see any specific problem in mlx4 (not that I'm familiar with this driver
> either).

I read this email again.

I think you misunderstood the race condition here. Even read and write
are atomic here, the race still exists. One can just set NETIF_F_LRO
asynchronously right before mlx4 check this flag in mlx4_en_process_rx_cq()
which doesn't take rtnl_lock.

Also, I don't think ACCESS_ONCE() can make things atomic here.

Am I missing something?

Thanks.

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Stanislaw Gruszka]

Hi Amerigo

Sorry for being silent in this thread before.

On Wed, Jun 09, 2010 at 05:23:35PM +0800, Cong Wang wrote:
>>>> Is that flag test actually unsafe - and if so, how is testing num_lro
>>>> any better?  Perhaps access to net_device::features should be wrapped
>>>> with ACCESS_ONCE() to ensure that reads and writes are atomic.
>>>>
>>>
>>> At least, I don't find there is any race with 'num_lro', thus
>>> no lock is needed.
>>
>> In both cases there is a race condition but it is harmless so long as
>> the read and the write are atomic.  There is a general assumption in
>> networking code that this is the case for int and long.  Personally I
>> would prefer to see this made explicit using ACCESS_ONCE(), but I don't
>> see any specific problem in mlx4 (not that I'm familiar with this driver
>> either).
>
> I read this email again.
>
> I think you misunderstood the race condition here. Even read and write
> are atomic here, the race still exists. One can just set NETIF_F_LRO
> asynchronously right before mlx4 check this flag in mlx4_en_process_rx_cq()
> which doesn't take rtnl_lock.

If so, it's better to stop device before modify LRO settings. I suggest
something like that in mlx4_ethtool_op_set_flags:

if (!!(data & ETH_FLAG_LRO) != !!(dev->features & NETIF_F_LRO)) {
	/* Need to toggle LRO */

	if (netdev_running(dev)) {
               mutex_lock(&mdev->state_lock);
               mlx4_en_stop_port(dev);
               rc = mlx4_en_start_port(dev);
               if (rc)
                       en_err(priv, "Failed to restart port\n");
	}

	dev->features ^= NETIF_F_LRO;

	if (netdev_running(dev))
               mutex_unlock(&mdev->state_lock);
}

Stanislaw

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Cong Wang]

On 06/09/10 18:49, Stanislaw Gruszka wrote:
> Hi Amerigo
>
> Sorry for being silent in this thread before.
>
> On Wed, Jun 09, 2010 at 05:23:35PM +0800, Cong Wang wrote:
>>>>> Is that flag test actually unsafe - and if so, how is testing num_lro
>>>>> any better?  Perhaps access to net_device::features should be wrapped
>>>>> with ACCESS_ONCE() to ensure that reads and writes are atomic.
>>>>>
>>>>
>>>> At least, I don't find there is any race with 'num_lro', thus
>>>> no lock is needed.
>>>
>>> In both cases there is a race condition but it is harmless so long as
>>> the read and the write are atomic.  There is a general assumption in
>>> networking code that this is the case for int and long.  Personally I
>>> would prefer to see this made explicit using ACCESS_ONCE(), but I don't
>>> see any specific problem in mlx4 (not that I'm familiar with this driver
>>> either).
>>
>> I read this email again.
>>
>> I think you misunderstood the race condition here. Even read and write
>> are atomic here, the race still exists. One can just set NETIF_F_LRO
>> asynchronously right before mlx4 check this flag in mlx4_en_process_rx_cq()
>> which doesn't take rtnl_lock.
>
> If so, it's better to stop device before modify LRO settings. I suggest
> something like that in mlx4_ethtool_op_set_flags:
>
> if (!!(data&  ETH_FLAG_LRO) != !!(dev->features&  NETIF_F_LRO)) {


What does this line mean? This is to ignore all other flags, right?

> 	/* Need to toggle LRO */
>
> 	if (netdev_running(dev)) {
>                 mutex_lock(&mdev->state_lock);
>                 mlx4_en_stop_port(dev);
>                 rc = mlx4_en_start_port(dev);
>                 if (rc)
>                         en_err(priv, "Failed to restart port\n");
> 	}
>
> 	dev->features ^= NETIF_F_LRO;
>
> 	if (netdev_running(dev))
>                 mutex_unlock(&mdev->state_lock);
> }
>

I don't think mdev->state_lock is used to protect dev->feature.
rtnl_lock is. I think switching to mlx4_ethtool_op_set_flags()
from the default one has already solved this.

Thanks!

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Stanislaw Gruszka]

On Tue, 15 Jun 2010 16:53:27 +0800
Cong Wang <amwang@redhat.com> wrote:

> > If so, it's better to stop device before modify LRO settings. I suggest
> > something like that in mlx4_ethtool_op_set_flags:
> >
> > if (!!(data&  ETH_FLAG_LRO) != !!(dev->features&  NETIF_F_LRO)) {
>
> What does this line mean? This is to ignore all other flags, right?

Yes, plus check if we are really changing current settings.
 
> > 	/* Need to toggle LRO */
> >
> > 	if (netdev_running(dev)) {
> >                 mutex_lock(&mdev->state_lock);
> >                 mlx4_en_stop_port(dev);
> >                 rc = mlx4_en_start_port(dev);
> >                 if (rc)
> >                         en_err(priv, "Failed to restart port\n");
> > 	}
> >
> > 	dev->features ^= NETIF_F_LRO;
> >
> > 	if (netdev_running(dev))
> >                 mutex_unlock(&mdev->state_lock);
> > }
> >
> 
> I don't think mdev->state_lock is used to protect dev->feature.
> rtnl_lock is. I think switching to mlx4_ethtool_op_set_flags()
> from the default one has already solved this.

Ahh, you have right, may intention was use it to stop and start
port. Code rather should look like below:

	if (netdev_running(dev)) {
		mutex_lock(&mdev->state_lock);
		mlx4_en_stop_port(dev);
	}

	dev->features ^= NETIF_F_LRO;

	if (netdev_running(dev)) {
		rc = mlx4_en_start_port(dev);
		mutex_unlock(&mdev->state_lock);
 		if (rc)
		en_err(priv, "Failed to restart port\n");
	}

Stanislaw

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Cong Wang]

On 06/15/10 17:39, Stanislaw Gruszka wrote:
> On Tue, 15 Jun 2010 16:53:27 +0800
> Cong Wang<amwang@redhat.com>  wrote:
>
>>> If so, it's better to stop device before modify LRO settings. I suggest
>>> something like that in mlx4_ethtool_op_set_flags:
>>>
>>> if (!!(data&   ETH_FLAG_LRO) != !!(dev->features&   NETIF_F_LRO)) {
>>
>> What does this line mean? This is to ignore all other flags, right?
>
> Yes, plus check if we are really changing current settings.
>
>>> 	/* Need to toggle LRO */
>>>
>>> 	if (netdev_running(dev)) {
>>>                  mutex_lock(&mdev->state_lock);
>>>                  mlx4_en_stop_port(dev);
>>>                  rc = mlx4_en_start_port(dev);
>>>                  if (rc)
>>>                          en_err(priv, "Failed to restart port\n");
>>> 	}
>>>
>>> 	dev->features ^= NETIF_F_LRO;
>>>
>>> 	if (netdev_running(dev))
>>>                  mutex_unlock(&mdev->state_lock);
>>> }
>>>
>>
>> I don't think mdev->state_lock is used to protect dev->feature.
>> rtnl_lock is. I think switching to mlx4_ethtool_op_set_flags()
>> from the default one has already solved this.
>
> Ahh, you have right, may intention was use it to stop and start
> port. Code rather should look like below:
>
> 	if (netdev_running(dev)) {
> 		mutex_lock(&mdev->state_lock);
> 		mlx4_en_stop_port(dev);
> 	}
>
> 	dev->features ^= NETIF_F_LRO;
>
> 	if (netdev_running(dev)) {
> 		rc = mlx4_en_start_port(dev);
> 		mutex_unlock(&mdev->state_lock);
>   		if (rc)
> 		en_err(priv, "Failed to restart port\n");
> 	}
>

Hmm, you mean ->features should be changed after port is stopped?
Why?

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Stanislaw Gruszka]

On Thu, Jun 17, 2010 at 06:54:28PM +0800, Cong Wang wrote:
>>> I don't think mdev->state_lock is used to protect dev->feature.
>>> rtnl_lock is. I think switching to mlx4_ethtool_op_set_flags()
>>> from the default one has already solved this.
>>
>> Ahh, you have right, may intention was use it to stop and start
>> port. Code rather should look like below:
>>
>> 	if (netdev_running(dev)) {
>> 		mutex_lock(&mdev->state_lock);
>> 		mlx4_en_stop_port(dev);
>> 	}
>>
>> 	dev->features ^= NETIF_F_LRO;
>>
>> 	if (netdev_running(dev)) {
>> 		rc = mlx4_en_start_port(dev);
>> 		mutex_unlock(&mdev->state_lock);
>>   		if (rc)
>> 		en_err(priv, "Failed to restart port\n");
>> 	}
>>
>
> Hmm, you mean ->features should be changed after port is stopped?

Actually not ->features variable, but NETIF_F_LRO bit, as only this
bit is used in rx path.

> Why?

For reasons you talked before in this thread :) to do not change
LRO in the middle of receiving packages.

Stanislaw

Re: [Patch 2/2] mlx4: add dynamic LRO disable support

[Cong Wang]

On 06/17/10 20:03, Stanislaw Gruszka wrote:
> On Thu, Jun 17, 2010 at 06:54:28PM +0800, Cong Wang wrote:
>>>> I don't think mdev->state_lock is used to protect dev->feature.
>>>> rtnl_lock is. I think switching to mlx4_ethtool_op_set_flags()
>>>> from the default one has already solved this.
>>>
>>> Ahh, you have right, may intention was use it to stop and start
>>> port. Code rather should look like below:
>>>
>>> 	if (netdev_running(dev)) {
>>> 		mutex_lock(&mdev->state_lock);
>>> 		mlx4_en_stop_port(dev);
>>> 	}
>>>
>>> 	dev->features ^= NETIF_F_LRO;
>>>
>>> 	if (netdev_running(dev)) {
>>> 		rc = mlx4_en_start_port(dev);
>>> 		mutex_unlock(&mdev->state_lock);
>>>    		if (rc)
>>> 		en_err(priv, "Failed to restart port\n");
>>> 	}
>>>
>>
>> Hmm, you mean ->features should be changed after port is stopped?
>
> Actually not ->features variable, but NETIF_F_LRO bit, as only this
> bit is used in rx path.
>

Yeah, this is what I meant.

>> Why?
>
> For reasons you talked before in this thread :) to do not change
> LRO in the middle of receiving packages.
>

Ohh... I missed this, seems reasonable, will fix this in the next update.

Thanks.