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

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

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Nogah says:

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.

---
v3->v4:
- patch1/4:
  - fixed "return ()" pointed out by EricD
- patch2/4:
  - fixed if_nlmsg_size as pointed out by EricD
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 | 110 +++++++++++++++++++++++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h |   5 ++
 include/linux/netdevice.h                      |  13 +++
 include/uapi/linux/if_link.h                   |   2 +
 net/core/dev.c                                 |  31 +++++++
 net/core/rtnetlink.c                           |  54 ++++++++++--
 6 files changed, 200 insertions(+), 15 deletions(-)

-- 
2.5.5

[patch net-next v4 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 compatible ndo exist

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 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 890158e..6338e38 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -890,6 +890,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
@@ -1132,6 +1140,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,
@@ -3779,6 +3790,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(const 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 b148357..087f0ea 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7368,6 +7368,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)
@@ -7389,6 +7391,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(const 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

[patch net-next v4 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 returns HW stats in the
default ndo stats)

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

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 eb49ca2..7958cde 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -901,6 +901,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size_64bit(sizeof(struct rtnl_link_ifmap))
 	       + nla_total_size(sizeof(struct rtnl_link_stats))
 	       + nla_total_size_64bit(sizeof(struct rtnl_link_stats64))
+	       + nla_total_size_64bit(sizeof(struct rtnl_link_stats64)) /* IFLA_SW_STATS64 */
 	       + nla_total_size(MAX_ADDR_LEN) /* IFLA_ADDRESS */
 	       + nla_total_size(MAX_ADDR_LEN) /* IFLA_BROADCAST */
 	       + nla_total_size(4) /* IFLA_TXQLEN */
@@ -928,7 +929,6 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
 	       + nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
 	       + nla_total_size(1); /* IFLA_PROTO_DOWN */
-
 }
 
 static int rtnl_vf_ports_fill(struct sk_buff *skb, struct net_device *dev)
@@ -1082,6 +1082,17 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 	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

[patch net-next v4 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>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 41 +++++++++++++++++++++++++++++++++++------
 2 files changed, 36 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 7958cde..d2acf9d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1068,6 +1068,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)
 {
@@ -3490,10 +3495,13 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 			       unsigned int flags, unsigned int filter_mask,
 			       int *idxattr, int *prividx)
 {
+	struct rtnl_link_stats64 *stats64_sp = NULL;
+	struct rtnl_link_stats64 *sp;
 	struct if_stats_msg *ifsm;
 	struct nlmsghdr *nlh;
 	struct nlattr *attr;
 	int s_prividx = *prividx;
+	int err;
 
 	ASSERT_RTNL();
 
@@ -3506,24 +3514,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);
@@ -3538,6 +3542,28 @@ 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 SW stats are not available we return default
+			 * stats. We query only if we don't already have them.
+			 */
+			if (stats64_sp)
+				copy_rtnl_link_stats64(sp, stats64_sp);
+			else
+				dev_get_stats(dev, sp);
+		}
+	}
+
 	nlmsg_end(skb, nlh);
 
 	return 0;
@@ -3554,6 +3580,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,
@@ -3563,6 +3590,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

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

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 16 Jun 2016 10:37:16 +0200

> @@ -1068,6 +1068,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));
> +}
 ...
> +	struct rtnl_link_stats64 *stats64_sp = NULL;
> +	struct rtnl_link_stats64 *sp;
 ...
> +				copy_rtnl_link_stats64(sp, stats64_sp);

I don't see any reason why copy_rtnl_link_stats64's first argument should be
"void *".  Please make it "struct rtnl_link_stats64 *".

In fact just doing a straight memcpy() inline is probably the best, there are
no special typing nor casting requirements here.

Thanks.

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

[Jiri Pirko]

Fri, Jun 17, 2016 at 02:20:48AM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu, 16 Jun 2016 10:37:16 +0200
>
>> @@ -1068,6 +1068,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));
>> +}
> ...
>> +	struct rtnl_link_stats64 *stats64_sp = NULL;
>> +	struct rtnl_link_stats64 *sp;
> ...
>> +				copy_rtnl_link_stats64(sp, stats64_sp);
>
>I don't see any reason why copy_rtnl_link_stats64's first argument should be
>"void *".  Please make it "struct rtnl_link_stats64 *".
>
>In fact just doing a straight memcpy() inline is probably the best, there are
>no special typing nor casting requirements here.

