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

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

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

The problem we try to handle is 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 the slow-path (through kernel).

Also note that HW stats are what someone calls "accumulated" stats.
Every packet counted by SW is also counted by HW. Not the other way around.

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

---
v4->v5:
- updated cover letter
- patch3/4:
  - using memcpy directly to copy stats as requested by DaveM
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 requested 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                           |  49 +++++++++--
 6 files changed, 195 insertions(+), 15 deletions(-)

-- 
2.5.5

[patch net-next v5 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 e84d9d2..e8e2172 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -892,6 +892,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
@@ -1121,6 +1129,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,
@@ -3759,6 +3770,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 d40593b..8ed28ea 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 v5 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 v5 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         | 36 ++++++++++++++++++++++++++++++------
 2 files changed, 31 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..eed015285 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3490,10 +3490,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 +3509,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 +3537,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)
+				memcpy(sp, stats64_sp, sizeof(*sp));
+			else
+				dev_get_stats(dev, sp);
+		}
+	}
+
 	nlmsg_end(skb, nlh);
 
 	return 0;
@@ -3554,6 +3575,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 +3585,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 v5 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 d23948b..855b1c0 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -534,8 +534,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;
@@ -565,6 +565,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)
 {
@@ -978,6 +1061,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,
@@ -1198,15 +1282,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;
 }
@@ -1718,6 +1797,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;
 
@@ -1814,6 +1903,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:
@@ -1830,6 +1920,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);
@@ -1863,6 +1955,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 */
@@ -1872,6 +1965,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 36c9835..e393c2c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -240,6 +240,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 v5 0/4] return offloaded stats as default and expose original sw stats

[Roopa Prabhu]

On Tue, Jun 21, 2016 at 8:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> The problem we try to handle is 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 the slow-path (through kernel).
>
> Also note that HW stats are what someone calls "accumulated" stats.
> Every packet counted by SW is also counted by HW. Not the other way around.
>
> As a default the accumulated stats (HW) will be exposed to user
> so the userspace apps can react properly.
>
>

curious, how do you plan to handle virtual device counters like vlan
and vxlan stats ?.
we can't separate CPU and HW stats there. In some cases (or ASICs) HW
counters do
not include CPU generated packets....you will have to add CPU
generated pkt counters to the
hw counters for such virtual device stats.

example: In the switchdev model, for bridge vlan stats, when user
queries bridge vlan stats,
you will have to add the hw stats to the bridge driver vlan stats and
return it to the user .

Having a consistent model for all kinds of stats will help.

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

[Jiri Pirko]

Wed, Jun 22, 2016 at 09:32:25PM CEST, roopa@cumulusnetworks.com wrote:
>On Tue, Jun 21, 2016 at 8:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> The problem we try to handle is 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 the slow-path (through kernel).
>>
>> Also note that HW stats are what someone calls "accumulated" stats.
>> Every packet counted by SW is also counted by HW. Not the other way around.
>>
>> As a default the accumulated stats (HW) will be exposed to user
>> so the userspace apps can react properly.
>>
>>
>
>curious, how do you plan to handle virtual device counters like vlan
>and vxlan stats ?.

Yes, that is another problem (1). We have to push stats up to this devices
most probably. But that problem is orthogonal to this. To the user, you
will still need 2 sets of stats and HW stats being default. So this
patchset infra is going to be used as well.


>we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>counters do
>not include CPU generated packets....you will have to add CPU
>generated pkt counters to the
>hw counters for such virtual device stats.

Can you please provide and example how that could happen?


>
>example: In the switchdev model, for bridge vlan stats, when user
>queries bridge vlan stats,
>you will have to add the hw stats to the bridge driver vlan stats and
>return it to the user .

Yep, that is (1).


>
>Having a consistent model for all kinds of stats will help.

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

[Roopa Prabhu]

On 6/22/16, 10:40 PM, Jiri Pirko wrote:
> Wed, Jun 22, 2016 at 09:32:25PM CEST, roopa@cumulusnetworks.com wrote:
>> On Tue, Jun 21, 2016 at 8:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> The problem we try to handle is 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 the slow-path (through kernel).
>>>
>>> Also note that HW stats are what someone calls "accumulated" stats.
>>> Every packet counted by SW is also counted by HW. Not the other way around.
>>>
>>> As a default the accumulated stats (HW) will be exposed to user
>>> so the userspace apps can react properly.
>>>
>>>
>> curious, how do you plan to handle virtual device counters like vlan
>> and vxlan stats ?.
> Yes, that is another problem (1). We have to push stats up to this devices
> most probably. But that problem is orthogonal to this. To the user, you
> will still need 2 sets of stats and HW stats being default. So this
> patchset infra is going to be used as well.
hmm...But, i don't think we should start adding different tlv's hw and sw for
every stats variant we add.
>
>
>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>> counters do
>> not include CPU generated packets....you will have to add CPU
>> generated pkt counters to the
>> hw counters for such virtual device stats.
> Can you please provide and example how that could happen?

example is the bridge vlan stats I mention below. These are usually counted
by attaching hw virtual counter resources. And CPU generated packets
in some cases maybe setup to bypass the ASIC pipeline because the CPU
has already made the required decisions. So, they may not be counted by
by such hw virtual counters.

>
>
>> example: In the switchdev model, for bridge vlan stats, when user
>> queries bridge vlan stats,
>> you will have to add the hw stats to the bridge driver vlan stats and
>> return it to the user .
> Yep, that is (1).

unless i misunderstood, this does not look like (1). In (1) you say hw stats
 already reflect sw stats. But in this case, hw counter does not include sw stats
for CPU generated packets.
>
>
>> Having a consistent model for all kinds of stats will help.

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

[Jiri Pirko]

