| Message ID | 1480608517.18162.313.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2016-12-01 at 08:08 -0800, Eric Dumazet wrote: > On Thu, 2016-12-01 at 07:55 -0800, Eric Dumazet wrote: > > > So removing the spinlock is doable, but needs to add a new parameter > > to mlx4_en_fold_software_stats() and call netdev_stats_to_stats64() > > before mlx4_en_fold_software_stats(dev) > > Untested patch would be : > > drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 2 - > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 10 +---- > drivers/net/ethernet/mellanox/mlx4/en_port.c | 24 +++++++++----- > drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 3 + > 4 files changed, 23 insertions(+), 16 deletions(-) The patch is wrong, since priv->port_up could change to false while we are running and using the about to be deleted tx/rx rings. So the only safe thing to do is to remove the _bh suffix. Not worth trying to avoid taking a spinlock in this code.
On Thu, Dec 1, 2016 at 7:36 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Thu, 2016-12-01 at 08:08 -0800, Eric Dumazet wrote: >> On Thu, 2016-12-01 at 07:55 -0800, Eric Dumazet wrote: >> >> > So removing the spinlock is doable, but needs to add a new parameter >> > to mlx4_en_fold_software_stats() and call netdev_stats_to_stats64() >> > before mlx4_en_fold_software_stats(dev) >> >> Untested patch would be : >> >> drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 2 - >> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 10 +---- >> drivers/net/ethernet/mellanox/mlx4/en_port.c | 24 +++++++++----- >> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 3 + >> 4 files changed, 23 insertions(+), 16 deletions(-) > > The patch is wrong, since priv->port_up could change to false while we > are running and using the about to be deleted tx/rx rings. > Right, hence the regression Jesper saw ;). > > So the only safe thing to do is to remove the _bh suffix. > > Not worth trying to avoid taking a spinlock in this code. > Ack.
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c index d9c9f86a30df953fa555934c5406057dcaf28960..676050e352703cebe7fcaa5202a06496f7a5a0df 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c @@ -367,7 +367,7 @@ static void mlx4_en_get_ethtool_stats(struct net_device *dev, spin_lock_bh(&priv->stats_lock); - mlx4_en_fold_software_stats(dev); + mlx4_en_fold_software_stats(dev, NULL); for (i = 0; i < NUM_MAIN_STATS; i++, bitmap_iterator_inc(&it)) if (bitmap_iterator_test(&it)) diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c index 091b904262bc7932d3edf99cf850affb23b9ce6e..6ee9e31e59c392cb88faedf9c541b3bc6d195228 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c @@ -1321,13 +1321,9 @@ static void mlx4_en_tx_timeout(struct net_device *dev) static struct rtnl_link_stats64 * mlx4_en_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats) { - struct mlx4_en_priv *priv = netdev_priv(dev); - - spin_lock_bh(&priv->stats_lock); - mlx4_en_fold_software_stats(dev); netdev_stats_to_stats64(stats, &dev->stats); - spin_unlock_bh(&priv->stats_lock); - + /* Must be called after netdev_stats_to_stats64() */ + mlx4_en_fold_software_stats(dev, stats); return stats; } @@ -1810,7 +1806,7 @@ void mlx4_en_stop_port(struct net_device *dev, int detach) netif_tx_disable(dev); spin_lock_bh(&priv->stats_lock); - mlx4_en_fold_software_stats(dev); + mlx4_en_fold_software_stats(dev, NULL); /* Set port as not active */ priv->port_up = false; spin_unlock_bh(&priv->stats_lock); diff --git a/drivers/net/ethernet/mellanox/mlx4/en_port.c b/drivers/net/ethernet/mellanox/mlx4/en_port.c index 9166d90e732858610b1407fe85cbf6cbe27f5e0b..eea042a18e3cfba62745ece4ca673c2db967b9aa 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_port.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_port.c @@ -147,7 +147,8 @@ static unsigned long en_stats_adder(__be64 *start, __be64 *next, int num) return ret; } -void mlx4_en_fold_software_stats(struct net_device *dev) +void mlx4_en_fold_software_stats(struct net_device *dev, + struct rtnl_link_stats64 *stats) { struct mlx4_en_priv *priv = netdev_priv(dev); struct mlx4_en_dev *mdev = priv->mdev; @@ -165,9 +166,13 @@ void mlx4_en_fold_software_stats(struct net_device *dev) packets += READ_ONCE(ring->packets); bytes += READ_ONCE(ring->bytes); } - dev->stats.rx_packets = packets; - dev->stats.rx_bytes = bytes; - + if (stats) { + stats->rx_packets = packets; + stats->rx_bytes = bytes; + } else { + dev->stats.rx_packets = packets; + dev->stats.rx_bytes = bytes; + } packets = 0; bytes = 0; for (i = 0; i < priv->tx_ring_num[TX]; i++) { @@ -176,8 +181,13 @@ void mlx4_en_fold_software_stats(struct net_device *dev) packets += READ_ONCE(ring->packets); bytes += READ_ONCE(ring->bytes); } - dev->stats.tx_packets = packets; - dev->stats.tx_bytes = bytes; + if (stats) { + stats->tx_packets = packets; + stats->tx_bytes = bytes; + } else { + dev->stats.tx_packets = packets; + dev->stats.tx_bytes = bytes; + } } int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset) @@ -208,7 +218,7 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset) spin_lock_bh(&priv->stats_lock); - mlx4_en_fold_software_stats(dev); + mlx4_en_fold_software_stats(dev, NULL); priv->port_stats.rx_chksum_good = 0; priv->port_stats.rx_chksum_none = 0; diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h index 20a936428f4a44c8ca0a7161855da310f9166b50..92dbb41f425b282e9ab7c8d534f091da0ba661c3 100644 --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h @@ -755,7 +755,8 @@ void mlx4_en_rx_irq(struct mlx4_cq *mcq); int mlx4_SET_MCAST_FLTR(struct mlx4_dev *dev, u8 port, u64 mac, u64 clear, u8 mode); int mlx4_SET_VLAN_FLTR(struct mlx4_dev *dev, struct mlx4_en_priv *priv); -void mlx4_en_fold_software_stats(struct net_device *dev); +void mlx4_en_fold_software_stats(struct net_device *dev, + struct rtnl_link_stats64 *stats); int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset); int mlx4_en_QUERY_PORT(struct mlx4_en_dev *mdev, u8 port);