Skip to contentSkip to conversation

s2io: add dynamic LRO disable support

PATCH Superseded Amerigo Wang 5 participants 11 messages Jun 9, 2010 – Jun 17, 2010

Subsystems (2)
Networking Network drivers

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Amerigo Wang

author
#0 lore raw

PATCH v2 1/2 s2io: add dynamic LRO disable support

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 <[email protected]>
Signed-off-by Neil Horman <[email protected]>
Acked-by Neil Horman <[email protected]>
Reviewed-by Stanislaw Gruszka <[email protected]>
Cc Ramkrishna Vepa <[email protected]>

---

drivers/net/s2io.c +32 -1
index 668327c..e558aa7 100644
@@ -795,7 +795,6 @@ static int init_shared_mem(struct s2io_nic *nic)
 		ring->rx_curr_put_info.ring_len = rx_cfg->num_rxd - 1;
 		ring->nic = nic;
 		ring->ring_no = i;
-		ring->lro = lro_enable;
 
 		blk_cnt = rx_cfg->num_rxd / (rxd_count[nic->rxd_mode] + 1);
 		/*  Allocating all the Rx blocks */

@@ -6684,6 +6683,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 +6729,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,

@@ -7229,6 +7259,7 @@ static int s2io_card_up(struct s2io_nic *sp)
 		struct ring_info *ring = &mac_control->rings[i];
 
 		ring->mtu = dev->mtu;
