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

[patch net-next v6_repost 0/3] 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.

---
v5->v6:
- patch 2/4 was dropped as requested by Roopa
- patch 1/3:
 - comment changed to indicate that default stats are combined stats
 - commit massage changed
- patch 2/3: (previously 3/4)
 - SW stats return nothing if there is no SW stats ndo
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 (3):
  netdevice: add SW statistics ndo
  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 | 105 +++++++++++++++++++++++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h |   5 ++
 include/linux/netdevice.h                      |  14 ++++
 include/uapi/linux/if_link.h                   |   1 +
 net/core/dev.c                                 |  31 ++++++++
 net/core/rtnetlink.c                           |  21 +++++
 6 files changed, 171 insertions(+), 6 deletions(-)

-- 
2.5.5

[patch net-next v6_repost 1/3] netdevice: add SW statistics ndo

[Jiri Pirko]

From: Nogah Frankel <nogahf@mellanox.com>

Stats should return the number of packets that went though a port by SW
or HW. But when one has offloaded traffic, one might want to know how
many packets went via slow path.
So this ndo return SW statistics for packets that went via CPU,
(opposed to HW counter that count all the packets, slow path or not).
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 | 14 ++++++++++++++
 net/core/dev.c            | 31 +++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 794bb07..8bc38ef 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -923,6 +923,15 @@ struct netdev_xdp {
  *	   field is written atomically.
  *	3. Update dev->stats asynchronously and atomically, and define
  *	   neither operation.
+ *	If there are both SW and HW stats, driver should return combined
+ *	stats.
+ *
+ * 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
@@ -1155,6 +1164,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,
@@ -3802,6 +3814,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 a75df86..c57c595 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7490,6 +7490,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 combined statistics.
  */
 struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					struct rtnl_link_stats64 *storage)
@@ -7511,6 +7513,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 v6_repost 2/3] net: core: add SW stats to if_stats_msg

[Jiri Pirko]

From: Nogah Frankel <nogahf@mellanox.com>

Add a nested attribute of SW stats to if_stats_msg
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         | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index a1b5202..1c9b808 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -825,6 +825,7 @@ enum {
 	IFLA_STATS_LINK_64,
 	IFLA_STATS_LINK_XSTATS,
 	IFLA_STATS_LINK_XSTATS_SLAVE,
+	IFLA_STATS_LINK_SW_64,
 	__IFLA_STATS_MAX,
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 189cc78..910f802 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3583,6 +3583,21 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 		dev_get_stats(dev, sp);
 	}
 
+	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_SW_64, *idxattr)) {
+		if (dev_have_sw_stats(dev)) {
+			struct rtnl_link_stats64 *sp;
+
+			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);
+			dev_get_sw_stats(dev, sp);
+		}
+	}
+
 	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_XSTATS, *idxattr)) {
 		const struct rtnl_link_ops *ops = dev->rtnl_link_ops;
 
@@ -3644,6 +3659,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,
@@ -3685,6 +3701,11 @@ static size_t if_nlmsg_stats_size(const struct net_device *dev,
 		}
 	}
 
+	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_SW_64, 0)) {
+		if (dev_have_sw_stats(dev))
+			size += nla_total_size_64bit(sizeof(struct rtnl_link_stats64));
+	}
+
 	return size;
 }
 
-- 
2.5.5

[patch net-next v6_repost 3/3] 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 | 105 +++++++++++++++++++++++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h |   5 ++
 2 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 1f81689..9dc5416 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -811,8 +811,8 @@ err_span_port_mtu_update:
 }
 
 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;
@@ -842,6 +842,86 @@ mlxsw_sp_port_get_stats64(struct net_device *dev,
 	return stats;
 }
 
