[PATCH net 0/4] net/mlx4_en: fix stats

[PATCH net 0/4] net/mlx4_en: fix stats

[Eric Dumazet]

mlx4 has various bugs in its ndo_get_stats() and related functions.
This patch series address the obvious issues.
Remaining ones will be discussed later.

Eric Dumazet (4):
  net/mlx4_en: fix tx_dropped bug
  net/mlx4_en: clear some TX ring stats in mlx4_en_clear_stats()
  net/mlx4_en: get rid of ret_stats
  net/mlx4_en: get rid of private net_device_stats

 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  | 17 +++++++++++------
 drivers/net/ethernet/mellanox/mlx4/en_port.c    | 18 ++++++------------
 drivers/net/ethernet/mellanox/mlx4/en_tx.c      |  8 ++++----
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  3 +--
 5 files changed, 23 insertions(+), 25 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH net 1/4] net/mlx4_en: fix tx_dropped bug

[Eric Dumazet]

1) mlx4_en_xmit() can increment priv->stats.tx_dropped, but this variable
is overwritten in mlx4_en_DUMP_ETH_STATS().

2) This increment was not SMP safe, as a port might have many TX queues.

Add a per TX ring tx_dropped to fix these issues.

This is u32 as mlx4_en_DUMP_ETH_STATS() will add a 32bit field.

So lets avoid bugs with SNMP agents having to cope with partial
overwraps. (One of these agents being bond_fold_stats())

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Willem de Bruijn <willemb@google.com>
Cc: Eugenia Emantayev <eugenia@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 1 +
 drivers/net/ethernet/mellanox/mlx4/en_port.c   | 4 +++-
 drivers/net/ethernet/mellanox/mlx4/en_tx.c     | 8 ++++----
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   | 1 +
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 92e0624f4cf0..cfd50206f7c3 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1892,6 +1892,7 @@ static void mlx4_en_clear_stats(struct net_device *dev)
 		priv->tx_ring[i]->bytes = 0;
 		priv->tx_ring[i]->packets = 0;
 		priv->tx_ring[i]->tx_csum = 0;
+		priv->tx_ring[i]->tx_dropped = 0;
 	}
 	for (i = 0; i < priv->rx_ring_num; i++) {
 		priv->rx_ring[i]->bytes = 0;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_port.c b/drivers/net/ethernet/mellanox/mlx4/en_port.c
index 20b6c2e678b8..3df8690154b1 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_port.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_port.c
@@ -188,6 +188,7 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset)
 	}
 	stats->tx_packets = 0;
 	stats->tx_bytes = 0;
+	stats->tx_dropped = 0;
 	priv->port_stats.tx_chksum_offload = 0;
 	priv->port_stats.queue_stopped = 0;
 	priv->port_stats.wake_queue = 0;
@@ -199,6 +200,7 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset)
 
 		stats->tx_packets += ring->packets;
 		stats->tx_bytes += ring->bytes;
+		stats->tx_dropped += ring->tx_dropped;
 		priv->port_stats.tx_chksum_offload += ring->tx_csum;
 		priv->port_stats.queue_stopped     += ring->queue_stopped;
 		priv->port_stats.wake_queue        += ring->wake_queue;
@@ -251,7 +253,7 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset)
 	stats->tx_fifo_errors = 0;
 	stats->tx_heartbeat_errors = 0;
 	stats->tx_window_errors = 0;
-	stats->tx_dropped = be32_to_cpu(mlx4_en_stats->TDROP);
+	stats->tx_dropped += be32_to_cpu(mlx4_en_stats->TDROP);
 
 	/* RX stats */
 	priv->pkstats.rx_multicast_packets = stats->multicast;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index f6e61570cb2c..76aa4d27183c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -726,12 +726,12 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 	bool inline_ok;
 	u32 ring_cons;
 
-	if (!priv->port_up)
-		goto tx_drop;
-
 	tx_ind = skb_get_queue_mapping(skb);
 	ring = priv->tx_ring[tx_ind];
 
