[patch net-next v2 00/12] net: allow user specify TC action HW stats type

[patch net-next v2 00/12] net: allow user specify TC action HW stats type

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Currently, when user adds a TC action and the action gets offloaded,
the user expects the HW stats to be counted and included in stats dump.
However, since drivers may implement different types of counting, there
is no way to specify which one the user is interested in.

For example for mlx5, only delayed counters are available as the driver
periodically polls for updated stats.

In case of mlxsw, the counters are queried on dump time. However, the
HW resources for this type of counters is quite limited (couple of
thousands). This limits the amount of supported offloaded filters
significantly. Without counter assigned, the HW is capable to carry
millions of those.

On top of that, mlxsw HW is able to support delayed counters as well in
greater numbers. That is going to be added in a follow-up patch.

This patchset allows user to specify one of the following types of HW
stats for added action:
any - current default, user does not care about the type, just expects
      any type of stats.
immediate - queried during dump time
delayed - polled from HW periodically or sent by HW in async manner
disabled - no stats needed

Examples:
$ tc filter add dev enp0s16np28 ingress proto ip handle 1 pref 1 flower skip_sw dst_ip 192.168.1.1 action drop hw_stats disabled
$ tc -s filter show dev enp0s16np28 ingress 
filter protocol ip pref 1 flower chain 0 
filter protocol ip pref 1 flower chain 0 handle 0x1 
  eth_type ipv4
  dst_ip 192.168.1.1
  skip_sw
  in_hw in_hw_count 2
        action order 1: gact action drop
         random type none pass val 0
         index 1 ref 1 bind 1 installed 13 sec used 4 sec
        Action statistics:
        Sent 1164 bytes 588185936 pkt (dropped 588185936, overlimits 0 requeues 0) 
        Sent software 0 bytes 0 pkt
        Sent hardware 1164 bytes 588185936 pkt
        backlog 0b 0p requeues 0
        hw_stats disabled
	
$ tc filter add dev enp0s16np28 ingress proto ip handle 1 pref 1 flower skip_sw dst_ip 192.168.1.1 action drop hw_stats immediate
$ tc -s filter show dev enp0s16np28 ingress
filter protocol ip pref 1 flower chain 0 
filter protocol ip pref 1 flower chain 0 handle 0x1 
  eth_type ipv4
  dst_ip 192.168.1.1
  skip_sw
  in_hw in_hw_count 2
        action order 1: gact action drop
         random type none pass val 0
         index 1 ref 1 bind 1 installed 11 sec used 4 sec
        Action statistics:
        Sent 102 bytes 1 pkt (dropped 1, overlimits 0 requeues 0) 
        Sent software 0 bytes 0 pkt
        Sent hardware 102 bytes 1 pkt
        backlog 0b 0p requeues 0
        hw_stats immediate

Jiri Pirko (12):
  flow_offload: Introduce offload of HW stats type
  ocelot_flower: use flow_offload_has_one_action() helper
  flow_offload: check for basic action hw stats type
  mlx5: en_tc: Do not allow mixing HW stats types for actions
  mlxsw: spectrum_flower: Do not allow mixing HW stats types for actions
  mlx5: restrict supported HW stats type to "any"
  mlxsw: restrict supported HW stats type to "any"
  flow_offload: introduce "immediate" HW stats type and allow it in
    mlxsw
  flow_offload: introduce "delayed" HW stats type and allow it in mlx5
  mlxsw: spectrum_acl: Ask device for rule stats only if counter was
    created
  flow_offload: introduce "disabled" HW stats type and allow it in mlxsw
  sched: act: allow user to specify type of HW stats for a filter

 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  |  9 +++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  |  8 +++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.h  |  3 +-
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         |  3 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.c    |  6 +++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 23 ++++++++++
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  3 +-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    | 25 ++++++----
 .../ethernet/mellanox/mlxsw/spectrum_flower.c | 21 +++++++--
 drivers/net/ethernet/mscc/ocelot_flower.c     |  6 ++-
 .../ethernet/netronome/nfp/flower/action.c    |  4 ++
 .../net/ethernet/qlogic/qede/qede_filter.c    | 10 ++--
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   |  9 +++-
 include/net/act_api.h                         |  1 +
 include/net/flow_offload.h                    | 46 +++++++++++++++++++
 include/uapi/linux/pkt_cls.h                  | 26 +++++++++++
 net/dsa/slave.c                               |  4 ++
 net/sched/act_api.c                           | 21 +++++++++
 net/sched/cls_api.c                           | 26 +++++++++++
 19 files changed, 229 insertions(+), 25 deletions(-)

-- 
2.21.1

[patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
current implicit value coming down to flow_offload. Add a bool
indicating that entries have mixed HW stats type.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- moved to actions
- add mixed bool
---
 include/net/flow_offload.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 4e864c34a1b0..eee1cbc5db3c 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -154,6 +154,10 @@ enum flow_action_mangle_base {
 	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
 };
 
+enum flow_action_hw_stats_type {
+	FLOW_ACTION_HW_STATS_TYPE_ANY,
+};
+
 typedef void (*action_destr)(void *priv);
 
 struct flow_action_cookie {
@@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
 
 struct flow_action_entry {
 	enum flow_action_id		id;
+	enum flow_action_hw_stats_type	hw_stats_type;
 	action_destr			destructor;
 	void				*destructor_priv;
 	union {
@@ -228,6 +233,7 @@ struct flow_action_entry {
 };
 
 struct flow_action {
+	bool				mixed_hw_stats_types;
 	unsigned int			num_entries;
 	struct flow_action_entry 	entries[0];
 };
-- 
2.21.1

[patch net-next v2 02/12] ocelot_flower: use flow_offload_has_one_action() helper

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Instead of directly checking number of action entries, use
flow_offload_has_one_action() helper.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- new patch
---
 drivers/net/ethernet/mscc/ocelot_flower.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 3d65b99b9734..b85111dcc2be 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -19,7 +19,7 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f,
 	const struct flow_action_entry *a;
 	int i;
 
-	if (f->rule->action.num_entries != 1)
+	if (!flow_offload_has_one_action(&f->rule->action))
 		return -EOPNOTSUPP;
 
 	flow_action_for_each(i, a, &f->rule->action) {
-- 
2.21.1

[patch net-next v2 03/12] flow_offload: check for basic action hw stats type

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce flow_action_basic_hw_stats_types_check() helper and use it
in drivers. That sanitizes the drivers which do not have support
for action HW stats types.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- new patch
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  |  9 ++++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  |  8 +++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.h  |  3 +-
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         |  3 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.c    |  6 +++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  3 ++
 drivers/net/ethernet/mscc/ocelot_flower.c     |  4 ++
 .../ethernet/netronome/nfp/flower/action.c    |  4 ++
 .../net/ethernet/qlogic/qede/qede_filter.c    | 10 +++--
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   |  9 ++++-
 include/net/flow_offload.h                    | 37 +++++++++++++++++++
 net/dsa/slave.c                               |  4 ++
 12 files changed, 89 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 9bec256b0934..523bf4be43cc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -279,7 +279,8 @@ bnxt_tc_parse_pedit(struct bnxt *bp, struct bnxt_tc_actions *actions,
 
 static int bnxt_tc_parse_actions(struct bnxt *bp,
 				 struct bnxt_tc_actions *actions,
-				 struct flow_action *flow_action)
+				 struct flow_action *flow_action,
+				 struct netlink_ext_ack *extack)
 {
 	/* Used to store the L2 rewrite mask for dmac (6 bytes) followed by
 	 * smac (6 bytes) if rewrite of both is specified, otherwise either
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
 		return -EINVAL;
 	}
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
@@ -491,7 +495,8 @@ static int bnxt_tc_parse_flow(struct bnxt *bp,
 		flow->tun_mask.tp_src = match.mask->src;
 	}
 
-	return bnxt_tc_parse_actions(bp, &flow->actions, &rule->action);
+	return bnxt_tc_parse_actions(bp, &flow->actions, &rule->action,
+				     tc_flow_cmd->common.extack);
 }
 
 static int bnxt_hwrm_cfa_flow_free(struct bnxt *bp,
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index bb5513bdd293..cc46277e98de 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -544,7 +544,8 @@ static bool valid_pedit_action(struct net_device *dev,
 }
 
 int cxgb4_validate_flow_actions(struct net_device *dev,
-				struct flow_action *actions)
+				struct flow_action *actions,
+				struct netlink_ext_ack *extack)
 {
 	struct flow_action_entry *act;
 	bool act_redir = false;
@@ -552,6 +553,9 @@ int cxgb4_validate_flow_actions(struct net_device *dev,
 	bool act_vlan = false;
 	int i;
 
+	if (!flow_action_basic_hw_stats_types_check(actions, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, actions) {
 		switch (act->id) {
 		case FLOW_ACTION_ACCEPT:
@@ -642,7 +646,7 @@ int cxgb4_tc_flower_replace(struct net_device *dev,
 	struct filter_ctx ctx;
 	int fidx, ret;
 
-	if (cxgb4_validate_flow_actions(dev, &rule->action))
+	if (cxgb4_validate_flow_actions(dev, &rule->action, extack))
 		return -EOPNOTSUPP;
 
 	if (cxgb4_validate_flow_match(dev, cls))
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
index e132516e9868..0a30c96b81ff 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
@@ -112,7 +112,8 @@ void cxgb4_process_flow_actions(struct net_device *in,
 				struct flow_action *actions,
 				struct ch_filter_specification *fs);
 int cxgb4_validate_flow_actions(struct net_device *dev,
-				struct flow_action *actions);
+				struct flow_action *actions,
+				struct netlink_ext_ack *extack);
 
 int cxgb4_tc_flower_replace(struct net_device *dev,
 			    struct flow_cls_offload *cls);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
index 1b7681a4eb32..d80dee4d316d 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
@@ -286,7 +286,8 @@ int cxgb4_tc_matchall_replace(struct net_device *dev,
 		}
 
 		ret = cxgb4_validate_flow_actions(dev,
-						  &cls_matchall->rule->action);
+						  &cls_matchall->rule->action,
+						  extack);
 		if (ret)
 			return ret;
 
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
index 35478cba2aa5..0a0c6ec2336c 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
@@ -1082,6 +1082,9 @@ static int mvpp2_port_c2_tcam_rule_add(struct mvpp2_port *port,
 	u8 qh, ql, pmap;
 	int index, ctx;
 
+	if (!flow_action_basic_hw_stats_types_check(&rule->flow->action, NULL))
+		return -EOPNOTSUPP;
+
 	memset(&c2, 0, sizeof(c2));
 
 	index = mvpp2_cls_c2_port_flow_index(port, rule->loc);
@@ -1305,6 +1308,9 @@ static int mvpp2_cls_rfs_parse_rule(struct mvpp2_rfs_rule *rule)
 	struct flow_rule *flow = rule->flow;
 	struct flow_action_entry *act;
 
+	if (!flow_action_basic_hw_stats_types_check(&rule->flow->action, NULL))
+		return -EOPNOTSUPP;
+
 	act = &flow->action.entries[0];
 	if (act->id != FLOW_ACTION_QUEUE && act->id != FLOW_ACTION_DROP)
 		return -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 74091f72c9a8..ae8bb77b262e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4125,6 +4125,9 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
 		return -EOPNOTSUPP;
 	}
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_POLICE:
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index b85111dcc2be..c1995511f4cd 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -22,6 +22,10 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f,
 	if (!flow_offload_has_one_action(&f->rule->action))
 		return -EOPNOTSUPP;
 
+	if (!flow_action_basic_hw_stats_types_check(&f->rule->action,
+						    f->common.extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, a, &f->rule->action) {
 		switch (a->id) {
 		case FLOW_ACTION_DROP:
diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c b/drivers/net/ethernet/netronome/nfp/flower/action.c
index c06600fb47ff..4aa7346cb040 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/action.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
@@ -1207,6 +1207,10 @@ int nfp_flower_compile_action(struct nfp_app *app,
 	bool pkt_host = false;
 	u32 csum_updated = 0;
 
+	if (!flow_action_basic_hw_stats_types_check(&flow->rule->action,
+						    extack))
+		return -EOPNOTSUPP;
+
 	memset(nfp_flow->action_data, 0, NFP_FL_MAX_A_SIZ);
 	nfp_flow->meta.act_len = 0;
 	tun_type = NFP_FL_TUNNEL_NONE;
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index d1ce4531d01a..6505f7e2d1db 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1746,7 +1746,8 @@ int qede_get_arfs_filter_count(struct qede_dev *edev)
 }
 
 static int qede_parse_actions(struct qede_dev *edev,
-			      struct flow_action *flow_action)
+			      struct flow_action *flow_action,
+			      struct netlink_ext_ack *extack)
 {
 	const struct flow_action_entry *act;
 	int i;
@@ -1756,6 +1757,9 @@ static int qede_parse_actions(struct qede_dev *edev,
 		return -EINVAL;
 	}
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
@@ -1970,7 +1974,7 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,
 	}
 
 	/* parse tc actions and get the vf_id */
-	if (qede_parse_actions(edev, &f->rule->action))
+	if (qede_parse_actions(edev, &f->rule->action, f->common.extack))
 		goto unlock;
 
 	if (qede_flow_find_fltr(edev, &t)) {
@@ -2038,7 +2042,7 @@ static int qede_flow_spec_validate(struct qede_dev *edev,
 		return -EINVAL;
 	}
 
-	if (qede_parse_actions(edev, flow_action))
+	if (qede_parse_actions(edev, flow_action, NULL))
 		return -EINVAL;
 
 	return 0;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 7a01dee2f9a8..a0e6118444b0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -367,7 +367,8 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
 
 static int tc_parse_flow_actions(struct stmmac_priv *priv,
 				 struct flow_action *action,
-				 struct stmmac_flow_entry *entry)
+				 struct stmmac_flow_entry *entry,
+				 struct netlink_ext_ack *extack)
 {
 	struct flow_action_entry *act;
 	int i;
@@ -375,6 +376,9 @@ static int tc_parse_flow_actions(struct stmmac_priv *priv,
 	if (!flow_action_has_entries(action))
 		return -EINVAL;
 
+	if (!flow_action_basic_hw_stats_types_check(action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
@@ -530,7 +534,8 @@ static int tc_add_flow(struct stmmac_priv *priv,
 			return -ENOENT;
 	}
 
-	ret = tc_parse_flow_actions(priv, &rule->action, entry);
+	ret = tc_parse_flow_actions(priv, &rule->action, entry,
+				    cls->common.extack);
 	if (ret)
 		return ret;
 
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index eee1cbc5db3c..69791494efc5 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -3,6 +3,7 @@
 
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/netlink.h>
 #include <net/flow_dissector.h>
 #include <linux/rhashtable.h>
 
@@ -254,6 +255,42 @@ static inline bool flow_offload_has_one_action(const struct flow_action *action)
 	return action->num_entries == 1;
 }
 
+static inline bool
+flow_action_mixed_hw_stats_types_check(const struct flow_action *action,
+				       struct netlink_ext_ack *extack)
+{
+	if (action->mixed_hw_stats_types) {
+		NL_SET_ERR_MSG_MOD(extack, "Mixing HW stats types for actions is not supported");
+		return false;
+	}
+	return true;
+}
+
+static inline const struct flow_action_entry *
+flow_action_first_entry_get(const struct flow_action *action)
+{
+	WARN_ON(!flow_action_has_entries(action));
+	return &action->entries[0];
+}
+
+static inline bool
+flow_action_basic_hw_stats_types_check(const struct flow_action *action,
+				       struct netlink_ext_ack *extack)
+{
+	const struct flow_action_entry *action_entry;
+
+	if (!flow_action_has_entries(action))
+		return true;
+	if (!flow_action_mixed_hw_stats_types_check(action, extack))
+		return false;
+	action_entry = flow_action_first_entry_get(action);
+	if (action_entry->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
+		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
+		return false;
+	}
+	return true;
+}
+
 #define flow_action_for_each(__i, __act, __actions)			\
         for (__i = 0, __act = &(__actions)->entries[0]; __i < (__actions)->num_entries; __act = &(__actions)->entries[++__i])
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 088c886e609e..0f3f05b2285f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -865,6 +865,10 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 	if (!flow_offload_has_one_action(&cls->rule->action))
 		return err;
 
+	if (!flow_action_basic_hw_stats_types_check(&cls->rule->action,
+						    cls->common.extack))
+		return err;
+
 	act = &cls->rule->action.entries[0];
 
 	if (act->id == FLOW_ACTION_MIRRED && protocol == htons(ETH_P_ALL)) {
-- 
2.21.1

[patch net-next v2 04/12] mlx5: en_tc: Do not allow mixing HW stats types for actions

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

As there is one set of counters for the whole action chain, forbid to
mix the HW stats types.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- new patch
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index ae8bb77b262e..82e08b10598f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2882,6 +2882,9 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
 	if (!flow_action_has_entries(flow_action))
 		return -EINVAL;
 
+	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
 
 	flow_action_for_each(i, act, flow_action) {
@@ -3334,6 +3337,9 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 	if (!flow_action_has_entries(flow_action))
 		return -EINVAL;
 
+	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
-- 
2.21.1

[patch net-next v2 05/12] mlxsw: spectrum_flower: Do not allow mixing HW stats types for actions

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

As there is one set of counters for the whole action chain, forbid to
mix the HW stats types.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- new patch
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 0011a71114e3..7435629c9e65 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -26,6 +26,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 
 	if (!flow_action_has_entries(flow_action))
 		return 0;
+	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
 
 	/* Count action is inserted first */
 	err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
-- 
2.21.1

[patch net-next v2 06/12] mlx5: restrict supported HW stats type to "any"

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Currently don't allow action with any other type than "any"
to be inserted.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- moved the check to action
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 82e08b10598f..9a8da84e88c9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2885,6 +2885,12 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
 	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
 		return -EOPNOTSUPP;
 
+	act = flow_action_first_entry_get(flow_action);
+	if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
+		NL_SET_ERR_MSG_MOD(extack, "Unsupported HW stats type");
+		return -EOPNOTSUPP;
+	}
+
 	attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
 
 	flow_action_for_each(i, act, flow_action) {
@@ -3340,6 +3346,12 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
 		return -EOPNOTSUPP;
 
+	act = flow_action_first_entry_get(flow_action);
+	if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
+		NL_SET_ERR_MSG_MOD(extack, "Unsupported HW stats type");
+		return -EOPNOTSUPP;
+	}
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
-- 
2.21.1

[patch net-next v2 07/12] mlxsw: restrict supported HW stats type to "any"

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Currently don't allow actions with any other type to be inserted.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- move the code to the first action processing
---
 .../ethernet/mellanox/mlxsw/spectrum_flower.c    | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 7435629c9e65..40d3ed2f4961 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -29,10 +29,18 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
 		return -EOPNOTSUPP;
 
-	/* Count action is inserted first */
-	err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
-	if (err)
-		return err;
+	act = flow_action_first_entry_get(flow_action);
+	switch (act->hw_stats_type) {
+	case FLOW_ACTION_HW_STATS_TYPE_ANY:
+		/* Count action is inserted first */
+		err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
+		if (err)
+			return err;
+		break;
+	default:
+		NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type");
+		return -EOPNOTSUPP;
+	}
 
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
-- 
2.21.1

[patch net-next v2 08/12] flow_offload: introduce "immediate" HW stats type and allow it in mlxsw

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce new type for immediate HW stats and allow the value in
mlxsw offload.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- moved to action
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 3 ++-
 include/net/flow_offload.h                            | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 40d3ed2f4961..dcf04fae0c59 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -31,7 +31,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 
 	act = flow_action_first_entry_get(flow_action);
 	switch (act->hw_stats_type) {
-	case FLOW_ACTION_HW_STATS_TYPE_ANY:
+	case FLOW_ACTION_HW_STATS_TYPE_ANY: /* fall-through */
+	case FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE:
 		/* Count action is inserted first */
 		err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
 		if (err)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 69791494efc5..7f0e2a20078f 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -157,6 +157,7 @@ enum flow_action_mangle_base {
 
 enum flow_action_hw_stats_type {
 	FLOW_ACTION_HW_STATS_TYPE_ANY,
+	FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE,
 };
 
 typedef void (*action_destr)(void *priv);
-- 
2.21.1

[patch net-next v2 09/12] flow_offload: introduce "delayed" HW stats type and allow it in mlx5

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce new type for delayed HW stats and allow the value in
mlx5 offload.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- moved to action
- fixed c&p error in patch description
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 6 ++++--
 include/net/flow_offload.h                      | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 9a8da84e88c9..81242ed4f0ca 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2886,7 +2886,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
 		return -EOPNOTSUPP;
 
 	act = flow_action_first_entry_get(flow_action);
-	if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
+	if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY &&
+	    act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_DELAYED) {
 		NL_SET_ERR_MSG_MOD(extack, "Unsupported HW stats type");
 		return -EOPNOTSUPP;
 	}
@@ -3347,7 +3348,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 		return -EOPNOTSUPP;
 
 	act = flow_action_first_entry_get(flow_action);
-	if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
+	if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY &&
+	    act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_DELAYED) {
 		NL_SET_ERR_MSG_MOD(extack, "Unsupported HW stats type");
 		return -EOPNOTSUPP;
 	}
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 7f0e2a20078f..79a0fda9b7bd 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -158,6 +158,7 @@ enum flow_action_mangle_base {
 enum flow_action_hw_stats_type {
 	FLOW_ACTION_HW_STATS_TYPE_ANY,
 	FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE,
+	FLOW_ACTION_HW_STATS_TYPE_DELAYED,
 };
 
 typedef void (*action_destr)(void *priv);
-- 
2.21.1

[patch net-next v2 10/12] mlxsw: spectrum_acl: Ask device for rule stats only if counter was created

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Set a flag in case rule counter was created. Only query the device for
stats of a rule, which has the valid counter assigned.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- new patch
---
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  3 ++-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    | 25 +++++++++++++------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 9708156e7871..ea712e6fa390 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -641,7 +641,8 @@ struct mlxsw_sp_acl_rule_info {
 	struct mlxsw_afa_block *act_block;
 	u8 action_created:1,
 	   ingress_bind_blocker:1,
-	   egress_bind_blocker:1;
+	   egress_bind_blocker:1,
+	   counter_valid:1;
 	unsigned int counter_index;
 };
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
index 36b264798f04..889330ac39dc 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
@@ -642,8 +642,14 @@ int mlxsw_sp_acl_rulei_act_count(struct mlxsw_sp *mlxsw_sp,
 				 struct mlxsw_sp_acl_rule_info *rulei,
 				 struct netlink_ext_ack *extack)
 {
-	return mlxsw_afa_block_append_counter(rulei->act_block,
-					      &rulei->counter_index, extack);
+	int err;
+
+	err = mlxsw_afa_block_append_counter(rulei->act_block,
+					     &rulei->counter_index, extack);
+	if (err)
+		return err;
+	rulei->counter_valid = true;
+	return 0;
 }
 
 int mlxsw_sp_acl_rulei_act_fid_set(struct mlxsw_sp *mlxsw_sp,
@@ -862,13 +868,16 @@ int mlxsw_sp_acl_rule_get_stats(struct mlxsw_sp *mlxsw_sp,
 	int err;
 
 	rulei = mlxsw_sp_acl_rule_rulei(rule);
-	err = mlxsw_sp_flow_counter_get(mlxsw_sp, rulei->counter_index,
-					&current_packets, &current_bytes);
-	if (err)
-		return err;
+	if (rulei->counter_valid) {
+		err = mlxsw_sp_flow_counter_get(mlxsw_sp, rulei->counter_index,
+						&current_packets,
+						&current_bytes);
+		if (err)
+			return err;
 
-	*packets = current_packets - rule->last_packets;
-	*bytes = current_bytes - rule->last_bytes;
+		*packets = current_packets - rule->last_packets;
+		*bytes = current_bytes - rule->last_bytes;
+	}
 	*last_use = rule->last_used;
 
 	rule->last_bytes = current_bytes;
-- 
2.21.1

[patch net-next v2 11/12] flow_offload: introduce "disabled" HW stats type and allow it in mlxsw

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce new type for disabled HW stats and allow the value in
mlxsw offload.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- moved to action
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 ++
 include/net/flow_offload.h                            | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index dcf04fae0c59..2f87b7c1e28e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -38,6 +38,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 		if (err)
 			return err;
 		break;
+	case FLOW_ACTION_HW_STATS_TYPE_DISABLED:
+		break;
 	default:
 		NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type");
 		return -EOPNOTSUPP;
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 79a0fda9b7bd..a5b50d8abc0a 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -159,6 +159,7 @@ enum flow_action_hw_stats_type {
 	FLOW_ACTION_HW_STATS_TYPE_ANY,
 	FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE,
 	FLOW_ACTION_HW_STATS_TYPE_DELAYED,
+	FLOW_ACTION_HW_STATS_TYPE_DISABLED,
 };
 
 typedef void (*action_destr)(void *priv);
-- 
2.21.1

[patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Currently, user who is adding an action expects HW to report stats,
however it does not have exact expectations about the stats types.
That is aligned with TCA_ACT_HW_STATS_TYPE_ANY.

Allow user to specify the type of HW stats for an action and require it.

Pass the information down to flow_offload layer.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- moved the stats attr from cls_flower (filter) to any action
- rebased on top of cookie offload changes
- adjusted the patch description a bit
---
 include/net/act_api.h        |  1 +
 include/uapi/linux/pkt_cls.h | 26 ++++++++++++++++++++++++++
 net/sched/act_api.c          | 21 +++++++++++++++++++++
 net/sched/cls_api.c          | 26 ++++++++++++++++++++++++++
 4 files changed, 74 insertions(+)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 71347a90a9d1..02b9bffa17ed 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -39,6 +39,7 @@ struct tc_action {
 	struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw;
 	struct gnet_stats_queue __percpu *cpu_qstats;
 	struct tc_cookie	__rcu *act_cookie;
+	enum tca_act_hw_stats_type	hw_stats_type;
 	struct tcf_chain	__rcu *goto_chain;
 	u32			tcfa_flags;
 };
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 449a63971451..096ea59a090b 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -17,6 +17,7 @@ enum {
 	TCA_ACT_PAD,
 	TCA_ACT_COOKIE,
 	TCA_ACT_FLAGS,
+	TCA_ACT_HW_STATS_TYPE,
 	__TCA_ACT_MAX
 };
 
@@ -118,6 +119,31 @@ enum tca_id {
 
 #define TCA_ID_MAX __TCA_ID_MAX
 
+/* tca HW stats type */
+enum tca_act_hw_stats_type {
+	TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default
+				    * when user does not pass the attr.
+				    * Instructs the driver that user does not
+				    * care if the HW stats are "immediate"
+				    * or "delayed".
+				    */
+	TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets
+					  * the current HW stats state from
+					  * the device queried at the dump time.
+					  */
+	TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets
+					* HW stats that might be out of date
+					* for some time, maybe couple of
+					* seconds. This is the case when driver
+					* polls stats updates periodically
+					* or when it gets async stats update
+					* from the device.
+					*/
+	TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting
+					 * any HW statistics.
+					 */
+};
+
 struct tc_police {
 	__u32			index;
 	int			action;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8c466a712cda..d6468b09b932 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -185,6 +185,7 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
 	return  nla_total_size(0) /* action number nested */
 		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
 		+ cookie_len /* TCA_ACT_COOKIE */
+		+ nla_total_size(sizeof(u8)) /* TCA_ACT_HW_STATS_TYPE */
 		+ nla_total_size(0) /* TCA_ACT_STATS nested */
 		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */
 		/* TCA_STATS_BASIC */
@@ -788,6 +789,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 	}
 	rcu_read_unlock();
 
+	if (nla_put_u8(skb, TCA_ACT_HW_STATS_TYPE, a->hw_stats_type))
+		goto nla_put_failure;
+
 	if (a->tcfa_flags) {
 		struct nla_bitfield32 flags = { a->tcfa_flags,
 						a->tcfa_flags, };
@@ -854,12 +858,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
 	return c;
 }
 
+static inline enum tca_act_hw_stats_type
+tcf_action_hw_stats_type_get(struct nlattr *hw_stats_type_attr)
+{
+	/* If the user did not pass the attr, that means he does
+	 * not care about the type. Return "any" in that case.
+	 */
+	return hw_stats_type_attr ? nla_get_u8(hw_stats_type_attr) :
+				    TCA_ACT_HW_STATS_TYPE_ANY;
+}
+
 static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS;
 static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
 	[TCA_ACT_KIND]		= { .type = NLA_STRING },
 	[TCA_ACT_INDEX]		= { .type = NLA_U32 },
 	[TCA_ACT_COOKIE]	= { .type = NLA_BINARY,
 				    .len = TC_COOKIE_MAX_SIZE },
+	[TCA_ACT_HW_STATS_TYPE]	= { .type = NLA_U8 },
 	[TCA_ACT_OPTIONS]	= { .type = NLA_NESTED },
 	[TCA_ACT_FLAGS]		= { .type = NLA_BITFIELD32,
 				    .validation_data = &tca_act_flags_allowed },
@@ -871,6 +886,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				    bool rtnl_held,
 				    struct netlink_ext_ack *extack)
 {
+	enum tca_act_hw_stats_type hw_stats_type = TCA_ACT_HW_STATS_TYPE_ANY;
 	struct nla_bitfield32 flags = { 0, 0 };
 	struct tc_action *a;
 	struct tc_action_ops *a_o;
@@ -903,6 +919,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				goto err_out;
 			}
 		}
+		hw_stats_type =
+			tcf_action_hw_stats_type_get(tb[TCA_ACT_HW_STATS_TYPE]);
 		if (tb[TCA_ACT_FLAGS])
 			flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]);
 	} else {
@@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (!name && tb[TCA_ACT_COOKIE])
 		tcf_set_action_cookie(&a->act_cookie, cookie);
 
+	if (!name)
+		a->hw_stats_type = hw_stats_type;
+
 	/* module count goes up only when brand new policy is created
 	 * if it exists and is only bound to in a_o->init() then
 	 * ACT_P_CREATED is not returned (a zero is).
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 4e766c5ab77a..21bf37242153 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3458,9 +3458,28 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
 #endif
 }
 
+static inline enum flow_action_hw_stats_type
+tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type)
+{
+	switch (hw_stats_type) {
+	default:
+		WARN_ON(1);
+		/* fall-through */
+	case TCA_ACT_HW_STATS_TYPE_ANY:
+		return FLOW_ACTION_HW_STATS_TYPE_ANY;
+	case TCA_ACT_HW_STATS_TYPE_IMMEDIATE:
+		return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE;
+	case TCA_ACT_HW_STATS_TYPE_DELAYED:
+		return FLOW_ACTION_HW_STATS_TYPE_DELAYED;
+	case TCA_ACT_HW_STATS_TYPE_DISABLED:
+		return FLOW_ACTION_HW_STATS_TYPE_DISABLED;
+	}
+}
+
 int tc_setup_flow_action(struct flow_action *flow_action,
 			 const struct tcf_exts *exts)
 {
+	enum flow_action_hw_stats_type uninitialized_var(last_hw_stats_type);
 	struct tc_action *act;
 	int i, j, k, err = 0;
 
@@ -3476,6 +3495,13 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 		err = tcf_act_get_cookie(entry, act);
 		if (err)
 			goto err_out_locked;
+
+		entry->hw_stats_type =
+			tcf_flow_action_hw_stats_type(act->hw_stats_type);
+		if (i && last_hw_stats_type != entry->hw_stats_type)
+			flow_action->mixed_hw_stats_types = true;
+		last_hw_stats_type = entry->hw_stats_type;
+
 		if (is_tcf_gact_ok(act)) {
 			entry->id = FLOW_ACTION_ACCEPT;
 		} else if (is_tcf_gact_shot(act)) {
-- 
2.21.1

Re: [patch net-next v2 03/12] flow_offload: check for basic action hw stats type

[Jakub Kicinski]

On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
> @@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
>  		return -EINVAL;
>  	}
>  
> +	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
> +		return -EOPNOTSUPP;

Could we have this helper take one stat type? To let drivers pass the
stat type they support? 

At some point we should come up with a way to express the limitations
at callback registration time so we don't need to add checks like this
to all the drivers. On the TODO list it goes :)

>  	flow_action_for_each(i, act, flow_action) {
>  		switch (act->id) {
>  		case FLOW_ACTION_DROP:

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jakub Kicinski]

On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Currently, user who is adding an action expects HW to report stats,
> however it does not have exact expectations about the stats types.
> That is aligned with TCA_ACT_HW_STATS_TYPE_ANY.
> 
> Allow user to specify the type of HW stats for an action and require it.
> 
> Pass the information down to flow_offload layer.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v1->v2:
> - moved the stats attr from cls_flower (filter) to any action
> - rebased on top of cookie offload changes
> - adjusted the patch description a bit

Thanks, this looks good... I mean I wish we could just share actions
instead but this set is less objectionable than v1 :)

> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 71347a90a9d1..02b9bffa17ed 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -39,6 +39,7 @@ struct tc_action {
>  	struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw;
>  	struct gnet_stats_queue __percpu *cpu_qstats;
>  	struct tc_cookie	__rcu *act_cookie;
> +	enum tca_act_hw_stats_type	hw_stats_type;
>  	struct tcf_chain	__rcu *goto_chain;
>  	u32			tcfa_flags;
>  };
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 449a63971451..096ea59a090b 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -17,6 +17,7 @@ enum {
>  	TCA_ACT_PAD,
>  	TCA_ACT_COOKIE,
>  	TCA_ACT_FLAGS,
> +	TCA_ACT_HW_STATS_TYPE,
>  	__TCA_ACT_MAX
>  };
>  
> @@ -118,6 +119,31 @@ enum tca_id {
>  
>  #define TCA_ID_MAX __TCA_ID_MAX
>  
> +/* tca HW stats type */
> +enum tca_act_hw_stats_type {
> +	TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default
> +				    * when user does not pass the attr.
> +				    * Instructs the driver that user does not
> +				    * care if the HW stats are "immediate"
> +				    * or "delayed".
> +				    */
> +	TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets
> +					  * the current HW stats state from
> +					  * the device queried at the dump time.
> +					  */
> +	TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets
> +					* HW stats that might be out of date
> +					* for some time, maybe couple of
> +					* seconds. This is the case when driver
> +					* polls stats updates periodically
> +					* or when it gets async stats update
> +					* from the device.
> +					*/
> +	TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting
> +					 * any HW statistics.
> +					 */
> +};

On the ABI I wonder if we can redefine it a little bit..

Can we make the stat types into a bitfield?

On request:
 - no attr -> any stats allowed but some stats must be provided *
 - 0       -> no stats requested / disabled
 - 0x1     -> must be stat type0
 - 0x6     -> stat type1 or stat type2 are both fine

* no attr kinda doesn't work 'cause u32 offload has no stats and this
  is action-level now, not flower-level :S What about u32 and matchall?

We can add a separate attribute with "active" stat types:
 - no attr -> old kernel
 - 0       -> no stats are provided / stats disabled
 - 0x1     -> only stat type0 is used by drivers
 - 0x6     -> at least one driver is using type1 and one type2

That assumes that we may one day add another stat type which would 
not be just based on the reporting time.

If we only foresee time-based reporting would it make sense to turn 
the attribute into max acceptable delay in ms?

0        -> only immediate / blocking stats
(0, MAX) -> given reporting delay in ms is acceptable
MAX      -> don't care about stats at all

>  struct tc_police {
>  	__u32			index;
>  	int			action;
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 8c466a712cda..d6468b09b932 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -185,6 +185,7 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
>  	return  nla_total_size(0) /* action number nested */
>  		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
>  		+ cookie_len /* TCA_ACT_COOKIE */
> +		+ nla_total_size(sizeof(u8)) /* TCA_ACT_HW_STATS_TYPE */
>  		+ nla_total_size(0) /* TCA_ACT_STATS nested */
>  		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */
>  		/* TCA_STATS_BASIC */
> @@ -788,6 +789,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>  	}
>  	rcu_read_unlock();
>  
> +	if (nla_put_u8(skb, TCA_ACT_HW_STATS_TYPE, a->hw_stats_type))
> +		goto nla_put_failure;
> +
>  	if (a->tcfa_flags) {
>  		struct nla_bitfield32 flags = { a->tcfa_flags,
>  						a->tcfa_flags, };
> @@ -854,12 +858,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
>  	return c;
>  }
>  
> +static inline enum tca_act_hw_stats_type

static inline in C source

> +tcf_action_hw_stats_type_get(struct nlattr *hw_stats_type_attr)
> +{
> +	/* If the user did not pass the attr, that means he does
> +	 * not care about the type. Return "any" in that case.
> +	 */
> +	return hw_stats_type_attr ? nla_get_u8(hw_stats_type_attr) :
> +				    TCA_ACT_HW_STATS_TYPE_ANY;
> +}
> +
>  static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS;
>  static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
>  	[TCA_ACT_KIND]		= { .type = NLA_STRING },
>  	[TCA_ACT_INDEX]		= { .type = NLA_U32 },
>  	[TCA_ACT_COOKIE]	= { .type = NLA_BINARY,
>  				    .len = TC_COOKIE_MAX_SIZE },
> +	[TCA_ACT_HW_STATS_TYPE]	= { .type = NLA_U8 },

We can use a POLICY with MIN/MAX here, perhaps?

>  	[TCA_ACT_OPTIONS]	= { .type = NLA_NESTED },
>  	[TCA_ACT_FLAGS]		= { .type = NLA_BITFIELD32,
>  				    .validation_data = &tca_act_flags_allowed },
> @@ -871,6 +886,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  				    bool rtnl_held,
>  				    struct netlink_ext_ack *extack)
>  {
> +	enum tca_act_hw_stats_type hw_stats_type = TCA_ACT_HW_STATS_TYPE_ANY;
>  	struct nla_bitfield32 flags = { 0, 0 };
>  	struct tc_action *a;
>  	struct tc_action_ops *a_o;
> @@ -903,6 +919,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  				goto err_out;
>  			}
>  		}
> +		hw_stats_type =
> +			tcf_action_hw_stats_type_get(tb[TCA_ACT_HW_STATS_TYPE]);
>  		if (tb[TCA_ACT_FLAGS])
>  			flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]);
>  	} else {
> @@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  	if (!name && tb[TCA_ACT_COOKIE])
>  		tcf_set_action_cookie(&a->act_cookie, cookie);
>  
> +	if (!name)
> +		a->hw_stats_type = hw_stats_type;
> +
>  	/* module count goes up only when brand new policy is created
>  	 * if it exists and is only bound to in a_o->init() then
>  	 * ACT_P_CREATED is not returned (a zero is).
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 4e766c5ab77a..21bf37242153 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3458,9 +3458,28 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
>  #endif
>  }
>  
> +static inline enum flow_action_hw_stats_type

static inline in C source

> +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type)
> +{
> +	switch (hw_stats_type) {
> +	default:
> +		WARN_ON(1);
> +		/* fall-through */

without the policy change this seems user-triggerable

> +	case TCA_ACT_HW_STATS_TYPE_ANY:
> +		return FLOW_ACTION_HW_STATS_TYPE_ANY;
> +	case TCA_ACT_HW_STATS_TYPE_IMMEDIATE:
> +		return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE;
> +	case TCA_ACT_HW_STATS_TYPE_DELAYED:
> +		return FLOW_ACTION_HW_STATS_TYPE_DELAYED;
> +	case TCA_ACT_HW_STATS_TYPE_DISABLED:
> +		return FLOW_ACTION_HW_STATS_TYPE_DISABLED;
> +	}
> +}
> +
>  int tc_setup_flow_action(struct flow_action *flow_action,
>  			 const struct tcf_exts *exts)
>  {
> +	enum flow_action_hw_stats_type uninitialized_var(last_hw_stats_type);
>  	struct tc_action *act;
>  	int i, j, k, err = 0;
>  
> @@ -3476,6 +3495,13 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>  		err = tcf_act_get_cookie(entry, act);
>  		if (err)
>  			goto err_out_locked;
> +
> +		entry->hw_stats_type =
> +			tcf_flow_action_hw_stats_type(act->hw_stats_type);
> +		if (i && last_hw_stats_type != entry->hw_stats_type)
> +			flow_action->mixed_hw_stats_types = true;
> +		last_hw_stats_type = entry->hw_stats_type;
> +
>  		if (is_tcf_gact_ok(act)) {
>  			entry->id = FLOW_ACTION_ACCEPT;
>  		} else if (is_tcf_gact_shot(act)) {

Re: [patch net-next v2 02/12] ocelot_flower: use flow_offload_has_one_action() helper

[Vladimir Oltean]

On Fri, 28 Feb 2020 at 19:25, Jiri Pirko <jiri@resnulli.us> wrote:
>
> From: Jiri Pirko <jiri@mellanox.com>
>
> Instead of directly checking number of action entries, use
> flow_offload_has_one_action() helper.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---

Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> v1->v2:
> - new patch
> ---
>  drivers/net/ethernet/mscc/ocelot_flower.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
> index 3d65b99b9734..b85111dcc2be 100644
> --- a/drivers/net/ethernet/mscc/ocelot_flower.c
> +++ b/drivers/net/ethernet/mscc/ocelot_flower.c
> @@ -19,7 +19,7 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f,
>         const struct flow_action_entry *a;
>         int i;
>
> -       if (f->rule->action.num_entries != 1)
> +       if (!flow_offload_has_one_action(&f->rule->action))
>                 return -EOPNOTSUPP;
>
>         flow_action_for_each(i, a, &f->rule->action) {
> --
> 2.21.1
>

Re: [patch net-next v2 03/12] flow_offload: check for basic action hw stats type

[Jiri Pirko]

Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
>On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
>> @@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
>> +		return -EOPNOTSUPP;
>
>Could we have this helper take one stat type? To let drivers pass the
>stat type they support? 

That would be always "any" as "any" is supported by all drivers.
And that is exactly what the helper checks..


>
>At some point we should come up with a way to express the limitations
>at callback registration time so we don't need to add checks like this
>to all the drivers. On the TODO list it goes :)

I was thinking about that. Not really easy with the currect infra.

>
>>  	flow_action_for_each(i, act, flow_action) {
>>  		switch (act->id) {
>>  		case FLOW_ACTION_DROP:
>

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jiri Pirko]

Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote:
>On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Currently, user who is adding an action expects HW to report stats,
>> however it does not have exact expectations about the stats types.
>> That is aligned with TCA_ACT_HW_STATS_TYPE_ANY.
>> 
>> Allow user to specify the type of HW stats for an action and require it.
>> 
>> Pass the information down to flow_offload layer.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> v1->v2:
>> - moved the stats attr from cls_flower (filter) to any action
>> - rebased on top of cookie offload changes
>> - adjusted the patch description a bit
>
>Thanks, this looks good... I mean I wish we could just share actions
>instead but this set is less objectionable than v1 :)

You can still share actions, this patchset does not stop you from doing
that.


>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>> index 71347a90a9d1..02b9bffa17ed 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -39,6 +39,7 @@ struct tc_action {
>>  	struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw;
>>  	struct gnet_stats_queue __percpu *cpu_qstats;
>>  	struct tc_cookie	__rcu *act_cookie;
>> +	enum tca_act_hw_stats_type	hw_stats_type;
>>  	struct tcf_chain	__rcu *goto_chain;
>>  	u32			tcfa_flags;
>>  };
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index 449a63971451..096ea59a090b 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -17,6 +17,7 @@ enum {
>>  	TCA_ACT_PAD,
>>  	TCA_ACT_COOKIE,
>>  	TCA_ACT_FLAGS,
>> +	TCA_ACT_HW_STATS_TYPE,
>>  	__TCA_ACT_MAX
>>  };
>>  
>> @@ -118,6 +119,31 @@ enum tca_id {
>>  
>>  #define TCA_ID_MAX __TCA_ID_MAX
>>  
>> +/* tca HW stats type */
>> +enum tca_act_hw_stats_type {
>> +	TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default
>> +				    * when user does not pass the attr.
>> +				    * Instructs the driver that user does not
>> +				    * care if the HW stats are "immediate"
>> +				    * or "delayed".
>> +				    */
>> +	TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets
>> +					  * the current HW stats state from
>> +					  * the device queried at the dump time.
>> +					  */
>> +	TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets
>> +					* HW stats that might be out of date
>> +					* for some time, maybe couple of
>> +					* seconds. This is the case when driver
>> +					* polls stats updates periodically
>> +					* or when it gets async stats update
>> +					* from the device.
>> +					*/
>> +	TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting
>> +					 * any HW statistics.
>> +					 */
>> +};
>
>On the ABI I wonder if we can redefine it a little bit..
>
>Can we make the stat types into a bitfield?
>
>On request:
> - no attr -> any stats allowed but some stats must be provided *
> - 0       -> no stats requested / disabled
> - 0x1     -> must be stat type0
> - 0x6     -> stat type1 or stat type2 are both fine

I was thinking about this of course. On the write side, this is ok
however, this is very tricky on read side. See below.


>
>* no attr kinda doesn't work 'cause u32 offload has no stats and this
>  is action-level now, not flower-level :S What about u32 and matchall?

The fact that cls does not implement stats offloading is a lack of
feature of the particular cls.


>
>We can add a separate attribute with "active" stat types:
> - no attr -> old kernel
> - 0       -> no stats are provided / stats disabled
> - 0x1     -> only stat type0 is used by drivers
> - 0x6     -> at least one driver is using type1 and one type2

There are 2 problems:
1) There is a mismatch between write and read. User might pass different
value than it eventually gets from kernel. I guess this might be fine.
2) Much bigger problem is, that since the same action may be offloaded
by multiple drivers, the read would have to provide an array of
bitfields, each array item would represent one offloaded driver. That is
why I decided for simple value instead of bitfield which is the same on
write and read.


>
>That assumes that we may one day add another stat type which would 
>not be just based on the reporting time.
>
>If we only foresee time-based reporting would it make sense to turn 
>the attribute into max acceptable delay in ms?
>
>0        -> only immediate / blocking stats
>(0, MAX) -> given reporting delay in ms is acceptable
>MAX      -> don't care about stats at all

Interesting, is this "delayed" granularity something that has a usecase?
It might turn into a guessing game between user and driver during action
insertion :/


>
>>  struct tc_police {
>>  	__u32			index;
>>  	int			action;
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 8c466a712cda..d6468b09b932 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -185,6 +185,7 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
>>  	return  nla_total_size(0) /* action number nested */
>>  		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
>>  		+ cookie_len /* TCA_ACT_COOKIE */
>> +		+ nla_total_size(sizeof(u8)) /* TCA_ACT_HW_STATS_TYPE */
>>  		+ nla_total_size(0) /* TCA_ACT_STATS nested */
>>  		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */
>>  		/* TCA_STATS_BASIC */
>> @@ -788,6 +789,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>>  	}
>>  	rcu_read_unlock();
>>  
>> +	if (nla_put_u8(skb, TCA_ACT_HW_STATS_TYPE, a->hw_stats_type))
>> +		goto nla_put_failure;
>> +
>>  	if (a->tcfa_flags) {
>>  		struct nla_bitfield32 flags = { a->tcfa_flags,
>>  						a->tcfa_flags, };
>> @@ -854,12 +858,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
>>  	return c;
>>  }
>>  
>> +static inline enum tca_act_hw_stats_type
>
>static inline in C source

Ah, I moved from .h and forgot. Thanks.


>
>> +tcf_action_hw_stats_type_get(struct nlattr *hw_stats_type_attr)
>> +{
>> +	/* If the user did not pass the attr, that means he does
>> +	 * not care about the type. Return "any" in that case.
>> +	 */
>> +	return hw_stats_type_attr ? nla_get_u8(hw_stats_type_attr) :
>> +				    TCA_ACT_HW_STATS_TYPE_ANY;
>> +}
>> +
>>  static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS;
>>  static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
>>  	[TCA_ACT_KIND]		= { .type = NLA_STRING },
>>  	[TCA_ACT_INDEX]		= { .type = NLA_U32 },
>>  	[TCA_ACT_COOKIE]	= { .type = NLA_BINARY,
>>  				    .len = TC_COOKIE_MAX_SIZE },
>> +	[TCA_ACT_HW_STATS_TYPE]	= { .type = NLA_U8 },
>
>We can use a POLICY with MIN/MAX here, perhaps?

Ok.


>
>>  	[TCA_ACT_OPTIONS]	= { .type = NLA_NESTED },
>>  	[TCA_ACT_FLAGS]		= { .type = NLA_BITFIELD32,
>>  				    .validation_data = &tca_act_flags_allowed },
>> @@ -871,6 +886,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>>  				    bool rtnl_held,
>>  				    struct netlink_ext_ack *extack)
>>  {
>> +	enum tca_act_hw_stats_type hw_stats_type = TCA_ACT_HW_STATS_TYPE_ANY;
>>  	struct nla_bitfield32 flags = { 0, 0 };
>>  	struct tc_action *a;
>>  	struct tc_action_ops *a_o;
>> @@ -903,6 +919,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>>  				goto err_out;
>>  			}
>>  		}
>> +		hw_stats_type =
>> +			tcf_action_hw_stats_type_get(tb[TCA_ACT_HW_STATS_TYPE]);
>>  		if (tb[TCA_ACT_FLAGS])
>>  			flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]);
>>  	} else {
>> @@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>>  	if (!name && tb[TCA_ACT_COOKIE])
>>  		tcf_set_action_cookie(&a->act_cookie, cookie);
>>  
>> +	if (!name)
>> +		a->hw_stats_type = hw_stats_type;
>> +
>>  	/* module count goes up only when brand new policy is created
>>  	 * if it exists and is only bound to in a_o->init() then
>>  	 * ACT_P_CREATED is not returned (a zero is).
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 4e766c5ab77a..21bf37242153 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -3458,9 +3458,28 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
>>  #endif
>>  }
>>  
>> +static inline enum flow_action_hw_stats_type
>
>static inline in C source

Right.


>
>> +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type)
>> +{
>> +	switch (hw_stats_type) {
>> +	default:
>> +		WARN_ON(1);
>> +		/* fall-through */
>
>without the policy change this seems user-triggerable

Nope. tcf_action_hw_stats_type_get() takes care of setting 
TCA_ACT_HW_STATS_TYPE_ANY when no attr is passed.


>
>> +	case TCA_ACT_HW_STATS_TYPE_ANY:
>> +		return FLOW_ACTION_HW_STATS_TYPE_ANY;
>> +	case TCA_ACT_HW_STATS_TYPE_IMMEDIATE:
>> +		return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE;
>> +	case TCA_ACT_HW_STATS_TYPE_DELAYED:
>> +		return FLOW_ACTION_HW_STATS_TYPE_DELAYED;
>> +	case TCA_ACT_HW_STATS_TYPE_DISABLED:
>> +		return FLOW_ACTION_HW_STATS_TYPE_DISABLED;
>> +	}
>> +}
>> +
>>  int tc_setup_flow_action(struct flow_action *flow_action,
>>  			 const struct tcf_exts *exts)
>>  {
>> +	enum flow_action_hw_stats_type uninitialized_var(last_hw_stats_type);
>>  	struct tc_action *act;
>>  	int i, j, k, err = 0;
>>  
>> @@ -3476,6 +3495,13 @@ int tc_setup_flow_action(struct flow_action *flow_action,
>>  		err = tcf_act_get_cookie(entry, act);
>>  		if (err)
>>  			goto err_out_locked;
>> +
>> +		entry->hw_stats_type =
>> +			tcf_flow_action_hw_stats_type(act->hw_stats_type);
>> +		if (i && last_hw_stats_type != entry->hw_stats_type)
>> +			flow_action->mixed_hw_stats_types = true;
>> +		last_hw_stats_type = entry->hw_stats_type;
>> +
>>  		if (is_tcf_gact_ok(act)) {
>>  			entry->id = FLOW_ACTION_ACCEPT;
>>  		} else if (is_tcf_gact_shot(act)) {
>

Re: [patch net-next v2 03/12] flow_offload: check for basic action hw stats type

[Jakub Kicinski]

On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:
> Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
> >On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:  
> >> @@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> +	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
> >> +		return -EOPNOTSUPP;  
> >
> >Could we have this helper take one stat type? To let drivers pass the
> >stat type they support?   
> 
> That would be always "any" as "any" is supported by all drivers.
> And that is exactly what the helper checks..

I'd think most drivers implement some form of DELAYED today, 'cause for
the number of flows things like OvS need that's the only practical one.
I was thinking to let drivers pass DELAYED here.

I agree that your patch would most likely pass ANY in almost all cases
as you shouldn't be expected to know all the drivers, but at least the
maintainers can easily just tweak the parameter.

Does that make sense? Maybe I'm missing something.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Pablo Neira Ayuso]

On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
> current implicit value coming down to flow_offload. Add a bool
> indicating that entries have mixed HW stats type.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v1->v2:
> - moved to actions
> - add mixed bool
> ---
>  include/net/flow_offload.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 4e864c34a1b0..eee1cbc5db3c 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -154,6 +154,10 @@ enum flow_action_mangle_base {
>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>  };
>  
> +enum flow_action_hw_stats_type {
> +	FLOW_ACTION_HW_STATS_TYPE_ANY,
> +};
> +
>  typedef void (*action_destr)(void *priv);
>  
>  struct flow_action_cookie {
> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>  
>  struct flow_action_entry {
>  	enum flow_action_id		id;
> +	enum flow_action_hw_stats_type	hw_stats_type;
>  	action_destr			destructor;
>  	void				*destructor_priv;
>  	union {
> @@ -228,6 +233,7 @@ struct flow_action_entry {
>  };
>  
>  struct flow_action {
> +	bool				mixed_hw_stats_types;

Why do you want to place this built-in into the struct flow_action as
a boolean?

You can express the same thing through a new FLOW_ACTION_COUNTER.
I know tc has implicit counters in actions, in that case tc can just
generate the counter right after the action.

Please, explain me why it would be a problem from the driver side to
provide a separated counter action.

Re: [patch net-next v2 08/12] flow_offload: introduce "immediate" HW stats type and allow it in mlxsw

[Pablo Neira Ayuso]

On Fri, Feb 28, 2020 at 06:25:01PM +0100, Jiri Pirko wrote:
[...]
> @@ -31,7 +31,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
>  
>  	act = flow_action_first_entry_get(flow_action);
>  	switch (act->hw_stats_type) {
> -	case FLOW_ACTION_HW_STATS_TYPE_ANY:
> +	case FLOW_ACTION_HW_STATS_TYPE_ANY: /* fall-through */
> +	case FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE:

This TYPE_ANY mean that driver picks the counter type for you?

Otherwise, users will not have a way to know how to interpret what
kind of counter this is.

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jakub Kicinski]

On Sat, 29 Feb 2020 08:52:09 +0100 Jiri Pirko wrote:
> Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote:
> >On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:  
> >> From: Jiri Pirko <jiri@mellanox.com>
> >> +/* tca HW stats type */
> >> +enum tca_act_hw_stats_type {
> >> +	TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default
> >> +				    * when user does not pass the attr.
> >> +				    * Instructs the driver that user does not
> >> +				    * care if the HW stats are "immediate"
> >> +				    * or "delayed".
> >> +				    */
> >> +	TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets
> >> +					  * the current HW stats state from
> >> +					  * the device queried at the dump time.
> >> +					  */
> >> +	TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets
> >> +					* HW stats that might be out of date
> >> +					* for some time, maybe couple of
> >> +					* seconds. This is the case when driver
> >> +					* polls stats updates periodically
> >> +					* or when it gets async stats update
> >> +					* from the device.
> >> +					*/
> >> +	TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting
> >> +					 * any HW statistics.
> >> +					 */
> >> +};  
> >
> >On the ABI I wonder if we can redefine it a little bit..
> >
> >Can we make the stat types into a bitfield?
> >
> >On request:
> > - no attr -> any stats allowed but some stats must be provided *
> > - 0       -> no stats requested / disabled
> > - 0x1     -> must be stat type0
> > - 0x6     -> stat type1 or stat type2 are both fine  
> 
> I was thinking about this of course. On the write side, this is ok
> however, this is very tricky on read side. See below.
> 
> >* no attr kinda doesn't work 'cause u32 offload has no stats and this
> >  is action-level now, not flower-level :S What about u32 and matchall?  
> 
> The fact that cls does not implement stats offloading is a lack of
> feature of the particular cls.

Yeah, I wonder how that squares with strict netlink parsing.

> >We can add a separate attribute with "active" stat types:
> > - no attr -> old kernel
> > - 0       -> no stats are provided / stats disabled
> > - 0x1     -> only stat type0 is used by drivers
> > - 0x6     -> at least one driver is using type1 and one type2  
> 
> There are 2 problems:
> 1) There is a mismatch between write and read. User might pass different
> value than it eventually gets from kernel. I guess this might be fine.

Separate attribute would work.

> 2) Much bigger problem is, that since the same action may be offloaded
> by multiple drivers, the read would have to provide an array of
> bitfields, each array item would represent one offloaded driver. That is
> why I decided for simple value instead of bitfield which is the same on
> write and read.

Why an array? The counter itself is added up from all the drivers.
If the value is a bitfield all drivers can just OR-in their type.

> >That assumes that we may one day add another stat type which would 
> >not be just based on the reporting time.
> >
> >If we only foresee time-based reporting would it make sense to turn 
> >the attribute into max acceptable delay in ms?
> >
> >0        -> only immediate / blocking stats
> >(0, MAX) -> given reporting delay in ms is acceptable
> >MAX      -> don't care about stats at all  
> 
> Interesting, is this "delayed" granularity something that has a usecase?
> It might turn into a guessing game between user and driver during action
> insertion :/

Yeah, I don't like the guessing part too, worst case refresh time may 
be system dependent.

With just "DELAYED" I'm worried users will think the delay may be too
long for OvS. Or simply poll the statistics more often than the HW
reports them, which would be pointless.

For the latter case I guess the best case refresh time is needed, 
while the former needs worst case. Hopefully the two are not too far
apart.

Maybe some day drivers may also tweak the refresh rate based on user
requests to save PCIe bandwidth and CPU..

Anyway.. maybe its not worth it today.

> >> +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type)
> >> +{
> >> +	switch (hw_stats_type) {
> >> +	default:
> >> +		WARN_ON(1);
> >> +		/* fall-through */  
> >
> >without the policy change this seems user-triggerable  
> 
> Nope. tcf_action_hw_stats_type_get() takes care of setting 
> TCA_ACT_HW_STATS_TYPE_ANY when no attr is passed.

I meant attribute is present but carries a large value.

> >> +	case TCA_ACT_HW_STATS_TYPE_ANY:
> >> +		return FLOW_ACTION_HW_STATS_TYPE_ANY;
> >> +	case TCA_ACT_HW_STATS_TYPE_IMMEDIATE:
> >> +		return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE;
> >> +	case TCA_ACT_HW_STATS_TYPE_DELAYED:
> >> +		return FLOW_ACTION_HW_STATS_TYPE_DELAYED;
> >> +	case TCA_ACT_HW_STATS_TYPE_DISABLED:
> >> +		return FLOW_ACTION_HW_STATS_TYPE_DISABLED;

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@netfilter.org wrote:
>On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
>> current implicit value coming down to flow_offload. Add a bool
>> indicating that entries have mixed HW stats type.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> v1->v2:
>> - moved to actions
>> - add mixed bool
>> ---
>>  include/net/flow_offload.h | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> index 4e864c34a1b0..eee1cbc5db3c 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -154,6 +154,10 @@ enum flow_action_mangle_base {
>>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>>  };
>>  
>> +enum flow_action_hw_stats_type {
>> +	FLOW_ACTION_HW_STATS_TYPE_ANY,
>> +};
>> +
>>  typedef void (*action_destr)(void *priv);
>>  
>>  struct flow_action_cookie {
>> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>>  
>>  struct flow_action_entry {
>>  	enum flow_action_id		id;
>> +	enum flow_action_hw_stats_type	hw_stats_type;
>>  	action_destr			destructor;
>>  	void				*destructor_priv;
>>  	union {
>> @@ -228,6 +233,7 @@ struct flow_action_entry {
>>  };
>>  
>>  struct flow_action {
>> +	bool				mixed_hw_stats_types;
>
>Why do you want to place this built-in into the struct flow_action as
>a boolean?

Because it is convenient for the driver to know if multiple hw_stats_type
values are used for multiple actions.


>
>You can express the same thing through a new FLOW_ACTION_COUNTER.

I don't see how.


>I know tc has implicit counters in actions, in that case tc can just
>generate the counter right after the action.

I don't follow. Each action has a separate stats.


>
>Please, explain me why it would be a problem from the driver side to
>provide a separated counter action.

I don't see any point in doing that. The action itself implies that has
stats, you don't need a separate action for that for the flow_offload
abstraction layer. What you would end up with is:
counter_action1, actual_action1, counter_action2, actual_action2,...

What is the point of that?

Re: [patch net-next v2 08/12] flow_offload: introduce "immediate" HW stats type and allow it in mlxsw

[Jiri Pirko]

Sat, Feb 29, 2020 at 08:32:18PM CET, pablo@netfilter.org wrote:
>On Fri, Feb 28, 2020 at 06:25:01PM +0100, Jiri Pirko wrote:
>[...]
>> @@ -31,7 +31,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
>>  
>>  	act = flow_action_first_entry_get(flow_action);
>>  	switch (act->hw_stats_type) {
>> -	case FLOW_ACTION_HW_STATS_TYPE_ANY:
>> +	case FLOW_ACTION_HW_STATS_TYPE_ANY: /* fall-through */
>> +	case FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE:
>
>This TYPE_ANY mean that driver picks the counter type for you?

Driver pick any counter, yes.

>
>Otherwise, users will not have a way to know how to interpret what
>kind of counter this is.

User does not care in this case.

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jiri Pirko]

Sat, Feb 29, 2020 at 09:14:52PM CET, kuba@kernel.org wrote:
>On Sat, 29 Feb 2020 08:52:09 +0100 Jiri Pirko wrote:
>> Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote:
>> >On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:  
>> >> From: Jiri Pirko <jiri@mellanox.com>
>> >> +/* tca HW stats type */
>> >> +enum tca_act_hw_stats_type {
>> >> +	TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default
>> >> +				    * when user does not pass the attr.
>> >> +				    * Instructs the driver that user does not
>> >> +				    * care if the HW stats are "immediate"
>> >> +				    * or "delayed".
>> >> +				    */
>> >> +	TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets
>> >> +					  * the current HW stats state from
>> >> +					  * the device queried at the dump time.
>> >> +					  */
>> >> +	TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets
>> >> +					* HW stats that might be out of date
>> >> +					* for some time, maybe couple of
>> >> +					* seconds. This is the case when driver
>> >> +					* polls stats updates periodically
>> >> +					* or when it gets async stats update
>> >> +					* from the device.
>> >> +					*/
>> >> +	TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting
>> >> +					 * any HW statistics.
>> >> +					 */
>> >> +};  
>> >
>> >On the ABI I wonder if we can redefine it a little bit..
>> >
>> >Can we make the stat types into a bitfield?
>> >
>> >On request:
>> > - no attr -> any stats allowed but some stats must be provided *
>> > - 0       -> no stats requested / disabled
>> > - 0x1     -> must be stat type0
>> > - 0x6     -> stat type1 or stat type2 are both fine  
>> 
>> I was thinking about this of course. On the write side, this is ok
>> however, this is very tricky on read side. See below.
>> 
>> >* no attr kinda doesn't work 'cause u32 offload has no stats and this
>> >  is action-level now, not flower-level :S What about u32 and matchall?  
>> 
>> The fact that cls does not implement stats offloading is a lack of
>> feature of the particular cls.
>
>Yeah, I wonder how that squares with strict netlink parsing.
>
>> >We can add a separate attribute with "active" stat types:
>> > - no attr -> old kernel
>> > - 0       -> no stats are provided / stats disabled
>> > - 0x1     -> only stat type0 is used by drivers
>> > - 0x6     -> at least one driver is using type1 and one type2  
>> 
>> There are 2 problems:
>> 1) There is a mismatch between write and read. User might pass different
>> value than it eventually gets from kernel. I guess this might be fine.
>
>Separate attribute would work.
>
>> 2) Much bigger problem is, that since the same action may be offloaded
>> by multiple drivers, the read would have to provide an array of
>> bitfields, each array item would represent one offloaded driver. That is
>> why I decided for simple value instead of bitfield which is the same on
>> write and read.
>
>Why an array? The counter itself is added up from all the drivers.
>If the value is a bitfield all drivers can just OR-in their type.

Yeah, for uapi. Internally the array would be still needed. Also the
driver would need to somehow "write-back" the value to the offload
caller and someone (caller/tc) would have to use the array to track
these bitfields for individual callbacks (probably idr of some sort).
I don't know, is this excercise worth it?

Seems to me like we are overengineering this one a bit.

Also there would be no "any" it would be type0|type1|type2 the user
would have to pass. If new type appears, the userspace would have to be
updated to do "any" again :/ This is inconvenient.


>
>> >That assumes that we may one day add another stat type which would 
>> >not be just based on the reporting time.
>> >
>> >If we only foresee time-based reporting would it make sense to turn 
>> >the attribute into max acceptable delay in ms?
>> >
>> >0        -> only immediate / blocking stats
>> >(0, MAX) -> given reporting delay in ms is acceptable
>> >MAX      -> don't care about stats at all  
>> 
>> Interesting, is this "delayed" granularity something that has a usecase?
>> It might turn into a guessing game between user and driver during action
>> insertion :/
>
>Yeah, I don't like the guessing part too, worst case refresh time may 
>be system dependent.
>
>With just "DELAYED" I'm worried users will think the delay may be too
>long for OvS. Or simply poll the statistics more often than the HW
>reports them, which would be pointless.
>
>For the latter case I guess the best case refresh time is needed, 
>while the former needs worst case. Hopefully the two are not too far
>apart.
>
>Maybe some day drivers may also tweak the refresh rate based on user
>requests to save PCIe bandwidth and CPU..
>
>Anyway.. maybe its not worth it today.
>
>> >> +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type)
>> >> +{
>> >> +	switch (hw_stats_type) {
>> >> +	default:
>> >> +		WARN_ON(1);
>> >> +		/* fall-through */  
>> >
>> >without the policy change this seems user-triggerable  
>> 
>> Nope. tcf_action_hw_stats_type_get() takes care of setting 
>> TCA_ACT_HW_STATS_TYPE_ANY when no attr is passed.
>
>I meant attribute is present but carries a large value.

Ah, will sanitize this.


>
>> >> +	case TCA_ACT_HW_STATS_TYPE_ANY:
>> >> +		return FLOW_ACTION_HW_STATS_TYPE_ANY;
>> >> +	case TCA_ACT_HW_STATS_TYPE_IMMEDIATE:
>> >> +		return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE;
>> >> +	case TCA_ACT_HW_STATS_TYPE_DELAYED:
>> >> +		return FLOW_ACTION_HW_STATS_TYPE_DELAYED;
>> >> +	case TCA_ACT_HW_STATS_TYPE_DISABLED:
>> >> +		return FLOW_ACTION_HW_STATS_TYPE_DISABLED;
>

Re: [patch net-next v2 03/12] flow_offload: check for basic action hw stats type

[Jiri Pirko]

Sat, Feb 29, 2020 at 08:18:48PM CET, kuba@kernel.org wrote:
>On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:
>> Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
>> >On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:  
>> >> @@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
>> >>  		return -EINVAL;
>> >>  	}
>> >>  
>> >> +	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
>> >> +		return -EOPNOTSUPP;  
>> >
>> >Could we have this helper take one stat type? To let drivers pass the
>> >stat type they support?   
>> 
>> That would be always "any" as "any" is supported by all drivers.
>> And that is exactly what the helper checks..
>
>I'd think most drivers implement some form of DELAYED today, 'cause for
>the number of flows things like OvS need that's the only practical one.
>I was thinking to let drivers pass DELAYED here.
>
>I agree that your patch would most likely pass ANY in almost all cases
>as you shouldn't be expected to know all the drivers, but at least the
>maintainers can easily just tweak the parameter.
>
>Does that make sense? Maybe I'm missing something.

Well, I guess. mlx5 only supports "delayed". It would work for it.
How about having flow_action_basic_hw_stats_types_check() as is and
add flow_action_basic_hw_stats_types_check_ext() that would accept extra
arg with enum?

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Pablo Neira Ayuso]

On Sun, Mar 01, 2020 at 09:44:43AM +0100, Jiri Pirko wrote:
> Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@netfilter.org wrote:
> >On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote:
[...]
> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> >> index 4e864c34a1b0..eee1cbc5db3c 100644
> >> --- a/include/net/flow_offload.h
> >> +++ b/include/net/flow_offload.h
> >> @@ -154,6 +154,10 @@ enum flow_action_mangle_base {
> >>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
> >>  };
> >>  
> >> +enum flow_action_hw_stats_type {
> >> +	FLOW_ACTION_HW_STATS_TYPE_ANY,
> >> +};
> >> +
> >>  typedef void (*action_destr)(void *priv);
> >>  
> >>  struct flow_action_cookie {
> >> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
> >>  
> >>  struct flow_action_entry {
> >>  	enum flow_action_id		id;
> >> +	enum flow_action_hw_stats_type	hw_stats_type;
> >>  	action_destr			destructor;
> >>  	void				*destructor_priv;
> >>  	union {
> >> @@ -228,6 +233,7 @@ struct flow_action_entry {
> >>  };
> >>  
> >>  struct flow_action {
> >> +	bool				mixed_hw_stats_types;
> >
> >Why do you want to place this built-in into the struct flow_action as
> >a boolean?
> 
> Because it is convenient for the driver to know if multiple hw_stats_type
> values are used for multiple actions.
> 
> >You can express the same thing through a new FLOW_ACTION_COUNTER.
[...]
> >Please, explain me why it would be a problem from the driver side to
> >provide a separated counter action.
> 
> I don't see any point in doing that. The action itself implies that has
> stats, you don't need a separate action for that for the flow_offload
> abstraction layer. What you would end up with is:
> counter_action1, actual_action1, counter_action2, actual_action2,...
> 
> What is the point of that?

Yes, it's a bit more work for tc to generate counter action + actual
action.

However, netfilter has two ways to use counters:

1) per-rule counter, in this case the counter is updated after rule
   matching, right before calling the action. This is the legacy mode.

2) explicit counter action, in this case the user specifies explicitly
   that it needs a counter in a given position of the rule. This
   counter might come before or after the actual action.

ethtool does not have counters yet. Now there is a netlink interface
for it, there might be counters there at some point.

I'm suggesting a model that would work for the existing front-ends
using the flow_action API.

Re: [patch net-next v2 08/12] flow_offload: introduce "immediate" HW stats type and allow it in mlxsw

[Pablo Neira Ayuso]

On Sun, Mar 01, 2020 at 09:47:16AM +0100, Jiri Pirko wrote:
> Sat, Feb 29, 2020 at 08:32:18PM CET, pablo@netfilter.org wrote:
> >On Fri, Feb 28, 2020 at 06:25:01PM +0100, Jiri Pirko wrote:
> >[...]
> >> @@ -31,7 +31,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
> >>  
> >>  	act = flow_action_first_entry_get(flow_action);
> >>  	switch (act->hw_stats_type) {
> >> -	case FLOW_ACTION_HW_STATS_TYPE_ANY:
> >> +	case FLOW_ACTION_HW_STATS_TYPE_ANY: /* fall-through */
> >> +	case FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE:
> >
> >This TYPE_ANY mean that driver picks the counter type for you?
> 
> Driver pick any counter, yes.
> 
> >Otherwise, users will not have a way to know how to interpret what
> >kind of counter this is.
> 
> User does not care in this case.

OK.

When listing back the counters, will the user get what counter type
the driver has selected?

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

Mon, Mar 02, 2020 at 02:20:16PM CET, pablo@netfilter.org wrote:
>On Sun, Mar 01, 2020 at 09:44:43AM +0100, Jiri Pirko wrote:
>> Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@netfilter.org wrote:
>> >On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote:
>[...]
>> >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> >> index 4e864c34a1b0..eee1cbc5db3c 100644
>> >> --- a/include/net/flow_offload.h
>> >> +++ b/include/net/flow_offload.h
>> >> @@ -154,6 +154,10 @@ enum flow_action_mangle_base {
>> >>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>> >>  };
>> >>  
>> >> +enum flow_action_hw_stats_type {
>> >> +	FLOW_ACTION_HW_STATS_TYPE_ANY,
>> >> +};
>> >> +
>> >>  typedef void (*action_destr)(void *priv);
>> >>  
>> >>  struct flow_action_cookie {
>> >> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>> >>  
>> >>  struct flow_action_entry {
>> >>  	enum flow_action_id		id;
>> >> +	enum flow_action_hw_stats_type	hw_stats_type;
>> >>  	action_destr			destructor;
>> >>  	void				*destructor_priv;
>> >>  	union {
>> >> @@ -228,6 +233,7 @@ struct flow_action_entry {
>> >>  };
>> >>  
>> >>  struct flow_action {
>> >> +	bool				mixed_hw_stats_types;
>> >
>> >Why do you want to place this built-in into the struct flow_action as
>> >a boolean?
>> 
>> Because it is convenient for the driver to know if multiple hw_stats_type
>> values are used for multiple actions.
>> 
>> >You can express the same thing through a new FLOW_ACTION_COUNTER.
>[...]
>> >Please, explain me why it would be a problem from the driver side to
>> >provide a separated counter action.
>> 
>> I don't see any point in doing that. The action itself implies that has
>> stats, you don't need a separate action for that for the flow_offload
>> abstraction layer. What you would end up with is:
>> counter_action1, actual_action1, counter_action2, actual_action2,...
>> 
>> What is the point of that?
>
>Yes, it's a bit more work for tc to generate counter action + actual
>action.
>
>However, netfilter has two ways to use counters:
>
>1) per-rule counter, in this case the counter is updated after rule
>   matching, right before calling the action. This is the legacy mode.
>
>2) explicit counter action, in this case the user specifies explicitly
>   that it needs a counter in a given position of the rule. This
>   counter might come before or after the actual action.
>
>ethtool does not have counters yet. Now there is a netlink interface
>for it, there might be counters there at some point.
>
>I'm suggesting a model that would work for the existing front-ends
>using the flow_action API.

I see. I'm interested in 1) now. If you ever want to implement 2), I see
no problem in doing it.

Re: [patch net-next v2 08/12] flow_offload: introduce "immediate" HW stats type and allow it in mlxsw

[Jiri Pirko]

Mon, Mar 02, 2020 at 02:23:42PM CET, pablo@netfilter.org wrote:
>On Sun, Mar 01, 2020 at 09:47:16AM +0100, Jiri Pirko wrote:
>> Sat, Feb 29, 2020 at 08:32:18PM CET, pablo@netfilter.org wrote:
>> >On Fri, Feb 28, 2020 at 06:25:01PM +0100, Jiri Pirko wrote:
>> >[...]
>> >> @@ -31,7 +31,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
>> >>  
>> >>  	act = flow_action_first_entry_get(flow_action);
>> >>  	switch (act->hw_stats_type) {
>> >> -	case FLOW_ACTION_HW_STATS_TYPE_ANY:
>> >> +	case FLOW_ACTION_HW_STATS_TYPE_ANY: /* fall-through */
>> >> +	case FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE:
>> >
>> >This TYPE_ANY mean that driver picks the counter type for you?
>> 
>> Driver pick any counter, yes.
>> 
>> >Otherwise, users will not have a way to know how to interpret what
>> >kind of counter this is.
>> 
>> User does not care in this case.
>
>OK.
>
>When listing back the counters, will the user get what counter type
>the driver has selected?

User will get back exactly what he passed.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Edward Cree]

On 02/03/2020 13:20, Pablo Neira Ayuso wrote:
> 2) explicit counter action, in this case the user specifies explicitly
>    that it needs a counter in a given position of the rule. This
>    counter might come before or after the actual action.
But the existing API can already do this, with a gact pipe.  Plus, Jiri's
 new API will allow specifying a counter on any action (rather than only,
 implicitly, those which have .stats_update()) should that prove to be
 necessary.

I really think the 'explicit counter action' is a solution in search of a
 problem, let's not add random orthogonality violations.  (Equally if the
 counter action had been there first, I'd be against adding counters to
 the other actions.)

-ed

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Pablo Neira Ayuso]

On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
> On 02/03/2020 13:20, Pablo Neira Ayuso wrote:
> > 2) explicit counter action, in this case the user specifies explicitly
> >    that it needs a counter in a given position of the rule. This
> >    counter might come before or after the actual action.
>
> But the existing API can already do this, with a gact pipe.  Plus, Jiri's
>  new API will allow specifying a counter on any action (rather than only,
>  implicitly, those which have .stats_update()) should that prove to be
>  necessary.
> 
> I really think the 'explicit counter action' is a solution in search of a
>  problem, let's not add random orthogonality violations.  (Equally if the
>  counter action had been there first, I'd be against adding counters to
>  the other actions.)

It looks to me that you want to restrict the API to tc for no good
_technical_ reason.

Re: [patch net-next v2 03/12] flow_offload: check for basic action hw stats type

[Jakub Kicinski]

On Sun, 1 Mar 2020 10:00:09 +0100 Jiri Pirko wrote:
> Sat, Feb 29, 2020 at 08:18:48PM CET, kuba@kernel.org wrote:
> >On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:  
> >> Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:  
> >> >On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:    
> >> >> @@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
> >> >>  		return -EINVAL;
> >> >>  	}
> >> >>  
> >> >> +	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
> >> >> +		return -EOPNOTSUPP;    
> >> >
> >> >Could we have this helper take one stat type? To let drivers pass the
> >> >stat type they support?     
> >> 
> >> That would be always "any" as "any" is supported by all drivers.
> >> And that is exactly what the helper checks..  
> >
> >I'd think most drivers implement some form of DELAYED today, 'cause for
> >the number of flows things like OvS need that's the only practical one.
> >I was thinking to let drivers pass DELAYED here.
> >
> >I agree that your patch would most likely pass ANY in almost all cases
> >as you shouldn't be expected to know all the drivers, but at least the
> >maintainers can easily just tweak the parameter.
> >
> >Does that make sense? Maybe I'm missing something.  
> 
> Well, I guess. mlx5 only supports "delayed". It would work for it.
> How about having flow_action_basic_hw_stats_types_check() as is and
> add flow_action_basic_hw_stats_types_check_ext() that would accept extra
> arg with enum?

SGTM, perhaps with a more concise name? 
Just flow_basic_hw_stats_check()?

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jakub Kicinski]

On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote:
> >> >On request:
> >> > - no attr -> any stats allowed but some stats must be provided *
> >> > - 0       -> no stats requested / disabled
> >> > - 0x1     -> must be stat type0
> >> > - 0x6     -> stat type1 or stat type2 are both fine    
> >> 
> >> I was thinking about this of course. On the write side, this is ok
> >> however, this is very tricky on read side. See below.
> >>   
> >> >* no attr kinda doesn't work 'cause u32 offload has no stats and this
> >> >  is action-level now, not flower-level :S What about u32 and matchall?    
> >> 
> >> The fact that cls does not implement stats offloading is a lack of
> >> feature of the particular cls.  
> >
> >Yeah, I wonder how that squares with strict netlink parsing.
> >  
> >> >We can add a separate attribute with "active" stat types:
> >> > - no attr -> old kernel
> >> > - 0       -> no stats are provided / stats disabled
> >> > - 0x1     -> only stat type0 is used by drivers
> >> > - 0x6     -> at least one driver is using type1 and one type2    
> >> 
> >> There are 2 problems:
> >> 1) There is a mismatch between write and read. User might pass different
> >> value than it eventually gets from kernel. I guess this might be fine.  
> >
> >Separate attribute would work.
> >  
> >> 2) Much bigger problem is, that since the same action may be offloaded
> >> by multiple drivers, the read would have to provide an array of
> >> bitfields, each array item would represent one offloaded driver. That is
> >> why I decided for simple value instead of bitfield which is the same on
> >> write and read.  
> >
> >Why an array? The counter itself is added up from all the drivers.
> >If the value is a bitfield all drivers can just OR-in their type.  
> 
> Yeah, for uapi. Internally the array would be still needed. Also the
> driver would need to somehow "write-back" the value to the offload
> caller and someone (caller/tc) would have to use the array to track
> these bitfields for individual callbacks (probably idr of some sort).
> I don't know, is this excercise worth it?

I was thinking of just doing this on HW stats dump. Drivers which don't
report stats by definition don't need to set any bit :)

> Seems to me like we are overengineering this one a bit.

That's possible, the reporting could be added later... I mostly wanted
to have the discussion.

> Also there would be no "any" it would be type0|type1|type2 the user
> would have to pass. If new type appears, the userspace would have to be
> updated to do "any" again :/ This is inconvenient.

In my proposal above I was suggesting no attr to mean any. I think in
your current code ANY already doesn't include disabled so old user
space should not see any change.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jakub Kicinski]

On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
> > On 02/03/2020 13:20, Pablo Neira Ayuso wrote:  
> > > 2) explicit counter action, in this case the user specifies explicitly
> > >    that it needs a counter in a given position of the rule. This
> > >    counter might come before or after the actual action.  
> >
> > But the existing API can already do this, with a gact pipe.  Plus, Jiri's
> >  new API will allow specifying a counter on any action (rather than only,
> >  implicitly, those which have .stats_update()) should that prove to be
> >  necessary.
> > 
> > I really think the 'explicit counter action' is a solution in search of a
> >  problem, let's not add random orthogonality violations.  (Equally if the
> >  counter action had been there first, I'd be against adding counters to
> >  the other actions.)  
> 
> It looks to me that you want to restrict the API to tc for no good
> _technical_ reason.