Thu, Jun 23, 2016 at 01:27:35PM CEST, roopa@cumulusnetworks.com wrote:
>On 6/22/16, 10:40 PM, Jiri Pirko wrote:
>> Wed, Jun 22, 2016 at 09:32:25PM CEST, roopa@cumulusnetworks.com wrote:
>>> On Tue, Jun 21, 2016 at 8:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> The problem we try to handle is 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 the slow-path (through kernel).
>>>>
>>>> Also note that HW stats are what someone calls "accumulated" stats.
>>>> Every packet counted by SW is also counted by HW. Not the other way around.
>>>>
>>>> As a default the accumulated stats (HW) will be exposed to user
>>>> so the userspace apps can react properly.
>>>>
>>>>
>>> curious, how do you plan to handle virtual device counters like vlan
>>> and vxlan stats ?.
>> Yes, that is another problem (1). We have to push stats up to this devices
>> most probably. But that problem is orthogonal to this. To the user, you
>> will still need 2 sets of stats and HW stats being default. So this
>> patchset infra is going to be used as well.
>hmm...But, i don't think we should start adding different tlv's hw and sw for
>every stats variant we add.
>>
>>
>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>> counters do
>>> not include CPU generated packets....you will have to add CPU
>>> generated pkt counters to the
>>> hw counters for such virtual device stats.
>> Can you please provide and example how that could happen?
>
>example is the bridge vlan stats I mention below. These are usually counted
>by attaching hw virtual counter resources. And CPU generated packets
>in some cases maybe setup to bypass the ASIC pipeline because the CPU
>has already made the required decisions. So, they may not be counted by
>by such hw virtual counters.

Bypass ASIC? How do the packets get on the wire?


>
>>
>>
>>> example: In the switchdev model, for bridge vlan stats, when user
>>> queries bridge vlan stats,
>>> you will have to add the hw stats to the bridge driver vlan stats and
>>> return it to the user .
>> Yep, that is (1).
>
>unless i misunderstood, this does not look like (1). In (1) you say hw stats
> already reflect sw stats. But in this case, hw counter does not include sw stats
>for CPU generated packets.
>>
>>
>>> Having a consistent model for all kinds of stats will help.
>

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

[Anuradha Karuppiah]

>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>> counters do
>>>> not include CPU generated packets....you will have to add CPU
>>>> generated pkt counters to the
>>>> hw counters for such virtual device stats.
>>> Can you please provide and example how that could happen?
>>
>>example is the bridge vlan stats I mention below. These are usually counted
>>by attaching hw virtual counter resources. And CPU generated packets
>>in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>has already made the required decisions. So, they may not be counted by
>>by such hw virtual counters.
>
> Bypass ASIC? How do the packets get on the wire?
>

Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
ASIC ships the CPU generated packet out of the switch/front-panel
port. Continuing Roopa's example of vlan netdev stats.... To get the
HW stats counters are typically tied to the ingress and egress vlan hw
entries. All the incoming packets are subject to the ingress vlan
lookup irrespective of whether they get punted to the CPU or whether
they are forwarded to another front panel port. In that case the
ingress HW stats does represent all packets. However for CPU
originated packets egress vlan lookups are bypassed in the ASIC (this
is common forwarding option in most ASICs) and the packet shipped as
is out of front-panel port specified by the CPU. Which means these
packets will NOT be counted against the egress VLAN HW counter; hence
the need for summation.

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

[Jiri Pirko]

Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>>> counters do
>>>>> not include CPU generated packets....you will have to add CPU
>>>>> generated pkt counters to the
>>>>> hw counters for such virtual device stats.
>>>> Can you please provide and example how that could happen?
>>>
>>>example is the bridge vlan stats I mention below. These are usually counted
>>>by attaching hw virtual counter resources. And CPU generated packets
>>>in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>>has already made the required decisions. So, they may not be counted by
>>>by such hw virtual counters.
>>
>> Bypass ASIC? How do the packets get on the wire?
>>
>
>Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>ASIC ships the CPU generated packet out of the switch/front-panel
>port. Continuing Roopa's example of vlan netdev stats.... To get the
>HW stats counters are typically tied to the ingress and egress vlan hw
>entries. All the incoming packets are subject to the ingress vlan
>lookup irrespective of whether they get punted to the CPU or whether
>they are forwarded to another front panel port. In that case the
>ingress HW stats does represent all packets. However for CPU
>originated packets egress vlan lookups are bypassed in the ASIC (this
>is common forwarding option in most ASICs) and the packet shipped as
>is out of front-panel port specified by the CPU. Which means these
>packets will NOT be counted against the egress VLAN HW counter; hence
>the need for summation.

Driver will know about this, and will provide the stats accordignly to
the core. Who else than driver should resolve this.

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

[Roopa Prabhu]

On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>>>> counters do
>>>>>> not include CPU generated packets....you will have to add CPU
>>>>>> generated pkt counters to the
>>>>>> hw counters for such virtual device stats.
>>>>> Can you please provide and example how that could happen?
>>>>
>>>>example is the bridge vlan stats I mention below. These are usually counted
>>>>by attaching hw virtual counter resources. And CPU generated packets
>>>>in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>>>has already made the required decisions. So, they may not be counted by
>>>>by such hw virtual counters.
>>>
>>> Bypass ASIC? How do the packets get on the wire?
>>>
>>
>>Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>>ASIC ships the CPU generated packet out of the switch/front-panel
>>port. Continuing Roopa's example of vlan netdev stats.... To get the
>>HW stats counters are typically tied to the ingress and egress vlan hw
>>entries. All the incoming packets are subject to the ingress vlan
>>lookup irrespective of whether they get punted to the CPU or whether
>>they are forwarded to another front panel port. In that case the
>>ingress HW stats does represent all packets. However for CPU
>>originated packets egress vlan lookups are bypassed in the ASIC (this
>>is common forwarding option in most ASICs) and the packet shipped as
>>is out of front-panel port specified by the CPU. Which means these
>>packets will NOT be counted against the egress VLAN HW counter; hence
>>the need for summation.
>
> Driver will know about this, and will provide the stats accordignly to
> the core. Who else than driver should resolve this.
>