+	if (!priv->port_up)
+		goto tx_drop;
+
 	/* fetch ring->cons far ahead before needing it to avoid stall */
 	ring_cons = ACCESS_ONCE(ring->cons);
 
@@ -1030,7 +1030,7 @@ tx_drop_unmap:
 
 tx_drop:
 	dev_kfree_skb_any(skb);
-	priv->stats.tx_dropped++;
+	ring->tx_dropped++;
 	return NETDEV_TX_OK;
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index cc84e09f324a..9a9124031fc7 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -270,6 +270,7 @@ struct mlx4_en_tx_ring {
 	unsigned long		tx_csum;
 	unsigned long		tso_packets;
 	unsigned long		xmit_more;
+	unsigned int		tx_dropped;
 	struct mlx4_bf		bf;
 	unsigned long		queue_stopped;
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH net 2/4] net/mlx4_en: clear some TX ring stats in mlx4_en_clear_stats()

[Eric Dumazet]

mlx4_en_clear_stats() clears about everything but few TX ring
fields are missing :
- queue_stopped, wake_queue, tso_packets, xmit_more

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Eugenia Emantayev <eugenia@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index cfd50206f7c3..bd637c4eff13 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1893,6 +1893,10 @@ static void mlx4_en_clear_stats(struct net_device *dev)
 		priv->tx_ring[i]->packets = 0;
 		priv->tx_ring[i]->tx_csum = 0;
 		priv->tx_ring[i]->tx_dropped = 0;
+		priv->tx_ring[i]->queue_stopped = 0;
+		priv->tx_ring[i]->wake_queue = 0;
+		priv->tx_ring[i]->tso_packets = 0;
+		priv->tx_ring[i]->xmit_more = 0;
 	}
 	for (i = 0; i < priv->rx_ring_num; i++) {
 		priv->rx_ring[i]->bytes = 0;
-- 
2.8.0.rc3.226.g39d4020

[PATCH net 3/4] net/mlx4_en: get rid of ret_stats

[Eric Dumazet]

mlx4 uses a private struct net_device_stats in a vain attempt
to avoid races.

This is buggy because multiple cpus could call mlx4_en_get_stats()
at the same time, so ret_stats can not guarantee stable results.

To fix this, we need to switch to ndo_get_stats64() as this
method provides per-thread storage.

This allows to reduce mlx4_en_priv bloat.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Eugenia Emantayev <eugenia@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 11 ++++++-----
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   |  1 -
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index bd637c4eff13..a4fc6e966505 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1296,15 +1296,16 @@ static void mlx4_en_tx_timeout(struct net_device *dev)
 }
 
 
-static struct net_device_stats *mlx4_en_get_stats(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);
-	memcpy(&priv->ret_stats, &priv->stats, sizeof(priv->stats));
+	netdev_stats_to_stats64(stats, &priv->stats);
 	spin_unlock_bh(&priv->stats_lock);
 
-	return &priv->ret_stats;
+	return stats;
 }
 
 static void mlx4_en_set_default_moderation(struct mlx4_en_priv *priv)
@@ -2487,7 +2488,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
 	.ndo_stop		= mlx4_en_close,
 	.ndo_start_xmit		= mlx4_en_xmit,
 	.ndo_select_queue	= mlx4_en_select_queue,
-	.ndo_get_stats		= mlx4_en_get_stats,
+	.ndo_get_stats64	= mlx4_en_get_stats64,
 	.ndo_set_rx_mode	= mlx4_en_set_rx_mode,
 	.ndo_set_mac_address	= mlx4_en_set_mac,
 	.ndo_validate_addr	= eth_validate_addr,