Undeniably part of the reason is that given how complex flow offloads
got there may be some resistance to large re-factoring. IMHO well
thought out refactoring of stats is needed.. but I'm not convinced 
this is the direction.

Could you give us clearer understanding of what the use cases for the
counter action is?

AFAIK right now actions do the accounting on input. That seems like the
only logical option. Either action takes the packet out of the action
pipeline, in which case even the counter action after will not see it,
or it doesn't and the input counter of the next action can be used.

Given counters must be next to real actions and not other counter
to have value, having them as a separate action seems to make no
difference at all (if users are silly, we can use the pipe/no-op).

IOW modeling the stats as attribute of other actions or a separate
action is entirely equivalent, and there's nothing to be gained from
moving from the existing scheme to explicit actions... other than it'd
make it look more like nft actions... :)

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Pablo Neira Ayuso]

On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
> > On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
> > > On 02/03/2020 13:20, Pablo Neira Ayuso wrote:  
> > > > 2) explicit counter action, in this case the user specifies explicitly
> > > >    that it needs a counter in a given position of the rule. This
> > > >    counter might come before or after the actual action.  
> > >
> > > But the existing API can already do this, with a gact pipe.  Plus, Jiri's
> > >  new API will allow specifying a counter on any action (rather than only,
> > >  implicitly, those which have .stats_update()) should that prove to be
> > >  necessary.
> > > 
> > > I really think the 'explicit counter action' is a solution in search of a
> > >  problem, let's not add random orthogonality violations.  (Equally if the
> > >  counter action had been there first, I'd be against adding counters to
> > >  the other actions.)  
> > 
> > It looks to me that you want to restrict the API to tc for no good
> > _technical_ reason.
> 
> Undeniably part of the reason is that given how complex flow offloads
> got there may be some resistance to large re-factoring. IMHO well
> thought out refactoring of stats is needed.. but I'm not convinced 
> this is the direction.
>
> Could you give us clearer understanding of what the use cases for the
> counter action is?
> 
> AFAIK right now actions do the accounting on input. That seems like the
> only logical option. Either action takes the packet out of the action
> pipeline, in which case even the counter action after will not see it,
> or it doesn't and the input counter of the next action can be used.
>
> Given counters must be next to real actions and not other counter
> to have value, having them as a separate action seems to make no
> difference at all (if users are silly, we can use the pipe/no-op).

