diff mbox

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

Message ID 20100603034312.5305.61000.sendpatchset@localhost.localdomain
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Amerigo Wang June 3, 2010, 3:39 a.m. UTC 
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>


---
--
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

Comments

Ben Hutchings June 3, 2010, 12:37 p.m. UTC | #1 
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.
Amerigo Wang June 4, 2010, 1:56 a.m. UTC | #2 
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.
--
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 June 4, 2010, 2:25 p.m. UTC | #3 
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.
Amerigo Wang June 7, 2010, 8:51 a.m. UTC | #4 
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!

--
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
Stanislaw Gruszka June 7, 2010, 11 a.m. UTC | #5 
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
--
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
Amerigo Wang June 7, 2010, 1:15 p.m. UTC | #6 
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. :-/

--
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
Amerigo Wang June 9, 2010, 9:23 a.m. UTC | #7 
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.
--
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
Stanislaw Gruszka June 9, 2010, 10:49 a.m. UTC | #8 
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
--
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
Amerigo Wang June 15, 2010, 8:53 a.m. UTC | #9 
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!
--
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
Stanislaw Gruszka June 15, 2010, 9:39 a.m. UTC | #10 
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
--
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
Amerigo Wang June 17, 2010, 10:54 a.m. UTC | #11 
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?
--
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
Stanislaw Gruszka June 17, 2010, 12:03 p.m. UTC | #12 
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
--
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
Amerigo Wang June 18, 2010, 3:10 a.m. UTC | #13 
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.
--
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
diff mbox

Patch

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,