@@ -2519,7 +2520,7 @@ static const struct net_device_ops mlx4_netdev_ops_master = {
 	.ndo_stop		= mlx4_en_close,
 	.ndo_start_xmit		= mlx4_en_xmit,
 	.ndo_select_queue	= mlx4_en_select_queue,
-	.ndo_get_stats		= mlx4_en_get_stats,
+	.ndo_get_stats64	= mlx4_en_get_stats64,
 	.ndo_set_rx_mode	= mlx4_en_set_rx_mode,
 	.ndo_set_mac_address	= mlx4_en_set_mac,
 	.ndo_validate_addr	= eth_validate_addr,
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index 9a9124031fc7..bfda84df8aae 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -484,7 +484,6 @@ struct mlx4_en_priv {
 	struct net_device *dev;
 	unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
 	struct net_device_stats stats;
-	struct net_device_stats ret_stats;
 	struct mlx4_en_port_state port_state;
 	spinlock_t stats_lock;
 	struct ethtool_flow_id ethtool_rules[MAX_NUM_OF_FS_RULES];
-- 
2.8.0.rc3.226.g39d4020

[PATCH net 4/4] net/mlx4_en: get rid of private net_device_stats

[Eric Dumazet]

We simply can use the standard net_device stats.

We do not need to clear fields that are already 0.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Eugenia Emantayev <eugenia@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  3 +--
 drivers/net/ethernet/mellanox/mlx4/en_port.c    | 14 +++-----------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  1 -
 4 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index c761194bb323..fc95affaf76b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -362,7 +362,7 @@ static void mlx4_en_get_ethtool_stats(struct net_device *dev,
 
 	for (i = 0; i < NUM_MAIN_STATS; i++, bitmap_iterator_inc(&it))
 		if (bitmap_iterator_test(&it))
-			data[index++] = ((unsigned long *)&priv->stats)[i];
+			data[index++] = ((unsigned long *)&dev->stats)[i];
 
 	for (i = 0; i < NUM_PORT_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 a4fc6e966505..19ceced6736c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1302,7 +1302,7 @@ 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);
-	netdev_stats_to_stats64(stats, &priv->stats);
+	netdev_stats_to_stats64(stats, &dev->stats);
 	spin_unlock_bh(&priv->stats_lock);
 
 	return stats;
@@ -1877,7 +1877,6 @@ static void mlx4_en_clear_stats(struct net_device *dev)
 	if (mlx4_en_DUMP_ETH_STATS(mdev, priv->port, 1))
 		en_dbg(HW, priv, "Failed dumping statistics\n");
 
-	memset(&priv->stats, 0, sizeof(priv->stats));
 	memset(&priv->pstats, 0, sizeof(priv->pstats));
 	memset(&priv->pkstats, 0, sizeof(priv->pkstats));
 	memset(&priv->port_stats, 0, sizeof(priv->port_stats));
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_port.c b/drivers/net/ethernet/mellanox/mlx4/en_port.c
index 3df8690154b1..5aa8b751f417 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_port.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_port.c
@@ -152,8 +152,9 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset)
 	struct mlx4_counter tmp_counter_stats;
 	struct mlx4_en_stat_out_mbox *mlx4_en_stats;
 	struct mlx4_en_stat_out_flow_control_mbox *flowstats;
-	struct mlx4_en_priv *priv = netdev_priv(mdev->pndev[port]);
-	struct net_device_stats *stats = &priv->stats;
+	struct net_device *dev = mdev->pndev[port];
+	struct mlx4_en_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
 	struct mlx4_cmd_mailbox *mailbox;
 	u64 in_mod = reset << 8 | port;
 	int err;
@@ -239,20 +240,11 @@ int mlx4_en_DUMP_ETH_STATS(struct mlx4_en_dev *mdev, u8 port, u8 reset)
 	stats->multicast = en_stats_adder(&mlx4_en_stats->MCAST_prio_0,
 					  &mlx4_en_stats->MCAST_prio_1,
 					  NUM_PRIORITIES);
-	stats->collisions = 0;
 	stats->rx_dropped = be32_to_cpu(mlx4_en_stats->RDROP) +
 			    sw_rx_dropped;
 	stats->rx_length_errors = be32_to_cpu(mlx4_en_stats->RdropLength);
-	stats->rx_over_errors = 0;
 	stats->rx_crc_errors = be32_to_cpu(mlx4_en_stats->RCRC);
-	stats->rx_frame_errors = 0;
 	stats->rx_fifo_errors = be32_to_cpu(mlx4_en_stats->RdropOvflw);
-	stats->rx_missed_errors = 0;
-	stats->tx_aborted_errors = 0;
-	stats->tx_carrier_errors = 0;
-	stats->tx_fifo_errors = 0;
-	stats->tx_heartbeat_errors = 0;
-	stats->tx_window_errors = 0;
 	stats->tx_dropped += be32_to_cpu(mlx4_en_stats->TDROP);
 
 	/* RX stats */
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index bfda84df8aae..467d47ed2c39 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -483,7 +483,6 @@ struct mlx4_en_priv {
 	struct mlx4_en_port_profile *prof;
 	struct net_device *dev;
 	unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
-	struct net_device_stats stats;
 	struct mlx4_en_port_state port_state;
 	spinlock_t stats_lock;
 	struct ethtool_flow_id ethtool_rules[MAX_NUM_OF_FS_RULES];
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH net 1/4] net/mlx4_en: fix tx_dropped bug

[Alexei Starovoitov]

On Wed, May 25, 2016 at 09:50:36AM -0700, Eric Dumazet wrote:
> 1) mlx4_en_xmit() can increment priv->stats.tx_dropped, but this variable
> is overwritten in mlx4_en_DUMP_ETH_STATS().
> 
> 2) This increment was not SMP safe, as a port might have many TX queues.
> 
> Add a per TX ring tx_dropped to fix these issues.
> 
> This is u32 as mlx4_en_DUMP_ETH_STATS() will add a 32bit field.
> 
> So lets avoid bugs with SNMP agents having to cope with partial
> overwraps. (One of these agents being bond_fold_stats())
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Willem de Bruijn <willemb@google.com>
> Cc: Eugenia Emantayev <eugenia@mellanox.com>