This model that is proposed here is correct in the tc world, where
counters are tied to actions (as you describe above). However, the
flow_offload API already supports for ethtool and netfilter these
days.

In Netfilter, counters are detached from actions. Obviously, a counter
must be placed before the action _if_ the action gets the packet out
of the pipeline, e.g.

     ip saddr 1.1.1.1 counter drop

In this case, the counter is placed before the 'drop' action. Users
that need no counters have to remove 'counter' from the rule syntax to
opt-out.

> IOW modeling the stats as attribute of other actions or a separate
> action is entirely equivalent, and there's nothing to be gained from
> moving from the existing scheme to explicit actions... other than it'd
> make it look more like nft actions... :)

I just wonder if a model that allows tc and netfilter to use this new
statistics infrastructure would make everyone happy. My understanding
is that it is not far away from what this patchset provides.

The retorical question here probably is if you still want to allow the
Netfilter front-end to benefit from this new flow_action API
extension.

The real question is: if you think this tc counter+action scheme can
be used by netfilter, then please explain how.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jakub Kicinski]

On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
> > On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:  
> > > On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:  
> > > > On 02/03/2020 13:20, Pablo Neira Ayuso wrote:    
> > > > > 2) explicit counter action, in this case the user specifies explicitly
> > > > >    that it needs a counter in a given position of the rule. This
> > > > >    counter might come before or after the actual action.    
> > > >
> > > > But the existing API can already do this, with a gact pipe.  Plus, Jiri's
> > > >  new API will allow specifying a counter on any action (rather than only,
> > > >  implicitly, those which have .stats_update()) should that prove to be
> > > >  necessary.
> > > > 
> > > > I really think the 'explicit counter action' is a solution in search of a
> > > >  problem, let's not add random orthogonality violations.  (Equally if the
> > > >  counter action had been there first, I'd be against adding counters to
> > > >  the other actions.)    
> > > 
> > > It looks to me that you want to restrict the API to tc for no good
> > > _technical_ reason.  
> > 
> > Undeniably part of the reason is that given how complex flow offloads
> > got there may be some resistance to large re-factoring. IMHO well
> > thought out refactoring of stats is needed.. but I'm not convinced 
> > this is the direction.
> >
> > Could you give us clearer understanding of what the use cases for the
> > counter action is?
> > 
> > AFAIK right now actions do the accounting on input. That seems like the
> > only logical option. Either action takes the packet out of the action
> > pipeline, in which case even the counter action after will not see it,
> > or it doesn't and the input counter of the next action can be used.
> >
> > Given counters must be next to real actions and not other counter
> > to have value, having them as a separate action seems to make no
> > difference at all (if users are silly, we can use the pipe/no-op).  
> 
> This model that is proposed here is correct in the tc world, where
> counters are tied to actions (as you describe above). However, the
> flow_offload API already supports for ethtool and netfilter these
> days.
> 
> In Netfilter, counters are detached from actions. Obviously, a counter
> must be placed before the action _if_ the action gets the packet out
> of the pipeline, e.g.
> 
>      ip saddr 1.1.1.1 counter drop
> 
> In this case, the counter is placed before the 'drop' action. Users
> that need no counters have to remove 'counter' from the rule syntax to
> opt-out.