The point was/is that there should be only two categories:
1) the base default stats: can contain 'only sw', 'only hw' or 'a
summation of hw and sw' in some cases.
The user does not care about the breakdown.

2) everything else falls into the second category: driver provided
breakdown of stats for easier debugging.
This today is ethtool stats and we can have an equivalent nested
attribute for this in the new stats api.
Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
nested and extensible (like ethtool is) and
driver can expose any kind of stats there.
ie lets move the stats you are proposing to this category of stats.....
instead of introducing a third category 'SW stats'.

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

[Jiri Pirko]

Sat, Jun 25, 2016 at 05:50:59PM CEST, roopa@cumulusnetworks.com wrote:
>On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>>>>> counters do
>>>>>>> not include CPU generated packets....you will have to add CPU
>>>>>>> generated pkt counters to the
>>>>>>> hw counters for such virtual device stats.
>>>>>> Can you please provide and example how that could happen?
>>>>>
>>>>>example is the bridge vlan stats I mention below. These are usually counted
>>>>>by attaching hw virtual counter resources. And CPU generated packets
>>>>>in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>>>>has already made the required decisions. So, they may not be counted by
>>>>>by such hw virtual counters.
>>>>
>>>> Bypass ASIC? How do the packets get on the wire?
>>>>
>>>
>>>Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>>>ASIC ships the CPU generated packet out of the switch/front-panel
>>>port. Continuing Roopa's example of vlan netdev stats.... To get the
>>>HW stats counters are typically tied to the ingress and egress vlan hw
>>>entries. All the incoming packets are subject to the ingress vlan
>>>lookup irrespective of whether they get punted to the CPU or whether
>>>they are forwarded to another front panel port. In that case the
>>>ingress HW stats does represent all packets. However for CPU
>>>originated packets egress vlan lookups are bypassed in the ASIC (this
>>>is common forwarding option in most ASICs) and the packet shipped as
>>>is out of front-panel port specified by the CPU. Which means these
>>>packets will NOT be counted against the egress VLAN HW counter; hence
>>>the need for summation.
>>
>> Driver will know about this, and will provide the stats accordignly to
>> the core. Who else than driver should resolve this.
>>
>
>The point was/is that there should be only two categories:
>1) the base default stats: can contain 'only sw', 'only hw' or 'a
>summation of hw and sw' in some cases.
>The user does not care about the breakdown.
>
>2) everything else falls into the second category: driver provided
>breakdown of stats for easier debugging.
>This today is ethtool stats and we can have an equivalent nested
>attribute for this in the new stats api.
>Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
>nested and extensible (like ethtool is) and
>driver can expose any kind of stats there.
>ie lets move the stats you are proposing to this category of stats.....
>instead of introducing a third category 'SW stats'.

What you are proposing is essentially what our patchset does. We expose
2 sets of stats. hw and pure sw. hw includes all, driver will take
care of it cause he knows what is going on in hw.

Btw mirroring random string stats into Netlink is not a good idea IMO.

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

[Roopa Prabhu]

On 6/26/16, 2:33 AM, Jiri Pirko wrote:
> Sat, Jun 25, 2016 at 05:50:59PM CEST, roopa@cumulusnetworks.com wrote:
>> On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>>>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>>>>>> counters do
>>>>>>>> not include CPU generated packets....you will have to add CPU
>>>>>>>> generated pkt counters to the
>>>>>>>> hw counters for such virtual device stats.
>>>>>>> Can you please provide and example how that could happen?
>>>>>> example is the bridge vlan stats I mention below. These are usually counted
>>>>>> by attaching hw virtual counter resources. And CPU generated packets
>>>>>> in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>>>>> has already made the required decisions. So, they may not be counted by
>>>>>> by such hw virtual counters.
>>>>> Bypass ASIC? How do the packets get on the wire?
>>>>>
>>>> Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>>>> ASIC ships the CPU generated packet out of the switch/front-panel
>>>> port. Continuing Roopa's example of vlan netdev stats.... To get the
>>>> HW stats counters are typically tied to the ingress and egress vlan hw
>>>> entries. All the incoming packets are subject to the ingress vlan
>>>> lookup irrespective of whether they get punted to the CPU or whether
>>>> they are forwarded to another front panel port. In that case the
>>>> ingress HW stats does represent all packets. However for CPU
>>>> originated packets egress vlan lookups are bypassed in the ASIC (this
>>>> is common forwarding option in most ASICs) and the packet shipped as
>>>> is out of front-panel port specified by the CPU. Which means these
>>>> packets will NOT be counted against the egress VLAN HW counter; hence
>>>> the need for summation.
>>> Driver will know about this, and will provide the stats accordignly to
>>> the core. Who else than driver should resolve this.
>>>
>> The point was/is that there should be only two categories:
>> 1) the base default stats: can contain 'only sw', 'only hw' or 'a
>> summation of hw and sw' in some cases.
>> The user does not care about the breakdown.
>>
>> 2) everything else falls into the second category: driver provided
>> breakdown of stats for easier debugging.
>> This today is ethtool stats and we can have an equivalent nested
>> attribute for this in the new stats api.
>> Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
>> nested and extensible (like ethtool is) and
>> driver can expose any kind of stats there.
>> ie lets move the stats you are proposing to this category of stats.....
>> instead of introducing a third category 'SW stats'.
> What you are proposing is essentially what our patchset does. We expose
> 2 sets of stats. hw and pure sw. hw includes all, driver will take
> care of it cause he knows what is going on in hw.
the splitting into hw and sw is causing some confusion with respect
to existing stats and will be confusing for future stats. And i am not sure how many
users would prefer the split this way.
So, instead of doing the split, i think we should at this time introduce
driver specific stats (like ethtool) as a nested netlink attribute.
>
> Btw mirroring random string stats into Netlink is not a good idea IMO.
Any reason you say that ?. I am thinking it would be much easier with netlink.
keeping it simple, it is a nested attribute with stat-name and value pair.