Will do. Thanks.

[patch net-next v4 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)
The HW stats are collected to a cache by delayed work every 1 sec.

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 110 +++++++++++++++++++++++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h |   5 ++
 2 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 6f9e3dd..b41ecf4 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -533,8 +533,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;
@@ -564,6 +564,89 @@ 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;
+
+	mlxsw_reg_ppcnt_pack(ppcnt_pl, mlxsw_sp_port->local_port,
+			     MLXSW_REG_PPCNT_IEEE_8023_CNT, 0)
+;
+	return mlxsw_reg_query(mlxsw_sp->core, MLXSW_REG(ppcnt), ppcnt_pl);
+}
+
+static int mlxsw_sp_port_get_hw_stats(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->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 err;
+}
+
+static void update_stats_cache(struct work_struct *work)
+{
+	struct mlxsw_sp_port *mlxsw_sp_port =
+		container_of(work, struct mlxsw_sp_port,
+			     hw_stats.update_dw.work);
+
+	if (!netif_carrier_ok(mlxsw_sp_port->dev))
+		goto out;
+
+	mlxsw_sp_port_get_hw_stats(mlxsw_sp_port->dev,
+				   mlxsw_sp_port->hw_stats.cache);
+
+out:
+	mlxsw_core_schedule_dw(&mlxsw_sp_port->hw_stats.update_dw,
+			       MLXSW_HW_STATS_UPDATE_TIME);
+}
+
+/* HW stats are being read by a delayed work to a cache
+ * and this function return it. The use for cache is because
+ * reading HW stats require EMAD and thatfor sleep and we might
+ * be asked to return stats in sleepless mode
+ */
+static struct rtnl_link_stats64 *
+mlxsw_sp_port_get_stats64(struct net_device *dev,
+			  struct rtnl_link_stats64 *stats)
+{
+	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
+
+	memcpy(stats, mlxsw_sp_port->hw_stats.cache, sizeof(*stats));
+
+	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)
 {
@@ -972,6 +1055,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,
@@ -1192,15 +1276,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;
 }
@@ -1712,6 +1791,16 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
 		goto err_alloc_stats;
 	}
 
+	mlxsw_sp_port->hw_stats.cache =
+		kzalloc(sizeof(*mlxsw_sp_port->hw_stats.cache), GFP_KERNEL);
+
+	if (!mlxsw_sp_port->hw_stats.cache) {
+		err = -ENOMEM;
+		goto err_alloc_hw_stats;
+	}
+	INIT_DELAYED_WORK(&mlxsw_sp_port->hw_stats.update_dw,
+			  &update_stats_cache);
+
 	dev->netdev_ops = &mlxsw_sp_port_netdev_ops;
 	dev->ethtool_ops = &mlxsw_sp_port_ethtool_ops;
 
@@ -1808,6 +1897,7 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
 		goto err_port_vlan_init;
 
 	mlxsw_sp->ports[local_port] = mlxsw_sp_port;
+	mlxsw_core_schedule_dw(&mlxsw_sp_port->hw_stats.update_dw, 0);
 	return 0;
 
 err_port_vlan_init:
@@ -1824,6 +1914,8 @@ err_port_speed_by_width_set:
 err_port_swid_set:
 err_port_system_port_mapping_set:
 err_dev_addr_init:
+	kfree(mlxsw_sp_port->hw_stats.cache);
+err_alloc_hw_stats:
 	free_percpu(mlxsw_sp_port->pcpu_stats);
 err_alloc_stats:
 	kfree(mlxsw_sp_port->untagged_vlans);
@@ -1857,6 +1949,7 @@ static void mlxsw_sp_port_remove(struct mlxsw_sp *mlxsw_sp, u8 local_port)
 
 	if (!mlxsw_sp_port)
 		return;
+	cancel_delayed_work_sync(&mlxsw_sp_port->hw_stats.update_dw);
 	mlxsw_sp->ports[local_port] = NULL;
 	mlxsw_core_port_fini(&mlxsw_sp_port->core_port);
 	unregister_netdev(mlxsw_sp_port->dev); /* This calls ndo_stop */
