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

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

[Jiri Pirko]

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.

---
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                      | 12 +++++
 include/uapi/linux/if_link.h                   |  2 +
 net/core/dev.c                                 | 25 ++++++++++
 net/core/rtnetlink.c                           | 55 +++++++++++++++++++---
 5 files changed, 145 insertions(+), 14 deletions(-)

-- 
2.5.5

[patch net-next 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>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h | 12 ++++++++++++
 net/core/dev.c            | 25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c2f5112..3ac3e8f 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,7 @@ 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);
 
 extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
diff --git a/net/core/dev.c b/net/core/dev.c
index 12436d1..a69e418 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,29 @@ 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);
+
 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 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>
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         | 20 ++++++++++++++++++++
 2 files changed, 21 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..a127d67 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1046,20 +1046,40 @@ 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)
 {
+	struct rtnl_link_stats64 sw_stats;
 	struct rtnl_link_stats64 *sp;
 	struct nlattr *attr;
+	int err;
 
 	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);
 
+	err = dev_get_sw_stats(dev, &sw_stats);
+	if (!err) {
+		attr = nla_reserve_64bit(skb, IFLA_SW_STATS64,
+					 sizeof(struct rtnl_link_stats64),
+					 IFLA_PAD);
+
+		if (!attr)
+			return -EMSGSIZE;
+
+		copy_rtnl_link_stats64(nla_data(attr), &sw_stats);
+	}
+
 	attr = nla_reserve(skb, IFLA_STATS,
 			   sizeof(struct rtnl_link_stats));
 	if (!attr)
-- 
2.5.5

[patch net-next 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>
---
v1->v2:
- fixed NULL initialization
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 35 +++++++++++++++++++++++++++++------
 2 files changed, 30 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 a127d67..9f670a8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3481,6 +3481,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();
 
@@ -3493,24 +3496,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);
@@ -3525,6 +3524,27 @@ 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);
+		err = dev_get_sw_stats(dev, sp);
+		if (err) {
+			/* if err it means there is no dedicated ndo to
+			 * get SW stats - so it is returned by the default
+			 * stats ndo
+			 */
+			if (stats64_sp)
+				copy_rtnl_link_stats64(sp, stats64_sp);
+			else
+				dev_get_stats(dev, sp);
+		}
+	}
+
 	nlmsg_end(skb, nlh);
 
 	return 0;
@@ -3541,6 +3561,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,
@@ -3550,6 +3571,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 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>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 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

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

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 12 May 2016 13:48:48 +0200

>  	sp = nla_data(attr);
>  	dev_get_stats(dev, sp);
>  
> +	err = dev_get_sw_stats(dev, &sw_stats);
> +	if (!err) {
> +		attr = nla_reserve_64bit(skb, IFLA_SW_STATS64,
> +					 sizeof(struct rtnl_link_stats64),
> +					 IFLA_PAD);
> +
> +		if (!attr)
> +			return -EMSGSIZE;
> +
> +		copy_rtnl_link_stats64(nla_data(attr), &sw_stats);
> +	}

Jiri, the whole point of the 64-bit padding stuff is so that we don't
need to copy the stats twice, once onto an on-stack copy and then
again to the attribute.

The way this sw stats stuff is designed you can't do it right.  It is
absolutely essential that the nla_reserve_64bits() call occur first,
and then you pass nla_data(attr) directly into the sw stats NDO operation.

Please rearrange this whole mechanism so that you can pass the NLA
attribute pointer down into the driver rather than doing all of these
copies.

Thanks.

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

[Roopa Prabhu]

On 5/12/16, 4:48 AM, Jiri Pirko wrote:
> 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>
> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>
To me netdev stats is  combined 'SW + HW' stats for that netdev.
ndo_get_stats64 callback into the drivers does the magic of adding HW stats
to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
(logical or physical). HW stats is queried and added to the SW stats.
In fact, we represent all our offloaded netdev stats following this model.
This fits well for logical devices like vlan/bond/vxlan which are hardware offloaded too.