struct stat {
    char stats_name[STATS_NAME_LEN];    /* STATS_NAME_LEN = 32 */
    __u64 stat;
};

IFLA_STATS_LINK_DRIVER is a nested attribute with multiple IFLA_STATS_LINK_DRIVER_ENTRY of type 'struct stat'.

(If using a struct is a concern: each IFLA_STATS_LINK_DRIVER_ENTRY can be a nested attribute
of stat-name/value pair. Though it does not seem very necessary in this case).

PS: not fond of the name IFLA_STATS_LINK_DRIVER...any other suggestions are welcome.

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

[Jiri Pirko]

Sun, Jun 26, 2016 at 08:06:40PM CEST, roopa@cumulusnetworks.com wrote:
>On 6/26/16, 2:33 AM, Jiri Pirko wrote:
>> Sat, Jun 25, 2016 at 05:50:59PM CEST, roopa@cumulusnetworks.com wrote:
>>> On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>>>>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>>>>>>> counters do
>>>>>>>>> not include CPU generated packets....you will have to add CPU
>>>>>>>>> generated pkt counters to the
>>>>>>>>> hw counters for such virtual device stats.
>>>>>>>> Can you please provide and example how that could happen?
>>>>>>> example is the bridge vlan stats I mention below. These are usually counted
>>>>>>> by attaching hw virtual counter resources. And CPU generated packets
>>>>>>> in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>>>>>> has already made the required decisions. So, they may not be counted by
>>>>>>> by such hw virtual counters.
>>>>>> Bypass ASIC? How do the packets get on the wire?
>>>>>>
>>>>> Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>>>>> ASIC ships the CPU generated packet out of the switch/front-panel
>>>>> port. Continuing Roopa's example of vlan netdev stats.... To get the
>>>>> HW stats counters are typically tied to the ingress and egress vlan hw
>>>>> entries. All the incoming packets are subject to the ingress vlan
>>>>> lookup irrespective of whether they get punted to the CPU or whether
>>>>> they are forwarded to another front panel port. In that case the
>>>>> ingress HW stats does represent all packets. However for CPU
>>>>> originated packets egress vlan lookups are bypassed in the ASIC (this
>>>>> is common forwarding option in most ASICs) and the packet shipped as
>>>>> is out of front-panel port specified by the CPU. Which means these
>>>>> packets will NOT be counted against the egress VLAN HW counter; hence
>>>>> the need for summation.
>>>> Driver will know about this, and will provide the stats accordignly to
>>>> the core. Who else than driver should resolve this.
>>>>
>>> The point was/is that there should be only two categories:
>>> 1) the base default stats: can contain 'only sw', 'only hw' or 'a
>>> summation of hw and sw' in some cases.
>>> The user does not care about the breakdown.
>>>
>>> 2) everything else falls into the second category: driver provided
>>> breakdown of stats for easier debugging.
>>> This today is ethtool stats and we can have an equivalent nested
>>> attribute for this in the new stats api.
>>> Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
>>> nested and extensible (like ethtool is) and
>>> driver can expose any kind of stats there.
>>> ie lets move the stats you are proposing to this category of stats.....
>>> instead of introducing a third category 'SW stats'.
>> What you are proposing is essentially what our patchset does. We expose
>> 2 sets of stats. hw and pure sw. hw includes all, driver will take
>> care of it cause he knows what is going on in hw.
>the splitting into hw and sw is causing some confusion with respect

I still don't get why you are talking about split :( I see no split.


>to existing stats and will be confusing for future stats. And i am not sure how many
>users would prefer the split this way.
>So, instead of doing the split, i think we should at this time introduce
>driver specific stats (like ethtool) as a nested netlink attribute.

The default netlink stats should be hw (or accumulated as you call it).
The reason is to avoid confusion for existing apps. Another attribute is
possible for more break-out stats - that is what this patchset is doing.

Ethtool stats are wrong and useless for apps as they are driver-specific.


>>
>> Btw mirroring random string stats into Netlink is not a good idea IMO.
>Any reason you say that ?. I am thinking it would be much easier with netlink.
>keeping it simple, it is a nested attribute with stat-name and value pair.
>
>struct stat {
>    char stats_name[STATS_NAME_LEN];    /* STATS_NAME_LEN = 32 */
>    __u64 stat;
>};

No please. This should be well defined generic group of stats.
Driver-specific names/stats stats are wrong.


>
>IFLA_STATS_LINK_DRIVER is a nested attribute with multiple IFLA_STATS_LINK_DRIVER_ENTRY of type 'struct stat'.
>
>(If using a struct is a concern: each IFLA_STATS_LINK_DRIVER_ENTRY can be a nested attribute
>of stat-name/value pair. Though it does not seem very necessary in this case).
>
>PS: not fond of the name IFLA_STATS_LINK_DRIVER...any other suggestions are welcome.
>

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

[Roopa Prabhu]

