[PATCH net-next 0/9] net: flow_offload: add support for per action hw stats

[PATCH net-next 0/9] net: flow_offload: add support for per action hw stats

[Oz Shlomo]

There are currently two mechanisms for populating hardware stats:
1. Using flow_offload api to query the flow's statistics.
   The api assumes that the same stats values apply to all
   the flow's actions.
   This assumption breaks when action drops or jumps over following
   actions.
2. Using hw_action api to query specific action stats via a driver
   callback method. This api assures the correct action stats for
   the offloaded action, however, it does not apply to the rest of the
   actions in the flow's actions array, as elaborated below.

The current hw_action api does not apply to the following use cases:
1. Actions that are implicitly created by filters (aka bind actions).
   In the following example only one counter will apply to the rule:
   tc filter add dev $DEV prio 2 protocol ip parent ffff: \
        flower ip_proto tcp dst_ip $IP2 \
        action police rate 1mbit burst 100k conform-exceed drop/pipe \
        action mirred egress redirect dev $DEV2

2. Action preceding a hw action.
   In the following example the same flow stats will apply to the sample and
   mirred actions:
    tc action add police rate 1mbit burst 100k conform-exceed drop / pipe
    tc filter add dev $DEV prio 2 protocol ip parent ffff: \
        flower ip_proto tcp dst_ip $IP2 \
        action sample rate 1 group 10 trunc 60 pipe \
        action police index 1 \
        action mirred egress redirect dev $DEV2

3. Meter action using jump control.
   In the following example the same flow stats will apply to both
   mirred actions:
    tc action add police rate 1mbit burst 100k conform-exceed jump 2 / pipe
    tc filter add dev $DEV prio 2 protocol ip parent ffff: \
        flower ip_proto tcp dst_ip $IP2 \
        action police index 1 \
        action mirred egress redirect dev $DEV2
        action mirred egress redirect dev $DEV3

This series provides the platform to query per action stats for in_hw flows.

The first four patches are preparation patches with no functionality change.
The fifth patch re-uses the existing flow action stats api to query action
stats for both classifier and action dumps.
The rest of the patches add per action stats support to the Mellanox driver.

Oz Shlomo (9):
  net/sched: optimize action stats api calls
  net/sched: act_pedit, setup offload action for action stats query
  net/sched: pass flow_stats instead of multiple stats args
  net/sched: introduce flow_offload action cookie
  net/sched: support per action hw stats
  net/mlx5e: TC, add hw counter to branching actions
  net/mlx5e: TC, store tc action cookies per attr
  net/sched: TC, map tc action cookie to a hw counter
  net/sched: TC, support per action stats

 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |   2 +-
 .../net/ethernet/mellanox/mlx5/core/en/rep/tc.c    |   2 +-
 .../ethernet/mellanox/mlx5/core/en/tc/act_stats.c  | 197 +++++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/en/tc/act_stats.h  |  27 +++
 .../net/ethernet/mellanox/mlx5/core/en/tc_priv.h   |   1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h   |   3 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    |  91 ++++++++--
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.h    |   4 +
 .../net/ethernet/mellanox/mlx5/core/fs_counters.c  |  10 ++
 include/linux/mlx5/fs.h                            |   2 +
 include/net/flow_offload.h                         |   3 +
 include/net/pkt_cls.h                              |  30 ++--
 net/sched/act_api.c                                |  14 +-
 net/sched/act_pedit.c                              |  24 ++-
 net/sched/cls_api.c                                |   1 +
 net/sched/cls_flower.c                             |   7 +-
 net/sched/cls_matchall.c                           |   6 +-
 17 files changed, 376 insertions(+), 48 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h

-- 
1.8.3.1

[PATCH net-next 1/9] net/sched: optimize action stats api calls

[Oz Shlomo]

Currently the hw action stats update is called from tcf_exts_hw_stats_update,
when a tc filter is dumped, and from tcf_action_copy_stats, when a hw
action is dumped.
However, the tcf_action_copy_stats is also called from tcf_action_dump.
As such, the hw action stats update cb is called 3 times for every
tc flower filter dump.

Move the tc action hw stats update from tcf_action_copy_stats to
tcf_dump_walker to update the hw action stats when tc action is dumped.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 net/sched/act_api.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index cd09ef49df22..f4fa6d7340f8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -539,6 +539,8 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 			       (unsigned long)p->tcfa_tm.lastuse))
 			continue;
 
+		tcf_action_update_hw_stats(p);
+
 		nest = nla_nest_start_noflag(skb, n_i);
 		if (!nest) {
 			index--;
@@ -1539,9 +1541,6 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
 	if (p == NULL)
 		goto errout;
 
-	/* update hw stats for this action */
-	tcf_action_update_hw_stats(p);
-
 	/* compat_mode being true specifies a call that is supposed
 	 * to add additional backward compatibility statistic TLVs.
 	 */
-- 
1.8.3.1

[PATCH net-next 2/9] net/sched: act_pedit, setup offload action for action stats query

[Oz Shlomo]

A single tc pedit action may be translated to multiple flow_offload
actions.
Offload only actions that translate to a single pedit command value.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 net/sched/act_pedit.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index a0378e9f0121..abceef794f28 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct tc_action *act, void *entry_data,
 		}
 		*index_inc = k;
 	} else {
-		return -EOPNOTSUPP;
+		struct flow_offload_action *fl_action = entry_data;
+		u32 last_cmd;
+		int k;
+
+		for (k = 0; k < tcf_pedit_nkeys(act); k++) {
+			u32 cmd = tcf_pedit_cmd(act, k);
+
+			if (k && cmd != last_cmd)
+				return -EOPNOTSUPP;
+
+			last_cmd = cmd;
+			switch (cmd) {
+			case TCA_PEDIT_KEY_EX_CMD_SET:
+				fl_action->id = FLOW_ACTION_MANGLE;
+				break;
+			case TCA_PEDIT_KEY_EX_CMD_ADD:
+				fl_action->id = FLOW_ACTION_ADD;
+				break;
+			default:
+				NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit command offload");
+				return -EOPNOTSUPP;
+			}
+		}
 	}
 
 	return 0;
-- 
1.8.3.1

[PATCH net-next 3/9] net/sched: pass flow_stats instead of multiple stats args

[Oz Shlomo]

Instead of passing 6 stats related args, pass the flow_stats.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 include/net/pkt_cls.h    | 11 +++++------
 net/sched/cls_flower.c   |  7 +------
 net/sched/cls_matchall.c |  6 +-----
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 4cabb32a2ad9..be21764a3b34 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -294,8 +294,7 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 
 static inline void
 tcf_exts_hw_stats_update(const struct tcf_exts *exts,
-			 u64 bytes, u64 packets, u64 drops, u64 lastuse,
-			 u8 used_hw_stats, bool used_hw_stats_valid)
+			 struct flow_stats *stats)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	int i;
@@ -306,12 +305,12 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 		/* if stats from hw, just skip */
 		if (tcf_action_update_hw_stats(a)) {
 			preempt_disable();
-			tcf_action_stats_update(a, bytes, packets, drops,
-						lastuse, true);
+			tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
+						stats->lastused, true);
 			preempt_enable();
 
-			a->used_hw_stats = used_hw_stats;
-			a->used_hw_stats_valid = used_hw_stats_valid;
+			a->used_hw_stats = stats->used_hw_stats;
+			a->used_hw_stats_valid = stats->used_hw_stats_valid;
 		}
 	}
 #endif
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 0b15698b3531..cb04739a13ce 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -502,12 +502,7 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
 	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
 			 rtnl_held);
 
-	tcf_exts_hw_stats_update(&f->exts, cls_flower.stats.bytes,
-				 cls_flower.stats.pkts,
-				 cls_flower.stats.drops,
-				 cls_flower.stats.lastused,
-				 cls_flower.stats.used_hw_stats,
-				 cls_flower.stats.used_hw_stats_valid);
+	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
 }
 
 static void __fl_put(struct cls_fl_filter *f)
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 705f63da2c21..b3883d3d4dbd 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -331,11 +331,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
 
 	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
 
-	tcf_exts_hw_stats_update(&head->exts, cls_mall.stats.bytes,
-				 cls_mall.stats.pkts, cls_mall.stats.drops,
-				 cls_mall.stats.lastused,
-				 cls_mall.stats.used_hw_stats,
-				 cls_mall.stats.used_hw_stats_valid);
+	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
 }
 
 static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
-- 
1.8.3.1

[PATCH net-next 4/9] net/sched: introduce flow_offload action cookie

[Oz Shlomo]

Currently a hardware action is uniquely identified by the <id, hw_index>
tuple. However, the id is set by the flow_act_setup callback and tc core
cannot enforce this, and it is possible that a future change could break
this. In addition, <id, hw_index> are not unique across network namespaces.

