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

[patch net-next v6 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 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 076df53..4f5c0875 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -922,6 +922,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
@@ -1154,6 +1163,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,
@@ -3798,6 +3810,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 4ce07dc..e5b8cbf 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7494,6 +7494,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)
@@ -7515,6 +7517,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 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 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 e1b8f62..29230e2 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)
 {
@@ -1234,6 +1314,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,
@@ -1572,8 +1653,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;
@@ -1582,8 +1661,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;
 }
@@ -2116,6 +2194,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;
 
@@ -2213,6 +2301,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:
@@ -2230,6 +2319,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);
@@ -2246,6 +2337,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 */
@@ -2255,6 +2347,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 f69aa37..e7f3a5d 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 2/3] net: core: add SW stats to if_stats_msg

[Roopa Prabhu]

On 8/9/16, 2:25 PM, 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,

sorry, don't mean to drag this discussion forever...but...

IIRC on the switchdev call, I thought we had agreed to just put these kind of breakdown
stats in one more layer of nesting.. which can be extended for everybody.

IFLA_STATS_LINK_DRIVER_XSTATS   (or some other name) [
                IFLA_STATS_LINK_SW_64  (struct rtnl_link_stats64)    <---- you get the well defined struct here as you wanted.
                IFLA_STATS_LINK_HW_ACL_DROPS  /* u64 */        <----- we can keep extending this for other stats people want.

                /* just keeps it extensible */
]

because in addition to all other reasons I have mentioned before, technically IFLA_STATS_LINK_64 (top-level aggregate stats)
are also software only stats for say most logical devices etc.


and instead of adding one ndo for your software only stats now and then again for other ethtool
like hw stats from all drivers, it could also be like the below:

new ndo_get_hw_stats

IFLA_STATS_LINK_HW_XSTATS   (or some other name) [
                IFLA_STATS_LINK_CPU_64  (struct rtnl_link_stats64)    <---- you get the well defined struct here as you wanted.
                IFLA_STATS_LINK_HW_ACL_DROPS  /* u64 */              <----- we can keep extending this for other stats people want.
                /* just keeps it extensible */
]

It gets you what you want and keeps the api extensible IMO. your call.

>  	__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;
>  }
>  

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

[Nogah Frankel]

> -----Original Message-----
> From: Roopa Prabhu [mailto:roopa@cumulusnetworks.com]
> Sent: Wednesday, August 10, 2016 9:09 AM
> To: Jiri Pirko <jiri@resnulli.us>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; Nogah Frankel
> <nogahf@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad
> Raz <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or
> Gerlitz <ogerlitz@mellanox.com>; nikolay@cumulusnetworks.com;
> linville@tuxdriver.com; tgraf@suug.ch; gospo@cumulusnetworks.com;
> sfeldma@gmail.com; sd@queasysnail.net; Eran Ben Elisha
> <eranbe@mellanox.com>; ast@plumgrid.com; edumazet@google.com;
> hannes@stressinduktion.org; f.fainelli@gmail.com;
> dsa@cumulusnetworks.com
> Subject: Re: [patch net-next v6 2/3] net: core: add SW stats to if_stats_msg
> 
> On 8/9/16, 2:25 PM, 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,
> 
> sorry, don't mean to drag this discussion forever...but...
> 
> IIRC on the switchdev call, I thought we had agreed to just put these kind of
> breakdown
> stats in one more layer of nesting.. which can be extended for everybody.
> 
> IFLA_STATS_LINK_DRIVER_XSTATS   (or some other name) [
>                 IFLA_STATS_LINK_SW_64  (struct rtnl_link_stats64)    <---- you get
> the well defined struct here as you wanted.
>                 IFLA_STATS_LINK_HW_ACL_DROPS  /* u64 */        <----- we can
> keep extending this for other stats people want.
> 
>                 /* just keeps it extensible */
> ]
> 
> because in addition to all other reasons I have mentioned before,
> technically IFLA_STATS_LINK_64 (top-level aggregate stats)
> are also software only stats for say most logical devices etc.
> 
> 
> and instead of adding one ndo for your software only stats now and then
> again for other ethtool
> like hw stats from all drivers, it could also be like the below:
> 
> new ndo_get_hw_stats
> 
> IFLA_STATS_LINK_HW_XSTATS   (or some other name) [
>                 IFLA_STATS_LINK_CPU_64  (struct rtnl_link_stats64)    <---- you
> get the well defined struct here as you wanted.
>                 IFLA_STATS_LINK_HW_ACL_DROPS  /* u64 */              <----- we can
> keep extending this for other stats people want.
>                 /* just keeps it extensible */
> ]
> 
> It gets you what you want and keeps the api extensible IMO. your call.
> 

I don't think extra nesting is a good idea because when we are called for 
IFLA_STATS_LINK_DRIVER_XSTATS  we can't distinguish between subtypes of it.
So for each driver stats we are going to need different XSTATS types anyway.

We shouldn't return all the stats a driver can provide when we are asked for one.
HW_ACL_DROPS shouldn't be queried when user want SW stats nor the  other
way around.
 (And I think that stats exposed here should be well defined so they could 
be exposed to ifstat. The place for not well define stats is in ethtool)

> >  	__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;
> >  }
> >