+		ring->lro = sp->lro;
 		ret = fill_rx_buffers(sp, ring, 1);
 		if (ret) {
 			DBG_PRINT(ERR_DBG, "%s: Out of memory in Open\n",
author
#1 lore raw

PATCH v2 2/2 mlx4: add dynamic LRO disable support

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 <[email protected]>
Signed-off-by Neil Horman <[email protected]>
Acked-by Neil Horman <[email protected]>
Reviewed-by Stanislaw Gruszka <[email protected]>
Cc Ben Hutchings <[email protected]>

---

drivers/net/mlx4/en_ethtool.c +32 -1
index d5afd03..2c77805 100644
@@ -387,6 +387,37 @@ 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;
+
+	if (data & ETH_FLAG_LRO) {
+		if (!(dev->features & NETIF_F_LRO)) {
+			dev->features |= NETIF_F_LRO;
+			changed = 1;
+			mdev->profile.num_lro = min_t(int, num_lro , MLX4_EN_MAX_LRO_DESCRIPTORS);
+		}
+	} else if (dev->features & NETIF_F_LRO) {
+		dev->features &= ~NETIF_F_LRO;
+		changed = 1;
+		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 +446,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,
 };
drivers/net/mlx4/en_main.c +3 -2
index cbabf14..4ef42e2 100644
@@ -70,8 +70,9 @@ MLX4_EN_PARM_INT(rss_xor, 0, "Use XOR hash function for RSS");
 MLX4_EN_PARM_INT(rss_mask, 0xf, "RSS hash type bitmask");
 
 /* Number of LRO sessions per Rx ring (rounded up to a power of two) */
-MLX4_EN_PARM_INT(num_lro, MLX4_EN_MAX_LRO_DESCRIPTORS,
-		 "Number of LRO sessions per ring or disabled (0)");
+unsigned int num_lro = MLX4_EN_MAX_LRO_DESCRIPTORS;
+module_param(num_lro , uint, 0444);
+MODULE_PARM_DESC(num_lro, "Number of LRO sessions per ring or disabled (0)");
 
 /* Priority pausing */
 MLX4_EN_PARM_INT(pfctx, 0, "Priority based Flow Control policy on TX[7:0]."
drivers/net/mlx4/mlx4_en.h +2 -0
index b55e46c..bbac975 100644
@@ -568,4 +568,6 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset);
  * Globals
  */
 extern const struct ethtool_ops mlx4_en_ethtool_ops;
+
+extern unsigned int num_lro;
 #endif
#2 lore raw

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

Quoted 10 lines · Amerigo Wang
drivers/net/mlx4/en_ethtool.c +2 -0
index d5afd03..2c77805 100644
@@ -387,6 +387,37 @@ 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)
+{

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 401

Quoted 10 lines
drivers/net/mlx4/en_ethtool.c +10 -0
+	struct mlx4_en_priv *priv = netdev_priv(dev);
+	struct mlx4_en_dev *mdev = priv->mdev;
+	int rc = 0;
+	int changed = 0;
+
+	if (data & ETH_FLAG_LRO) {
+		if (!(dev->features & NETIF_F_LRO)) {
+			dev->features |= NETIF_F_LRO;
+			changed = 1;
+			mdev->profile.num_lro = min_t(int, num_lro , MLX4_EN_MAX_LRO_DESCRIPTORS);

I do not understand why you override mdev->profile.num_lro in v2 patch. If in Rx patch NETIF_F_LRO flag is used we do not need this IMHO.

Quoted 6 lines
drivers/net/mlx4/en_ethtool.c +6 -0
+		}
+	} else if (dev->features & NETIF_F_LRO) {
+		dev->features &= ~NETIF_F_LRO;
+		changed = 1;
+		mdev->profile.num_lro = 0;
+	}

[snip]

drivers/net/mlx4/en_ethtool.c const struct ethtool_ops mlx4_en_ethtool_ops = { · line 450

Quoted 10 lines

const struct ethtool_ops mlx4_en_ethtool_ops = {

drivers/net/mlx4/en_ethtool.c +1 -1
 	.get_drvinfo = mlx4_en_get_drvinfo,
 	.get_settings = mlx4_en_get_settings,

@@ -415,7 +446,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,
 };

Since we modify .set_flags, please assure we return -EOPNOTSUPP if someone will try to setup ETH_FLAG_NTUPLE and ETH_FLAG_RXHASH.

BTW: seems default ethtool_op_set_flags introduce a bug on many devices regarding ETH_FLAG_RXHASH. I think default should be EOPNOTSUPP, and these few devices that actually support RXHASH should have custom ethtool_ops->set_flags

Stanislaw

author
#3 lore raw

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

Quoted 9 lines · Stanislaw Gruszka
Quoted 8 lines · Amerigo Wang
drivers/net/mlx4/en_ethtool.c +0 -0
index d5afd03..2c77805 100644
@@ -387,6 +387,37 @@ static void mlx4_en_get_ringparam(struct net_device *dev,
  	param->tx_pending = mdev->profile.prof[priv->port].tx_ring_size;
  }
Quoted 15 lines

drivers/net/mlx4/en_ethtool.c static void mlx4_en_get_ringparam(struct net_device *dev, · line 401

Quoted 12 lines
drivers/net/mlx4/en_ethtool.c +12 -0
+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;
+
+	if (data&  ETH_FLAG_LRO) {
+		if (!(dev->features&  NETIF_F_LRO)) {
+			dev->features |= NETIF_F_LRO;
+			changed = 1;
+			mdev->profile.num_lro = min_t(int, num_lro , MLX4_EN_MAX_LRO_DESCRIPTORS);

I do not understand why you override mdev->profile.num_lro in v2 patch. If in Rx patch NETIF_F_LRO flag is used we do not need this IMHO.

Oh, I thought you meant this in our previous disccussion. :-/ I will drop this line.

Quoted 8 lines
Quoted 7 lines
drivers/net/mlx4/en_ethtool.c +6 -0
+		}
+	} else if (dev->features&  NETIF_F_LRO) {
+		dev->features&= ~NETIF_F_LRO;
+		changed = 1;
+		mdev->profile.num_lro = 0;
+	}

[snip]

Quoted 13 lines

drivers/net/mlx4/en_ethtool.c const struct ethtool_ops mlx4_en_ethtool_ops = { · line 450

Quoted 10 lines
  const struct ethtool_ops mlx4_en_ethtool_ops = {
  	.get_drvinfo = mlx4_en_get_drvinfo,
  	.get_settings = mlx4_en_get_settings,
drivers/net/mlx4/en_ethtool.c +1 -1
@@ -415,7 +446,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,
  };

Since we modify .set_flags, please assure we return -EOPNOTSUPP if someone will try to setup ETH_FLAG_NTUPLE and ETH_FLAG_RXHASH.

Yeah, good point!

Quoted 5 lines

BTW: seems default ethtool_op_set_flags introduce a bug on many devices regarding ETH_FLAG_RXHASH. I think default should be EOPNOTSUPP, and these few devices that actually support RXHASH should have custom ethtool_ops->set_flags

Hmm, you mean this?

if (data & ETH_FLAG_RXHASH)
+ if (!ops->set_flags)
+ return -EOPNOTSUPP;
....

#4 lore raw

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

Review

Quoted 11 lines · On Tue, 15 Jun 2010 16:35:35 +0800 Cong Wang <[email protected]> wrote:
Quoted 4 lines

BTW: seems default ethtool_op_set_flags introduce a bug on many devices regarding ETH_FLAG_RXHASH. I think default should be EOPNOTSUPP, and these few devices that actually support RXHASH should have custom ethtool_ops->set_flags

Hmm, you mean this?

if (data & ETH_FLAG_RXHASH)

(quoted diff) +2 -0
+    if (!ops->set_flags)
+        return -EOPNOTSUPP;
....

Not really, but I do not have good idea how patch with fix should looks.

I dislike fact that we setup ->feature that are not in real supported by particular device instead of returning EOPNOTSUPP. This actually include both flags NETIF_F_LRO and NETIF_F_RXHASH.

Perhaps ethtool_op_set_flags should be removed and drivers should use only custom version. In particular seems e1000e and sfc use this function improperly and should have NULL as .set_flags.

I will think more about that and maybe cook some patches.

Stanislaw

author
#5 lore raw

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

Quoted 17 lines · Stanislaw Gruszka

Review

Quoted 11 lines · On Tue, 15 Jun 2010 16:35:35 +0800 Cong Wang<[email protected]> wrote:
Quoted 4 lines

BTW: seems default ethtool_op_set_flags introduce a bug on many devices regarding ETH_FLAG_RXHASH. I think default should be EOPNOTSUPP, and these few devices that actually support RXHASH should have custom ethtool_ops->set_flags

Hmm, you mean this?

if (data& ETH_FLAG_RXHASH)

(quoted diff) +2 -0
+    if (!ops->set_flags)
+        return -EOPNOTSUPP;
....

Not really, but I do not have good idea how patch with fix should looks.

Quoted 7 lines

I dislike fact that we setup ->feature that are not in real supported by particular device instead of returning EOPNOTSUPP. This actually include both flags NETIF_F_LRO and NETIF_F_RXHASH.

Perhaps ethtool_op_set_flags should be removed and drivers should use only custom version. In particular seems e1000e and sfc use this function improperly and should have NULL as .set_flags.

This depends on if what ethtool_op_set_flags() does is common for net drivers.

Quoted 3 lines

I will think more about that and maybe cook some patches.

Yes, please. Cc me when you post patches.

Thanks.

#6 lore raw
In reply to Amerigo Wang

On Wed, 2010-06-09 at 06:05 -0400, Amerigo Wang wrote:
[...]

Quoted 15 lines
drivers/net/s2io.c +15 -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) {

drivers/net/s2io.c static int s2io_ethtool_op_set_tso(struct net_device *dev, u32 data) · line 6710

Quoted 10 lines
drivers/net/s2io.c +10 -0
+		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);

[...]

Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?

Ben.

Signature · 3 lines
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.
#7 lore raw
Quoted 29 lines · Amerigo Wang

[...]

Quoted 25 lines
drivers/net/s2io.c +25 -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);

[...]

Quoted 1 line

Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?

Ben,
Good point. If s2io_card_up() fails the chip will not be accessed, so it's safe but all transmit skbs will be freed without the user knowing the reason for failing to transmit or receive for that matter. The other option is to return with a failure and get the watchdog timer reset the adapter.

Ram

Quoted 7 lines

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.

The information and any attached documents contained in this message may be confidential and/or legally privileged. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, dissemination, or reproduction is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender immediately by return e-mail and destroy all copies of the original message.

author
#8 lore raw
Quoted 29 lines · Ramkrishna Vepa
Quoted 29 lines · Amerigo Wang

[...]

Quoted 25 lines
drivers/net/s2io.c +25 -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);

[...]

Quoted 4 lines
Quoted 1 line

Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?

Ben,
Good point. If s2io_card_up() fails the chip will not be accessed, so it's safe but all transmit skbs will be freed without the user knowing the reason for failing to transmit or receive for that matter. The other option is to return with a failure and get the watchdog timer reset the adapter.

(Sorry for the delay, I was on vacation.)

So it seems the latter option is better?

Thanks.

#9 lore raw
Quoted 38 lines
Quoted 29 lines
Quoted 29 lines · Amerigo Wang

[...]

Quoted 25 lines
drivers/net/s2io.c +25 -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);

[...]

Quoted 3 lines
Quoted 1 line

Is it safe to call s2io_start_all_tx_queue() if s2io_card_up() failed?

Ben,
Good point. If s2io_card_up() fails the chip will not be accessed, so

it's safe but all transmit skbs will be freed without the user knowing the reason for failing to transmit or receive for that matter. The other option is to return with a failure and get the watchdog timer reset the adapter.

Quoted 1 line
Quoted 3 lines

(Sorry for the delay, I was on vacation.)

So it seems the latter option is better?

Yes, this will work.

Thanks,
Ram

The information and any attached documents contained in this message may be confidential and/or legally privileged. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, dissemination, or reproduction is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender immediately by return e-mail and destroy all copies of the original message.

maintainer
#10 lore raw

From: Ramkrishna Vepa <[email protected]>
Date: Tue, 15 Jun 2010 09:07:47 -0700

Quoted 5 lines

Yes, this will work.

Thanks,
Ram

Quoted 7 lines

The information and any attached documents contained in this message may be confidential and/or legally privileged. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, dissemination, or reproduction is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender immediately by return e-mail and destroy all copies of the original message.

Please, your legal notice is 10 times larger than what you're actually saying.

That is rediculious. Even more rediculious is using such a notice for a posting to a public mailing list that is published and duplicated hundreds and hundreds of times to public archive sites and elsewhere.

Your notice has no meaning in this context, so not only is it wasteful and annoying, it's useless.

Please us a gmail or yahoo email account to post to these mailing lists if you can't avoid attaching this legal notice to your postings here.

It's wasting bandwidth and people's time unnecessarily.

Thank you.