this problem was bugging me as well. thank you for fixing it.
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH net 0/4] net/mlx4_en: fix stats

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Wed, 25 May 2016 09:50:35 -0700

> mlx4 has various bugs in its ndo_get_stats() and related functions.
> This patch series address the obvious issues.
> Remaining ones will be discussed later.

Series applied, thanks.

Re: [PATCH net 4/4] net/mlx4_en: get rid of private net_device_stats

[Tariq Toukan]

Hi Eric,

Thanks for the fix.

> We do not need to clear fields that are already 0.
Why is it always true that dev->stats is already 0 at the point ndo_open 
is called?
Is it true also in a flow of open -> stop -> open? I searched the kernel 
stack for this but couldn't find.
> @@ -1877,7 +1877,6 @@ static void mlx4_en_clear_stats(struct net_device *dev)
>   	if (mlx4_en_DUMP_ETH_STATS(mdev, priv->port, 1))
>   		en_dbg(HW, priv, "Failed dumping statistics\n");
>   
> -	memset(&priv->stats, 0, sizeof(priv->stats));
>   	memset(&priv->pstats, 0, sizeof(priv->pstats));
>   	memset(&priv->pkstats, 0, sizeof(priv->pkstats));
>   	memset(&priv->port_stats, 0, sizeof(priv->port_stats));
The role of this function is to clear the stats, no matter when and 
where it is called.
I am aware that clearing the stats structure might be redundant today, 
as the function is called only within mlx4_en_open, but we might want to 
call the function in other flows in the future.

Regards,
Tariq

Re: [PATCH net 0/4] net/mlx4_en: fix stats

[Tariq Toukan]

Hi Eric,

> mlx4 has various bugs in its ndo_get_stats() and related functions.
> This patch series address the obvious issues.
> Remaining ones will be discussed later.
>
Thanks for the fixes.
I see they were already applied.
I reviewed them all and replied to patch 4/4, the rest look good to me.
Please CC me as the maintainer of mlx4_en on future patches.

Regards,
Tariq

Re: [PATCH net 4/4] net/mlx4_en: get rid of private net_device_stats