There is HW extended stats which today are represented by ethtool
and I had an example of a new nested attribute like IFLA_STATS_LINK_HW_EXTENDED
to provide ethtool like extensible stats. If people want pure hardware stats, they
get it via this. In which case I am not seeing the point in adding a new ndo and a new
stats attribute just for this. If you are looking for pure hardware stats, any reason we cant
just use the IFLA_STATS_LINK_HW_EXTENDED and add ethtool like hw stats ?.
This will be one place to look for all hardware stats and will be extensible too.

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

[Florian Fainelli]

2016-05-12 14:10 GMT-07:00 Roopa Prabhu <roopa@cumulusnetworks.com>:
> On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>> 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>
>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>
> To me netdev stats is  combined 'SW + HW' stats for that netdev.
> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
> (logical or physical). HW stats is queried and added to the SW stats.
> In fact, we represent all our offloaded netdev stats following this model.
> This fits well for logical devices like vlan/bond/vxlan which are hardware offloaded too.

Agreed, if you take a look at what the DSA slave network devices do in
net/dsa/slave.c they basically overload the switch-mainainted
statistics with some per-port net_device software stats. We would
probably want to do the same thing

>
> There is HW extended stats which today are represented by ethtool
> and I had an example of a new nested attribute like IFLA_STATS_LINK_HW_EXTENDED
> to provide ethtool like extensible stats. If people want pure hardware stats, they
> get it via this. In which case I am not seeing the point in adding a new ndo and a new
> stats attribute just for this. If you are looking for pure hardware stats, any reason we cant
> just use the IFLA_STATS_LINK_HW_EXTENDED and add ethtool like hw stats ?.
> This will be one place to look for all hardware stats and will be extensible too.

Agreed, worste case, you format your statistics such that they reflect
they come from the HW (e.g: hw_TxOctets...)
-- 
Florian

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

[Jiri Pirko]

Thu, May 12, 2016 at 11:10:08PM CEST, roopa@cumulusnetworks.com wrote:
>On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>> 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>
>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>
>To me netdev stats is  combined 'SW + HW' stats for that netdev.
>ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>(logical or physical). HW stats is queried and added to the SW stats.

I'm not sure I follow. HW stats already contain SW stats. Because on
slow path every packet that is not offloaded and goes through kernel is
counted into HW stats as well (because it goes through HW port). If you
do HW stats + SW stats, what you get makes no sense. Am I missing something?
Btw, looking at enic_get_stats, looks exactly what we introduce for
mlxsw in this patchset.

With this patchset, we only allow user to se the actual stats for
slow-path aka SW stats.


>In fact, we represent all our offloaded netdev stats following this model.
>This fits well for logical devices like vlan/bond/vxlan which are hardware offloaded too.
>
>There is HW extended stats which today are represented by ethtool
>and I had an example of a new nested attribute like IFLA_STATS_LINK_HW_EXTENDED
>to provide ethtool like extensible stats. If people want pure hardware stats, they
>get it via this. In which case I am not seeing the point in adding a new ndo and a new
>stats attribute just for this. If you are looking for pure hardware stats, any reason we cant
>just use the IFLA_STATS_LINK_HW_EXTENDED and add ethtool like hw stats ?.
>This will be one place to look for all hardware stats and will be extensible too.
>
>

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

[Jiri Pirko]

Fri, May 13, 2016 at 12:23:13AM CEST, f.fainelli@gmail.com wrote:
>2016-05-12 14:10 GMT-07:00 Roopa Prabhu <roopa@cumulusnetworks.com>:
>> On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>>> 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>
>>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>> ---
>>>
>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>> (logical or physical). HW stats is queried and added to the SW stats.
>> In fact, we represent all our offloaded netdev stats following this model.
>> This fits well for logical devices like vlan/bond/vxlan which are hardware offloaded too.
>
>Agreed, if you take a look at what the DSA slave network devices do in
>net/dsa/slave.c they basically overload the switch-mainainted
>statistics with some per-port net_device software stats. We would
>probably want to do the same thing