+static int mlxsw_sp_port_get_stats_raw(struct net_device *dev, int grp,
+				       int prio, 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, grp, prio);
+	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, MLXSW_REG_PPCNT_IEEE_8023_CNT,
+					  0, 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);
+}
+
+/* Return the stats from a cache that is updated periodically,
+ * as this function might get called in an atomic context.
+ */
+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)
 {
@@ -1208,6 +1288,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_neigh_construct	= mlxsw_sp_router_neigh_construct,
@@ -1546,8 +1627,6 @@ static void __mlxsw_sp_port_get_stats(struct net_device *dev,
 				      enum mlxsw_reg_ppcnt_grp grp, int prio,
 				      u64 *data, int data_index)
 {
-	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
 	struct mlxsw_sp_port_hw_stats *hw_stats;
 	char ppcnt_pl[MLXSW_REG_PPCNT_LEN];
 	int i, len;
@@ -1556,8 +1635,7 @@ static void __mlxsw_sp_port_get_stats(struct net_device *dev,
 	err = mlxsw_sp_get_hw_stats_by_group(&hw_stats, &len, grp);
 	if (err)
 		return;
-	mlxsw_reg_ppcnt_pack(ppcnt_pl, mlxsw_sp_port->local_port, grp, prio);
-	err = mlxsw_reg_query(mlxsw_sp->core, MLXSW_REG(ppcnt), ppcnt_pl);
+	mlxsw_sp_port_get_stats_raw(dev, grp, prio, ppcnt_pl);
 	for (i = 0; i < len; i++)
 		data[data_index + i] = !err ? hw_stats[i].getter(ppcnt_pl) : 0;
 }
@@ -2102,6 +2180,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;
 
@@ -2202,6 +2290,7 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
 		goto err_core_port_init;
 	}
 
+	mlxsw_core_schedule_dw(&mlxsw_sp_port->hw_stats.update_dw, 0);
 	return 0;
 
 err_core_port_init:
@@ -2222,6 +2311,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);
@@ -2238,6 +2329,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_core_port_fini(&mlxsw_sp_port->core_port);
 	unregister_netdev(mlxsw_sp_port->dev); /* This calls ndo_stop */
 	mlxsw_sp->ports[local_port] = NULL;
@@ -2247,6 +2339,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);
 	WARN_ON_ONCE(!list_empty(&mlxsw_sp_port->vports_list));
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index ab3feb8..1c63666 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -360,6 +360,11 @@ struct mlxsw_sp_port {
 	struct list_head vports_list;
 	/* TC handles */
 	struct list_head mall_tc_list;
+	struct {
+		#define MLXSW_HW_STATS_UPDATE_TIME HZ
+		struct rtnl_link_stats64 *cache;
+		struct delayed_work update_dw;
+	} hw_stats;
 };
 
 struct mlxsw_sp_port *mlxsw_sp_port_lower_dev_hold(struct net_device *dev);
-- 
2.5.5

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

[David Miller]

Roopa, please review this patch series.

Thank you.

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

[Roopa Prabhu]