[Eric Dumazet]

On Thu, 2016-05-26 at 12:38 +0300, Tariq Toukan wrote:
> Hi Eric,
> 
> Thanks for the fix.
> 
> > We do not need to clear fields that are already 0.
> Why is it always true that dev->stats is already 0 at the point ndo_open 
> is called?
> Is it true also in a flow of open -> stop -> open? I searched the kernel 
> stack for this but couldn't find.
> > @@ -1877,7 +1877,6 @@ static void mlx4_en_clear_stats(struct net_device *dev)
> >   	if (mlx4_en_DUMP_ETH_STATS(mdev, priv->port, 1))
> >   		en_dbg(HW, priv, "Failed dumping statistics\n");
> >   
> > -	memset(&priv->stats, 0, sizeof(priv->stats));
> >   	memset(&priv->pstats, 0, sizeof(priv->pstats));
> >   	memset(&priv->pkstats, 0, sizeof(priv->pkstats));
> >   	memset(&priv->port_stats, 0, sizeof(priv->port_stats));
> The role of this function is to clear the stats, no matter when and 
> where it is called.
> I am aware that clearing the stats structure might be redundant today, 
> as the function is called only within mlx4_en_open, but we might want to 
> call the function in other flows in the future.

This is the ' non obvious fix' we need to discuss for net-next

When we enslave a mlx4 NIC in a bonding driver, we sometime get
incorrect bond stats because mlx4 stats are reset.

These stats being computed using deltas, this can not work as is.

I believe the rule is to not clear the netdev stats at open()

Anyway, for this particular path, it does not matter, as
mlx4_en_DUMP_ETH_STATS() will set all the fields to their value.

Re: [PATCH net 0/4] net/mlx4_en: fix stats

[Eric Dumazet]

On Thu, 2016-05-26 at 12:44 +0300, Tariq Toukan wrote:
> Hi Eric,
> 
> > mlx4 has various bugs in its ndo_get_stats() and related functions.
> > This patch series address the obvious issues.
> > Remaining ones will be discussed later.
> >
> Thanks for the fixes.
> I see they were already applied.
> I reviewed them all and replied to patch 4/4, the rest look good to me.
> Please CC me as the maintainer of mlx4_en on future patches.

If you are mlx4_en maintainer, please submit an official patch so that
non Mellanox employees can get this information using the normal way ?

Thanks.

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d397157981c..4a35221e4819 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7250,6 +7250,7 @@ F:	drivers/scsi/megaraid/
 
 MELLANOX ETHERNET DRIVER (mlx4_en)
 M: 	Eugenia Emantayev <eugenia@mellanox.com>
+M:	Tariq Toukan <tariqt@mellanox.com>
 L:	netdev@vger.kernel.org
 S:	Supported
 W:	http://www.mellanox.com

RE: [PATCH net 4/4] net/mlx4_en: get rid of private net_device_stats

[David Laight]

From: Tariq Toukan
> Sent: 26 May 2016 10:39
...
> I am aware that clearing the stats structure might be redundant today,
> as the function is called only within mlx4_en_open, but we might want to
> call the function in other flows in the future.

My personal view is that stats should never be cleared.
Any code that wants stats deltas should be remembering the old
values itself.

For SNMP (etc) you may want to save the time when the stats block
was created (when everything would be zero).

	David

Re: [PATCH net 4/4] net/mlx4_en: get rid of private net_device_stats

[Eric Dumazet]

On Thu, 2016-05-26 at 12:38 +0300, Tariq Toukan wrote:
> Hi Eric,
> 
> Thanks for the fix.
> 
> > We do not need to clear fields that are already 0.
> Why is it always true that dev->stats is already 0 at the point ndo_open 
> is called?

netdev structs are zero allocated. (kzalloc)

Re: [PATCH net 0/4] net/mlx4_en: fix stats

[Or Gerlitz]