On Sun, Jun 26, 2016 at 11:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Sun, Jun 26, 2016 at 08:06:40PM CEST, roopa@cumulusnetworks.com wrote:
>>On 6/26/16, 2:33 AM, Jiri Pirko wrote:
>>> Sat, Jun 25, 2016 at 05:50:59PM CEST, roopa@cumulusnetworks.com wrote:
>>>> On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>>>>>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>>>>>>>> counters do
>>>>>>>>>> not include CPU generated packets....you will have to add CPU
>>>>>>>>>> generated pkt counters to the
>>>>>>>>>> hw counters for such virtual device stats.
>>>>>>>>> Can you please provide and example how that could happen?
>>>>>>>> example is the bridge vlan stats I mention below. These are usually counted
>>>>>>>> by attaching hw virtual counter resources. And CPU generated packets
>>>>>>>> in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>>>>>>> has already made the required decisions. So, they may not be counted by
>>>>>>>> by such hw virtual counters.
>>>>>>> Bypass ASIC? How do the packets get on the wire?
>>>>>>>
>>>>>> Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>>>>>> ASIC ships the CPU generated packet out of the switch/front-panel
>>>>>> port. Continuing Roopa's example of vlan netdev stats.... To get the
>>>>>> HW stats counters are typically tied to the ingress and egress vlan hw
>>>>>> entries. All the incoming packets are subject to the ingress vlan
>>>>>> lookup irrespective of whether they get punted to the CPU or whether
>>>>>> they are forwarded to another front panel port. In that case the
>>>>>> ingress HW stats does represent all packets. However for CPU
>>>>>> originated packets egress vlan lookups are bypassed in the ASIC (this
>>>>>> is common forwarding option in most ASICs) and the packet shipped as
>>>>>> is out of front-panel port specified by the CPU. Which means these
>>>>>> packets will NOT be counted against the egress VLAN HW counter; hence
>>>>>> the need for summation.
>>>>> Driver will know about this, and will provide the stats accordignly to
>>>>> the core. Who else than driver should resolve this.
>>>>>
>>>> The point was/is that there should be only two categories:
>>>> 1) the base default stats: can contain 'only sw', 'only hw' or 'a
>>>> summation of hw and sw' in some cases.
>>>> The user does not care about the breakdown.
>>>>
>>>> 2) everything else falls into the second category: driver provided
>>>> breakdown of stats for easier debugging.
>>>> This today is ethtool stats and we can have an equivalent nested
>>>> attribute for this in the new stats api.
>>>> Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
>>>> nested and extensible (like ethtool is) and
>>>> driver can expose any kind of stats there.
>>>> ie lets move the stats you are proposing to this category of stats.....
>>>> instead of introducing a third category 'SW stats'.
>>> What you are proposing is essentially what our patchset does. We expose
>>> 2 sets of stats. hw and pure sw. hw includes all, driver will take
>>> care of it cause he knows what is going on in hw.
>>the splitting into hw and sw is causing some confusion with respect
>
> I still don't get why you are talking about split :( I see no split.
>
>
>>to existing stats and will be confusing for future stats. And i am not sure how many
>>users would prefer the split this way.
>>So, instead of doing the split, i think we should at this time introduce
>>driver specific stats (like ethtool) as a nested netlink attribute.
>
> The default netlink stats should be hw (or accumulated as you call it).
> The reason is to avoid confusion for existing apps. Another attribute is
> possible for more break-out stats - that is what this patchset is doing.
>
> Ethtool stats are wrong and useless for apps as they are driver-specific.

apps only care about overall stats. thats the aggregate stats
provided by the default netlink netdev api to the user...which already exists.

they don't care about your new breakdown either.

breakdown of stats are used for debugging and thats what ethtool stats provide.



>
>
>>>
>>> Btw mirroring random string stats into Netlink is not a good idea IMO.
>>Any reason you say that ?. I am thinking it would be much easier with netlink.
>>keeping it simple, it is a nested attribute with stat-name and value pair.
>>
>>struct stat {
>>    char stats_name[STATS_NAME_LEN];    /* STATS_NAME_LEN = 32 */
>>    __u64 stat;
>>};
>
> No please. This should be well defined generic group of stats.
> Driver-specific names/stats stats are wrong.
>

they are meant for debugging. are you saying the new stats api should
not contain 'ethtool like' stats ?

ethtool stats are very valuable today. They are extensible.
They cannot be made generic and they are specific to a hardware or use case.

We use it for our switch port stats too. Base aggregate stats summed
up and provided as default netdev stats. via ethtool we provide lot
more hardware specific breakdown.

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

[Jiri Pirko]

Mon, Jun 27, 2016 at 04:53:53AM CEST, roopa@cumulusnetworks.com wrote:
>On Sun, Jun 26, 2016 at 11:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Sun, Jun 26, 2016 at 08:06:40PM CEST, roopa@cumulusnetworks.com wrote:
>>>On 6/26/16, 2:33 AM, Jiri Pirko wrote:
>>>> Sat, Jun 25, 2016 at 05:50:59PM CEST, roopa@cumulusnetworks.com wrote:
>>>>> On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>>>>>>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>>>>>>>>>>> counters do
>>>>>>>>>>> not include CPU generated packets....you will have to add CPU
>>>>>>>>>>> generated pkt counters to the
>>>>>>>>>>> hw counters for such virtual device stats.
>>>>>>>>>> Can you please provide and example how that could happen?
>>>>>>>>> example is the bridge vlan stats I mention below. These are usually counted
>>>>>>>>> by attaching hw virtual counter resources. And CPU generated packets
>>>>>>>>> in some cases maybe setup to bypass the ASIC pipeline because the CPU
>>>>>>>>> has already made the required decisions. So, they may not be counted by
>>>>>>>>> by such hw virtual counters.
>>>>>>>> Bypass ASIC? How do the packets get on the wire?
>>>>>>>>
>>>>>>> Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>>>>>>> ASIC ships the CPU generated packet out of the switch/front-panel
>>>>>>> port. Continuing Roopa's example of vlan netdev stats.... To get the
>>>>>>> HW stats counters are typically tied to the ingress and egress vlan hw
>>>>>>> entries. All the incoming packets are subject to the ingress vlan
>>>>>>> lookup irrespective of whether they get punted to the CPU or whether
>>>>>>> they are forwarded to another front panel port. In that case the
>>>>>>> ingress HW stats does represent all packets. However for CPU
>>>>>>> originated packets egress vlan lookups are bypassed in the ASIC (this
>>>>>>> is common forwarding option in most ASICs) and the packet shipped as
>>>>>>> is out of front-panel port specified by the CPU. Which means these
>>>>>>> packets will NOT be counted against the egress VLAN HW counter; hence
>>>>>>> the need for summation.
>>>>>> Driver will know about this, and will provide the stats accordignly to
>>>>>> the core. Who else than driver should resolve this.
>>>>>>
>>>>> The point was/is that there should be only two categories:
>>>>> 1) the base default stats: can contain 'only sw', 'only hw' or 'a
>>>>> summation of hw and sw' in some cases.
>>>>> The user does not care about the breakdown.
>>>>>
>>>>> 2) everything else falls into the second category: driver provided
>>>>> breakdown of stats for easier debugging.
>>>>> This today is ethtool stats and we can have an equivalent nested
>>>>> attribute for this in the new stats api.
>>>>> Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
>>>>> nested and extensible (like ethtool is) and
>>>>> driver can expose any kind of stats there.
>>>>> ie lets move the stats you are proposing to this category of stats.....
>>>>> instead of introducing a third category 'SW stats'.
>>>> What you are proposing is essentially what our patchset does. We expose
>>>> 2 sets of stats. hw and pure sw. hw includes all, driver will take
>>>> care of it cause he knows what is going on in hw.
>>>the splitting into hw and sw is causing some confusion with respect
>>
>> I still don't get why you are talking about split :( I see no split.
>>
>>
>>>to existing stats and will be confusing for future stats. And i am not sure how many
>>>users would prefer the split this way.
>>>So, instead of doing the split, i think we should at this time introduce
>>>driver specific stats (like ethtool) as a nested netlink attribute.
>>
>> The default netlink stats should be hw (or accumulated as you call it).
>> The reason is to avoid confusion for existing apps. Another attribute is
>> possible for more break-out stats - that is what this patchset is doing.
>>
>> Ethtool stats are wrong and useless for apps as they are driver-specific.
>
>apps only care about overall stats. thats the aggregate stats
>provided by the default netlink netdev api to the user...which already exists.
>
>they don't care about your new breakdown either.