net/dsa/slave.c does not define ndo_get_stats*. I don't follow.


>
>>
>> There is HW extended stats which today are represented by ethtool
>> and I had an example of a new nested attribute like IFLA_STATS_LINK_HW_EXTENDED
>> to provide ethtool like extensible stats. If people want pure hardware stats, they
>> get it via this. In which case I am not seeing the point in adding a new ndo and a new
>> stats attribute just for this. If you are looking for pure hardware stats, any reason we cant
>> just use the IFLA_STATS_LINK_HW_EXTENDED and add ethtool like hw stats ?.
>> This will be one place to look for all hardware stats and will be extensible too.
>
>Agreed, worste case, you format your statistics such that they reflect
>they come from the HW (e.g: hw_TxOctets...)
>-- 
>Florian

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

[Roopa Prabhu]

On 5/12/16, 11:03 PM, Jiri Pirko wrote:
> Thu, May 12, 2016 at 11:10:08PM CEST, roopa@cumulusnetworks.com wrote:
>> On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>>> 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>
>>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>> ---
>>>
>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>> (logical or physical). HW stats is queried and added to the SW stats.
> I'm not sure I follow. HW stats already contain SW stats. Because on
> slow path every packet that is not offloaded and goes through kernel is
> counted into HW stats as well (because it goes through HW port). 
yes, correct... we don't want to double count those. But since these stats are
generally queried from hw, I am calling them HW stats.
you will not really maintain a software counter for this. But, the driver can maintain its own
 counters for rx and tx errors etc and I call these SW stats. They are counted at the driver.

> If you
> do HW stats + SW stats, what you get makes no sense. Am I missing something?
If you go by my definition of HW and SW stats above, on a ndo_get_stats64() call,
you will add the SW counters + HW counters and return. In my definition, the pkts
that was rx'ed or tx'ed successfully are always in the HW count.

> Btw, looking at enic_get_stats, looks exactly what we introduce for
> mlxsw in this patchset.

In enic_get_stats, the ones counted in software are the ones taken from 'enic->'
     net_stats->rx_over_errors = enic->rq_truncated_pkts;
     net_stats->rx_crc_errors = enic->rq_bad_fcs;

>
> With this patchset, we only allow user to se the actual stats for
> slow-path aka SW stats.
hmm...ok. But i am not sure how many will use this new attribute.
When you do 'ip -s link show' you really want all counters on that port
hardware or software does not matter at that point.

My suggestion to move this to ethtool like attribute is because that is an existing
 way to break down your stats which ever way you want. And the best part is it can be
customized (say rx_pkts_cpu_saw)

Thanks,
Roopa

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

[Jiri Pirko]