Uniquely identify the action by setting an action cookie by the tc core.
Use the unique action cookie to query the action's hardware stats.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 include/net/flow_offload.h | 2 ++
 net/sched/act_api.c        | 1 +
 net/sched/cls_api.c        | 1 +
 3 files changed, 4 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 0400a0ac8a29..d177bf5f0e1a 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data,
 struct flow_action_entry {
 	enum flow_action_id		id;
 	u32				hw_index;
+	unsigned long			act_cookie;
 	enum flow_action_hw_stats	hw_stats;
 	action_destr			destructor;
 	void				*destructor_priv;
@@ -610,6 +611,7 @@ struct flow_offload_action {
 	enum offload_act_command  command;
 	enum flow_action_id id;
 	u32 index;
+	unsigned long cookie;
 	struct flow_stats stats;
 	struct flow_action action;
 };
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f4fa6d7340f8..917827199102 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -192,6 +192,7 @@ static int offload_action_init(struct flow_offload_action *fl_action,
 	fl_action->extack = extack;
 	fl_action->command = cmd;
 	fl_action->index = act->tcfa_index;
+	fl_action->cookie = (unsigned long)act;
 
 	if (act->ops->offload_act_setup) {
 		spin_lock_bh(&act->tcfa_lock);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 5b4a95e8a1ee..bfabc9c95fa9 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3577,6 +3577,7 @@ int tc_setup_action(struct flow_action *flow_action,
 		for (k = 0; k < index ; k++) {
 			entry[k].hw_stats = tc_act_hw_stats(act->hw_stats);
 			entry[k].hw_index = act->tcfa_index;
+			entry[k].act_cookie = (unsigned long)act;
 		}
 
 		j += index;
-- 
1.8.3.1

[PATCH net-next 5/9] net/sched: support per action hw stats

[Oz Shlomo]

There are currently two mechanisms for populating hardware stats:
1. Using flow_offload api to query the flow's statistics.
   The api assumes that the same stats values apply to all
   the flow's actions.
   This assumption breaks when action drops or jumps over following
   actions.
2. Using hw_action api to query specific action stats via a driver
   callback method. This api assures the correct action stats for
   the offloaded action, however, it does not apply to the rest of the
   actions in the flow's actions array.

Extend the flow_offload stats callback to indicate that a per action
stats update is required.
Use the existing flow_offload_action api to query the action's hw stats.
In addition, currently the tc action stats utility only updates hw actions.
Reuse the existing action stats cb infrastructure to query any action
stats.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 include/net/flow_offload.h |  1 +
 include/net/pkt_cls.h      | 29 +++++++++++++++++++----------
 net/sched/act_api.c        |  8 --------
 net/sched/cls_flower.c     |  2 +-
 net/sched/cls_matchall.c   |  2 +-
 5 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index d177bf5f0e1a..27decadd4f5f 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -597,6 +597,7 @@ struct flow_cls_offload {
 	unsigned long cookie;
 	struct flow_rule *rule;
 	struct flow_stats stats;
+	bool use_act_stats;
 	u32 classid;
 };
 
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index be21764a3b34..d4315757d1a2 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -292,9 +292,15 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 #define tcf_act_for_each_action(i, a, actions) \
 	for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
 
+static bool tc_act_in_hw(struct tc_action *act)
+{
+	return !!act->in_hw_count;
+}
+
 static inline void
 tcf_exts_hw_stats_update(const struct tcf_exts *exts,
-			 struct flow_stats *stats)
+			 struct flow_stats *stats,
+			 bool use_act_stats)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	int i;
@@ -302,16 +308,18 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 	for (i = 0; i < exts->nr_actions; i++) {
 		struct tc_action *a = exts->actions[i];
 
-		/* if stats from hw, just skip */
-		if (tcf_action_update_hw_stats(a)) {
-			preempt_disable();
-			tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
-						stats->lastused, true);
-			preempt_enable();
-
-			a->used_hw_stats = stats->used_hw_stats;
-			a->used_hw_stats_valid = stats->used_hw_stats_valid;
+		if (use_act_stats || tc_act_in_hw(a)) {
+			tcf_action_update_hw_stats(a);
+			continue;
 		}
+
+		preempt_disable();
+		tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
+					stats->lastused, true);
+		preempt_enable();
+
+		a->used_hw_stats = stats->used_hw_stats;
+		a->used_hw_stats_valid = stats->used_hw_stats_valid;
 	}
 #endif
 }
@@ -769,6 +777,7 @@ struct tc_cls_matchall_offload {
 	enum tc_matchall_command command;
 	struct flow_rule *rule;
 	struct flow_stats stats;
+	bool use_act_stats;
 	unsigned long cookie;
 };
 
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 917827199102..eda58b78da13 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -169,11 +169,6 @@ static bool tc_act_skip_sw(u32 flags)
 	return (flags & TCA_ACT_FLAGS_SKIP_SW) ? true : false;
 }
 
-static bool tc_act_in_hw(struct tc_action *act)
-{
-	return !!act->in_hw_count;
-}
-
 /* SKIP_HW and SKIP_SW are mutually exclusive flags. */
 static bool tc_act_flags_valid(u32 flags)
 {
@@ -308,9 +303,6 @@ int tcf_action_update_hw_stats(struct tc_action *action)
 	struct flow_offload_action fl_act = {};
 	int err;
 
-	if (!tc_act_in_hw(action))
-		return -EOPNOTSUPP;
-
 	err = offload_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
 	if (err)
 		return err;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index cb04739a13ce..885c95191ccf 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -502,7 +502,7 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
 	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
 			 rtnl_held);
 
-	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
+	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats, cls_flower.use_act_stats);
 }
 
 static void __fl_put(struct cls_fl_filter *f)
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index b3883d3d4dbd..fa3bbd187eb9 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -331,7 +331,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
 
 	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
 
-	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
+	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats, cls_mall.use_act_stats);
 }
 
 static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
-- 
1.8.3.1

[PATCH net-next 6/9] net/mlx5e: TC, add hw counter to branching actions

[Oz Shlomo]

Currently a hw count action is appended to the last action of the action
list. However, a branching action may terminate the action list before
reaching the last action.

Append a count action to a branching action.
In the next patches, filters with branching actions will read this counter
when reporting stats per action.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 4e6f5caf8ab6..39f75f7d5c8b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -3796,6 +3796,7 @@ bool mlx5e_same_hw_devs(struct mlx5e_priv *priv, struct mlx5e_priv *peer_priv)
 	INIT_LIST_HEAD(&attr2->list);
 	parse_attr->filter_dev = attr->parse_attr->filter_dev;
 	attr2->action = 0;
+	attr2->counter = NULL;
 	attr2->flags = 0;
 	attr2->parse_attr = parse_attr;
 	attr2->dest_chain = 0;
@@ -4095,6 +4096,10 @@ struct mlx5_flow_attr *
 		jump_state->jumping_attr = attr->branch_false;
 
 	jump_state->jump_count = jump_count;
+
+	/* branching action requires its own counter */
+	attr->action |= MLX5_FLOW_CONTEXT_ACTION_COUNT;
+
 	return 0;
 
 err_branch_false:
-- 
1.8.3.1

[PATCH net-next 7/9] net/mlx5e: TC, store tc action cookies per attr

[Oz Shlomo]

The tc parse action phase translates the tc actions to mlx5 flow
attributes data structure that is used during the flow offload phase.
Currently, the flow offload stage instantiates hw counters while
associating them to flow cookie. However, flows with branching
actions are required to associate a hardware counter with its action
cookies.

Store the parsed tc action cookies on the flow attribute.
Use the list of cookies in the next patch to associate a tc action cookie
with its allocated hw counter.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 39f75f7d5c8b..a5118da3ed6c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -3797,6 +3797,7 @@ bool mlx5e_same_hw_devs(struct mlx5e_priv *priv, struct mlx5e_priv *peer_priv)
 	parse_attr->filter_dev = attr->parse_attr->filter_dev;
 	attr2->action = 0;
 	attr2->counter = NULL;
+	attr->tc_act_cookies_count = 0;
 	attr2->flags = 0;
 	attr2->parse_attr = parse_attr;
 	attr2->dest_chain = 0;
@@ -4160,6 +4161,8 @@ struct mlx5_flow_attr *
 			goto out_free;
 
 		parse_state->actions |= attr->action;