On Thu, May 26, 2016 at 3:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-05-26 at 12:44 +0300, Tariq Toukan wrote:
>> Hi Eric,
>>
>> > mlx4 has various bugs in its ndo_get_stats() and related functions.
>> > This patch series address the obvious issues.
>> > Remaining ones will be discussed later.
>> >
>> Thanks for the fixes.
>> I see they were already applied.
>> I reviewed them all and replied to patch 4/4, the rest look good to me.
>> Please CC me as the maintainer of mlx4_en on future patches.
>
> If you are mlx4_en maintainer, please submit an official patch so that
> non Mellanox employees can get this information using the normal way ?

Eric, sure, we were on a transition period and Tariq was not fully
familiar with that practice,
I'd like to make sure the move is finalized internally and then we'll
send the patch..

On a somehow related note, Dave, Eric's patches were sent after our Wed working
hours ended and accepted before our Thu working hours started.. could we get a
better chance to review driver patches before acceptance?  I know
there were times
where we've screwed up and things didn't get fast attention, but we're
working to improve
so... get us a chance [1]?

Or.

[1] when it comes to weekends, the IL WW ends on Thu when the US WW
has almost two
full days to go (Thu, Fri), so here the response latency might be
bigger, but Sun-Thu we should
be responding on the same day.

Re: [PATCH net 0/4] net/mlx4_en: fix stats

[Eric Dumazet]

On Thu, 2016-05-26 at 18:19 +0300, Or Gerlitz wrote:
> On Thu, May 26, 2016 at 3:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2016-05-26 at 12:44 +0300, Tariq Toukan wrote:
> >> Hi Eric,
> >>
> >> > mlx4 has various bugs in its ndo_get_stats() and related functions.
> >> > This patch series address the obvious issues.
> >> > Remaining ones will be discussed later.
> >> >
> >> Thanks for the fixes.
> >> I see they were already applied.
> >> I reviewed them all and replied to patch 4/4, the rest look good to me.
> >> Please CC me as the maintainer of mlx4_en on future patches.
> >
> > If you are mlx4_en maintainer, please submit an official patch so that
> > non Mellanox employees can get this information using the normal way ?
> 
> Eric, sure, we were on a transition period and Tariq was not fully
> familiar with that practice,
> I'd like to make sure the move is finalized internally and then we'll
> send the patch..

Sure ! Please note I gave a polite answer, I am sorry if you felt any
hidden intent from my side.

I did CC the official mlx4_en maintainer on this patch series, by simply
looking at MAINTAINERS file.

Re: [PATCH net 4/4] net/mlx4_en: get rid of private net_device_stats

[David Miller]

From: Tariq Toukan <ttoukan.linux@gmail.com>
Date: Thu, 26 May 2016 12:38:58 +0300

> I am aware that clearing the stats structure might be redundant today,
> as the function is called only within mlx4_en_open, but we might want
> to call the function in other flows in the future.

You really should not arbitrarily clear the statistics, a down/up is not
supposed to rest them, for example.

Re: [PATCH net 4/4] net/mlx4_en: get rid of private net_device_stats

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 26 May 2016 05:54:10 -0700

> These stats being computed using deltas, this can not work as is.
> 
> I believe the rule is to not clear the netdev stats at open()

+1

Any driver clearing stats at open is broken and will break many,
many, networking tools.  And bonding too.

Re: [PATCH net 0/4] net/mlx4_en: fix stats

[David Miller]

From: Or Gerlitz <gerlitz.or@gmail.com>
Date: Thu, 26 May 2016 18:19:34 +0300

> Eric, sure, we were on a transition period and Tariq was not fully
> familiar with that practice,
> I'd like to make sure the move is finalized internally and then we'll
> send the patch..
> 
> On a somehow related note, Dave, Eric's patches were sent after our
> Wed working hours ended and accepted before our Thu working hours
> started.. could we get a better chance to review driver patches
> before acceptance?  I know there were times where we've screwed up
> and things didn't get fast attention, but we're working to improve
> so... get us a chance [1]?

If Eric's patches are clearly correct, I will apply them if I want to.