Fri, May 13, 2016 at 08:47:48PM CEST, roopa@cumulusnetworks.com wrote:
>On 5/12/16, 11:03 PM, Jiri Pirko wrote:
>> Thu, May 12, 2016 at 11:10:08PM CEST, roopa@cumulusnetworks.com wrote:
>>> On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>>>> 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>
>>>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>> ---
>>>>
>>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>>> (logical or physical). HW stats is queried and added to the SW stats.
>> I'm not sure I follow. HW stats already contain SW stats. Because on
>> slow path every packet that is not offloaded and goes through kernel is
>> counted into HW stats as well (because it goes through HW port). 
>yes, correct... we don't want to double count those. But since these stats are
>generally queried from hw, I am calling them HW stats.
>you will not really maintain a software counter for this. But, the driver can maintain its own
> counters for rx and tx errors etc and I call these SW stats. They are counted at the driver.
>
>> If you
>> do HW stats + SW stats, what you get makes no sense. Am I missing something?
>If you go by my definition of HW and SW stats above, on a ndo_get_stats64() call,
>you will add the SW counters + HW counters and return. In my definition, the pkts
>that was rx'ed or tx'ed successfully are always in the HW count.
>
>> Btw, looking at enic_get_stats, looks exactly what we introduce for
>> mlxsw in this patchset.
>
>In enic_get_stats, the ones counted in software are the ones taken from 'enic->'
>     net_stats->rx_over_errors = enic->rq_truncated_pkts;
>     net_stats->rx_crc_errors = enic->rq_bad_fcs;
>
>>
>> With this patchset, we only allow user to se the actual stats for
>> slow-path aka SW stats.
>hmm...ok. But i am not sure how many will use this new attribute.
>When you do 'ip -s link show' you really want all counters on that port
>hardware or software does not matter at that point.
>
>My suggestion to move this to ethtool like attribute is because that is an existing
> way to break down your stats which ever way you want. And the best part is it can be
>customized (say rx_pkts_cpu_saw)

I bevieve that ethtool is really not a place to expose sw stats. Does
not make sense.

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

[Roopa Prabhu]

On 5/14/16, 5:49 AM, Jiri Pirko wrote:
> Fri, May 13, 2016 at 08:47:48PM CEST, roopa@cumulusnetworks.com wrote:
>> On 5/12/16, 11:03 PM, Jiri Pirko wrote:
>>> Thu, May 12, 2016 at 11:10:08PM CEST, roopa@cumulusnetworks.com wrote:
>>>> On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>>>>> 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>
>>>>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>>> ---
>>>>>
>>>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>>>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>>>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>>>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>>>> (logical or physical). HW stats is queried and added to the SW stats.
>>> I'm not sure I follow. HW stats already contain SW stats. Because on
>>> slow path every packet that is not offloaded and goes through kernel is
>>> counted into HW stats as well (because it goes through HW port). 
>> yes, correct... we don't want to double count those. But since these stats are
>> generally queried from hw, I am calling them HW stats.
>> you will not really maintain a software counter for this. But, the driver can maintain its own
>> counters for rx and tx errors etc and I call these SW stats. They are counted at the driver.
>>
>>> If you
>>> do HW stats + SW stats, what you get makes no sense. Am I missing something?
>> If you go by my definition of HW and SW stats above, on a ndo_get_stats64() call,
>> you will add the SW counters + HW counters and return. In my definition, the pkts
>> that was rx'ed or tx'ed successfully are always in the HW count.
>>
>>> Btw, looking at enic_get_stats, looks exactly what we introduce for
>>> mlxsw in this patchset.
>> In enic_get_stats, the ones counted in software are the ones taken from 'enic->'
>>     net_stats->rx_over_errors = enic->rq_truncated_pkts;
>>     net_stats->rx_crc_errors = enic->rq_bad_fcs;
>>
>>> With this patchset, we only allow user to se the actual stats for
>>> slow-path aka SW stats.
>> hmm...ok. But i am not sure how many will use this new attribute.
>> When you do 'ip -s link show' you really want all counters on that port
>> hardware or software does not matter at that point.
>>
>> My suggestion to move this to ethtool like attribute is because that is an existing
>> way to break down your stats which ever way you want. And the best part is it can be
>> customized (say rx_pkts_cpu_saw)
> I bevieve that ethtool is really not a place to expose sw stats. Does
> not make sense.
2 things:
- i was surprised you don't want your ndo_get_stats64 to be a unified view of HW and SW stats
- by bringing up ethtool like stats (IFLA_STATS_LINK_HW_EXTENDED) I am just saying
it has always been a way to breakdown stats. If you don't want to show explicit SW stats there,
there is always a way to show HW only stats....and now you know the delta between the unified stats
and the HW only stats is your SW stats.

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

[Jiri Pirko]