In Jiri's set if counter exists DROP should get the ANY flag, if
counter is not there - DISABLED.

> > IOW modeling the stats as attribute of other actions or a separate
> > action is entirely equivalent, and there's nothing to be gained from
> > moving from the existing scheme to explicit actions... other than it'd
> > make it look more like nft actions... :)  
> 
> I just wonder if a model that allows tc and netfilter to use this new
> statistics infrastructure would make everyone happy. My understanding
> is that it is not far away from what this patchset provides.
> 
> The retorical question here probably is if you still want to allow the
> Netfilter front-end to benefit from this new flow_action API
> extension.
> 
> The real question is: if you think this tc counter+action scheme can
> be used by netfilter, then please explain how.

In Jiri's latest patch set the counter type is per action, so just
"merge right" the counter info into the next action and the models 
are converted.

If user is silly and has multiple counter actions in a row - the
pipe/no-op action comes into play (that isn't part of this set, 
as Jiri said).

Can you give us examples of what wouldn't work? Can you for instance
share the counter across rules?

Also neither proposal addresses the problem of reporting _different_
counter values at different stages in the pipeline, i.e. moving from
stats per flow to per action. But nobody seems to be willing to work 
on that.

AFAICT with Jiri's change we only need one check in the drivers to
convert from old scheme to new, with explicit action we need two
(additional one being ignoring the counter action). Not a big deal,
but 1 is less than 2 🤷‍♂️