Agreed. That is what our patchset is doing.


>
>breakdown of stats are used for debugging and thats what ethtool stats provide.
>
>
>
>>
>>
>>>>
>>>> Btw mirroring random string stats into Netlink is not a good idea IMO.
>>>Any reason you say that ?. I am thinking it would be much easier with netlink.
>>>keeping it simple, it is a nested attribute with stat-name and value pair.
>>>
>>>struct stat {
>>>    char stats_name[STATS_NAME_LEN];    /* STATS_NAME_LEN = 32 */
>>>    __u64 stat;
>>>};
>>
>> No please. This should be well defined generic group of stats.
>> Driver-specific names/stats stats are wrong.
>>
>
>they are meant for debugging. are you saying the new stats api should
>not contain 'ethtool like' stats ?
>
>ethtool stats are very valuable today. They are extensible.
>They cannot be made generic and they are specific to a hardware or use case.
>
>We use it for our switch port stats too. Base aggregate stats summed
>up and provided as default netdev stats. via ethtool we provide lot
>more hardware specific breakdown.

Leave it in ethtool then. I really think it is not idea to put random
named-stats in netlink.

This patchset uses well defined values for slowpatch(/sw/cpu) stats.
That is I believe the only way to do this.

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

[Roopa Prabhu]

[resending ...my previous reply sent some non-text content]