Sat, May 14, 2016 at 05:47:41PM CEST, roopa@cumulusnetworks.com wrote:
>On 5/14/16, 5:49 AM, Jiri Pirko wrote:
>> Fri, May 13, 2016 at 08:47:48PM CEST, roopa@cumulusnetworks.com wrote:
>>> On 5/12/16, 11:03 PM, Jiri Pirko wrote:
>>>> Thu, May 12, 2016 at 11:10:08PM CEST, roopa@cumulusnetworks.com wrote:
>>>>> On 5/12/16, 4:48 AM, Jiri Pirko wrote:
>>>>>> 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>
>>>>>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>>>>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>>>>> ---
>>>>>>
>>>>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>>>>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>>>>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>>>>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>>>>> (logical or physical). HW stats is queried and added to the SW stats.
>>>> I'm not sure I follow. HW stats already contain SW stats. Because on
>>>> slow path every packet that is not offloaded and goes through kernel is
>>>> counted into HW stats as well (because it goes through HW port). 
>>> yes, correct... we don't want to double count those. But since these stats are
>>> generally queried from hw, I am calling them HW stats.
>>> you will not really maintain a software counter for this. But, the driver can maintain its own
>>> counters for rx and tx errors etc and I call these SW stats. They are counted at the driver.
>>>
>>>> If you
>>>> do HW stats + SW stats, what you get makes no sense. Am I missing something?
>>> If you go by my definition of HW and SW stats above, on a ndo_get_stats64() call,
>>> you will add the SW counters + HW counters and return. In my definition, the pkts
>>> that was rx'ed or tx'ed successfully are always in the HW count.
>>>
>>>> Btw, looking at enic_get_stats, looks exactly what we introduce for
>>>> mlxsw in this patchset.
>>> In enic_get_stats, the ones counted in software are the ones taken from 'enic->'
>>>     net_stats->rx_over_errors = enic->rq_truncated_pkts;
>>>     net_stats->rx_crc_errors = enic->rq_bad_fcs;
>>>
>>>> With this patchset, we only allow user to se the actual stats for
>>>> slow-path aka SW stats.
>>> hmm...ok. But i am not sure how many will use this new attribute.
>>> When you do 'ip -s link show' you really want all counters on that port
>>> hardware or software does not matter at that point.
>>>
>>> My suggestion to move this to ethtool like attribute is because that is an existing
>>> way to break down your stats which ever way you want. And the best part is it can be
>>> customized (say rx_pkts_cpu_saw)
>> I bevieve that ethtool is really not a place to expose sw stats. Does
>> not make sense.
>2 things:
>- i was surprised you don't want your ndo_get_stats64 to be a unified view of HW and SW stats

Roopa, please, look at the patch 4/4. That is exactly what we are doing.
We expose HW stats via ndo_get_stats64 and that is of course including
whatever comes through slowpath (non-forwarded in HW).


>- by bringing up ethtool like stats (IFLA_STATS_LINK_HW_EXTENDED) I am just saying
>it has always been a way to breakdown stats. If you don't want to show explicit SW stats there,
>there is always a way to show HW only stats....and now you know the delta between the unified stats
>and the HW only stats is your SW stats.

I think we don/t understand each other. HW stats always include SW
stats. Because whatever goes in or out goes through HW. Therefore, the
"unified stats" you mention are exactly HW stats.

This is fine, Patch 4/4 would do to make this correct. However, I think
it has value for user to know what went via slowpath (non-forwarded in HW).
And that is exacly exposed by the SW stats we try to add.

Is that confusing?

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

[Roopa Prabhu]

