[patch net-next v3 0/4] return offloaded stats as default and expose original sw start

[patch net-next v3 0/4] return offloaded stats as default and expose original sw start

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

From: Jiri Pirko <jiri@mellanox.com>

Until now we had stats functions return SW statistics. However, it makes
a lot of sense to return HW stats as default. The existing apps count with
having the defaults stats complete, but that is not true now as the offloaded
forward traffic is not visible there.

If user wants to know real SW stats, this patchset provides way to get
it as well.

---
v2->v3:
- patch1/4:
  - added dev_have_sw_stats helper
- patch2/4:
  - avoided memcpy as requerted by DaveM
- patch3/4:
  - use new dev_have_sw_stats helper
v1->v2:
- patch3/4: 
  - fixed NULL initialization

Nogah Frankel (4):
  netdevice: add SW statistics ndo
  rtnetlink: add HW/SW stats distinction in rtnl_fill_stats
  net: core: add SW stats to if_stats_msg
  mlxsw: spectrum: Implement SW stats ndo and expose HW stats by default

 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 65 ++++++++++++++++++++++----
 include/linux/netdevice.h                      | 13 ++++++
 include/uapi/linux/if_link.h                   |  2 +
 net/core/dev.c                                 | 31 ++++++++++++
 net/core/rtnetlink.c                           | 58 ++++++++++++++++++++---
 5 files changed, 155 insertions(+), 14 deletions(-)

-- 
2.5.5

[patch net-next v3 1/4] netdevice: add SW statistics ndo

[Jiri Pirko]

From: Nogah Frankel <nogahf@mellanox.com>

Till now we had a ndo statistics function that returned SW statistics.
We want to change the "basic" statistics to return HW statistics if
available.
In this case we need to expose a new ndo to return the SW statistics.
Add a new ndo declaration to get SW statistics
Add a function that gets SW statistics if a competible ndo exist

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
v2->v3:
- added dev_have_sw_stats helper
v1->v2:
- no change
---
 include/linux/netdevice.h | 13 +++++++++++++
 net/core/dev.c            | 31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c2f5112..1513d8b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -891,6 +891,14 @@ struct tc_to_netdev {
  *	   field is written atomically.
  *	3. Update dev->stats asynchronously and atomically, and define
  *	   neither operation.
+ *	Driver should return HW statistics, if available.
+ *
+ * struct rtnl_link_stats64* (*ndo_get_sw_stats64)(struct net_device *dev,
+ *			struct rtnl_link_stats64 *storage);
+ *	Similar to rtnl_link_stats64 but used to get SW statistics,
+ *	if it is possible to get HW and SW statistics separately.
+ *	If this option isn't valid - driver doesn't need to define
+ *	this function.
  *
  * int (*ndo_vlan_rx_add_vid)(struct net_device *dev, __be16 proto, u16 vid);
  *	If device supports VLAN filtering this function is called when a
@@ -1133,6 +1141,9 @@ struct net_device_ops {
 
 	struct rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev,
 						     struct rtnl_link_stats64 *storage);
+	struct rtnl_link_stats64* (*ndo_get_sw_stats64)(struct net_device *dev,
+							struct rtnl_link_stats64 *storage);
+
 	struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
 
 	int			(*ndo_vlan_rx_add_vid)(struct net_device *dev,
@@ -3755,6 +3766,8 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					struct rtnl_link_stats64 *storage);
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 			     const struct net_device_stats *netdev_stats);
+int dev_get_sw_stats(struct net_device *dev, struct rtnl_link_stats64 *storage);
+bool dev_have_sw_stats(struct net_device *dev);
 
 extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
diff --git a/net/core/dev.c b/net/core/dev.c
index 12436d1..82967b7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7376,6 +7376,8 @@ EXPORT_SYMBOL(netdev_stats_to_stats64);
  *	The device driver may provide its own method by setting
  *	dev->netdev_ops->get_stats64 or dev->netdev_ops->get_stats;
  *	otherwise the internal statistics structure is used.