@@ -1866,6 +1959,7 @@ static void mlxsw_sp_port_remove(struct mlxsw_sp *mlxsw_sp, u8 local_port)
 	mlxsw_sp_port_swid_set(mlxsw_sp_port, MLXSW_PORT_SWID_DISABLED_PORT);
 	mlxsw_sp_port_module_unmap(mlxsw_sp, mlxsw_sp_port->local_port);
 	free_percpu(mlxsw_sp_port->pcpu_stats);
+	kfree(mlxsw_sp_port->hw_stats.cache);
 	kfree(mlxsw_sp_port->untagged_vlans);
 	kfree(mlxsw_sp_port->active_vlans);
 	free_netdev(mlxsw_sp_port->dev);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 13b30ea..1826407 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -239,6 +239,11 @@ struct mlxsw_sp_port {
 	unsigned long *untagged_vlans;
 	/* VLAN interfaces */
 	struct list_head vports_list;
+	struct {
+		#define MLXSW_HW_STATS_UPDATE_TIME HZ
+		struct rtnl_link_stats64 *cache;
+		struct delayed_work update_dw;
+	} hw_stats;
 };
 
 static inline bool
-- 
2.5.5

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 16 Jun 2016 10:37:13 +0200

> 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.

I think we haven't been very good about defining good rules nor
guidelines for how to handle HW vs SW stats, and I'm talking
strictly about what we publish via rtnl_link_stats64.

However, if I were working from scratch on a new driver what I would
be inclined to do is use HW stats for everything that the chip
provides direct and accurate support for, and fill in the gaps with SW
stats.

Because to me, stats are stats, the user wants to know (for example)
how many broadcast packets have gone through the port and doesn't care
how you obtain that number.

If the problem being addressed is that drivers aren't reporting
information on all the packets going through the device, then that's a
bug.

But it seems to me like mlxsw is already maintaining the software
statistic counters, so I can't see a performance reason for not
properly providing all of the statistics using HW vs. SW as is
appropriate for each and every value to fix this problem.  Why
create an entire new facility just for that?  It doesn't seem to
be needed.

Maybe you just need to describe things a bit more completely in this
header posting.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jiri Pirko]

Fri, Jun 17, 2016 at 02:26:32AM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu, 16 Jun 2016 10:37:13 +0200
>
>> 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.
>
>I think we haven't been very good about defining good rules nor
>guidelines for how to handle HW vs SW stats, and I'm talking
>strictly about what we publish via rtnl_link_stats64.
>
>However, if I were working from scratch on a new driver what I would
>be inclined to do is use HW stats for everything that the chip
>provides direct and accurate support for, and fill in the gaps with SW
>stats.
>
>Because to me, stats are stats, the user wants to know (for example)
>how many broadcast packets have gone through the port and doesn't care
>how you obtain that number.
>
>If the problem being addressed is that drivers aren't reporting
>information on all the packets going through the device, then that's a
>bug.
>
>But it seems to me like mlxsw is already maintaining the software
>statistic counters, so I can't see a performance reason for not
>properly providing all of the statistics using HW vs. SW as is
>appropriate for each and every value to fix this problem.  Why
>create an entire new facility just for that?  It doesn't seem to
>be needed.
>
>Maybe you just need to describe things a bit more completely in this
>header posting.


The problem we try to handle is different, it's about offloaded
forwarded packets which are not seen by kernel. Let me try to draw it :)

    port1                       port2 (HW stats are counted here)
      \                          /
       \                        /
        \                      /
         --(A)---- ASIC --(B)--
                    |
                   (C)
                    |
                   CPU (SW stats are counted here)


Now we have couple of flows for TX and RX (direction does not matter here):

1) port1->A->ASIC->C->CPU

   For this flow, HW and SW stats are equal.

2) port1->A->ASIC->C->CPU->C->ASIC->B->port2

   For this flow, HW and SW stats are equal.

3) port1->A->ASIC->B->port2

   For this flow, SW stats are 0.

The purpose of this patchset is to provide facility for user to
find out the difference between flows 1+2 and 3. In other words, user
will be able to see the statistics for his slow-path (through kernel).

Also, as a default the accumulated stats (HW) will be exposed to user
so the userspace apps can react properly.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[David Ahern]