Re: [patch net-next v2 03/12] flow_offload: check for basic action hw stats type

[Jiri Pirko]

Mon, Mar 02, 2020 at 08:33:19PM CET, kuba@kernel.org wrote:
>On Sun, 1 Mar 2020 10:00:09 +0100 Jiri Pirko wrote:
>> Sat, Feb 29, 2020 at 08:18:48PM CET, kuba@kernel.org wrote:
>> >On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:  
>> >> Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:  
>> >> >On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:    
>> >> >> @@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
>> >> >>  		return -EINVAL;
>> >> >>  	}
>> >> >>  
>> >> >> +	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
>> >> >> +		return -EOPNOTSUPP;    
>> >> >
>> >> >Could we have this helper take one stat type? To let drivers pass the
>> >> >stat type they support?     
>> >> 
>> >> That would be always "any" as "any" is supported by all drivers.
>> >> And that is exactly what the helper checks..  
>> >
>> >I'd think most drivers implement some form of DELAYED today, 'cause for
>> >the number of flows things like OvS need that's the only practical one.
>> >I was thinking to let drivers pass DELAYED here.
>> >
>> >I agree that your patch would most likely pass ANY in almost all cases
>> >as you shouldn't be expected to know all the drivers, but at least the
>> >maintainers can easily just tweak the parameter.
>> >
>> >Does that make sense? Maybe I'm missing something.  
>> 
>> Well, I guess. mlx5 only supports "delayed". It would work for it.
>> How about having flow_action_basic_hw_stats_types_check() as is and
>> add flow_action_basic_hw_stats_types_check_ext() that would accept extra
>> arg with enum?
>
>SGTM, perhaps with a more concise name? 
>Just flow_basic_hw_stats_check()?