On 5/14/16, 11:46 AM, Jiri Pirko wrote:
> Sat, May 14, 2016 at 05:47:41PM CEST, roopa@cumulusnetworks.com wrote:
>> On 5/14/16, 5:49 AM, Jiri Pirko wrote:
>>> Fri, May 13, 2016 at 08:47:48PM CEST, roopa@cumulusnetworks.com wrote:
>>>>
[snip]
>>>>  Jiri Pirko <jiri@mellanox.com>
>>>> ---
>>>>
>>>>>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>>>>>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>>>>>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>>>>>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>>>>>> (logical or physical). HW stats is queried and added to the SW stats.
>>>>> I'm not sure I follow. HW stats already contain SW stats. Because on
>>>>> slow path every packet that is not offloaded and goes through kernel is
>>>>> counted into HW stats as well (because it goes through HW port). 
>>>> yes, correct... we don't want to double count those. But since these stats are
>>>> generally queried from hw, I am calling them HW stats.
>>>> you will not really maintain a software counter for this. But, the driver can maintain its own
>>>> counters for rx and tx errors etc and I call these SW stats. They are counted at the driver.
>>>>
>>>>> If you
>>>>> do HW stats + SW stats, what you get makes no sense. Am I missing something?
>>>> If you go by my definition of HW and SW stats above, on a ndo_get_stats64() call,
>>>> you will add the SW counters + HW counters and return. In my definition, the pkts
>>>> that was rx'ed or tx'ed successfully are always in the HW count.
>>>>
>>>>> Btw, looking at enic_get_stats, looks exactly what we introduce for
>>>>> mlxsw in this patchset.
>>>> In enic_get_stats, the ones counted in software are the ones taken from 'enic->'
>>>>     net_stats->rx_over_errors = enic->rq_truncated_pkts;
>>>>     net_stats->rx_crc_errors = enic->rq_bad_fcs;
>>>>
>>>>> With this patchset, we only allow user to se the actual stats for
>>>>> slow-path aka SW stats.
>>>> hmm...ok. But i am not sure how many will use this new attribute.
>>>> When you do 'ip -s link show' you really want all counters on that port
>>>> hardware or software does not matter at that point.
>>>>
>>>> My suggestion to move this to ethtool like attribute is because that is an existing
>>>> way to break down your stats which ever way you want. And the best part is it can be
>>>> customized (say rx_pkts_cpu_saw)
>>> I bevieve that ethtool is really not a place to expose sw stats. Does
>>> not make sense.
>> 2 things:
>> - i was surprised you don't want your ndo_get_stats64 to be a unified view of HW and SW stats
> Roopa, please, look at the patch 4/4. That is exactly what we are doing.
> We expose HW stats via ndo_get_stats64 and that is of course including
> whatever comes through slowpath (non-forwarded in HW).

Maybe i missed it but i did not think it included any rx or tx err counters counted solely
by the driver.
>
>
>> - by bringing up ethtool like stats (IFLA_STATS_LINK_HW_EXTENDED) I am just saying
>> it has always been a way to breakdown stats. If you don't want to show explicit SW stats there,
>> there is always a way to show HW only stats....and now you know the delta between the unified stats
>> and the HW only stats is your SW stats.
> I think we don/t understand each other. HW stats always include SW
> stats. Because whatever goes in or out goes through HW. Therefore, the
> "unified stats" you mention are exactly HW stats.
>
> This is fine, Patch 4/4 would do to make this correct. However, I think
> it has value for user to know what went via slowpath (non-forwarded in HW).
> And that is exacly exposed by the SW stats we try to add.
>
> Is that confusing?

Its not confusing. I understand what you are doing.
The only point I was making was that most drivers have unified stats via ndo
and there are also hw stats via ethtool like api (which will also be part of the stats
api in the future). And sw only stats can be derived from that...which is the way most
people do today.
But that's fine. If you think it will be useful/easier to have a new api/attribute
for software only stats for some drivers, sure, fine. Lets move on.

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

[Jiri Pirko]