+		if (!tc_act->stats_action)
+			attr->tc_act_cookies[attr->tc_act_cookies_count++] = act->act_cookie;
 
 		/* Split attr for multi table act if not the last act. */
 		if (jump_state.jump_target ||
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
index ce516dc7f3fd..8aa25d8bac86 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
@@ -70,6 +70,8 @@ struct mlx5_nic_flow_attr {
 struct mlx5_flow_attr {
 	u32 action;
 	struct mlx5_fc *counter;
+	unsigned long tc_act_cookies[TCA_ACT_MAX_PRIO];
+	int tc_act_cookies_count;
 	struct mlx5_modify_hdr *modify_hdr;
 	struct mlx5e_mod_hdr_handle *mh; /* attached mod header instance */
 	struct mlx5e_mod_hdr_handle *slow_mh; /* attached mod header instance for slow path */
-- 
1.8.3.1

[PATCH net-next 8/9] net/sched: TC, map tc action cookie to a hw counter

[Oz Shlomo]

Currently a hardware counter is associated with a flow cookie.
This does not apply to flows using branching action which are required to
return per action stats.

A single counter may apply to multiple actions.
Scan the flow actions in reverse (from the last to the first action) while
caching the last counter.
Associate all the flow attribute tc action cookies with the current
cached counter.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |   2 +-
 .../ethernet/mellanox/mlx5/core/en/tc/act_stats.c  | 153 +++++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/en/tc/act_stats.h  |  23 ++++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h   |   3 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    |  44 ++++++
 5 files changed, 224 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index cd4a1ab0ea78..06f511fcbd8e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -47,7 +47,7 @@ mlx5_core-$(CONFIG_MLX5_CLS_ACT)     += en_tc.o en/rep/tc.o en/rep/neigh.o \
 					en/tc_tun_vxlan.o en/tc_tun_gre.o en/tc_tun_geneve.o \
 					en/tc_tun_mplsoudp.o diag/en_tc_tracepoint.o \
 					en/tc/post_act.o en/tc/int_port.o en/tc/meter.o \
-					en/tc/post_meter.o
+					en/tc/post_meter.o en/tc/act_stats.o
 
 mlx5_core-$(CONFIG_MLX5_CLS_ACT)     += en/tc/act/act.o en/tc/act/drop.o en/tc/act/trap.o \
 					en/tc/act/accept.o en/tc/act/mark.o en/tc/act/goto.o \
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c
new file mode 100644
index 000000000000..d1272c0f883c
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+// Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+
+#include <linux/rhashtable.h>
+#include <net/flow_offload.h>
+#include "en/tc_priv.h"
+#include "act_stats.h"
+#include "en/fs.h"
+
+struct mlx5e_tc_act_stats_handle {
+	struct rhashtable ht;
+	spinlock_t ht_lock; /* protects hashtable */
+};
+
+struct mlx5e_tc_act_stats {
+	unsigned long		tc_act_cookie;
+
+	struct mlx5_fc		*counter;
+	u64			lastpackets;
+	u64			lastbytes;
+
+	struct rhash_head	hash;
+	struct rcu_head		rcu_head;
+};
+
+static const struct rhashtable_params act_counters_ht_params = {
+	.head_offset = offsetof(struct mlx5e_tc_act_stats, hash),
+	.key_offset = 0,
+	.key_len = offsetof(struct mlx5e_tc_act_stats, counter),
+	.automatic_shrinking = true,
+};
+
+struct mlx5e_tc_act_stats_handle *
+mlx5e_tc_act_stats_create(void)
+{
+	struct mlx5e_tc_act_stats_handle *handle;
+	int err;
+
+	handle = kvzalloc(sizeof(*handle), GFP_KERNEL);
+	if (IS_ERR(handle))
+		return ERR_PTR(-ENOMEM);
+
+	err = rhashtable_init(&handle->ht, &act_counters_ht_params);
+	if (err)
+		goto err;
+
+	spin_lock_init(&handle->ht_lock);
+	return handle;
+err:
+	kvfree(handle);
+	return ERR_PTR(err);
+}
+
+void mlx5e_tc_act_stats_free(struct mlx5e_tc_act_stats_handle *handle)
+{
+	rhashtable_destroy(&handle->ht);
+	kvfree(handle);
+}
+
+static int
+mlx5e_tc_act_stats_add(struct mlx5e_tc_act_stats_handle *handle,
+		       unsigned long act_cookie,
+		       struct mlx5_fc *counter)
+{
+	struct mlx5e_tc_act_stats *act_stats, *old_act_stats;
+	struct rhashtable *ht = &handle->ht;
+	int err = 0;
+
+	act_stats = kvzalloc(sizeof(*act_stats), GFP_KERNEL);
+	if (!act_stats)
+		return -ENOMEM;
+
+	act_stats->tc_act_cookie = act_cookie;
+	act_stats->counter = counter;
+
+	rcu_read_lock();
+	old_act_stats = rhashtable_lookup_get_insert_fast(ht,
+							  &act_stats->hash,
+							  act_counters_ht_params);
+	if (IS_ERR(old_act_stats)) {
+		err = PTR_ERR(old_act_stats);
+		goto err_hash_insert;
+	} else if (old_act_stats) {
+		err = -EEXIST;
+		goto err_hash_insert;
+	}
+	rcu_read_unlock();
+
+	return 0;
+
+err_hash_insert:
+	rcu_read_unlock();
+	kvfree(act_stats);
+	return err;
+}
+
+void
+mlx5e_tc_act_stats_del_flow(struct mlx5e_tc_act_stats_handle *handle,
+			    struct mlx5e_tc_flow *flow)
+{
+	struct mlx5_flow_attr *attr;
+	struct mlx5e_tc_act_stats *act_stats;
+	int i;
+
+	list_for_each_entry(attr, &flow->attrs, list) {
+		for (i = 0; i < attr->tc_act_cookies_count; i++) {
+			struct rhashtable *ht = &handle->ht;
+
+			spin_lock(&handle->ht_lock);
+			act_stats = rhashtable_lookup_fast(ht,
+							   &attr->tc_act_cookies[i],
+							   act_counters_ht_params);
+			if (act_stats &&
+			    rhashtable_remove_fast(ht, &act_stats->hash,
+						   act_counters_ht_params) == 0)
+				kvfree_rcu(act_stats, rcu_head);
+
+			spin_unlock(&handle->ht_lock);
+		}
+	}
+}
+
+int
+mlx5e_tc_act_stats_add_flow(struct mlx5e_tc_act_stats_handle *handle,
+			    struct mlx5e_tc_flow *flow)
+{
+	struct mlx5_fc *curr_counter = NULL;
+	unsigned long last_cookie = 0;
+	struct mlx5_flow_attr *attr;
+	int err;
+	int i;
+
+	list_for_each_entry(attr, &flow->attrs, list) {
+		if (attr->counter)
+			curr_counter = attr->counter;
+
+		for (i = 0; i < attr->tc_act_cookies_count; i++) {
+			/* jump over identical ids (e.g. pedit)*/
+			if (last_cookie == attr->tc_act_cookies[i])
+				continue;
+
+			err = mlx5e_tc_act_stats_add(handle, attr->tc_act_cookies[i], curr_counter);
+			if (err)
+				goto out_err;
+			last_cookie = attr->tc_act_cookies[i];
+		}
+	}
+
+	return 0;
+out_err:
+	mlx5e_tc_act_stats_del_flow(handle, flow);
+	return err;
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h
new file mode 100644
index 000000000000..4929301a5260
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
+/* Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
+
+#ifndef __MLX5_EN_ACT_STATS_H__
+#define __MLX5_EN_ACT_STATS_H__
+
+#include <net/flow_offload.h>
+#include "en/tc_priv.h"
+
+struct mlx5e_tc_act_stats_handle;
+
+struct mlx5e_tc_act_stats_handle *mlx5e_tc_act_stats_create(void);
+void mlx5e_tc_act_stats_free(struct mlx5e_tc_act_stats_handle *handle);
+
+int
+mlx5e_tc_act_stats_add_flow(struct mlx5e_tc_act_stats_handle *handle,
+			    struct mlx5e_tc_flow *flow);
+
+void
+mlx5e_tc_act_stats_del_flow(struct mlx5e_tc_act_stats_handle *handle,
+			    struct mlx5e_tc_flow *flow);
+
+#endif /* __MLX5_EN_ACT_STATS_H__ */
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index b4e691760da9..0abe3313c673 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
@@ -100,6 +100,9 @@ struct mlx5_rep_uplink_priv {
 	struct mlx5e_tc_int_port_priv *int_port_priv;
 
 	struct mlx5e_flow_meters *flow_meters;
+
+	/* tc action stats */
+	struct mlx5e_tc_act_stats_handle *action_stats_handle;
 };
 
 struct mlx5e_rep_priv {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index a5118da3ed6c..ec5d1fbae22e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -45,6 +45,7 @@
 #include <net/bonding.h>
 #include "en.h"
 #include "en/tc/post_act.h"
+#include "en/tc/act_stats.h"
 #include "en_rep.h"
 #include "en/rep/tc.h"
 #include "en/rep/neigh.h"
@@ -101,6 +102,9 @@ struct mlx5e_tc_table {
 	struct mapping_ctx             *mapping;
 	struct mlx5e_hairpin_params    hairpin_params;
 	struct dentry                  *dfs_root;
+
+	/* tc action stats */
+	struct mlx5e_tc_act_stats_handle *action_stats_handle;
 };
 
 struct mlx5e_tc_attr_to_reg_mapping mlx5e_tc_attr_to_reg_mappings[] = {
@@ -286,6 +290,24 @@ struct mlx5_fs_chains *mlx5e_nic_chains(struct mlx5e_tc_table *tc)
 	return err;
 }
 
+static struct mlx5e_tc_act_stats_handle  *
+get_act_stats_handle(struct mlx5e_priv *priv)
+{
+	struct mlx5e_tc_table *tc = mlx5e_fs_get_tc(priv->fs);
+	struct mlx5_eswitch *esw = priv->mdev->priv.eswitch;
+	struct mlx5_rep_uplink_priv *uplink_priv;
+	struct mlx5e_rep_priv *uplink_rpriv;
+
+	if (is_mdev_switchdev_mode(priv->mdev)) {
+		uplink_rpriv = mlx5_eswitch_get_uplink_priv(esw, REP_ETH);
+		uplink_priv = &uplink_rpriv->uplink_priv;
+
+		return uplink_priv->action_stats_handle;
+	}
+
+	return tc->action_stats_handle;
+}
+
 struct mlx5e_tc_int_port_priv *
 mlx5e_get_int_port_priv(struct mlx5e_priv *priv)
 {
@@ -2035,6 +2057,10 @@ int mlx5e_tc_query_route_vport(struct net_device *out_dev, struct net_device *ro
 	if (err)
 		goto err_out;
 
+	err = mlx5e_tc_act_stats_add_flow(get_act_stats_handle(priv), flow);
+	if (err)
+		goto err_out;
+
 	/* we get here if one of the following takes place:
 	 * (1) there's no error
 	 * (2) there's an encap action and we don't have valid neigh
@@ -2131,6 +2157,8 @@ static void mlx5e_tc_del_fdb_flow(struct mlx5e_priv *priv,
 	if (flow_flag_test(flow, L3_TO_L2_DECAP))
 		mlx5e_detach_decap(priv, flow);
 
+	mlx5e_tc_act_stats_del_flow(get_act_stats_handle(priv), flow);
+
 	free_flow_post_acts(flow);
 	free_branch_attr(flow, attr->branch_true);
 	free_branch_attr(flow, attr->branch_false);
@@ -5342,8 +5370,16 @@ int mlx5e_tc_nic_init(struct mlx5e_priv *priv)
 
 	mlx5e_tc_debugfs_init(tc, mlx5e_fs_get_debugfs_root(priv->fs));
 
+	tc->action_stats_handle = mlx5e_tc_act_stats_create();
+	if (IS_ERR(tc->action_stats_handle))
+		goto err_act_stats;
+
 	return 0;
 
+err_act_stats:
+	unregister_netdevice_notifier_dev_net(priv->netdev,
+					      &tc->netdevice_nb,
+					      &tc->netdevice_nn);
 err_reg:
 	mlx5_tc_ct_clean(tc->ct);
 	mlx5e_tc_post_act_destroy(tc->post_act);
@@ -5393,6 +5429,7 @@ void mlx5e_tc_nic_cleanup(struct mlx5e_priv *priv)
 	mapping_destroy(tc->mapping);
 	mlx5_chains_destroy(tc->chains);
 	mlx5e_tc_nic_destroy_miss_table(priv);
+	mlx5e_tc_act_stats_free(tc->action_stats_handle);
 }
 
 int mlx5e_tc_ht_init(struct rhashtable *tc_ht)
@@ -5469,8 +5506,14 @@ int mlx5e_tc_esw_init(struct mlx5_rep_uplink_priv *uplink_priv)
 		goto err_register_fib_notifier;
 	}
 
+	uplink_priv->action_stats_handle = mlx5e_tc_act_stats_create();
+	if (IS_ERR(uplink_priv->action_stats_handle))
+		goto err_action_counter;
+
 	return 0;
 
+err_action_counter:
+	mlx5e_tc_tun_cleanup(uplink_priv->encap);
 err_register_fib_notifier:
 	mapping_destroy(uplink_priv->tunnel_enc_opts_mapping);
 err_enc_opts_mapping:
@@ -5497,6 +5540,7 @@ void mlx5e_tc_esw_cleanup(struct mlx5_rep_uplink_priv *uplink_priv)
 	mlx5_tc_ct_clean(uplink_priv->ct_priv);
 	mlx5e_flow_meters_cleanup(uplink_priv->flow_meters);
 	mlx5e_tc_post_act_destroy(uplink_priv->post_act);
+	mlx5e_tc_act_stats_free(uplink_priv->action_stats_handle);
 }
 
 int mlx5e_tc_num_filters(struct mlx5e_priv *priv, unsigned long flags)
-- 
1.8.3.1

[PATCH net-next 9/9] net/sched: TC, support per action stats

[Oz Shlomo]

Extend the action stats callback implementation to update stats for actions
that are associated with hw counters.
Note that the callback may be called from tc action utility or from tc
flower. Both apis expect the driver to return the stats difference from
the last update. As such, query the raw counter value and maintain
the diff from the last api call in the tc layer, instead of the fs_core
layer.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en/rep/tc.c    |  2 +-
 .../ethernet/mellanox/mlx5/core/en/tc/act_stats.c  | 44 ++++++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/en/tc/act_stats.h  |  4 ++
 .../net/ethernet/mellanox/mlx5/core/en/tc_priv.h   |  1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c    | 39 +++++++++++++------
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.h    |  2 +
 .../net/ethernet/mellanox/mlx5/core/fs_counters.c  | 10 +++++
 include/linux/mlx5/fs.h                            |  2 +
 8 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
index b08339d986d5..3b590cfe33b8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
@@ -589,7 +589,7 @@ static bool mlx5e_rep_macvlan_mode_supported(const struct net_device *dev)
 
 	act = mlx5e_tc_act_get(fl_act->id, ns_type);
 	if (!act || !act->stats_action)
-		return -EOPNOTSUPP;
+		return mlx5e_tc_fill_action_stats(priv, fl_act);
 
 	return act->stats_action(priv, fl_act);
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c
index d1272c0f883c..f71766dca660 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.c
@@ -102,6 +102,9 @@ void mlx5e_tc_act_stats_free(struct mlx5e_tc_act_stats_handle *handle)
 	struct mlx5e_tc_act_stats *act_stats;
 	int i;
 
+	if (!flow_flag_test(flow, USE_ACT_STATS))
+		return;
+
 	list_for_each_entry(attr, &flow->attrs, list) {
 		for (i = 0; i < attr->tc_act_cookies_count; i++) {
 			struct rhashtable *ht = &handle->ht;
@@ -130,6 +133,9 @@ void mlx5e_tc_act_stats_free(struct mlx5e_tc_act_stats_handle *handle)
 	int err;
 	int i;
 
+	if (!flow_flag_test(flow, USE_ACT_STATS))
+		return 0;
+
 	list_for_each_entry(attr, &flow->attrs, list) {
 		if (attr->counter)
 			curr_counter = attr->counter;
@@ -151,3 +157,41 @@ void mlx5e_tc_act_stats_free(struct mlx5e_tc_act_stats_handle *handle)
 	mlx5e_tc_act_stats_del_flow(handle, flow);
 	return err;
 }
+
+int
+mlx5e_tc_act_stats_fill_stats(struct mlx5e_tc_act_stats_handle *handle,
+			      struct flow_offload_action *fl_act)
+{
+	struct rhashtable *ht = &handle->ht;
+	struct mlx5e_tc_act_stats *item;
+	struct mlx5e_tc_act_stats key;
+	u64 pkts, bytes, lastused;
+	int err = 0;
+
+	key.tc_act_cookie = fl_act->cookie;
+
+	rcu_read_lock();
+	item = rhashtable_lookup(ht, &key, act_counters_ht_params);
+	if (!item) {
+		rcu_read_unlock();
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	mlx5_fc_query_cached_raw(item->counter,
+				 &bytes, &pkts, &lastused);
+
+	flow_stats_update(&fl_act->stats,
+			  bytes - item->lastbytes,
+			  pkts - item->lastpackets,
+			  0, lastused, FLOW_ACTION_HW_STATS_DELAYED);
+
+	item->lastpackets = pkts;
+	item->lastbytes = bytes;
+	rcu_read_unlock();
+
+	return 0;
+
+err_out:
+	return err;
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h
index 4929301a5260..002292c2567c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act_stats.h
@@ -20,4 +20,8 @@
 mlx5e_tc_act_stats_del_flow(struct mlx5e_tc_act_stats_handle *handle,
 			    struct mlx5e_tc_flow *flow);
 
+int
+mlx5e_tc_act_stats_fill_stats(struct mlx5e_tc_act_stats_handle *handle,
+			      struct flow_offload_action *fl_act);
+
 #endif /* __MLX5_EN_ACT_STATS_H__ */
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_priv.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_priv.h
index f575646d2f50..451fd4342a5a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_priv.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_priv.h
@@ -30,6 +30,7 @@ enum {
 	MLX5E_TC_FLOW_FLAG_TUN_RX                = MLX5E_TC_FLOW_BASE + 9,
 	MLX5E_TC_FLOW_FLAG_FAILED                = MLX5E_TC_FLOW_BASE + 10,
 	MLX5E_TC_FLOW_FLAG_SAMPLE                = MLX5E_TC_FLOW_BASE + 11,
+	MLX5E_TC_FLOW_FLAG_USE_ACT_STATS	 = MLX5E_TC_FLOW_BASE + 12,
 };
 
 struct mlx5e_tc_flow_parse_attr {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index ec5d1fbae22e..50432c9d78c0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4128,6 +4128,7 @@ struct mlx5_flow_attr *
 
 	/* branching action requires its own counter */
 	attr->action |= MLX5_FLOW_CONTEXT_ACTION_COUNT;
+	flow_flag_set(flow, USE_ACT_STATS);
 
 	return 0;
 
@@ -4978,6 +4979,12 @@ int mlx5e_delete_flower(struct net_device *dev, struct mlx5e_priv *priv,
 	return err;
 }
 
+int mlx5e_tc_fill_action_stats(struct mlx5e_priv *priv,
+			       struct flow_offload_action *fl_act)
+{
+	return mlx5e_tc_act_stats_fill_stats(get_act_stats_handle(priv), fl_act);
+}
+
 int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv,
 		       struct flow_cls_offload *f, unsigned long flags)
 {
@@ -5004,11 +5011,15 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv,
 	}
 
 	if (mlx5e_is_offloaded_flow(flow) || flow_flag_test(flow, CT)) {
-		counter = mlx5e_tc_get_counter(flow);
-		if (!counter)
-			goto errout;
+		if (flow_flag_test(flow, USE_ACT_STATS)) {
+			f->use_act_stats = true;
+		} else {
+			counter = mlx5e_tc_get_counter(flow);
+			if (!counter)
+				goto errout;
 
-		mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
+			mlx5_fc_query_cached(counter, &bytes, &packets, &lastuse);
+		}
 	}
 
 	/* Under multipath it's possible for one rule to be currently
@@ -5024,14 +5035,18 @@ int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv,
 		u64 packets2;
 		u64 lastuse2;
 
-		counter = mlx5e_tc_get_counter(flow->peer_flow);
-		if (!counter)
-			goto no_peer_counter;
-		mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2);
-
-		bytes += bytes2;
-		packets += packets2;
-		lastuse = max_t(u64, lastuse, lastuse2);
+		if (flow_flag_test(flow, USE_ACT_STATS)) {
+			f->use_act_stats = true;
+		} else {
+			counter = mlx5e_tc_get_counter(flow->peer_flow);
+			if (!counter)
+				goto no_peer_counter;
+			mlx5_fc_query_cached(counter, &bytes2, &packets2, &lastuse2);
+
+			bytes += bytes2;
+			packets += packets2;
+			lastuse = max_t(u64, lastuse, lastuse2);
+		}
 	}
 
 no_peer_counter:
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
index 8aa25d8bac86..a54f26dcd23a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
@@ -199,6 +199,8 @@ int mlx5e_delete_flower(struct net_device *dev, struct mlx5e_priv *priv,
 
 int mlx5e_stats_flower(struct net_device *dev, struct mlx5e_priv *priv,
 		       struct flow_cls_offload *f, unsigned long flags);
+int mlx5e_tc_fill_action_stats(struct mlx5e_priv *priv,
+			       struct flow_offload_action *fl_act);
 
 int mlx5e_tc_configure_matchall(struct mlx5e_priv *priv,
 				struct tc_cls_matchall_offload *f);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
index b406e0367af6..17fe30a4c06c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
@@ -504,6 +504,16 @@ void mlx5_fc_query_cached(struct mlx5_fc *counter,
 	counter->lastpackets = c.packets;
 }
 
+void mlx5_fc_query_cached_raw(struct mlx5_fc *counter,
+			      u64 *bytes, u64 *packets, u64 *lastuse)
+{
+	struct mlx5_fc_cache c = counter->cache;
+
+	*bytes = c.bytes;
+	*packets = c.packets;
+	*lastuse = c.lastuse;
+}
+
 void mlx5_fc_queue_stats_work(struct mlx5_core_dev *dev,
 			      struct delayed_work *dwork,
 			      unsigned long delay)
diff --git a/include/linux/mlx5/fs.h b/include/linux/mlx5/fs.h
index ba6958b49a8e..90a2fe5839fa 100644
--- a/include/linux/mlx5/fs.h
+++ b/include/linux/mlx5/fs.h
@@ -296,6 +296,8 @@ int mlx5_modify_rule_destination(struct mlx5_flow_handle *handler,
 u64 mlx5_fc_query_lastuse(struct mlx5_fc *counter);
 void mlx5_fc_query_cached(struct mlx5_fc *counter,
 			  u64 *bytes, u64 *packets, u64 *lastuse);
+void mlx5_fc_query_cached_raw(struct mlx5_fc *counter,
+			      u64 *bytes, u64 *packets, u64 *lastuse);
 int mlx5_fc_query(struct mlx5_core_dev *dev, struct mlx5_fc *counter,
 		  u64 *packets, u64 *bytes);
 u32 mlx5_fc_id(struct mlx5_fc *counter);
-- 
1.8.3.1

Re: [PATCH net-next 2/9] net/sched: act_pedit, setup offload action for action stats query

[Pedro Tammela]

On 01/02/2023 13:10, Oz Shlomo wrote:
> A single tc pedit action may be translated to multiple flow_offload
> actions.
> Offload only actions that translate to a single pedit command value.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
> ---
>   net/sched/act_pedit.c | 24 +++++++++++++++++++++++-
>   1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index a0378e9f0121..abceef794f28 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct tc_action *act, void *entry_data,
>   		}
>   		*index_inc = k;
>   	} else {
> -		return -EOPNOTSUPP;
> +		struct flow_offload_action *fl_action = entry_data;
> +		u32 last_cmd;
> +		int k;
> +
> +		for (k = 0; k < tcf_pedit_nkeys(act); k++) {
> +			u32 cmd = tcf_pedit_cmd(act, k);
> +
> +			if (k && cmd != last_cmd)
> +				return -EOPNOTSUPP;

I believe an extack message here is very valuable

> +
> +			last_cmd = cmd;
> +			switch (cmd) {
> +			case TCA_PEDIT_KEY_EX_CMD_SET:
> +				fl_action->id = FLOW_ACTION_MANGLE;
> +				break;
> +			case TCA_PEDIT_KEY_EX_CMD_ADD:
> +				fl_action->id = FLOW_ACTION_ADD;
> +				break;
> +			default:
> +				NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit command offload");
> +				return -EOPNOTSUPP;
> +			}
> +		}

Shouldn't this switch case be outside of the for-loop?

>   	}
>   
>   	return 0;

Re: [PATCH net-next 2/9] net/sched: act_pedit, setup offload action for action stats query

[Oz Shlomo]

On 01/02/2023 22:59, Pedro Tammela wrote:
> On 01/02/2023 13:10, Oz Shlomo wrote:
>> A single tc pedit action may be translated to multiple flow_offload
>> actions.
>> Offload only actions that translate to a single pedit command value.
>>
>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
>> ---
>>   net/sched/act_pedit.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
>> index a0378e9f0121..abceef794f28 100644
>> --- a/net/sched/act_pedit.c
>> +++ b/net/sched/act_pedit.c
>> @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct 
>> tc_action *act, void *entry_data,
>>           }
>>           *index_inc = k;
>>       } else {
>> -        return -EOPNOTSUPP;
>> +        struct flow_offload_action *fl_action = entry_data;
>> +        u32 last_cmd;
>> +        int k;
>> +
>> +        for (k = 0; k < tcf_pedit_nkeys(act); k++) {
>> +            u32 cmd = tcf_pedit_cmd(act, k);
>> +
>> +            if (k && cmd != last_cmd)
>> +                return -EOPNOTSUPP;
>
> I believe an extack message here is very valuable
Sure thing, I will add one
>
>> +
>> +            last_cmd = cmd;
>> +            switch (cmd) {
>> +            case TCA_PEDIT_KEY_EX_CMD_SET:
>> +                fl_action->id = FLOW_ACTION_MANGLE;
>> +                break;
>> +            case TCA_PEDIT_KEY_EX_CMD_ADD:
>> +                fl_action->id = FLOW_ACTION_ADD;
>> +                break;
>> +            default:
>> +                NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit 
>> command offload");
>> +                return -EOPNOTSUPP;
>> +            }
>> +        }
>
> Shouldn't this switch case be outside of the for-loop?

You are right, this can be done outside the for loop.

I will refactor the code

>
>>       }
>>         return 0;
>

Re: [PATCH net-next 5/9] net/sched: support per action hw stats

[Simon Horman]

On Wed, Feb 01, 2023 at 06:10:34PM +0200, Oz Shlomo wrote:
> There are currently two mechanisms for populating hardware stats:
> 1. Using flow_offload api to query the flow's statistics.
>    The api assumes that the same stats values apply to all
>    the flow's actions.
>    This assumption breaks when action drops or jumps over following
>    actions.
> 2. Using hw_action api to query specific action stats via a driver
>    callback method. This api assures the correct action stats for
>    the offloaded action, however, it does not apply to the rest of the
>    actions in the flow's actions array.
> 
> Extend the flow_offload stats callback to indicate that a per action
> stats update is required.
> Use the existing flow_offload_action api to query the action's hw stats.
> In addition, currently the tc action stats utility only updates hw actions.
> Reuse the existing action stats cb infrastructure to query any action
> stats.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>

...

> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index be21764a3b34..d4315757d1a2 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -292,9 +292,15 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>  #define tcf_act_for_each_action(i, a, actions) \
>  	for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
>  
> +static bool tc_act_in_hw(struct tc_action *act)
> +{
> +	return !!act->in_hw_count;
> +}
> +
>  static inline void
>  tcf_exts_hw_stats_update(const struct tcf_exts *exts,
> -			 struct flow_stats *stats)
> +			 struct flow_stats *stats,
> +			 bool use_act_stats)
>  {
>  #ifdef CONFIG_NET_CLS_ACT
>  	int i;
> @@ -302,16 +308,18 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>  	for (i = 0; i < exts->nr_actions; i++) {
>  		struct tc_action *a = exts->actions[i];
>  
> -		/* if stats from hw, just skip */
> -		if (tcf_action_update_hw_stats(a)) {
> -			preempt_disable();
> -			tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
> -						stats->lastused, true);
> -			preempt_enable();
> -
> -			a->used_hw_stats = stats->used_hw_stats;
> -			a->used_hw_stats_valid = stats->used_hw_stats_valid;
> +		if (use_act_stats || tc_act_in_hw(a)) {
> +			tcf_action_update_hw_stats(a);

Hi Oz,

I am a unclear why it is ok to continue here even if
tcf_action_update_hw_stats() fails.  There seem to be cases other than
!tc_act_in_hw() at play here, which prior to this patch, would execute the
code below that is now outside this if clause.

> +			continue;
>  		}
> +
> +		preempt_disable();
> +		tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
> +					stats->lastused, true);
> +		preempt_enable();
> +
> +		a->used_hw_stats = stats->used_hw_stats;
> +		a->used_hw_stats_valid = stats->used_hw_stats_valid;
>  	}
>  #endif
>  }
> @@ -769,6 +777,7 @@ struct tc_cls_matchall_offload {
>  	enum tc_matchall_command command;
>  	struct flow_rule *rule;
>  	struct flow_stats stats;
> +	bool use_act_stats;
>  	unsigned long cookie;
>  };
>  
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 917827199102..eda58b78da13 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -169,11 +169,6 @@ static bool tc_act_skip_sw(u32 flags)
>  	return (flags & TCA_ACT_FLAGS_SKIP_SW) ? true : false;
>  }
>  
> -static bool tc_act_in_hw(struct tc_action *act)
> -{
> -	return !!act->in_hw_count;
> -}
> -
>  /* SKIP_HW and SKIP_SW are mutually exclusive flags. */
>  static bool tc_act_flags_valid(u32 flags)
>  {
> @@ -308,9 +303,6 @@ int tcf_action_update_hw_stats(struct tc_action *action)
>  	struct flow_offload_action fl_act = {};
>  	int err;
>  
> -	if (!tc_act_in_hw(action))
> -		return -EOPNOTSUPP;
> -
>  	err = offload_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
>  	if (err)
>  		return err;

...

Re: [PATCH net-next 1/9] net/sched: optimize action stats api calls

[Simon Horman]

On Wed, Feb 01, 2023 at 06:10:30PM +0200, Oz Shlomo wrote:
> Currently the hw action stats update is called from tcf_exts_hw_stats_update,
> when a tc filter is dumped, and from tcf_action_copy_stats, when a hw
> action is dumped.
> However, the tcf_action_copy_stats is also called from tcf_action_dump.
> As such, the hw action stats update cb is called 3 times for every
> tc flower filter dump.
> 
> Move the tc action hw stats update from tcf_action_copy_stats to
> tcf_dump_walker to update the hw action stats when tc action is dumped.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 3/9] net/sched: pass flow_stats instead of multiple stats args

[Simon Horman]

On Wed, Feb 01, 2023 at 06:10:32PM +0200, Oz Shlomo wrote:
> Instead of passing 6 stats related args, pass the flow_stats.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 4/9] net/sched: introduce flow_offload action cookie

[Simon Horman]

On Wed, Feb 01, 2023 at 06:10:33PM +0200, Oz Shlomo wrote:
> Currently a hardware action is uniquely identified by the <id, hw_index>
> tuple. However, the id is set by the flow_act_setup callback and tc core
> cannot enforce this, and it is possible that a future change could break
> this. In addition, <id, hw_index> are not unique across network namespaces.
> 
> Uniquely identify the action by setting an action cookie by the tc core.
> Use the unique action cookie to query the action's hardware stats.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 5/9] net/sched: support per action hw stats

[Simon Horman]

On Wed, Feb 01, 2023 at 06:10:34PM +0200, Oz Shlomo wrote:
> There are currently two mechanisms for populating hardware stats:
> 1. Using flow_offload api to query the flow's statistics.
>    The api assumes that the same stats values apply to all
>    the flow's actions.
>    This assumption breaks when action drops or jumps over following
>    actions.
> 2. Using hw_action api to query specific action stats via a driver
>    callback method. This api assures the correct action stats for
>    the offloaded action, however, it does not apply to the rest of the
>    actions in the flow's actions array.
> 
> Extend the flow_offload stats callback to indicate that a per action
> stats update is required.
> Use the existing flow_offload_action api to query the action's hw stats.
> In addition, currently the tc action stats utility only updates hw actions.
> Reuse the existing action stats cb infrastructure to query any action
> stats.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
> ---
>  include/net/flow_offload.h |  1 +
>  include/net/pkt_cls.h      | 29 +++++++++++++++++++----------
>  net/sched/act_api.c        |  8 --------
>  net/sched/cls_flower.c     |  2 +-
>  net/sched/cls_matchall.c   |  2 +-
>  5 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index d177bf5f0e1a..27decadd4f5f 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -597,6 +597,7 @@ struct flow_cls_offload {
>  	unsigned long cookie;
>  	struct flow_rule *rule;
>  	struct flow_stats stats;
> +	bool use_act_stats;
>  	u32 classid;
>  };

Hi Oz,

It's probably not important, but I thought it is worth bringing
to your attention.

The placement of use_act_stats above  puts it on a different
cacheline (on x86_64) to stats. Which does not seem to be idea
as those fields are accessed together.

There is a 4 byte hole immediately above above cookie,
on the same cacheline as stats, which can accommodate use_act_stats.

>  
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index be21764a3b34..d4315757d1a2 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h

...

> @@ -769,6 +777,7 @@ struct tc_cls_matchall_offload {
>  	enum tc_matchall_command command;
>  	struct flow_rule *rule;
>  	struct flow_stats stats;
> +	bool use_act_stats;
>  	unsigned long cookie;
>  };

I believe the same logic applies to this change too.

...

> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index cb04739a13ce..885c95191ccf 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -502,7 +502,7 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
>  	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
>  			 rtnl_held);
>  
> -	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
> +	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats, cls_flower.use_act_stats);
>  }
>  
>  static void __fl_put(struct cls_fl_filter *f)
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index b3883d3d4dbd..fa3bbd187eb9 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -331,7 +331,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
>  
>  	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
>  
> -	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
> +	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats, cls_mall.use_act_stats);
>  }
>  
>  static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,