Sorry.

[PATCH net-next] net/mlx4_en: mlx4_en_netpoll() should schedule TX, not RX

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

I am not sure mlx4_en_netpoll() is doing anything useful right now.

mlx4 has different NAPI structures for RX and TX, and netpoll only wants
to drain TX queues.

Lets schedule NAPI polls on TX, not RX.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Maciej Żenczykowski <maze@google.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
Totally untested patch, I would appreciate some feedback before merge,
thanks !

 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 19ceced6736c..973391bfe286 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1197,8 +1197,8 @@ static void mlx4_en_netpoll(struct net_device *dev)
 	struct mlx4_en_cq *cq;
 	int i;
 
-	for (i = 0; i < priv->rx_ring_num; i++) {
-		cq = priv->rx_cq[i];
+	for (i = 0; i < priv->tx_ring_num; i++) {
+		cq = priv->tx_cq[i];
 		napi_schedule(&cq->napi);
 	}
 }

Re: [PATCH net-next] net/mlx4_en: mlx4_en_netpoll() should schedule TX, not RX

[Tariq Toukan]

Thanks Eric.

> Totally untested patch, I would appreciate some feedback before merge,
> thanks !
I'm testing it, and will reply shortly.

Regards,
Tariq

[PATCH net-next] net/mlx4_en: fix ethtool -x

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

mlx4 RSS is limited to spread incoming packets to a power of two number
of queues.

An uniformly distibuted traffic would be split on queues 0 to N-1, N
being a power of two, each queue having a 1/N weight.

If number of RX queues is not a power of two, upper RX queues do not
receive traffic.

ethtool -x is lying, because it pretends some queues have higher weight.

Before patch:

lpaa24:~# ethtool -L eth1 rx 24
lpaa24:~# ethtool -x eth1
RX flow hash indirection table for eth1 with 24 RX ring(s):
    0:      0     1     2     3     4     5     6     7
    8:      8     9    10    11    12    13    14    15
   16:      0     1     2     3     4     5     6     7
RSS hash key:
e0:7c:3a:89:07:55:b6:58:69:cc:f4:e5:24:62:e3:25:88:6c:42:5b:d2:cb:9a:d2:e0:06:e1:dc:f9:09:a1:89:0f:a0:30:43:73:6f:0c:b6

If this information was correct, user space tools could expect queues 0
to 7 to receive twice more traffic than queues 8 to 15

After patch :

lpaa24:~# ethtool -L eth1 rx 24
lpaa24:~# ethtool -x eth1      
RX flow hash indirection table for eth1 with 24 RX ring(s):
    0:      0     1     2     3     4     5     6     7
    8:      8     9    10    11    12    13    14    15
RSS hash key:
da:7b:09:60:f1:ac:67:b4:d0:72:d4:ec:a2:e5:80:0a:ad:50:22:1a:f8:f9:66:54:5f:22:45:c3:88:f4:57:82:c1:c1:90:ed:70:cb:40:ce
lpaa24:~# ethtool -X eth1 equal 8
lpaa24:~# ethtool -x eth1 
RX flow hash indirection table for eth1 with 24 RX ring(s):
    0:      0     1     2     3     4     5     6     7
    8:      0     1     2     3     4     5     6     7
RSS hash key:
da:7b:09:60:f1:ac:67:b4:d0:72:d4:ec:a2:e5:80:0a:ad:50:22:1a:f8:f9:66:54:5f:22:45:c3:88:f4:57:82:c1:c1:90:ed:70:cb:40:ce

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Maciej Żenczykowski <maze@google.com>
Cc: Eugenia Emantayev <eugenia@mellanox.com>
Cc: Wei Wang <weiwan@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |   25 ++++++--------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index fc95affaf76b..51a2e8252b82 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1107,7 +1107,7 @@ static u32 mlx4_en_get_rxfh_indir_size(struct net_device *dev)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 
-	return priv->rx_ring_num;
+	return rounddown_pow_of_two(priv->rx_ring_num);
 }
 
 static u32 mlx4_en_get_rxfh_key_size(struct net_device *netdev)