On 6/17/16 2:24 AM, Jiri Pirko wrote:
>
> The problem we try to handle is different, it's about offloaded
> forwarded packets which are not seen by kernel. Let me try to draw it :)
>
>     port1                       port2 (HW stats are counted here)
>       \                          /
>        \                        /
>         \                      /
>          --(A)---- ASIC --(B)--
>                     |
>                    (C)
>                     |
>                    CPU (SW stats are counted here)
>
>
> Now we have couple of flows for TX and RX (direction does not matter here):
>
> 1) port1->A->ASIC->C->CPU
>
>    For this flow, HW and SW stats are equal.
>
> 2) port1->A->ASIC->C->CPU->C->ASIC->B->port2
>
>    For this flow, HW and SW stats are equal.
>
> 3) port1->A->ASIC->B->port2
>
>    For this flow, SW stats are 0.
>
> The purpose of this patchset is to provide facility for user to
> find out the difference between flows 1+2 and 3. In other words, user
> will be able to see the statistics for his slow-path (through kernel).
>
> Also, as a default the accumulated stats (HW) will be exposed to user
> so the userspace apps can react properly.
>

You no longer agree with this discussion?
   http://comments.gmane.org/gmane.linux.network/346740

Essentially netdevice stats show counters for packets punted to the cpu 
and ethool -S shows h/w stats. This patch set seems to invert that.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jiri Pirko]

Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>
>>The problem we try to handle is different, it's about offloaded
>>forwarded packets which are not seen by kernel. Let me try to draw it :)
>>
>>    port1                       port2 (HW stats are counted here)
>>      \                          /
>>       \                        /
>>        \                      /
>>         --(A)---- ASIC --(B)--
>>                    |
>>                   (C)
>>                    |
>>                   CPU (SW stats are counted here)
>>
>>
>>Now we have couple of flows for TX and RX (direction does not matter here):
>>
>>1) port1->A->ASIC->C->CPU
>>
>>   For this flow, HW and SW stats are equal.
>>
>>2) port1->A->ASIC->C->CPU->C->ASIC->B->port2
>>
>>   For this flow, HW and SW stats are equal.
>>
>>3) port1->A->ASIC->B->port2
>>
>>   For this flow, SW stats are 0.
>>
>>The purpose of this patchset is to provide facility for user to
>>find out the difference between flows 1+2 and 3. In other words, user
>>will be able to see the statistics for his slow-path (through kernel).
>>
>>Also, as a default the accumulated stats (HW) will be exposed to user
>>so the userspace apps can react properly.
>>
>
>You no longer agree with this discussion?
>  http://comments.gmane.org/gmane.linux.network/346740
>
>Essentially netdevice stats show counters for packets punted to the cpu and
>ethool -S shows h/w stats. This patch set seems to invert that.

That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jamal Hadi Salim]

On 16-06-17 10:05 AM, Jiri Pirko wrote:
> Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>> On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>

>
> That is problematic. Existing apps depend on rtnetlink stats. But if we
> don't count offloaded forwarded packets, the apps don't see anything.
> Therefore I believe that this patchset approach is better. The existing
> apps continue to work and future apps can use newly introduces sw_stats
> to query slowpath traffic. Makes sense to me.
>

I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
We already have a use case with the tc where someone could get/set
hardware and/or software.

cheers,
jamal

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jiri Pirko]

Fri, Jun 17, 2016 at 04:54:34PM CEST, jhs@mojatatu.com wrote:
>On 16-06-17 10:05 AM, Jiri Pirko wrote:
>>Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>>On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>>
>
>>
>>That is problematic. Existing apps depend on rtnetlink stats. But if we
>>don't count offloaded forwarded packets, the apps don't see anything.
>>Therefore I believe that this patchset approach is better. The existing
>>apps continue to work and future apps can use newly introduces sw_stats
>>to query slowpath traffic. Makes sense to me.
>>
>
>I agree with Jiri. It is a bad idea to depend on ethtool for any of
>this stuff. Is there a way we can tag netlink stats instead
>to indicate they are hardware or software?

In this patchset, those are 2 nl attrs. And they come kernel->user at once.
So I see no need for any tagging. Also won't be appropriate.

>We already have a use case with the tc where someone could get/set
>hardware and/or software.

That is user->kernel.

>
>cheers,
>jamal

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[David Ahern]

