Message ID | 20100603034312.5305.61000.sendpatchset@localhost.localdomain |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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.
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
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.
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
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
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
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
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
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
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
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
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
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 --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,