Sun, May 15, 2016 at 06:11:20AM CEST, roopa@cumulusnetworks.com wrote:
>On 5/14/16, 11:46 AM, Jiri Pirko wrote:
>> Sat, May 14, 2016 at 05:47:41PM CEST, roopa@cumulusnetworks.com wrote:
>>> On 5/14/16, 5:49 AM, Jiri Pirko wrote:
>>>> Fri, May 13, 2016 at 08:47:48PM CEST, roopa@cumulusnetworks.com wrote:
>>>>>
>[snip]
>>>>>  Jiri Pirko <jiri@mellanox.com>
>>>>> ---
>>>>>
>>>>>>> To me netdev stats is  combined 'SW + HW' stats for that netdev.
>>>>>>> ndo_get_stats64 callback into the drivers does the magic of adding HW stats
>>>>>>> to SW (netdev) stats and returning (see enic_get_stats). HW stats is available for netdevs
>>>>>>> that are offloaded or are backed by hardware. SW stats is the stats that the driver maintains
>>>>>>> (logical or physical). HW stats is queried and added to the SW stats.
>>>>>> I'm not sure I follow. HW stats already contain SW stats. Because on
>>>>>> slow path every packet that is not offloaded and goes through kernel is
>>>>>> counted into HW stats as well (because it goes through HW port). 
>>>>> yes, correct... we don't want to double count those. But since these stats are
>>>>> generally queried from hw, I am calling them HW stats.
>>>>> you will not really maintain a software counter for this. But, the driver can maintain its own
>>>>> counters for rx and tx errors etc and I call these SW stats. They are counted at the driver.
>>>>>
>>>>>> If you
>>>>>> do HW stats + SW stats, what you get makes no sense. Am I missing something?
>>>>> If you go by my definition of HW and SW stats above, on a ndo_get_stats64() call,
>>>>> you will add the SW counters + HW counters and return. In my definition, the pkts
>>>>> that was rx'ed or tx'ed successfully are always in the HW count.
>>>>>
>>>>>> Btw, looking at enic_get_stats, looks exactly what we introduce for
>>>>>> mlxsw in this patchset.
>>>>> In enic_get_stats, the ones counted in software are the ones taken from 'enic->'
>>>>>     net_stats->rx_over_errors = enic->rq_truncated_pkts;
>>>>>     net_stats->rx_crc_errors = enic->rq_bad_fcs;
>>>>>
>>>>>> With this patchset, we only allow user to se the actual stats for
>>>>>> slow-path aka SW stats.
>>>>> hmm...ok. But i am not sure how many will use this new attribute.
>>>>> When you do 'ip -s link show' you really want all counters on that port
>>>>> hardware or software does not matter at that point.
>>>>>
>>>>> My suggestion to move this to ethtool like attribute is because that is an existing
>>>>> way to break down your stats which ever way you want. And the best part is it can be
>>>>> customized (say rx_pkts_cpu_saw)
>>>> I bevieve that ethtool is really not a place to expose sw stats. Does
>>>> not make sense.
>>> 2 things:
>>> - i was surprised you don't want your ndo_get_stats64 to be a unified view of HW and SW stats
>> Roopa, please, look at the patch 4/4. That is exactly what we are doing.
>> We expose HW stats via ndo_get_stats64 and that is of course including
>> whatever comes through slowpath (non-forwarded in HW).
>
>Maybe i missed it but i did not think it included any rx or tx err counters counted solely
>by the driver.
>>
>>
>>> - by bringing up ethtool like stats (IFLA_STATS_LINK_HW_EXTENDED) I am just saying
>>> it has always been a way to breakdown stats. If you don't want to show explicit SW stats there,
>>> there is always a way to show HW only stats....and now you know the delta between the unified stats
>>> and the HW only stats is your SW stats.
>> I think we don/t understand each other. HW stats always include SW
>> stats. Because whatever goes in or out goes through HW. Therefore, the
>> "unified stats" you mention are exactly HW stats.
>>
>> This is fine, Patch 4/4 would do to make this correct. However, I think
>> it has value for user to know what went via slowpath (non-forwarded in HW).
>> And that is exacly exposed by the SW stats we try to add.
>>
>> Is that confusing?
>
>Its not confusing. I understand what you are doing.
>The only point I was making was that most drivers have unified stats via ndo
>and there are also hw stats via ethtool like api (which will also be part of the stats
>api in the future). And sw only stats can be derived from that...which is the way most