On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
> On 16-06-17 10:05 AM, Jiri Pirko wrote:
>> Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>> On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>>
>
>>
>> That is problematic. Existing apps depend on rtnetlink stats. But if we
>> don't count offloaded forwarded packets, the apps don't see anything.
>> Therefore I believe that this patchset approach is better. The existing
>> apps continue to work and future apps can use newly introduces sw_stats
>> to query slowpath traffic. Makes sense to me.
>>
>
> I agree with Jiri. It is a bad idea to depend on ethtool for any of
> this stuff. Is there a way we can tag netlink stats instead
> to indicate they are hardware or software?

Right, old API but the key here is that low level h/w stats are returned 
by a different API.

By default ip, ifconfig, snmpd, etc all continue to get traditional S/W 
stats - counters as seen by the CPU.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jiri Pirko]

Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
>On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
>>On 16-06-17 10:05 AM, Jiri Pirko wrote:
>>>Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>>>On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>>>
>>
>>>
>>>That is problematic. Existing apps depend on rtnetlink stats. But if we
>>>don't count offloaded forwarded packets, the apps don't see anything.
>>>Therefore I believe that this patchset approach is better. The existing
>>>apps continue to work and future apps can use newly introduces sw_stats
>>>to query slowpath traffic. Makes sense to me.
>>>
>>
>>I agree with Jiri. It is a bad idea to depend on ethtool for any of
>>this stuff. Is there a way we can tag netlink stats instead
>>to indicate they are hardware or software?
>
>Right, old API but the key here is that low level h/w stats are returned by a
>different API.
>
>By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
>- counters as seen by the CPU.

Yep. And I believe that for offloaded forwarding, this tools should see
hw counters, as they show what is going on in real.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Florian Fainelli]

On 06/17/2016 08:42 AM, Jiri Pirko wrote:
> Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
>> On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
>>> On 16-06-17 10:05 AM, Jiri Pirko wrote:
>>>> Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>>>> On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>>>>
>>>
>>>>
>>>> That is problematic. Existing apps depend on rtnetlink stats. But if we
>>>> don't count offloaded forwarded packets, the apps don't see anything.
>>>> Therefore I believe that this patchset approach is better. The existing
>>>> apps continue to work and future apps can use newly introduces sw_stats
>>>> to query slowpath traffic. Makes sense to me.
>>>>
>>>
>>> I agree with Jiri. It is a bad idea to depend on ethtool for any of
>>> this stuff. Is there a way we can tag netlink stats instead
>>> to indicate they are hardware or software?
>>
>> Right, old API but the key here is that low level h/w stats are returned by a
>> different API.
>>
>> By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
>> - counters as seen by the CPU.
> 
> Yep. And I believe that for offloaded forwarding, this tools should see
> hw counters, as they show what is going on in real.

If your NIC is offloading packets today, these tools typically won't see
these stats, but ethtool -S likely will report what is going on under
the hood.

Do we actually need to tell apart SW maintained from HW maintained
stats, or at the end all that matters is just, as DaveM pointed out,
getting the information, and in the case of an Ethernet switch, return
HW stats by default and supplement with SW stats whenever we have them,
all in the same namespace?
-- 
Florian

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jiri Pirko]

Fri, Jun 17, 2016 at 07:12:22PM CEST, f.fainelli@gmail.com wrote:
>On 06/17/2016 08:42 AM, Jiri Pirko wrote:
>> Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
>>> On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
>>>> On 16-06-17 10:05 AM, Jiri Pirko wrote:
>>>>> Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>>>>> On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>>>>>
>>>>
>>>>>
>>>>> That is problematic. Existing apps depend on rtnetlink stats. But if we
>>>>> don't count offloaded forwarded packets, the apps don't see anything.
>>>>> Therefore I believe that this patchset approach is better. The existing
>>>>> apps continue to work and future apps can use newly introduces sw_stats
>>>>> to query slowpath traffic. Makes sense to me.
>>>>>
>>>>
>>>> I agree with Jiri. It is a bad idea to depend on ethtool for any of
>>>> this stuff. Is there a way we can tag netlink stats instead
>>>> to indicate they are hardware or software?
>>>
>>> Right, old API but the key here is that low level h/w stats are returned by a
>>> different API.
>>>
>>> By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
>>> - counters as seen by the CPU.
>> 
>> Yep. And I believe that for offloaded forwarding, this tools should see
>> hw counters, as they show what is going on in real.
>
>If your NIC is offloading packets today, these tools typically won't see
>these stats, but ethtool -S likely will report what is going on under
>the hood.
>
>Do we actually need to tell apart SW maintained from HW maintained
>stats, or at the end all that matters is just, as DaveM pointed out,
>getting the information, and in the case of an Ethernet switch, return
>HW stats by default and supplement with SW stats whenever we have them,
>all in the same namespace?