Re: [PATCH net-next 2/9] net/sched: act_pedit, setup offload action for action stats query

[Marcelo Ricardo Leitner]

On Thu, Feb 02, 2023 at 09:21:15AM +0200, Oz Shlomo wrote:
>
> On 01/02/2023 22:59, Pedro Tammela wrote:
> > On 01/02/2023 13:10, Oz Shlomo wrote:
> > > A single tc pedit action may be translated to multiple flow_offload
> > > actions.
> > > Offload only actions that translate to a single pedit command value.
> > >
> > > Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
> > > ---
> > >   net/sched/act_pedit.c | 24 +++++++++++++++++++++++-
> > >   1 file changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > > index a0378e9f0121..abceef794f28 100644
> > > --- a/net/sched/act_pedit.c
> > > +++ b/net/sched/act_pedit.c
> > > @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct
> > > tc_action *act, void *entry_data,
> > >           }
> > >           *index_inc = k;
> > >       } else {
> > > -        return -EOPNOTSUPP;
> > > +        struct flow_offload_action *fl_action = entry_data;
> > > +        u32 last_cmd;
> > > +        int k;
> > > +
> > > +        for (k = 0; k < tcf_pedit_nkeys(act); k++) {
> > > +            u32 cmd = tcf_pedit_cmd(act, k);
> > > +
> > > +            if (k && cmd != last_cmd)
> > > +                return -EOPNOTSUPP;
> >
> > I believe an extack message here is very valuable
> Sure thing, I will add one
> >
> > > +
> > > +            last_cmd = cmd;
> > > +            switch (cmd) {
> > > +            case TCA_PEDIT_KEY_EX_CMD_SET:
> > > +                fl_action->id = FLOW_ACTION_MANGLE;
> > > +                break;
> > > +            case TCA_PEDIT_KEY_EX_CMD_ADD:
> > > +                fl_action->id = FLOW_ACTION_ADD;
> > > +                break;
> > > +            default:
> > > +                NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit
> > > command offload");
> > > +                return -EOPNOTSUPP;
> > > +            }
> > > +        }
> >
> > Shouldn't this switch case be outside of the for-loop?
>
> You are right, this can be done outside the for loop.

To before the for-loop, that is?
Because otherwise it will parse all commands and then fail, which seems heavier
than how it is here.

- validate the first one
- ensure the rest follows

>
> I will refactor the code
>
> >
> > >       }
> > >         return 0;
> >
>