The thing is, they can't be derived from it. That is my whole point.
HW-HW=0


>people do today.
>But that's fine. If you think it will be useful/easier to have a new api/attribute
>for software only stats for some drivers, sure, fine. Lets move on.
>
>

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

[Andrew Lunn]

> I think we don/t understand each other. HW stats always include SW
> stats. Because whatever goes in or out goes through HW.

Hi Jiri

Bit of a corner case, but what about multicast and broadcast? Can you
do the replication in the switch, so that only a single copy is sent
from the host to the switch?

    Andrew

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

[Jiri Pirko]

Sun, May 15, 2016 at 05:44:11PM CEST, andrew@lunn.ch wrote:
>> I think we don/t understand each other. HW stats always include SW
>> stats. Because whatever goes in or out goes through HW.
>
>Hi Jiri
>
>Bit of a corner case, but what about multicast and broadcast? Can you
>do the replication in the switch, so that only a single copy is sent
>from the host to the switch?

If kernel is originator of such packets, it always sends them through
specific netdevs.

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

[Thomas Graf]

On 05/12/16 at 01:48pm, Jiri Pirko wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 12436d1..a69e418 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)

Can we clarify in a comment that given HW stats are available whether this
would continue to account for something like a memory allocation failure
through tx_dropped (for drivers which do skb_realloc_headroom() in xmit)
or tx_busy for when the ring is busy? HW would never see such failures Is
the expectation that dev_get_stats() continues to include these software
failures?

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

[Jiri Pirko]

Tue, May 17, 2016 at 10:39:08AM CEST, tgraf@suug.ch wrote:
>On 05/12/16 at 01:48pm, Jiri Pirko wrote:
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 12436d1..a69e418 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)
>
>Can we clarify in a comment that given HW stats are available whether this
>would continue to account for something like a memory allocation failure
>through tx_dropped (for drivers which do skb_realloc_headroom() in xmit)
>or tx_busy for when the ring is busy? HW would never see such failures Is
>the expectation that dev_get_stats() continues to include these software
>failures?

I think it makes sense to merge HW counters with the kernel error counters
you mention. We'll fix that. Thanks.

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

[Roopa Prabhu]

On 5/17/16, 1:41 AM, Jiri Pirko wrote:
> Tue, May 17, 2016 at 10:39:08AM CEST, tgraf@suug.ch wrote:
>> On 05/12/16 at 01:48pm, Jiri Pirko wrote:
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 12436d1..a69e418 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)
>> Can we clarify in a comment that given HW stats are available whether this
>> would continue to account for something like a memory allocation failure
>> through tx_dropped (for drivers which do skb_realloc_headroom() in xmit)
>> or tx_busy for when the ring is busy? HW would never see such failures Is
>> the expectation that dev_get_stats() continues to include these software
>> failures?
> I think it makes sense to merge HW counters with the kernel error counters
> you mention. We'll fix that. Thanks.
yep. This is what i was pointing out as well with enic and possibly other drivers.
Mellanox driver does it or not...,..the comment should definitely be fixed since
some drivers already do this.

[patch net-next 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>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netdevice.h | 12 ++++++++++++
 net/core/dev.c            | 25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c2f5112..3ac3e8f 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,7 @@ 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);
 
 extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
diff --git a/net/core/dev.c b/net/core/dev.c
index 12436d1..a69e418 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,29 @@ 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);
+
 struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
 {
 	struct netdev_queue *queue = dev_ingress_queue(dev);
-- 
2.5.5