On Sun, Jun 26, 2016 at 11:51 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 27, 2016 at 04:53:53AM CEST, roopa@cumulusnetworks.com wrote:
> >On Sun, Jun 26, 2016 at 11:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> Sun, Jun 26, 2016 at 08:06:40PM CEST, roopa@cumulusnetworks.com wrote:
> >>>On 6/26/16, 2:33 AM, Jiri Pirko wrote:
> >>>> Sat, Jun 25, 2016 at 05:50:59PM CEST, roopa@cumulusnetworks.com wrote:
> >>>>> On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >>>>>> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
> >>>>>>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
> >>>>>>>>>>> counters do
> >>>>>>>>>>> not include CPU generated packets....you will have to add CPU
> >>>>>>>>>>> generated pkt counters to the
> >>>>>>>>>>> hw counters for such virtual device stats.
> >>>>>>>>>> Can you please provide and example how that could happen?
> >>>>>>>>> example is the bridge vlan stats I mention below. These are usually counted
> >>>>>>>>> by attaching hw virtual counter resources. And CPU generated packets
> >>>>>>>>> in some cases maybe setup to bypass the ASIC pipeline because the CPU
> >>>>>>>>> has already made the required decisions. So, they may not be counted by
> >>>>>>>>> by such hw virtual counters.
> >>>>>>>> Bypass ASIC? How do the packets get on the wire?
> >>>>>>>>
> >>>>>>> Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
> >>>>>>> ASIC ships the CPU generated packet out of the switch/front-panel
> >>>>>>> port. Continuing Roopa's example of vlan netdev stats.... To get the
> >>>>>>> HW stats counters are typically tied to the ingress and egress vlan hw
> >>>>>>> entries. All the incoming packets are subject to the ingress vlan
> >>>>>>> lookup irrespective of whether they get punted to the CPU or whether
> >>>>>>> they are forwarded to another front panel port. In that case the
> >>>>>>> ingress HW stats does represent all packets. However for CPU
> >>>>>>> originated packets egress vlan lookups are bypassed in the ASIC (this
> >>>>>>> is common forwarding option in most ASICs) and the packet shipped as
> >>>>>>> is out of front-panel port specified by the CPU. Which means these
> >>>>>>> packets will NOT be counted against the egress VLAN HW counter; hence
> >>>>>>> the need for summation.
> >>>>>> Driver will know about this, and will provide the stats accordignly to
> >>>>>> the core. Who else than driver should resolve this.
> >>>>>>
> >>>>> The point was/is that there should be only two categories:
> >>>>> 1) the base default stats: can contain 'only sw', 'only hw' or 'a
> >>>>> summation of hw and sw' in some cases.
> >>>>> The user does not care about the breakdown.
> >>>>>
> >>>>> 2) everything else falls into the second category: driver provided
> >>>>> breakdown of stats for easier debugging.
> >>>>> This today is ethtool stats and we can have an equivalent nested
> >>>>> attribute for this in the new stats api.
> >>>>> Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
> >>>>> nested and extensible (like ethtool is) and
> >>>>> driver can expose any kind of stats there.
> >>>>> ie lets move the stats you are proposing to this category of stats.....
> >>>>> instead of introducing a third category 'SW stats'.
> >>>> What you are proposing is essentially what our patchset does. We expose
> >>>> 2 sets of stats. hw and pure sw. hw includes all, driver will take
> >>>> care of it cause he knows what is going on in hw.
> >>>the splitting into hw and sw is causing some confusion with respect
> >>
> >> I still don't get why you are talking about split :( I see no split.
> >>
> >>
> >>>to existing stats and will be confusing for future stats. And i am not sure how many
> >>>users would prefer the split this way.
> >>>So, instead of doing the split, i think we should at this time introduce
> >>>driver specific stats (like ethtool) as a nested netlink attribute.
> >>
> >> The default netlink stats should be hw (or accumulated as you call it).
> >> The reason is to avoid confusion for existing apps. Another attribute is
> >> possible for more break-out stats - that is what this patchset is doing.
> >>
> >> Ethtool stats are wrong and useless for apps as they are driver-specific.
> >
> >apps only care about overall stats. thats the aggregate stats
> >provided by the default netlink netdev api to the user...which already exists.
> >
> >they don't care about your new breakdown either.
>
> Agreed. That is what our patchset is doing.
>
>
> >
> >breakdown of stats are used for debugging and thats what ethtool stats provide.
> >
> >
> >
> >>
> >>
> >>>>
> >>>> Btw mirroring random string stats into Netlink is not a good idea IMO.
> >>>Any reason you say that ?. I am thinking it would be much easier with netlink.
> >>>keeping it simple, it is a nested attribute with stat-name and value pair.
> >>>
> >>>struct stat {
> >>>    char stats_name[STATS_NAME_LEN];    /* STATS_NAME_LEN = 32 */
> >>>    __u64 stat;
> >>>};
> >>
> >> No please. This should be well defined generic group of stats.
> >> Driver-specific names/stats stats are wrong.
> >>
> >
> >they are meant for debugging. are you saying the new stats api should
> >not contain 'ethtool like' stats ?
> >
> >ethtool stats are very valuable today. They are extensible.
> >They cannot be made generic and they are specific to a hardware or use case.
> >
> >We use it for our switch port stats too. Base aggregate stats summed
> >up and provided as default netdev stats. via ethtool we provide lot
> >more hardware specific breakdown.
>
> Leave it in ethtool then. I really think it is not idea to put random
> named-stats in netlink.



The point of the stats api is to bring all stats together.
ethtool stats are very valuable. so there is no point leaving them out.
they will be in here sooner or later.

when there is already a discussion of moving ethtool to netlink,..i am not sure
I understand the hesitation on moving ethtool stats to netlink.

>
>
> This patchset uses well defined values for slowpatch(/sw/cpu) stats.
> That is I believe the only way to do this.
>

It seems to be your preferred way. I don't think it is the only way to
do it. It can very well be part of the nested
breakdown stats i am talking about.

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

[Jiri Pirko]