@@ -1141,19 +1141,17 @@ static int mlx4_en_get_rxfh(struct net_device *dev, u32 *ring_index, u8 *key,
 			    u8 *hfunc)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
-	struct mlx4_en_rss_map *rss_map = &priv->rss_map;
-	int rss_rings;
-	size_t n = priv->rx_ring_num;
+	u32 n = mlx4_en_get_rxfh_indir_size(dev);
+	u32 i, rss_rings;
 	int err = 0;
 
-	rss_rings = priv->prof->rss_rings ?: priv->rx_ring_num;
-	rss_rings = 1 << ilog2(rss_rings);
+	rss_rings = priv->prof->rss_rings ?: n;
+	rss_rings = rounddown_pow_of_two(rss_rings);
 
-	while (n--) {
+	for (i = 0; i < n; i++) {
 		if (!ring_index)
 			break;
-		ring_index[n] = rss_map->qps[n % rss_rings].qpn -
-			rss_map->base_qpn;
+		ring_index[i] = i % rss_rings;
 	}
 	if (key)
 		memcpy(key, priv->rss_key, MLX4_EN_RSS_KEY_SIZE);
@@ -1166,6 +1164,7 @@ static int mlx4_en_set_rxfh(struct net_device *dev, const u32 *ring_index,
 			    const u8 *key, const u8 hfunc)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
+	u32 n = mlx4_en_get_rxfh_indir_size(dev);
 	struct mlx4_en_dev *mdev = priv->mdev;
 	int port_up = 0;
 	int err = 0;
@@ -1175,18 +1174,18 @@ static int mlx4_en_set_rxfh(struct net_device *dev, const u32 *ring_index,
 	/* Calculate RSS table size and make sure flows are spread evenly
 	 * between rings
 	 */
-	for (i = 0; i < priv->rx_ring_num; i++) {
+	for (i = 0; i < n; i++) {
 		if (!ring_index)
-			continue;
+			break;
 		if (i > 0 && !ring_index[i] && !rss_rings)
 			rss_rings = i;
 
-		if (ring_index[i] != (i % (rss_rings ?: priv->rx_ring_num)))
+		if (ring_index[i] != (i % (rss_rings ?: n)))
 			return -EINVAL;
 	}
 
 	if (!rss_rings)
-		rss_rings = priv->rx_ring_num;
+		rss_rings = n;
 
 	/* RSS table size must be an order of 2 */
 	if (!is_power_of_2(rss_rings))

Re: [PATCH net-next] net/mlx4_en: fix ethtool -x

[Tariq Toukan]

Thanks, looks good.

Reviewed-by: Tariq Toukan <tariqt@mellanox.com>

Re: [PATCH net-next] net/mlx4_en: mlx4_en_netpoll() should schedule TX, not RX

[Tariq Toukan]

Acked-by: Tariq Toukan <tariqt@mellanox.com>

Re: [PATCH net-next] net/mlx4_en: mlx4_en_netpoll() should schedule TX, not RX

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 03 Jun 2016 11:52:49 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> I am not sure mlx4_en_netpoll() is doing anything useful right now.
> 
> mlx4 has different NAPI structures for RX and TX, and netpoll only wants
> to drain TX queues.
> 
> Lets schedule NAPI polls on TX, not RX.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Maciej Żenczykowski <maze@google.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>

Applied.

Re: [PATCH net-next] net/mlx4_en: fix ethtool -x

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 07 Jun 2016 21:24:18 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> mlx4 RSS is limited to spread incoming packets to a power of two number
> of queues.
> 
> An uniformly distibuted traffic would be split on queues 0 to N-1, N
> being a power of two, each queue having a 1/N weight.
> 
> If number of RX queues is not a power of two, upper RX queues do not
> receive traffic.
> 
> ethtool -x is lying, because it pretends some queues have higher weight.
 ...
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Maciej Żenczykowski <maze@google.com>

Applied.