On 8/19/16, 6:17 AM, Jiri Pirko wrote:
> From: Nogah Frankel <nogahf@mellanox.com>
>
> Add a nested attribute of SW stats to if_stats_msg
> 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         | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index a1b5202..1c9b808 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -825,6 +825,7 @@ enum {
>  	IFLA_STATS_LINK_64,
>  	IFLA_STATS_LINK_XSTATS,
>  	IFLA_STATS_LINK_XSTATS_SLAVE,
> +	IFLA_STATS_LINK_SW_64,
>  
hate to sound like a broken record here...

sorry, but like I have been saying throughout this patch series,
i don't think I can ack a new attribute for so called 'software stats' at the
same level as existing IFLA_STATS_LINK_64. It just adds ambiguity for existing stats and
confusion for future stats.

Today's IFLA_STATS64 or IFLA_STATS_LINK_64 provides aggregate stats
and historically its been 'HW only' or 'SW only' or 'HW + SW'. It depends on the driver. logical devices
provide pure 'SW only' stats here. There is no real reason to qualify them now. The user/app
has only cared about and will continue to care about only aggregate stats. That requirement
has never changed.

Everything else is breakdown for debug-ability.., hence should be in this second bucket I
have been talking about (these are traditionally available via ethtool today).

I am not arguing against the value of the stats this patch series provides. But, since the beginning
of the stats api, I have always talked about a nested extensible attribute for drivers who wish to
break their stats down. so, I have only been requesting to put this so called 'software stats' attribute
in a new nested attribute which can be extended for other such specially qualified stats.

And, I thought we had agreed on such an attribute. I mention such a nested attribute
here again on your previous post: http://marc.info/?l=linux-netdev&m=147085641703885&w=2

Thanks,
Roopa

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

[Jiri Pirko]

Tue, Aug 23, 2016 at 07:51:57AM CEST, roopa@cumulusnetworks.com wrote:
>On 8/19/16, 6:17 AM, Jiri Pirko wrote:
>> From: Nogah Frankel <nogahf@mellanox.com>
>>
>> Add a nested attribute of SW stats to if_stats_msg
>> 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         | 21 +++++++++++++++++++++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> index a1b5202..1c9b808 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -825,6 +825,7 @@ enum {
>>  	IFLA_STATS_LINK_64,
>>  	IFLA_STATS_LINK_XSTATS,
>>  	IFLA_STATS_LINK_XSTATS_SLAVE,
>> +	IFLA_STATS_LINK_SW_64,
>>  
>hate to sound like a broken record here...
>
>sorry, but like I have been saying throughout this patch series,
>i don't think I can ack a new attribute for so called 'software stats' at the
>same level as existing IFLA_STATS_LINK_64. It just adds ambiguity for existing stats and
>confusion for future stats.
>
>Today's IFLA_STATS64 or IFLA_STATS_LINK_64 provides aggregate stats
>and historically its been 'HW only' or 'SW only' or 'HW + SW'. It depends on the driver. logical devices
>provide pure 'SW only' stats here. There is no real reason to qualify them now. The user/app
>has only cared about and will continue to care about only aggregate stats. That requirement
>has never changed.
>
>Everything else is breakdown for debug-ability.., hence should be in this second bucket I
>have been talking about (these are traditionally available via ethtool today).
>
>I am not arguing against the value of the stats this patch series provides. But, since the beginning
>of the stats api, I have always talked about a nested extensible attribute for drivers who wish to
>break their stats down. so, I have only been requesting to put this so called 'software stats' attribute
>in a new nested attribute which can be extended for other such specially qualified stats.
>
>And, I thought we had agreed on such an attribute. I mention such a nested attribute
>here again on your previous post: http://marc.info/?l=linux-netdev&m=147085641703885&w=2

And Nogah replied.

Anyway I think that next level of nesting is not necessary. On
contrary, it is wrong. The current level is extensible, mixed and
flagged already. I don't see any reason why not to add whatever kind of
stats here. What makes IFLA_STATS_LINK_SW_64 or for example
IFLA_STATS_LINK_HW_ACL so special it has to be nested in some other
attr? I would understand it it would be values of one family, but that
is not the case.

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

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 23 Aug 2016 08:53:18 +0200

> Anyway I think that next level of nesting is not necessary. On
> contrary, it is wrong. The current level is extensible, mixed and
> flagged already. I don't see any reason why not to add whatever kind of
> stats here. What makes IFLA_STATS_LINK_SW_64 or for example
> IFLA_STATS_LINK_HW_ACL so special it has to be nested in some other
> attr? I would understand it it would be values of one family, but that
> is not the case.

First, I agree with Roopa.  If we want to put this stuff out
there is should be bucketed together in a nested attribute with
other similar stats specifications.

Second, the more I think about this what you're providing isn't
actually a new statistic type.

It's a filter.

So why don't we just provide a filter specification that gets passed
down into the driver.  And the user can ask for "SW stats" or whatever
using that.

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

[Jiri Pirko]

Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Tue, 23 Aug 2016 08:53:18 +0200
>
>> Anyway I think that next level of nesting is not necessary. On
>> contrary, it is wrong. The current level is extensible, mixed and
>> flagged already. I don't see any reason why not to add whatever kind of
>> stats here. What makes IFLA_STATS_LINK_SW_64 or for example
>> IFLA_STATS_LINK_HW_ACL so special it has to be nested in some other
>> attr? I would understand it it would be values of one family, but that
>> is not the case.
>
>First, I agree with Roopa.  If we want to put this stuff out
>there is should be bucketed together in a nested attribute with
>other similar stats specifications.

Well I still don't think that IFLA_STATS_LINK_SW_64 and
IFLA_STATS_LINK_HW_ACL are related. You cannot put it under *DRIVER*
nest as IFLA_STATS_LINK_SW_64 are core stats. So we can put them under
*MISC* nest attr. But that is exactly purpose of the top-level here.
/me confused


>
>Second, the more I think about this what you're providing isn't
>actually a new statistic type.
>
>It's a filter.
>
>So why don't we just provide a filter specification that gets passed
>down into the driver.  And the user can ask for "SW stats" or whatever
>using that.

for this particular stats (sw stats) you can look at it that way. The
question is that in case we handle it the filter way instead of
multiattr way. Is it convenient for user to do 2 netlink calls instead
of one? I tend to like the one-call-flag-what-you-want approach better.

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

[Roopa Prabhu]

On 8/23/16, 12:26 AM, Jiri Pirko wrote:
> Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote:
>> From: Jiri Pirko <jiri@resnulli.us>
>> Date: Tue, 23 Aug 2016 08:53:18 +0200
>>
>>> Anyway I think that next level of nesting is not necessary. On
>>> contrary, it is wrong. The current level is extensible, mixed and
>>> flagged already. I don't see any reason why not to add whatever kind of
>>> stats here. What makes IFLA_STATS_LINK_SW_64 or for example
>>> IFLA_STATS_LINK_HW_ACL so special it has to be nested in some other
>>> attr? I would understand it it would be values of one family, but that
>>> is not the case.
>> First, I agree with Roopa.  If we want to put this stuff out
>> there is should be bucketed together in a nested attribute with
>> other similar stats specifications.
> Well I still don't think that IFLA_STATS_LINK_SW_64 and
> IFLA_STATS_LINK_HW_ACL are related. You cannot put it under *DRIVER*
> nest as IFLA_STATS_LINK_SW_64 are core stats.
not sure i understand, why is this core stats ?.
should a new logical device implement  IFLA_STATS_LINK_64 or IFLA_STATS_LINK_SW_64 ?
any other users ?.


>  So we can put them under
> *MISC* nest attr. But that is exactly purpose of the top-level here.
> /me confused

By design top level is for higher level grouping of stats (that also helps us maintain a lean higher
level filter space). They are mainly categories of stats. for example we have a nested link
XSTATS attribute..which are again a break down of stats already counted in IFLA_STATS_LINK_64.
That's why I think we can group this into another kind of breakdown stats.

thanks,
Roopa

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

[Jiri Pirko]

Tue, Aug 23, 2016 at 04:46:37PM CEST, roopa@cumulusnetworks.com wrote:
>On 8/23/16, 12:26 AM, Jiri Pirko wrote:
>> Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote:
>>> From: Jiri Pirko <jiri@resnulli.us>
>>> Date: Tue, 23 Aug 2016 08:53:18 +0200
>>>
>>>> Anyway I think that next level of nesting is not necessary. On
>>>> contrary, it is wrong. The current level is extensible, mixed and
>>>> flagged already. I don't see any reason why not to add whatever kind of
>>>> stats here. What makes IFLA_STATS_LINK_SW_64 or for example
>>>> IFLA_STATS_LINK_HW_ACL so special it has to be nested in some other
>>>> attr? I would understand it it would be values of one family, but that
>>>> is not the case.
>>> First, I agree with Roopa.  If we want to put this stuff out
>>> there is should be bucketed together in a nested attribute with
>>> other similar stats specifications.
>> Well I still don't think that IFLA_STATS_LINK_SW_64 and
>> IFLA_STATS_LINK_HW_ACL are related. You cannot put it under *DRIVER*
>> nest as IFLA_STATS_LINK_SW_64 are core stats.
>not sure i understand, why is this core stats ?.
>should a new logical device implement  IFLA_STATS_LINK_64 or IFLA_STATS_LINK_SW_64 ?
>any other users ?.
>
>
>>  So we can put them under
>> *MISC* nest attr. But that is exactly purpose of the top-level here.
>> /me confused
>
>By design top level is for higher level grouping of stats (that also helps us maintain a lean higher
>level filter space). They are mainly categories of stats. for example we have a nested link
>XSTATS attribute..which are again a break down of stats already counted in IFLA_STATS_LINK_64.
>That's why I think we can group this into another kind of breakdown stats.

I give up. What name do you suggest for the nested attribute?

Thanks.

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

[Roopa Prabhu]

On 8/23/16, 7:52 AM, Jiri Pirko wrote:
> Tue, Aug 23, 2016 at 04:46:37PM CEST, roopa@cumulusnetworks.com wrote:
>> On 8/23/16, 12:26 AM, Jiri Pirko wrote:
>>> Tue, Aug 23, 2016 at 09:04:15AM CEST, davem@davemloft.net wrote:
>>>> From: Jiri Pirko <jiri@resnulli.us>
>>>> Date: Tue, 23 Aug 2016 08:53:18 +0200
>>>>
>>>>> Anyway I think that next level of nesting is not necessary. On
>>>>> contrary, it is wrong. The current level is extensible, mixed and
>>>>> flagged already. I don't see any reason why not to add whatever kind of
>>>>> stats here. What makes IFLA_STATS_LINK_SW_64 or for example
>>>>> IFLA_STATS_LINK_HW_ACL so special it has to be nested in some other
>>>>> attr? I would understand it it would be values of one family, but that
>>>>> is not the case.
>>>> First, I agree with Roopa.  If we want to put this stuff out
>>>> there is should be bucketed together in a nested attribute with
>>>> other similar stats specifications.
>>> Well I still don't think that IFLA_STATS_LINK_SW_64 and
>>> IFLA_STATS_LINK_HW_ACL are related. You cannot put it under *DRIVER*
>>> nest as IFLA_STATS_LINK_SW_64 are core stats.
>> not sure i understand, why is this core stats ?.
>> should a new logical device implement  IFLA_STATS_LINK_64 or IFLA_STATS_LINK_SW_64 ?
>> any other users ?.
>>
>>
>>>  So we can put them under
>>> *MISC* nest attr. But that is exactly purpose of the top-level here.
>>> /me confused
>> By design top level is for higher level grouping of stats (that also helps us maintain a lean higher
>> level filter space). They are mainly categories of stats. for example we have a nested link
>> XSTATS attribute..which are again a break down of stats already counted in IFLA_STATS_LINK_64.
>> That's why I think we can group this into another kind of breakdown stats.
> I give up. What name do you suggest for the nested attribute?

how about the below ?
IFLA_STATS_LINK_OFFLOAD_XSTATS [
        IFLA_OFFLOAD_XSTATS_SW_HIT or IFLA_OFFLOAD_XSTATS_CPU_HIT or whatever you want to call it ?
]

reasons: we need an offload driver stats bucket anyways (for the hw driver extended stats.
similar to ethtool stats). keeping the name generic will help cover switchdev and other hw offload uses.
If there is a need for a second level filter in the future, we can always
add a IFLA_OFFLOAD_XSTATS_FILTER attribute when that becomes necessary.

I agree with dave that this is essentially a filter on the existing stats and can be implemented as
a flag someplace. But, the best way I can see that filter implemented in the current code/api
is with a nested attribute like above.

Thanks Jiri!