I believe it is valuable for user to know stats for slow path
(non-forwarded by ASIC). Also, it's just another rtnl attr. Easy.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jamal Hadi Salim]

On 16-06-18 04:00 AM, Jiri Pirko wrote:
> Fri, Jun 17, 2016 at 07:12:22PM CEST, f.fainelli@gmail.com wrote:

>>> Yep. And I believe that for offloaded forwarding, this tools should see
>>> hw counters, as they show what is going on in real.
>>
>> If your NIC is offloading packets today, these tools typically won't see
>> these stats, but ethtool -S likely will report what is going on under
>> the hood.
>>
>> Do we actually need to tell apart SW maintained from HW maintained
>> stats, or at the end all that matters is just, as DaveM pointed out,
>> getting the information, and in the case of an Ethernet switch, return
>> HW stats by default and supplement with SW stats whenever we have them,
>> all in the same namespace?
>

In general it is extremely useful for debugging to be able to see them
separately. One API to unify them (and that API being netlink) is
the way to go. I dont know if you can ever obsolete ethtool if lots
of other utils are using it - but would be nice.
It is also useful to just get the sum of them - but user space can
take care of that. David A., whatever user space tools that depended
on ethtool should now be able to retrieve them via netlink, no?

> I believe it is valuable for user to know stats for slow path
> (non-forwarded by ASIC). Also, it's just another rtnl attr. Easy.
>

So Jiri, I see:
IFLA_SW_STATS64 should that be: IFLA_HW_STATS_LINK_64?
I think IFLA_STATS_LINK_64 should continue to send s/ware stats.

cheers,
jamal

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jiri Pirko]

Sat, Jun 18, 2016 at 03:58:56PM CEST, jhs@mojatatu.com wrote:
>On 16-06-18 04:00 AM, Jiri Pirko wrote:
>>Fri, Jun 17, 2016 at 07:12:22PM CEST, f.fainelli@gmail.com wrote:
>
>>>>Yep. And I believe that for offloaded forwarding, this tools should see
>>>>hw counters, as they show what is going on in real.
>>>
>>>If your NIC is offloading packets today, these tools typically won't see
>>>these stats, but ethtool -S likely will report what is going on under
>>>the hood.
>>>
>>>Do we actually need to tell apart SW maintained from HW maintained
>>>stats, or at the end all that matters is just, as DaveM pointed out,
>>>getting the information, and in the case of an Ethernet switch, return
>>>HW stats by default and supplement with SW stats whenever we have them,
>>>all in the same namespace?
>>
>
>In general it is extremely useful for debugging to be able to see them
>separately. One API to unify them (and that API being netlink) is
>the way to go. I dont know if you can ever obsolete ethtool if lots
>of other utils are using it - but would be nice.
>It is also useful to just get the sum of them - but user space can
>take care of that. David A., whatever user space tools that depended
>on ethtool should now be able to retrieve them via netlink, no?
>
>>I believe it is valuable for user to know stats for slow path
>>(non-forwarded by ASIC). Also, it's just another rtnl attr. Easy.
>>
>
>So Jiri, I see:
>IFLA_SW_STATS64 should that be: IFLA_HW_STATS_LINK_64?
>I think IFLA_STATS_LINK_64 should continue to send s/ware stats.

Well, we spent a lot of time to think about this. The problem with your
approach is that existing apps don't see "real-stats" - hw stats. For
example snmp daemon takes IFLA_STATS_LINK_64i, so it has to see HW stats
there. In order to not break existing apps, we expose HW stats as default.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Roopa Prabhu]