Will try :)

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jiri Pirko]

Mon, Mar 02, 2020 at 08:39:33PM CET, kuba@kernel.org wrote:
>On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote:
>> >> >On request:
>> >> > - no attr -> any stats allowed but some stats must be provided *
>> >> > - 0       -> no stats requested / disabled
>> >> > - 0x1     -> must be stat type0
>> >> > - 0x6     -> stat type1 or stat type2 are both fine    
>> >> 
>> >> I was thinking about this of course. On the write side, this is ok
>> >> however, this is very tricky on read side. See below.
>> >>   
>> >> >* no attr kinda doesn't work 'cause u32 offload has no stats and this
>> >> >  is action-level now, not flower-level :S What about u32 and matchall?    
>> >> 
>> >> The fact that cls does not implement stats offloading is a lack of
>> >> feature of the particular cls.  
>> >
>> >Yeah, I wonder how that squares with strict netlink parsing.
>> >  
>> >> >We can add a separate attribute with "active" stat types:
>> >> > - no attr -> old kernel
>> >> > - 0       -> no stats are provided / stats disabled
>> >> > - 0x1     -> only stat type0 is used by drivers
>> >> > - 0x6     -> at least one driver is using type1 and one type2    
>> >> 
>> >> There are 2 problems:
>> >> 1) There is a mismatch between write and read. User might pass different
>> >> value than it eventually gets from kernel. I guess this might be fine.  
>> >
>> >Separate attribute would work.
>> >  
>> >> 2) Much bigger problem is, that since the same action may be offloaded
>> >> by multiple drivers, the read would have to provide an array of
>> >> bitfields, each array item would represent one offloaded driver. That is
>> >> why I decided for simple value instead of bitfield which is the same on
>> >> write and read.  
>> >
>> >Why an array? The counter itself is added up from all the drivers.
>> >If the value is a bitfield all drivers can just OR-in their type.  
>> 
>> Yeah, for uapi. Internally the array would be still needed. Also the
>> driver would need to somehow "write-back" the value to the offload
>> caller and someone (caller/tc) would have to use the array to track
>> these bitfields for individual callbacks (probably idr of some sort).
>> I don't know, is this excercise worth it?
>
>I was thinking of just doing this on HW stats dump. Drivers which don't
>report stats by definition don't need to set any bit :)
>
>> Seems to me like we are overengineering this one a bit.
>
>That's possible, the reporting could be added later... I mostly wanted
>to have the discussion.

Okay.

>
>> Also there would be no "any" it would be type0|type1|type2 the user
>> would have to pass. If new type appears, the userspace would have to be
>> updated to do "any" again :/ This is inconvenient.
>
>In my proposal above I was suggesting no attr to mean any. I think in
>your current code ANY already doesn't include disabled so old user
>space should not see any change.

Odd, no attribute meaning "any". I think it is polite to fillup the
attribute for dump if kernel supports the attribute. However, here, we
would not fill it up in case of "any". That is quite odd.

We can have a bit that would mean "any" though. What do you think?

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Pablo Neira Ayuso]

On Mon, Mar 02, 2020 at 02:49:28PM -0800, Jakub Kicinski wrote:
> On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
[...]
> > The real question is: if you think this tc counter+action scheme can
> > be used by netfilter, then please explain how.
> 
> In Jiri's latest patch set the counter type is per action, so just
> "merge right" the counter info into the next action and the models 
> are converted.

The input "merge right" approach might work.

> If user is silly and has multiple counter actions in a row - the
> pipe/no-op action comes into play (that isn't part of this set, 
> as Jiri said).

Probably gact pipe action with counters can be mapped to the counter
action that netfilter needs. Is this a valid use-case you consider for
the tc hardware offload?

> Can you give us examples of what wouldn't work? Can you for instance
> share the counter across rules?

Yes, there might be counters that are shared accross rules, see
nfacct. Two different rules might refer to the same counter, IIRC
there is a way to do this in tc too.

> Also neither proposal addresses the problem of reporting _different_
> counter values at different stages in the pipeline, i.e. moving from
> stats per flow to per action. But nobody seems to be willing to work 
> on that.

You mean, in case that different counter types are specified, eg. one
action using delayed and another action using immediate?

> AFAICT with Jiri's change we only need one check in the drivers to
> convert from old scheme to new, with explicit action we need two
> (additional one being ignoring the counter action). Not a big deal,
> but 1 is less than 2 🤷‍♂️

What changes are expected to retrieve counter stats?

Will per-flow stats remain in place after this place?

Thank you.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Edward Cree]

On 02/03/2020 22:49, Jakub Kicinski wrote:
> On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
>> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
>>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:  
>>>> It looks to me that you want to restrict the API to tc for no good
>>>> _technical_ reason.  

The technical reason is that having two ways to do things where one would
 suffice means more code to be written, tested, debugged.  So if you want
 to add this you need to convince us that the existing way (a) doesn't
 meet your needs and (b) can't be extended to cover them.

> Also neither proposal addresses the problem of reporting _different_
> counter values at different stages in the pipeline, i.e. moving from
> stats per flow to per action. But nobody seems to be willing to work 
> on that.
For the record, I produced a patch series[1] to support that, but it
 wasn't acceptable because none of the in-tree drivers implemented the
 facility.  My hope is that we'll be upstreaming our new driver Real
 Soon Now™, at which point I'll rebase and repost those changes.
Alternatively if any other vendor wants to support it in their driver
 they could use those patches as a base.

-ed

[1]: http://patchwork.ozlabs.org/cover/1110071/ ("flow_offload: Re-add per-action statistics")

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Edward Cree]