Mon, Jun 27, 2016 at 03:02:31PM CEST, roopa@cumulusnetworks.com wrote:
>[resending ...my previous reply sent some non-text content]
>
>On Sun, Jun 26, 2016 at 11:51 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 27, 2016 at 04:53:53AM CEST, roopa@cumulusnetworks.com wrote:
>> >On Sun, Jun 26, 2016 at 11:15 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> Sun, Jun 26, 2016 at 08:06:40PM CEST, roopa@cumulusnetworks.com wrote:
>> >>>On 6/26/16, 2:33 AM, Jiri Pirko wrote:
>> >>>> Sat, Jun 25, 2016 at 05:50:59PM CEST, roopa@cumulusnetworks.com wrote:
>> >>>>> On Thu, Jun 23, 2016 at 8:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >>>>>> Thu, Jun 23, 2016 at 05:11:26PM CEST, anuradhak@cumulusnetworks.com wrote:
>> >>>>>>>>>>> we can't separate CPU and HW stats there. In some cases (or ASICs) HW
>> >>>>>>>>>>> counters do
>> >>>>>>>>>>> not include CPU generated packets....you will have to add CPU
>> >>>>>>>>>>> generated pkt counters to the
>> >>>>>>>>>>> hw counters for such virtual device stats.
>> >>>>>>>>>> Can you please provide and example how that could happen?
>> >>>>>>>>> example is the bridge vlan stats I mention below. These are usually counted
>> >>>>>>>>> by attaching hw virtual counter resources. And CPU generated packets
>> >>>>>>>>> in some cases maybe setup to bypass the ASIC pipeline because the CPU
>> >>>>>>>>> has already made the required decisions. So, they may not be counted by
>> >>>>>>>>> by such hw virtual counters.
>> >>>>>>>> Bypass ASIC? How do the packets get on the wire?
>> >>>>>>>>
>> >>>>>>> Bypass the "forwarding pipeline" in the ASIC that is. Obviously the
>> >>>>>>> ASIC ships the CPU generated packet out of the switch/front-panel
>> >>>>>>> port. Continuing Roopa's example of vlan netdev stats.... To get the
>> >>>>>>> HW stats counters are typically tied to the ingress and egress vlan hw
>> >>>>>>> entries. All the incoming packets are subject to the ingress vlan
>> >>>>>>> lookup irrespective of whether they get punted to the CPU or whether
>> >>>>>>> they are forwarded to another front panel port. In that case the
>> >>>>>>> ingress HW stats does represent all packets. However for CPU
>> >>>>>>> originated packets egress vlan lookups are bypassed in the ASIC (this
>> >>>>>>> is common forwarding option in most ASICs) and the packet shipped as
>> >>>>>>> is out of front-panel port specified by the CPU. Which means these
>> >>>>>>> packets will NOT be counted against the egress VLAN HW counter; hence
>> >>>>>>> the need for summation.
>> >>>>>> Driver will know about this, and will provide the stats accordignly to
>> >>>>>> the core. Who else than driver should resolve this.
>> >>>>>>
>> >>>>> The point was/is that there should be only two categories:
>> >>>>> 1) the base default stats: can contain 'only sw', 'only hw' or 'a
>> >>>>> summation of hw and sw' in some cases.
>> >>>>> The user does not care about the breakdown.
>> >>>>>
>> >>>>> 2) everything else falls into the second category: driver provided
>> >>>>> breakdown of stats for easier debugging.
>> >>>>> This today is ethtool stats and we can have an equivalent nested
>> >>>>> attribute for this in the new stats api.
>> >>>>> Lets call it IFLA_STATS_LINK_DRIVER or you pick a name. Lets make it
>> >>>>> nested and extensible (like ethtool is) and
>> >>>>> driver can expose any kind of stats there.
>> >>>>> ie lets move the stats you are proposing to this category of stats.....
>> >>>>> instead of introducing a third category 'SW stats'.
>> >>>> What you are proposing is essentially what our patchset does. We expose
>> >>>> 2 sets of stats. hw and pure sw. hw includes all, driver will take
>> >>>> care of it cause he knows what is going on in hw.
>> >>>the splitting into hw and sw is causing some confusion with respect
>> >>
>> >> I still don't get why you are talking about split :( I see no split.
>> >>
>> >>
>> >>>to existing stats and will be confusing for future stats. And i am not sure how many
>> >>>users would prefer the split this way.
>> >>>So, instead of doing the split, i think we should at this time introduce
>> >>>driver specific stats (like ethtool) as a nested netlink attribute.
>> >>
>> >> The default netlink stats should be hw (or accumulated as you call it).
>> >> The reason is to avoid confusion for existing apps. Another attribute is
>> >> possible for more break-out stats - that is what this patchset is doing.
>> >>
>> >> Ethtool stats are wrong and useless for apps as they are driver-specific.
>> >
>> >apps only care about overall stats. thats the aggregate stats
>> >provided by the default netlink netdev api to the user...which already exists.
>> >
>> >they don't care about your new breakdown either.
>>
>> Agreed. That is what our patchset is doing.
>>
>>
>> >
>> >breakdown of stats are used for debugging and thats what ethtool stats provide.
>> >
>> >
>> >
>> >>
>> >>
>> >>>>
>> >>>> Btw mirroring random string stats into Netlink is not a good idea IMO.
>> >>>Any reason you say that ?. I am thinking it would be much easier with netlink.
>> >>>keeping it simple, it is a nested attribute with stat-name and value pair.
>> >>>
>> >>>struct stat {
>> >>>    char stats_name[STATS_NAME_LEN];    /* STATS_NAME_LEN = 32 */
>> >>>    __u64 stat;
>> >>>};
>> >>
>> >> No please. This should be well defined generic group of stats.
>> >> Driver-specific names/stats stats are wrong.
>> >>
>> >
>> >they are meant for debugging. are you saying the new stats api should
>> >not contain 'ethtool like' stats ?
>> >
>> >ethtool stats are very valuable today. They are extensible.
>> >They cannot be made generic and they are specific to a hardware or use case.
>> >
>> >We use it for our switch port stats too. Base aggregate stats summed
>> >up and provided as default netdev stats. via ethtool we provide lot
>> >more hardware specific breakdown.
>>
>> Leave it in ethtool then. I really think it is not idea to put random
>> named-stats in netlink.
>
>
>
>The point of the stats api is to bring all stats together.
>ethtool stats are very valuable. so there is no point leaving them out.
>they will be in here sooner or later.
>
>when there is already a discussion of moving ethtool to netlink,..i am not sure
>I understand the hesitation on moving ethtool stats to netlink.

It's not well defined. That's the issue. It's "debugfs"...


>
>>
>>
>> This patchset uses well defined values for slowpatch(/sw/cpu) stats.
>> That is I believe the only way to do this.
>>
>
>It seems to be your preferred way. I don't think it is the only way to
>do it. It can very well be part of the nested
>breakdown stats i am talking about.

We are using existing well-defined structure. Very easy and clean.

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

[Roopa Prabhu]

On Mon, Jun 27, 2016 at 6:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Jun 27, 2016 at 03:02:31PM CEST, roopa@cumulusnetworks.com wrote:

[snip]

>>
>>The point of the stats api is to bring all stats together.
>>ethtool stats are very valuable. so there is no point leaving them out.
>>they will be in here sooner or later.
>>
>>when there is already a discussion of moving ethtool to netlink,..i am not sure
>>I understand the hesitation on moving ethtool stats to netlink.
>
> It's not well defined. That's the issue. It's "debugfs"...
>
>
>>
>>>
>>>
>>> This patchset uses well defined values for slowpatch(/sw/cpu) stats.
>>> That is I believe the only way to do this.
>>>
>>
>>It seems to be your preferred way. I don't think it is the only way to
>>do it. It can very well be part of the nested
>>breakdown stats i am talking about.
>
> We are using existing well-defined structure. Very easy and clean.

sure, it is well-defined.

but my only point was the aggregate stats that the user or app cares
about is in your
so called already present 'well-defined structure'. everything
else...ie the breakdowns are purely for
debuggability and hence falls into the other bucket which you call
"debugfs" like.

splitting the kernel default interface stats into HW and SW is going
to confuse people
on the existing api and may introduce ambiguity for future stats is
all i am saying.