+ *	If device supports both HW & SW statistics - this function should
+ *	return the HW statistics.
  */
 struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					struct rtnl_link_stats64 *storage)
@@ -7397,6 +7399,35 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 }
 EXPORT_SYMBOL(dev_get_stats);
 
+/*	dev_get_sw_stats    - get network device SW statistics
+ *	(if it is possible to get HW & SW statistics separately)
+ *	@dev: device to get statistics from
+ *	@storage: place to store stats
+ *
+ *	if exist a function to query the netdev SW statistics get it to storage
+ *	return 0 if did, or -EINVAL if this function doesn't exist
+ */
+int dev_get_sw_stats(struct net_device *dev,
+		     struct rtnl_link_stats64 *storage)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+
+	if (ops->ndo_get_sw_stats64) {
+		memset(storage, 0, sizeof(*storage));
+		ops->ndo_get_sw_stats64(dev, storage);
+	} else {
+		return -EINVAL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(dev_get_sw_stats);
+
+bool dev_have_sw_stats(struct net_device *dev)
+{
+	return (dev->netdev_ops->ndo_get_sw_stats64 != NULL);
+}
+EXPORT_SYMBOL(dev_have_sw_stats);
+
 struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
 {
 	struct netdev_queue *queue = dev_ingress_queue(dev);
-- 
2.5.5

Re: [patch net-next v3 1/4] netdevice: add SW statistics ndo

[Eric Dumazet]

On Fri, 2016-05-13 at 16:54 +0200, Jiri Pirko wrote:
> From: Nogah Frankel <nogahf@mellanox.com>
]
> +bool dev_have_sw_stats(struct net_device *dev)
> +{
> +	return (dev->netdev_ops->ndo_get_sw_stats64 != NULL);
> +}
> +EXPORT_SYMBOL(dev_have_sw_stats);
> +

2 things : const, and return is not a function.

bool dev_have_sw_stats(const struct net_device *dev)
{
       return dev->netdev_ops->ndo_get_sw_stats64 != NULL;
}

[patch net-next v3 2/4] rtnetlink: add HW/SW stats distinction in rtnl_fill_stats

[Jiri Pirko]

From: Nogah Frankel <nogahf@mellanox.com>

Since hardware stats are now returned by default, add a way to
query only software stats.
They are saved in IFLA_SW_STATS64.
(This option is valid only if the driver return HW stats in the
default ndo stats)

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- avoided memcpy as requerted by DaveM
v1->v2:
- no change
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bb36bd5..98175e7 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -156,6 +156,7 @@ enum {
 	IFLA_GSO_MAX_SEGS,
 	IFLA_GSO_MAX_SIZE,
 	IFLA_PAD,
+	IFLA_SW_STATS64,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d69c464..b0ebac6 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1054,12 +1054,25 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 
 	attr = nla_reserve_64bit(skb, IFLA_STATS64,
 				 sizeof(struct rtnl_link_stats64), IFLA_PAD);
+
 	if (!attr)
 		return -EMSGSIZE;
 
 	sp = nla_data(attr);
 	dev_get_stats(dev, sp);
 
+	if (dev_have_sw_stats(dev)) {
+		attr = nla_reserve_64bit(skb, IFLA_SW_STATS64,
+					 sizeof(struct rtnl_link_stats64),
+					 IFLA_PAD);
+
+		if (!attr)
+			return -EMSGSIZE;
+
+		sp = nla_data(attr);
+		dev_get_sw_stats(dev, sp);
+	}
+
 	attr = nla_reserve(skb, IFLA_STATS,
 			   sizeof(struct rtnl_link_stats));
 	if (!attr)
-- 
2.5.5

Re: [patch net-next v3 2/4] rtnetlink: add HW/SW stats distinction in rtnl_fill_stats

[Eric Dumazet]

On Fri, 2016-05-13 at 16:54 +0200, Jiri Pirko wrote:
> From: Nogah Frankel <nogahf@mellanox.com>
> 
> Since hardware stats are now returned by default, add a way to
> query only software stats.
> They are saved in IFLA_SW_STATS64.
> (This option is valid only if the driver return HW stats in the
> default ndo stats)
> 
> Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v2->v3:
> - avoided memcpy as requerted by DaveM
> v1->v2:
> - no change
> ---
>  include/uapi/linux/if_link.h |  1 +
>  net/core/rtnetlink.c         | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index bb36bd5..98175e7 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -156,6 +156,7 @@ enum {
>  	IFLA_GSO_MAX_SEGS,
>  	IFLA_GSO_MAX_SIZE,
>  	IFLA_PAD,
> +	IFLA_SW_STATS64,
>  	__IFLA_MAX
>  };
>  
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index d69c464..b0ebac6 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1054,12 +1054,25 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
>  
>  	attr = nla_reserve_64bit(skb, IFLA_STATS64,
>  				 sizeof(struct rtnl_link_stats64), IFLA_PAD);
> +
>  	if (!attr)
>  		return -EMSGSIZE;
>  
>  	sp = nla_data(attr);
>  	dev_get_stats(dev, sp);
>  
> +	if (dev_have_sw_stats(dev)) {
> +		attr = nla_reserve_64bit(skb, IFLA_SW_STATS64,
> +					 sizeof(struct rtnl_link_stats64),
> +					 IFLA_PAD);
> +
> +		if (!attr)
> +			return -EMSGSIZE;
> +
> +		sp = nla_data(attr);
> +		dev_get_sw_stats(dev, sp);
> +	}
> +
>  	attr = nla_reserve(skb, IFLA_STATS,
>  			   sizeof(struct rtnl_link_stats));
>  	if (!attr)


You forgot to change if_nlmsg_size() ?

nla_total_size_64bit(sizeof(struct rtnl_link_stats64)) is quite big.

[patch net-next v3 3/4] net: core: add SW stats to if_stats_msg

[Jiri Pirko]

From: Nogah Frankel <nogahf@mellanox.com>

If there is a dedicated ndo to return SW stats - use
it. Otherwise (indicates that there is no HW stats) use
the default stats ndo.
Return results under IFLA_STATS_LINK_SW_64.

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- use new dev_have_sw_stats helper
v1->v2:
- fixed NULL initialization
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 45 ++++++++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 98175e7..fcfb944 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -823,6 +823,7 @@ enum {
 	IFLA_STATS_UNSPEC, /* also used as 64bit pad attribute */
 	IFLA_STATS_LINK_64,
 	IFLA_STATS_LINK_XSTATS,
+	IFLA_STATS_LINK_SW_64,
 	__IFLA_STATS_MAX,
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index b0ebac6..22df012 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1046,6 +1046,11 @@ static int rtnl_phys_switch_id_fill(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static void copy_rtnl_link_stats64(void *v, const struct rtnl_link_stats64 *b)
+{
+	memcpy(v, b, sizeof(*b));
+}
+
 static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 					      struct net_device *dev)
 {
@@ -3474,6 +3479,9 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 	struct nlmsghdr *nlh;
 	struct nlattr *attr;
 	int s_prividx = *prividx;
+	struct rtnl_link_stats64 *sp;
+	struct rtnl_link_stats64 *stats64_sp = NULL;
+	int err;
 
 	ASSERT_RTNL();
 
@@ -3486,24 +3494,20 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 	ifsm->filter_mask = filter_mask;
 
 	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_64, *idxattr)) {
-		struct rtnl_link_stats64 *sp;
-
 		attr = nla_reserve_64bit(skb, IFLA_STATS_LINK_64,
 					 sizeof(struct rtnl_link_stats64),
 					 IFLA_STATS_UNSPEC);
 		if (!attr)
 			goto nla_put_failure;
 
-		sp = nla_data(attr);
-		dev_get_stats(dev, sp);
+		stats64_sp = nla_data(attr);
+		dev_get_stats(dev, stats64_sp);
 	}
 
 	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_XSTATS, *idxattr)) {
 		const struct rtnl_link_ops *ops = dev->rtnl_link_ops;
 
 		if (ops && ops->fill_linkxstats) {
-			int err;
-
 			*idxattr = IFLA_STATS_LINK_XSTATS;
 			attr = nla_nest_start(skb,
 					      IFLA_STATS_LINK_XSTATS);
@@ -3518,6 +3522,32 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
+	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_SW_64, *idxattr)) {
+		attr = nla_reserve_64bit(skb, IFLA_STATS_LINK_SW_64,
+					 sizeof(struct rtnl_link_stats64),
+					 IFLA_STATS_UNSPEC);
+		if (!attr)
+			goto nla_put_failure;
+
+		sp = nla_data(attr);
+
+		if (dev_have_sw_stats(dev)) {
+			dev_get_sw_stats(dev, sp);
+		} else {
+			/* if we don't have an ndo to get SW stats it implied
+			 * that the sw stats are return by
+			 * copy_rtnl_link_stats64 (probably because we don't
+			 * have hw stats). if we already called
+			 * copy_rtnl_link_stats64 we don't call it again but
+			 * copy the results.
+			 */
+			if (stats64_sp)
+				copy_rtnl_link_stats64(sp, stats64_sp);
+			else
+				dev_get_stats(dev, sp);
+		}
+	}
+
 	nlmsg_end(skb, nlh);
 
 	return 0;