Re: [PATCH net-next 7/9] net/mlx5e: TC, store tc action cookies per attr

[Marcelo Ricardo Leitner]

On Wed, Feb 01, 2023 at 06:10:36PM +0200, Oz Shlomo wrote:
> The tc parse action phase translates the tc actions to mlx5 flow
> attributes data structure that is used during the flow offload phase.
> Currently, the flow offload stage instantiates hw counters while
> associating them to flow cookie. However, flows with branching
> actions are required to associate a hardware counter with its action
> cookies.
>
> Store the parsed tc action cookies on the flow attribute.
> Use the list of cookies in the next patch to associate a tc action cookie
> with its allocated hw counter.
>
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 3 +++
>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.h | 2 ++
>  2 files changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 39f75f7d5c8b..a5118da3ed6c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -3797,6 +3797,7 @@ bool mlx5e_same_hw_devs(struct mlx5e_priv *priv, struct mlx5e_priv *peer_priv)
>  	parse_attr->filter_dev = attr->parse_attr->filter_dev;
>  	attr2->action = 0;
>  	attr2->counter = NULL;
> +	attr->tc_act_cookies_count = 0;
>  	attr2->flags = 0;
>  	attr2->parse_attr = parse_attr;
>  	attr2->dest_chain = 0;
> @@ -4160,6 +4161,8 @@ struct mlx5_flow_attr *
>  			goto out_free;
>
>  		parse_state->actions |= attr->action;
> +		if (!tc_act->stats_action)
> +			attr->tc_act_cookies[attr->tc_act_cookies_count++] = act->act_cookie;
>
>  		/* Split attr for multi table act if not the last act. */
>  		if (jump_state.jump_target ||
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
> index ce516dc7f3fd..8aa25d8bac86 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
> @@ -70,6 +70,8 @@ struct mlx5_nic_flow_attr {
>  struct mlx5_flow_attr {
>  	u32 action;
>  	struct mlx5_fc *counter;
> +	unsigned long tc_act_cookies[TCA_ACT_MAX_PRIO];
> +	int tc_act_cookies_count;

This one won't count much, as it is limited by TCA_ACT_MAX_PRIO above
andi which is 32.
Maybe this can be an u8 or u16 instead and be added together with 'prio'?
To save 2 bytes, yes, but with a 1M flows, that's 2Mbytes.
Or below 'action' above, to keep it on the same cache line.

>  	struct mlx5_modify_hdr *modify_hdr;
>  	struct mlx5e_mod_hdr_handle *mh; /* attached mod header instance */
>  	struct mlx5e_mod_hdr_handle *slow_mh; /* attached mod header instance for slow path */
> --
> 1.8.3.1
>

Re: [PATCH net-next 2/9] net/sched: act_pedit, setup offload action for action stats query

[Oz Shlomo]

On 03/02/2023 17:31, Marcelo Ricardo Leitner wrote:
> On Thu, Feb 02, 2023 at 09:21:15AM +0200, Oz Shlomo wrote:
>> On 01/02/2023 22:59, Pedro Tammela wrote:
>>> On 01/02/2023 13:10, Oz Shlomo wrote:
>>>> A single tc pedit action may be translated to multiple flow_offload
>>>> actions.
>>>> Offload only actions that translate to a single pedit command value.
>>>>
>>>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
>>>> ---
>>>>    net/sched/act_pedit.c | 24 +++++++++++++++++++++++-
>>>>    1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
>>>> index a0378e9f0121..abceef794f28 100644
>>>> --- a/net/sched/act_pedit.c
>>>> +++ b/net/sched/act_pedit.c
>>>> @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct
>>>> tc_action *act, void *entry_data,
>>>>            }
>>>>            *index_inc = k;
>>>>        } else {
>>>> -        return -EOPNOTSUPP;
>>>> +        struct flow_offload_action *fl_action = entry_data;
>>>> +        u32 last_cmd;
>>>> +        int k;
>>>> +
>>>> +        for (k = 0; k < tcf_pedit_nkeys(act); k++) {
>>>> +            u32 cmd = tcf_pedit_cmd(act, k);
>>>> +
>>>> +            if (k && cmd != last_cmd)
>>>> +                return -EOPNOTSUPP;
>>> I believe an extack message here is very valuable
>> Sure thing, I will add one
>>>> +
>>>> +            last_cmd = cmd;
>>>> +            switch (cmd) {
>>>> +            case TCA_PEDIT_KEY_EX_CMD_SET:
>>>> +                fl_action->id = FLOW_ACTION_MANGLE;
>>>> +                break;
>>>> +            case TCA_PEDIT_KEY_EX_CMD_ADD:
>>>> +                fl_action->id = FLOW_ACTION_ADD;
>>>> +                break;
>>>> +            default:
>>>> +                NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit
>>>> command offload");
>>>> +                return -EOPNOTSUPP;
>>>> +            }
>>>> +        }
>>> Shouldn't this switch case be outside of the for-loop?
>> You are right, this can be done outside the for loop.
> To before the for-loop, that is?
> Because otherwise it will parse all commands and then fail, which seems heavier
> than how it is here.
>
> - validate the first one
> - ensure the rest follows
Right
>> I will refactor the code
>>
>>>>        }
>>>>          return 0;

Re: [PATCH net-next 5/9] net/sched: support per action hw stats

[Oz Shlomo]

On 03/02/2023 16:33, Simon Horman wrote:
> On Wed, Feb 01, 2023 at 06:10:34PM +0200, Oz Shlomo wrote:
>> There are currently two mechanisms for populating hardware stats:
>> 1. Using flow_offload api to query the flow's statistics.
>>     The api assumes that the same stats values apply to all
>>     the flow's actions.
>>     This assumption breaks when action drops or jumps over following
>>     actions.
>> 2. Using hw_action api to query specific action stats via a driver
>>     callback method. This api assures the correct action stats for
>>     the offloaded action, however, it does not apply to the rest of the
>>     actions in the flow's actions array.
>>
>> Extend the flow_offload stats callback to indicate that a per action
>> stats update is required.
>> Use the existing flow_offload_action api to query the action's hw stats.
>> In addition, currently the tc action stats utility only updates hw actions.
>> Reuse the existing action stats cb infrastructure to query any action
>> stats.
>>
>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
> ...
>
>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>> index be21764a3b34..d4315757d1a2 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -292,9 +292,15 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>>   #define tcf_act_for_each_action(i, a, actions) \
>>   	for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++)
>>   
>> +static bool tc_act_in_hw(struct tc_action *act)
>> +{
>> +	return !!act->in_hw_count;
>> +}
>> +
>>   static inline void
>>   tcf_exts_hw_stats_update(const struct tcf_exts *exts,
>> -			 struct flow_stats *stats)
>> +			 struct flow_stats *stats,
>> +			 bool use_act_stats)
>>   {
>>   #ifdef CONFIG_NET_CLS_ACT
>>   	int i;
>> @@ -302,16 +308,18 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>>   	for (i = 0; i < exts->nr_actions; i++) {
>>   		struct tc_action *a = exts->actions[i];
>>   
>> -		/* if stats from hw, just skip */
>> -		if (tcf_action_update_hw_stats(a)) {
>> -			preempt_disable();
>> -			tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
>> -						stats->lastused, true);
>> -			preempt_enable();
>> -
>> -			a->used_hw_stats = stats->used_hw_stats;
>> -			a->used_hw_stats_valid = stats->used_hw_stats_valid;
>> +		if (use_act_stats || tc_act_in_hw(a)) {
>> +			tcf_action_update_hw_stats(a);
> Hi Oz,
>
> I am a unclear why it is ok to continue here even if
> tcf_action_update_hw_stats() fails.  There seem to be cases other than
> !tc_act_in_hw() at play here, which prior to this patch, would execute the
> code below that is now outside this if clause.
You're right, my bad
>> +			continue;
>>   		}
>> +
>> +		preempt_disable();
>> +		tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
>> +					stats->lastused, true);
>> +		preempt_enable();
>> +
>> +		a->used_hw_stats = stats->used_hw_stats;
>> +		a->used_hw_stats_valid = stats->used_hw_stats_valid;
>>   	}
>>   #endif
>>   }
>> @@ -769,6 +777,7 @@ struct tc_cls_matchall_offload {
>>   	enum tc_matchall_command command;
>>   	struct flow_rule *rule;
>>   	struct flow_stats stats;
>> +	bool use_act_stats;
>>   	unsigned long cookie;
>>   };
>>   
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 917827199102..eda58b78da13 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -169,11 +169,6 @@ static bool tc_act_skip_sw(u32 flags)
>>   	return (flags & TCA_ACT_FLAGS_SKIP_SW) ? true : false;
>>   }
>>   
>> -static bool tc_act_in_hw(struct tc_action *act)
>> -{
>> -	return !!act->in_hw_count;
>> -}
>> -
>>   /* SKIP_HW and SKIP_SW are mutually exclusive flags. */
>>   static bool tc_act_flags_valid(u32 flags)
>>   {
>> @@ -308,9 +303,6 @@ int tcf_action_update_hw_stats(struct tc_action *action)
>>   	struct flow_offload_action fl_act = {};
>>   	int err;
>>   
>> -	if (!tc_act_in_hw(action))
>> -		return -EOPNOTSUPP;
>> -
>>   	err = offload_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
>>   	if (err)
>>   		return err;
> ...

Re: [PATCH net-next 5/9] net/sched: support per action hw stats

[Oz Shlomo]

On 03/02/2023 16:51, Simon Horman wrote:
> On Wed, Feb 01, 2023 at 06:10:34PM +0200, Oz Shlomo wrote:
>> There are currently two mechanisms for populating hardware stats:
>> 1. Using flow_offload api to query the flow's statistics.
>>     The api assumes that the same stats values apply to all
>>     the flow's actions.
>>     This assumption breaks when action drops or jumps over following
>>     actions.
>> 2. Using hw_action api to query specific action stats via a driver
>>     callback method. This api assures the correct action stats for
>>     the offloaded action, however, it does not apply to the rest of the
>>     actions in the flow's actions array.
>>
>> Extend the flow_offload stats callback to indicate that a per action
>> stats update is required.
>> Use the existing flow_offload_action api to query the action's hw stats.
>> In addition, currently the tc action stats utility only updates hw actions.
>> Reuse the existing action stats cb infrastructure to query any action
>> stats.
>>
>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
>> ---
>>   include/net/flow_offload.h |  1 +
>>   include/net/pkt_cls.h      | 29 +++++++++++++++++++----------
>>   net/sched/act_api.c        |  8 --------
>>   net/sched/cls_flower.c     |  2 +-
>>   net/sched/cls_matchall.c   |  2 +-
>>   5 files changed, 22 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> index d177bf5f0e1a..27decadd4f5f 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -597,6 +597,7 @@ struct flow_cls_offload {
>>   	unsigned long cookie;
>>   	struct flow_rule *rule;
>>   	struct flow_stats stats;
>> +	bool use_act_stats;
>>   	u32 classid;
>>   };
> Hi Oz,
>
> It's probably not important, but I thought it is worth bringing
> to your attention.
>
> The placement of use_act_stats above  puts it on a different
> cacheline (on x86_64) to stats. Which does not seem to be idea
> as those fields are accessed together.
>
> There is a 4 byte hole immediately above above cookie,
> on the same cacheline as stats, which can accommodate use_act_stats.

Good catch.

I will move use_act_stats for better cache alignment.

>>   
>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>> index be21764a3b34..d4315757d1a2 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
> ...
>
>> @@ -769,6 +777,7 @@ struct tc_cls_matchall_offload {
>>   	enum tc_matchall_command command;
>>   	struct flow_rule *rule;
>>   	struct flow_stats stats;
>> +	bool use_act_stats;
>>   	unsigned long cookie;
>>   };
> I believe the same logic applies to this change too.
>
> ...
>
>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index cb04739a13ce..885c95191ccf 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -502,7 +502,7 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
>>   	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
>>   			 rtnl_held);
>>   
>> -	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
>> +	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats, cls_flower.use_act_stats);
>>   }
>>   
>>   static void __fl_put(struct cls_fl_filter *f)
>> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
>> index b3883d3d4dbd..fa3bbd187eb9 100644
>> --- a/net/sched/cls_matchall.c
>> +++ b/net/sched/cls_matchall.c
>> @@ -331,7 +331,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
>>   
>>   	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
>>   
>> -	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
>> +	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats, cls_mall.use_act_stats);
>>   }
>>   
>>   static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,