On 28/02/2020 17:24, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
> current implicit value coming down to flow_offload. Add a bool
> indicating that entries have mixed HW stats type.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v1->v2:
> - moved to actions
> - add mixed bool
> ---
>  include/net/flow_offload.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 4e864c34a1b0..eee1cbc5db3c 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -154,6 +154,10 @@ enum flow_action_mangle_base {
>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>  };
>  
> +enum flow_action_hw_stats_type {
> +	FLOW_ACTION_HW_STATS_TYPE_ANY,
> +};
> +
>  typedef void (*action_destr)(void *priv);
>  
>  struct flow_action_cookie {
> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>  
>  struct flow_action_entry {
>  	enum flow_action_id		id;
> +	enum flow_action_hw_stats_type	hw_stats_type;
>  	action_destr			destructor;
>  	void				*destructor_priv;
>  	union {
> @@ -228,6 +233,7 @@ struct flow_action_entry {
>  };
>  
>  struct flow_action {
> +	bool				mixed_hw_stats_types;
Some sort of comment in the commit message the effect that this will
 be set in patch #12 would be nice (and would have saved me some
 reviewing time looking for it ;)
Strictly speaking this violates SPOT; I know a helper to calculate
 this 'at runtime' in the driver would have to loop over actions,
 but it's control-plane so performance doesn't matter :grin:
I'd suggest something like adding an internal-use-only MIXED value to
 the enum, and then having a helper
 enum flow_action_hw_state_type flow_action_single_stats_type(struct flow_action *action);
 which could return FLOW_ACTION_HW_STATS_TYPE_MIXED or else whichever
 type all the actions have (except that the 'different' check might
 be further complicated to ignore DISABLED, since most
 flow_action_entries will be for TC actions with no .update_stats()
 which thus don't want stats and can use DISABLED to express that).
That then avoids having to rely on the first entry having the stats
 type (so flow_action_first_entry_get() goes away).

-ed
>  	unsigned int			num_entries;
>  	struct flow_action_entry 	entries[0];
>  };

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jakub Kicinski]

On Tue, 3 Mar 2020 18:55:54 +0000 Edward Cree wrote:
> > Also neither proposal addresses the problem of reporting _different_
> > counter values at different stages in the pipeline, i.e. moving from
> > stats per flow to per action. But nobody seems to be willing to work 
> > on that.  
> For the record, I produced a patch series[1] to support that, but it
>  wasn't acceptable because none of the in-tree drivers implemented the
>  facility.  My hope is that we'll be upstreaming our new driver Real
>  Soon Now™, at which point I'll rebase and repost those changes.

Sorry, I wasn't completely fair :) Looking forward :)

> Alternatively if any other vendor wants to support it in their driver
>  they could use those patches as a base.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jakub Kicinski]

On Tue, 3 Mar 2020 18:25:25 +0100 Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2020 at 02:49:28PM -0800, Jakub Kicinski wrote:
> > On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:  
> [...]
> > > The real question is: if you think this tc counter+action scheme can
> > > be used by netfilter, then please explain how.  
> > 
> > In Jiri's latest patch set the counter type is per action, so just
> > "merge right" the counter info into the next action and the models 
> > are converted.  
> 
> The input "merge right" approach might work.
> 
> > If user is silly and has multiple counter actions in a row - the
> > pipe/no-op action comes into play (that isn't part of this set, 
> > as Jiri said).  
> 
> Probably gact pipe action with counters can be mapped to the counter
> action that netfilter needs. Is this a valid use-case you consider for
> the tc hardware offload?

Once actions can be shared I think it'd be a pretty useful thing for tc
hardware offloads in case HW has limited counters.

> > Can you give us examples of what wouldn't work? Can you for instance
> > share the counter across rules?  
> 
> Yes, there might be counters that are shared accross rules, see
> nfacct. Two different rules might refer to the same counter, IIRC
> there is a way to do this in tc too.

Yup, not implemented for offload, tho.

> > Also neither proposal addresses the problem of reporting _different_
> > counter values at different stages in the pipeline, i.e. moving from
> > stats per flow to per action. But nobody seems to be willing to work 
> > on that.  
> 
> You mean, in case that different counter types are specified, eg. one
> action using delayed and another action using immediate?

I meant the work Ed just pointed to, and what you ask about below.

> > AFAICT with Jiri's change we only need one check in the drivers to
> > convert from old scheme to new, with explicit action we need two
> > (additional one being ignoring the counter action). Not a big deal,
> > but 1 is less than 2 🤷‍♂️  
> 
> What changes are expected to retrieve counter stats?
> 
> Will per-flow stats remain in place after this place?

In theory it doesn't have to, because action stats are more flexible.

In practice I doubt anyone will take on the conversion, so we'll have
to live with two ways for a while..

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jakub Kicinski]

On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:
>  static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS;
>  static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
>  	[TCA_ACT_KIND]		= { .type = NLA_STRING },
>  	[TCA_ACT_INDEX]		= { .type = NLA_U32 },
>  	[TCA_ACT_COOKIE]	= { .type = NLA_BINARY,
>  				    .len = TC_COOKIE_MAX_SIZE },
> +	[TCA_ACT_HW_STATS_TYPE]	= { .type = NLA_U8 },
>  	[TCA_ACT_OPTIONS]	= { .type = NLA_NESTED },
>  	[TCA_ACT_FLAGS]		= { .type = NLA_BITFIELD32,
>  				    .validation_data = &tca_act_flags_allowed },

Ah, we will probably also want to set the strict checking up for new
attrs here.

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jakub Kicinski]

On Tue, 3 Mar 2020 14:20:35 +0100 Jiri Pirko wrote:
> >> Also there would be no "any" it would be type0|type1|type2 the user
> >> would have to pass. If new type appears, the userspace would have to be
> >> updated to do "any" again :/ This is inconvenient.  
> >
> >In my proposal above I was suggesting no attr to mean any. I think in
> >your current code ANY already doesn't include disabled so old user
> >space should not see any change.  
> 
> Odd, no attribute meaning "any".

OTOH it does match up with old kernel behavior quite nicely, today
there is no attribute and it means "any".

> I think it is polite to fillup the attribute for dump if kernel
> supports the attribute. However, here, we would not fill it up in
> case of "any". That is quite odd.

I see, it does seem nice to report the attribute, but again, won't 
the user space which wants to run on older kernels have to treat
no attr as "any"?

> We can have a bit that would mean "any" though. What do you think?

It'd be a dead bit for the "stat types used" attribute, but I don't
mind it if you prefer to go this way.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Pablo Neira Ayuso]

On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote:
> On 02/03/2020 22:49, Jakub Kicinski wrote:
> > On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
> >> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
> >>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:  
> >>>> It looks to me that you want to restrict the API to tc for no good
> >>>> _technical_ reason.  
> 
> The technical reason is that having two ways to do things where one would
>  suffice means more code to be written, tested, debugged.  So if you want
>  to add this you need to convince us that the existing way (a) doesn't
>  meet your needs and (b) can't be extended to cover them.

One single unified way to express the hardware offload for _every_
supported frontend is the way to go. The flow_offload API provides a
framework to model all hardware offloads for each existing front-end.

I understand your motivation might be a specific front-end of your
choice, that's fair enough.

> > Also neither proposal addresses the problem of reporting _different_
> > counter values at different stages in the pipeline, i.e. moving from
> > stats per flow to per action. But nobody seems to be willing to work 
> > on that.
> For the record, I produced a patch series[1] to support that, but it
>  wasn't acceptable because none of the in-tree drivers implemented the
>  facility.  My hope is that we'll be upstreaming our new driver Real
>  Soon Now™, at which point I'll rebase and repost those changes.
> Alternatively if any other vendor wants to support it in their driver
>  they could use those patches as a base.

Great, I am very much looking forward to reviewing your upstream code.

Just keep in my mind that whatever proposal you make must work for
netfilter too.

Thank you.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Edward Cree]

On 03/03/2020 20:27, Pablo Neira Ayuso wrote:
> On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote:
>> On 02/03/2020 22:49, Jakub Kicinski wrote:
>>> On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
>>>> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
>>>>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:  
>>>>>> It looks to me that you want to restrict the API to tc for no good
>>>>>> _technical_ reason.  
>> The technical reason is that having two ways to do things where one would
>>  suffice means more code to be written, tested, debugged.  So if you want
>>  to add this you need to convince us that the existing way (a) doesn't
>>  meet your needs and (b) can't be extended to cover them.
> One single unified way to express the hardware offload for _every_
> supported frontend is the way to go. The flow_offload API provides a
> framework to model all hardware offloads for each existing front-end.
>
> I understand your motivation might be a specific front-end of your
> choice, that's fair enough.
I think we've misunderstood each other (90% my fault).

When you wrote "restrict the API to tc" I read that as "restrict growth of
 the API for flow offloading" (which I *do* want); I've now re-parsed and
 believe you meant it as "limit the API so that only tc may use it" (which
 is not my desire at all).

Thus, when I spoke of "two ways to do things" I meant that _within_ the
 (unified) flow_offload API there should be a single approach to stats
 (the counters attached to actions), to which levels above and below it
 impedance-match as necessary (e.g. by merging netfilter count actions
 onto the following action as Jakub described), rather than bundling
 two interfaces (tc-style counters and separate counter actions) into
 one API (which would mean that drivers would all need to write code to
 handle both kinds, at no gain of expressiveness).
I was *not* referring to tc and netfilter as the "two different ways", but
 I can see why you read it that way.

I hope that makes sense now.
-ed

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Pablo Neira Ayuso]

On Tue, Mar 03, 2020 at 09:06:48PM +0000, Edward Cree wrote:
> On 03/03/2020 20:27, Pablo Neira Ayuso wrote:
> > On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote:
> >> On 02/03/2020 22:49, Jakub Kicinski wrote:
> >>> On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
> >>>> On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
> >>>>> On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:  
> >>>>>> It looks to me that you want to restrict the API to tc for no good
> >>>>>> _technical_ reason.  
> >> The technical reason is that having two ways to do things where one would
> >>  suffice means more code to be written, tested, debugged.  So if you want
> >>  to add this you need to convince us that the existing way (a) doesn't
> >>  meet your needs and (b) can't be extended to cover them.
> > One single unified way to express the hardware offload for _every_
> > supported frontend is the way to go. The flow_offload API provides a
> > framework to model all hardware offloads for each existing front-end.
> >
> > I understand your motivation might be a specific front-end of your
> > choice, that's fair enough.
> I think we've misunderstood each other (90% my fault).
> 
> When you wrote "restrict the API to tc" I read that as "restrict growth of
>  the API for flow offloading" (which I *do* want); I've now re-parsed and
>  believe you meant it as "limit the API so that only tc may use it" (which
>  is not my desire at all).
> 
> Thus, when I spoke of "two ways to do things" I meant that _within_ the
>  (unified) flow_offload API there should be a single approach to stats
>  (the counters attached to actions), to which levels above and below it
>  impedance-match as necessary (e.g. by merging netfilter count actions
>  onto the following action as Jakub described) rather than bundling
>  two interfaces (tc-style counters and separate counter actions)
>  into one API (which would mean that drivers would all need to write
>  code to handle both kinds, at no gain of expressiveness).

It's not that natural to express counters like you prefer for
netfilter, but fair enough, we'll carry on that extra burden of
merging counters to actions.

Sometimes decisions just need a second round: I will expect broken
endianness in drivers because of the 32-bit word choice for the
payload mangling API. But that's a different story.

Noone to blame, there is still experimentation going on in this API.

> I was *not* referring to tc and netfilter
>  as the "two different ways", but  I can see why you read it that
>  way.

I don't follow, sorry.

> I hope that makes sense now.

Sure, thank you.

Re: [patch net-next v2 12/12] sched: act: allow user to specify type of HW stats for a filter

[Jiri Pirko]

Tue, Mar 03, 2020 at 08:48:25PM CET, kuba@kernel.org wrote:
>On Tue, 3 Mar 2020 14:20:35 +0100 Jiri Pirko wrote:
>> >> Also there would be no "any" it would be type0|type1|type2 the user
>> >> would have to pass. If new type appears, the userspace would have to be
>> >> updated to do "any" again :/ This is inconvenient.  
>> >
>> >In my proposal above I was suggesting no attr to mean any. I think in
>> >your current code ANY already doesn't include disabled so old user
>> >space should not see any change.  
>> 
>> Odd, no attribute meaning "any".
>
>OTOH it does match up with old kernel behavior quite nicely, today
>there is no attribute and it means "any".
>
>> I think it is polite to fillup the attribute for dump if kernel
>> supports the attribute. However, here, we would not fill it up in
>> case of "any". That is quite odd.
>
>I see, it does seem nice to report the attribute, but again, won't 
>the user space which wants to run on older kernels have to treat
>no attr as "any"?

Okay.

>
>> We can have a bit that would mean "any" though. What do you think?
>
>It'd be a dead bit for the "stat types used" attribute, but I don't
>mind it if you prefer to go this way.

Re: [patch net-next v2 01/12] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

Tue, Mar 03, 2020 at 08:13:23PM CET, ecree@solarflare.com wrote:
>On 28/02/2020 17:24, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
>> current implicit value coming down to flow_offload. Add a bool
>> indicating that entries have mixed HW stats type.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> v1->v2:
>> - moved to actions
>> - add mixed bool
>> ---
>>  include/net/flow_offload.h | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> index 4e864c34a1b0..eee1cbc5db3c 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -154,6 +154,10 @@ enum flow_action_mangle_base {
>>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>>  };
>>  
>> +enum flow_action_hw_stats_type {
>> +	FLOW_ACTION_HW_STATS_TYPE_ANY,
>> +};
>> +
>>  typedef void (*action_destr)(void *priv);
>>  
>>  struct flow_action_cookie {
>> @@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>>  
>>  struct flow_action_entry {
>>  	enum flow_action_id		id;
>> +	enum flow_action_hw_stats_type	hw_stats_type;
>>  	action_destr			destructor;
>>  	void				*destructor_priv;
>>  	union {
>> @@ -228,6 +233,7 @@ struct flow_action_entry {
>>  };
>>  
>>  struct flow_action {
>> +	bool				mixed_hw_stats_types;
>Some sort of comment in the commit message the effect that this will
> be set in patch #12 would be nice (and would have saved me some
> reviewing time looking for it ;)
>Strictly speaking this violates SPOT; I know a helper to calculate
> this 'at runtime' in the driver would have to loop over actions,
> but it's control-plane so performance doesn't matter :grin:

That is what I wanted to avoid.


>I'd suggest something like adding an internal-use-only MIXED value to
> the enum, and then having a helper
> enum flow_action_hw_state_type flow_action_single_stats_type(struct flow_action *action);
> which could return FLOW_ACTION_HW_STATS_TYPE_MIXED or else whichever
> type all the actions have (except that the 'different' check might
> be further complicated to ignore DISABLED, since most
> flow_action_entries will be for TC actions with no .update_stats()
> which thus don't want stats and can use DISABLED to express that).
>That then avoids having to rely on the first entry having the stats
> type (so flow_action_first_entry_get() goes away).

No problem. I can call a helper that would go over the entries from
driver. As you say, it is a slow path.

Will do.


>
>-ed
>>  	unsigned int			num_entries;
>>  	struct flow_action_entry 	entries[0];
>>  };