@@ -3534,6 +3564,7 @@ nla_put_failure:
 
 static const struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
 	[IFLA_STATS_LINK_64]	= { .len = sizeof(struct rtnl_link_stats64) },
+	[IFLA_STATS_LINK_SW_64] = { .len = sizeof(struct rtnl_link_stats64) },
 };
 
 static size_t if_nlmsg_stats_size(const struct net_device *dev,
@@ -3543,6 +3574,8 @@ static size_t if_nlmsg_stats_size(const struct net_device *dev,
 
 	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_64, 0))
 		size += nla_total_size_64bit(sizeof(struct rtnl_link_stats64));
+	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_SW_64, 0))
+		size += nla_total_size_64bit(sizeof(struct rtnl_link_stats64));
 
 	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_XSTATS, 0)) {
 		const struct rtnl_link_ops *ops = dev->rtnl_link_ops;
-- 
2.5.5

[patch net-next v3 4/4] mlxsw: spectrum: Implement SW stats ndo and expose HW stats by default

[Jiri Pirko]

From: Nogah Frankel <nogahf@mellanox.com>

Add a function to get the SW statistics with an ndo.
Change the default statistics ndo to return HW statistics
(like the one returned by ethtool_ops)

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- no change
v1->v2:
- no change
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 65 ++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 4a72737..956e724 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -535,8 +535,8 @@ err_port_mtu_set:
 }
 
 static struct rtnl_link_stats64 *