Re: [PATCH net-next 7/9] net/mlx5e: TC, store tc action cookies per attr

[Oz Shlomo]

On 03/02/2023 18:11, Marcelo Ricardo Leitner wrote:
> On Wed, Feb 01, 2023 at 06:10:36PM +0200, Oz Shlomo wrote:
>> The tc parse action phase translates the tc actions to mlx5 flow
>> attributes data structure that is used during the flow offload phase.
>> Currently, the flow offload stage instantiates hw counters while
>> associating them to flow cookie. However, flows with branching
>> actions are required to associate a hardware counter with its action
>> cookies.
>>
>> Store the parsed tc action cookies on the flow attribute.
>> Use the list of cookies in the next patch to associate a tc action cookie
>> with its allocated hw counter.
>>
>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 3 +++
>>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.h | 2 ++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> index 39f75f7d5c8b..a5118da3ed6c 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> @@ -3797,6 +3797,7 @@ bool mlx5e_same_hw_devs(struct mlx5e_priv *priv, struct mlx5e_priv *peer_priv)
>>   	parse_attr->filter_dev = attr->parse_attr->filter_dev;
>>   	attr2->action = 0;
>>   	attr2->counter = NULL;
>> +	attr->tc_act_cookies_count = 0;
>>   	attr2->flags = 0;
>>   	attr2->parse_attr = parse_attr;
>>   	attr2->dest_chain = 0;
>> @@ -4160,6 +4161,8 @@ struct mlx5_flow_attr *
>>   			goto out_free;
>>
>>   		parse_state->actions |= attr->action;
>> +		if (!tc_act->stats_action)
>> +			attr->tc_act_cookies[attr->tc_act_cookies_count++] = act->act_cookie;
>>
>>   		/* Split attr for multi table act if not the last act. */
>>   		if (jump_state.jump_target ||
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
>> index ce516dc7f3fd..8aa25d8bac86 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
>> @@ -70,6 +70,8 @@ struct mlx5_nic_flow_attr {
>>   struct mlx5_flow_attr {
>>   	u32 action;
>>   	struct mlx5_fc *counter;
>> +	unsigned long tc_act_cookies[TCA_ACT_MAX_PRIO];
>> +	int tc_act_cookies_count;
> This one won't count much, as it is limited by TCA_ACT_MAX_PRIO above
> andi which is 32.
> Maybe this can be an u8 or u16 instead and be added together with 'prio'?
> To save 2 bytes, yes, but with a 1M flows, that's 2Mbytes.
> Or below 'action' above, to keep it on the same cache line.
I agree
>>   	struct mlx5_modify_hdr *modify_hdr;
>>   	struct mlx5e_mod_hdr_handle *mh; /* attached mod header instance */
>>   	struct mlx5e_mod_hdr_handle *slow_mh; /* attached mod header instance for slow path */
>> --
>> 1.8.3.1
>>