On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/17/2016 08:42 AM, Jiri Pirko wrote:
>> Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
>>> On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
>>>> On 16-06-17 10:05 AM, Jiri Pirko wrote:
>>>>> Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>>>>> On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>>>>>
>>>>
>>>>>
>>>>> That is problematic. Existing apps depend on rtnetlink stats. But if we
>>>>> don't count offloaded forwarded packets, the apps don't see anything.
>>>>> Therefore I believe that this patchset approach is better. The existing
>>>>> apps continue to work and future apps can use newly introduces sw_stats
>>>>> to query slowpath traffic. Makes sense to me.
>>>>>
>>>>
>>>> I agree with Jiri. It is a bad idea to depend on ethtool for any of
>>>> this stuff. Is there a way we can tag netlink stats instead
>>>> to indicate they are hardware or software?
>>>
>>> Right, old API but the key here is that low level h/w stats are returned by a
>>> different API.
>>>
>>> By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
>>> - counters as seen by the CPU.
>>
>> Yep. And I believe that for offloaded forwarding, this tools should see
>> hw counters, as they show what is going on in real.
>
> If your NIC is offloading packets today, these tools typically won't see
> these stats, but ethtool -S likely will report what is going on under
> the hood.
>
> Do we actually need to tell apart SW maintained from HW maintained
> stats, or at the end all that matters is just, as DaveM pointed out,
> getting the information, and in the case of an Ethernet switch, return
> HW stats by default and supplement with SW stats whenever we have them,
> all in the same namespace?
> --

I have also mentioned this before, the default api must provide
accumulated (hw and sw) stats...,
because this is the api that the user queries on an interface.
For advanced debugging, people do want a break down and thats what
traditionally ethtool has provided
and the new stats api should eventually include support for ethtool like stats.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jamal Hadi Salim]

On 16-06-19 11:14 PM, Roopa Prabhu wrote:
> On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:


>
> I have also mentioned this before, the default api must provide
> accumulated (hw and sw) stats...,
> because this is the api that the user queries on an interface.

Sorry - I missed those discussions.
What is current practise? Do people request for one via ip link
stats and the other via ethtool?
What do you guys do in your implementation?
Yes, it would be more accurate to provide aggregated stats but
it may break backward compat if expectation is both are read
separately today.
Maybe it makes sense to have a brand new TLV for these aggregated
stats as Jiri was suggesting.That means two new TLVs not one.
1) TLV for aggregated stats - which cant be current one
2) TLV for h/w stats

The existing stat implies s/ware only.

cheers,
jamal

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Jiri Pirko]

Mon, Jun 20, 2016 at 02:28:31PM CEST, jhs@mojatatu.com wrote:
>On 16-06-19 11:14 PM, Roopa Prabhu wrote:
>>On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>>
>>I have also mentioned this before, the default api must provide
>>accumulated (hw and sw) stats...,
>>because this is the api that the user queries on an interface.
>
>Sorry - I missed those discussions.
>What is current practise? Do people request for one via ip link
>stats and the other via ethtool?

Yes.


>What do you guys do in your implementation?

Currently we do what you described. This patchset changes it.


>Yes, it would be more accurate to provide aggregated stats but
>it may break backward compat if expectation is both are read
>separately today.
>Maybe it makes sense to have a brand new TLV for these aggregated
>stats as Jiri was suggesting.That means two new TLVs not one.
>1) TLV for aggregated stats - which cant be current one
>2) TLV for h/w stats
>
>The existing stat implies s/ware only.


What is "aggregated"? if hw counts it, sw counts it as well.
HW stats are in fact aggregated. It includes all.

Apps should see HW stats as they don't care if the forwarding is
offloaded to HW or not. And if someone is aware there is a forward
offload, he can query sw-only stats via iface proposed by this patchset.

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Roopa Prabhu]

On Mon, Jun 20, 2016 at 5:28 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 16-06-19 11:14 PM, Roopa Prabhu wrote:
>>
>> On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli <f.fainelli@gmail.com>
>> wrote:
>
>
>
>>
>> I have also mentioned this before, the default api must provide
>> accumulated (hw and sw) stats...,
>> because this is the api that the user queries on an interface.
>
>
> Sorry - I missed those discussions.
> What is current practise? Do people request for one via ip link
> stats and the other via ethtool?
> What do you guys do in your implementation?

for us the standard netlink api that returns netdev stats includes all
stats hw and sw.
When i say hw and sw, I mean some of the error counters can also include errors
counted by sw.

ethtool stats has always provided drivers/users with additional stats
that the hw or driver
can expose.


> Yes, it would be more accurate to provide aggregated stats but
> it may break backward compat if expectation is both are read
> separately today.

I don't think people see netdev stats as sw and ethtool as hw stats.
The latter just provides more granularity for debugging.
Thats the way i have looked at it forever.