-mlxsw_sp_port_get_stats64(struct net_device *dev,
-			  struct rtnl_link_stats64 *stats)
+mlxsw_sp_port_get_sw_stats64(struct net_device *dev,
+			     struct rtnl_link_stats64 *stats)
 {
 	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
 	struct mlxsw_sp_port_pcpu_stats *p;
@@ -566,6 +566,59 @@ mlxsw_sp_port_get_stats64(struct net_device *dev,
 	return stats;
 }
 
+static int mlxsw_sp_port_get_stats_raw(struct net_device *dev,
+				       char *ppcnt_pl)
+{
+	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
+	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
+	int err;
+
+	mlxsw_reg_ppcnt_pack(ppcnt_pl, mlxsw_sp_port->local_port,
+			     MLXSW_REG_PPCNT_IEEE_8023_CNT, 0);
+	err = mlxsw_reg_query(mlxsw_sp->core, MLXSW_REG(ppcnt), ppcnt_pl);
+	return err;
+}
+
+static struct rtnl_link_stats64 *
+mlxsw_sp_port_get_stats64(struct net_device *dev,
+			  struct rtnl_link_stats64 *stats)
+{
+	char ppcnt_pl[MLXSW_REG_PPCNT_LEN];
+	int err;
+
+	err =  mlxsw_sp_port_get_stats_raw(dev, ppcnt_pl);
+	if (err)
+		goto out;
+
+	stats->tx_packets =
+		mlxsw_reg_ppcnt_a_frames_transmitted_ok_get(ppcnt_pl);
+	stats->rx_packets =
+		mlxsw_reg_ppcnt_a_frames_received_ok_get(ppcnt_pl);
+	stats->tx_bytes =
+		mlxsw_reg_ppcnt_a_octets_transmitted_ok_get(ppcnt_pl);
+	stats->rx_bytes =
+		mlxsw_reg_ppcnt_a_octets_received_ok_get(ppcnt_pl);
+	stats->multicast =
+		mlxsw_reg_ppcnt_a_multicast_frames_received_ok_get(ppcnt_pl);
+	stats->multicast +=
+		mlxsw_reg_ppcnt_a_broadcast_frames_received_ok_get(ppcnt_pl);
+
+	stats->rx_crc_errors =
+		mlxsw_reg_ppcnt_a_frame_check_sequence_errors_get(ppcnt_pl);
+	stats->rx_frame_errors =
+		mlxsw_reg_ppcnt_a_alignment_errors_get(ppcnt_pl);
+
+	stats->rx_length_errors = (
+		mlxsw_reg_ppcnt_a_in_range_length_errors_get(ppcnt_pl) +
+		mlxsw_reg_ppcnt_a_out_of_range_length_field_get(ppcnt_pl) +
+		mlxsw_reg_ppcnt_a_frame_too_long_errors_get(ppcnt_pl));
+
+	stats->rx_errors = (stats->rx_crc_errors +
+		stats->rx_frame_errors + stats->rx_length_errors);
+out:
+	return stats;
+}
+
 int mlxsw_sp_port_vlan_set(struct mlxsw_sp_port *mlxsw_sp_port, u16 vid_begin,
 			   u16 vid_end, bool is_member, bool untagged)
 {
@@ -980,6 +1033,7 @@ static const struct net_device_ops mlxsw_sp_port_netdev_ops = {
 	.ndo_set_mac_address	= mlxsw_sp_port_set_mac_address,
 	.ndo_change_mtu		= mlxsw_sp_port_change_mtu,
 	.ndo_get_stats64	= mlxsw_sp_port_get_stats64,
+	.ndo_get_sw_stats64     = mlxsw_sp_port_get_sw_stats64,
 	.ndo_vlan_rx_add_vid	= mlxsw_sp_port_add_vid,
 	.ndo_vlan_rx_kill_vid	= mlxsw_sp_port_kill_vid,
 	.ndo_fdb_add		= switchdev_port_fdb_add,
@@ -1200,15 +1254,10 @@ static int mlxsw_sp_port_set_phys_id(struct net_device *dev,
 static void mlxsw_sp_port_get_stats(struct net_device *dev,
 				    struct ethtool_stats *stats, u64 *data)
 {
-	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
 	char ppcnt_pl[MLXSW_REG_PPCNT_LEN];
 	int i;
 	int err;
-
-	mlxsw_reg_ppcnt_pack(ppcnt_pl, mlxsw_sp_port->local_port,
-			     MLXSW_REG_PPCNT_IEEE_8023_CNT, 0);
-	err = mlxsw_reg_query(mlxsw_sp->core, MLXSW_REG(ppcnt), ppcnt_pl);
+	err = mlxsw_sp_port_get_stats_raw(dev, ppcnt_pl);
 	for (i = 0; i < MLXSW_SP_PORT_HW_STATS_LEN; i++)
 		data[i] = !err ? mlxsw_sp_port_hw_stats[i].getter(ppcnt_pl) : 0;
 }
-- 
2.5.5