> Maybe it makes sense to have a brand new TLV for these aggregated
> stats as Jiri was suggesting.That means two new TLVs not one.
> 1) TLV for aggregated stats - which cant be current one
> 2) TLV for h/w stats
>
> The existing stat implies s/ware only.
>

I don't think existing stat implies s/ware stats only. so, I think we
should be careful
about changing the meaning of existing stats.

logical devices like bridge stats have always been software only...but
with switchdev
the way we see these or implement these is to also include hardware stats when
they are hw offloaded. For us bridge vlan stats, vxlan stats and so on
will follow the
same model. You cannot introduce separate sw and hw stats for these.
All stats will have to follow a consistent model.

Thanks,
Roopa

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Roopa Prabhu]

On Fri, Jun 17, 2016 at 7:54 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 16-06-17 10:05 AM, Jiri Pirko wrote:
>>
>> Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>>
>>> On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>>
>>>>
>
>>
>> That is problematic. Existing apps depend on rtnetlink stats. But if we
>> don't count offloaded forwarded packets, the apps don't see anything.
>> Therefore I believe that this patchset approach is better. The existing
>> apps continue to work and future apps can use newly introduces sw_stats
>> to query slowpath traffic. Makes sense to me.
>>
>
> I agree with Jiri. It is a bad idea to depend on ethtool for any of
> this stuff.

The concern should not be that it is an ethtool api.
In all previous discussions on this patchset and also my
stats api patches, i have indicated that we have to move all stats
in one place, so naturally, ethtool stats should move eventually to the
stats api as a new nested netlink attribute. I think i called it
IFLA_STATS_LINK_HW  (or something like that)...
and this nested attribute should provide the flexibility and extensibility
of the current ethtool stats api.


> Is there a way we can tag netlink stats instead
> to indicate they are hardware or software?
> We already have a use case with the tc where someone could get/set
> hardware and/or software.
>
> cheers,
> jamal

Re: [patch net-next v4 0/4] return offloaded stats as default and expose original sw stats

[Roopa Prabhu]

On Fri, Jun 17, 2016 at 7:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
>>On 6/17/16 2:24 AM, Jiri Pirko wrote:
>>>
>>>The problem we try to handle is different, it's about offloaded
>>>forwarded packets which are not seen by kernel. Let me try to draw it :)
>>>
>>>    port1                       port2 (HW stats are counted here)
>>>      \                          /
>>>       \                        /
>>>        \                      /
>>>         --(A)---- ASIC --(B)--
>>>                    |
>>>                   (C)
>>>                    |
>>>                   CPU (SW stats are counted here)
>>>
>>>
>>>Now we have couple of flows for TX and RX (direction does not matter here):
>>>
>>>1) port1->A->ASIC->C->CPU
>>>
>>>   For this flow, HW and SW stats are equal.
>>>
>>>2) port1->A->ASIC->C->CPU->C->ASIC->B->port2
>>>
>>>   For this flow, HW and SW stats are equal.
>>>
>>>3) port1->A->ASIC->B->port2
>>>
>>>   For this flow, SW stats are 0.
>>>
>>>The purpose of this patchset is to provide facility for user to
>>>find out the difference between flows 1+2 and 3. In other words, user
>>>will be able to see the statistics for his slow-path (through kernel).
>>>
>>>Also, as a default the accumulated stats (HW) will be exposed to user
>>>so the userspace apps can react properly.
>>>
>>
>>You no longer agree with this discussion?
>>  http://comments.gmane.org/gmane.linux.network/346740
>>
>>Essentially netdevice stats show counters for packets punted to the cpu and
>>ethool -S shows h/w stats. This patch set seems to invert that.
>
> That is problematic. Existing apps depend on rtnetlink stats. But if we
> don't count offloaded forwarded packets, the apps don't see anything.
> Therefore I believe that this patchset approach is better. The existing
> apps continue to work and future apps can use newly introduces sw_stats
> to query slowpath traffic. Makes sense to me.
>

Apps only care about stats. they don't care about sw vs hardware
stats. what apps are these ?.
For debugging, I agree it would be useful, but thats why we have
always had ethtool stats which
the driver can break down and display. And in all my patches about the
new stats api, i have indicated
that we will migrate the existing ethtool stats to a new netlink
attribute in the new stats api.