[PATCH net-next 0/7] tc: Introduce a trap-and-forward action

[PATCH net-next 0/7] tc: Introduce a trap-and-forward action

[Petr Machata]

The TC action "trap" is used to instruct the HW datapath to drop the
matched packet and transfer it to the host for processing in the SW
pipeline. If instead it is desirable to forward the packet in the HW
datapath, and to transfer a _copy_ to the SW pipeline, there is no
practical way to achieve that.

As a particular use case, the mlxsw driver could instruct a Spectrum
machine to mirror packets that are ECN-marked to the host. However these
packets are still forwarded in the HW datapath, therefore describing this
mirroring through the "trap" action is incorrect. A new action is needed.

To that end, this patchset introduces a new generic action, trap_fwd. In
the software pipeline, it is equivalent to an OK. When offloading, it
should forward the packet to the host, but unlike trap it should not drop
the packet.

This patchset proceeds as follows:

- In patch #1, introduce the new action, and modify the TC code to
  recognize it as an OK.

- In patches #2 and #3, introduce the artifacts necessary for offloading
  the trap_fwd action, and a new trap so that drivers can report the
  trapped packets.

- Patches #4 and #5 offload trap_fwd in mlxsw.

- Patches #6 and #7 add selftests.

Petr Machata (7):
  net: sched: Add a trap-and-forward action
  net: sched: Make the action trap_fwd offloadable
  devlink: Add a new trap for the trap_fwd action
  mlxsw: Propagate extack to mlxsw_afa_block_commit()
  mlxsw: Offload trap_fwd
  selftests: forwarding: Add a test for TC trapping behavior
  selftests: mlxsw: Add a trap_fwd test to devlink_trap_control

 .../networking/devlink/devlink-trap.rst       |   4 +
 .../mellanox/mlxsw/core_acl_flex_actions.c    |  28 ++-
 .../mellanox/mlxsw/core_acl_flex_actions.h    |   3 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   4 +-
 .../mellanox/mlxsw/spectrum1_acl_tcam.c       |   2 +-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    |  11 +-
 .../ethernet/mellanox/mlxsw/spectrum_flower.c |   9 +-
 .../mellanox/mlxsw/spectrum_mr_tcam.c         |   2 +-
 .../ethernet/mellanox/mlxsw/spectrum_trap.c   |   8 +
 drivers/net/ethernet/mellanox/mlxsw/trap.h    |   2 +
 include/net/devlink.h                         |   3 +
 include/net/flow_offload.h                    |   1 +
 include/net/tc_act/tc_gact.h                  |   5 +
 include/uapi/linux/pkt_cls.h                  |   6 +-
 net/core/dev.c                                |   2 +
 net/core/devlink.c                            |   1 +
 net/sched/act_bpf.c                           |  13 +-
 net/sched/cls_api.c                           |   2 +
 net/sched/cls_bpf.c                           |   1 +
 net/sched/sch_dsmark.c                        |   1 +
 tools/include/uapi/linux/pkt_cls.h            |   6 +-
 .../drivers/net/mlxsw/devlink_trap_control.sh |  23 ++-
 .../selftests/net/forwarding/tc_trap.sh       | 170 ++++++++++++++++++
 23 files changed, 288 insertions(+), 19 deletions(-)
 create mode 100755 tools/testing/selftests/net/forwarding/tc_trap.sh

-- 
2.26.2

[PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Petr Machata]

The TC action "trap" is used to instruct the HW datapath to drop the
matched packet and transfer it for processing in the SW pipeline. If
instead it is desirable to forward the packet and transferring a _copy_ to
the SW pipeline, there is no practical way to achieve that.

To that end add a new generic action, trap_fwd. In the software pipeline,
it is equivalent to an OK. When offloading, it should forward the packet to
the host, but unlike trap it should not drop the packet.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 include/uapi/linux/pkt_cls.h       |  6 +++++-
 net/core/dev.c                     |  2 ++
 net/sched/act_bpf.c                | 13 +++++++++++--
 net/sched/cls_bpf.c                |  1 +
 net/sched/sch_dsmark.c             |  1 +
 tools/include/uapi/linux/pkt_cls.h |  6 +++++-
 6 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 025c40fef93d..a1bbccb88e67 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -72,7 +72,11 @@ enum {
 				   * the skb and act like everything
 				   * is alright.
 				   */
-#define TC_ACT_VALUE_MAX	TC_ACT_TRAP
+#define TC_ACT_TRAP_FWD		9 /* For hw path, this means "send a copy
+				   * of the packet to the cpu". For sw
+				   * datapath, this is like TC_ACT_OK.
+				   */
+#define TC_ACT_VALUE_MAX	TC_ACT_TRAP_FWD
 
 /* There is a special kind of actions called "extended actions",
  * which need a value parameter. These have a local opcode located in
diff --git a/net/core/dev.c b/net/core/dev.c
index 9d1a8fac793f..f0b8c16dbf12 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3975,6 +3975,7 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
 	switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {
 	case TC_ACT_OK:
 	case TC_ACT_RECLASSIFY:
+	case TC_ACT_TRAP_FWD:
 		skb->tc_index = TC_H_MIN(cl_res.classid);
 		break;
 	case TC_ACT_SHOT:
@@ -5083,6 +5084,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 				     &cl_res, false)) {
 	case TC_ACT_OK:
 	case TC_ACT_RECLASSIFY:
+	case TC_ACT_TRAP_FWD:
 		skb->tc_index = TC_H_MIN(cl_res.classid);
 		break;
 	case TC_ACT_SHOT:
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index e48e980c3b93..be2a51c6f84e 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -54,8 +54,16 @@ static int tcf_bpf_act(struct sk_buff *skb, const struct tc_action *act,
 		bpf_compute_data_pointers(skb);
 		filter_res = BPF_PROG_RUN(filter, skb);
 	}
-	if (skb_sk_is_prefetched(skb) && filter_res != TC_ACT_OK)
-		skb_orphan(skb);
+	if (skb_sk_is_prefetched(skb)) {
+		switch (filter_res) {
+		case TC_ACT_OK:
+		case TC_ACT_TRAP_FWD:
+			break;
+		default:
+			skb_orphan(skb);
+			break;
+		}
+	}
 	rcu_read_unlock();
 
 	/* A BPF program may overwrite the default action opcode.
@@ -72,6 +80,7 @@ static int tcf_bpf_act(struct sk_buff *skb, const struct tc_action *act,
 	case TC_ACT_PIPE:
 	case TC_ACT_RECLASSIFY:
 	case TC_ACT_OK:
+	case TC_ACT_TRAP_FWD:
 	case TC_ACT_REDIRECT:
 		action = filter_res;
 		break;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 6e3e63db0e01..5fd96cf2dca7 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -69,6 +69,7 @@ static int cls_bpf_exec_opcode(int code)
 	case TC_ACT_SHOT:
 	case TC_ACT_STOLEN:
 	case TC_ACT_TRAP:
+	case TC_ACT_TRAP_FWD:
 	case TC_ACT_REDIRECT:
 	case TC_ACT_UNSPEC:
 		return code;
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index cd2748e2d4a2..054a06bd9dc8 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -258,6 +258,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			goto drop;
 #endif
 		case TC_ACT_OK:
+		case TC_ACT_TRAP_FWD:
 			skb->tc_index = TC_H_MIN(res.classid);
 			break;
 
diff --git a/tools/include/uapi/linux/pkt_cls.h b/tools/include/uapi/linux/pkt_cls.h
index 12153771396a..ccfa424dfeaf 100644
--- a/tools/include/uapi/linux/pkt_cls.h
+++ b/tools/include/uapi/linux/pkt_cls.h
@@ -45,7 +45,11 @@ enum {
 				   * the skb and act like everything
 				   * is alright.
 				   */
-#define TC_ACT_VALUE_MAX	TC_ACT_TRAP
+#define TC_ACT_TRAP_FWD		9 /* For hw path, this means "send a copy
+				   * of the packet to the cpu". For sw
+				   * datapath, this is like TC_ACT_OK.
+				   */
+#define TC_ACT_VALUE_MAX	TC_ACT_TRAP_FWD
 
 /* There is a special kind of actions called "extended actions",
  * which need a value parameter. These have a local opcode located in
-- 
2.26.2

[PATCH net-next 2/7] net: sched: Make the action trap_fwd offloadable

[Petr Machata]

Add the new flow action and related support so that drivers can offload the
trap_fwd action.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 include/net/flow_offload.h   | 1 +
 include/net/tc_act/tc_gact.h | 5 +++++
 net/sched/cls_api.c          | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index dc5c1e69cd9f..5f35523f12b5 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -121,6 +121,7 @@ enum flow_action_id {
 	FLOW_ACTION_ACCEPT		= 0,
 	FLOW_ACTION_DROP,
 	FLOW_ACTION_TRAP,
+	FLOW_ACTION_TRAP_FWD,
 	FLOW_ACTION_GOTO,
 	FLOW_ACTION_REDIRECT,
 	FLOW_ACTION_MIRRED,
diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h
index eb8f01c819e6..df9e0a19c826 100644
--- a/include/net/tc_act/tc_gact.h
+++ b/include/net/tc_act/tc_gact.h
@@ -49,6 +49,11 @@ static inline bool is_tcf_gact_trap(const struct tc_action *a)
 	return __is_tcf_gact_act(a, TC_ACT_TRAP, false);
 }
 
+static inline bool is_tcf_gact_trap_fwd(const struct tc_action *a)
+{
+	return __is_tcf_gact_act(a, TC_ACT_TRAP_FWD, false);
+}
+
 static inline bool is_tcf_gact_goto_chain(const struct tc_action *a)
 {
 	return __is_tcf_gact_act(a, TC_ACT_GOTO_CHAIN, true);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d3db70865d66..95e37eb50173 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3582,6 +3582,8 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 			entry->id = FLOW_ACTION_DROP;
 		} else if (is_tcf_gact_trap(act)) {
 			entry->id = FLOW_ACTION_TRAP;
+		} else if (is_tcf_gact_trap_fwd(act)) {
+			entry->id = FLOW_ACTION_TRAP_FWD;
 		} else if (is_tcf_gact_goto_chain(act)) {
 			entry->id = FLOW_ACTION_GOTO;
 			entry->chain_index = tcf_gact_goto_chain_index(act);
-- 
2.26.2

[PATCH net-next 3/7] devlink: Add a new trap for the trap_fwd action

[Petr Machata]

Add a new trap so that drivers can report packets forwarded due to the
trap_fwd action correctly.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/devlink/devlink-trap.rst | 4 ++++
 include/net/devlink.h                             | 3 +++
 net/core/devlink.c                                | 1 +
 3 files changed, 8 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-trap.rst b/Documentation/networking/devlink/devlink-trap.rst
index 935b6397e8cf..3f1c0f89d284 100644
--- a/Documentation/networking/devlink/devlink-trap.rst
+++ b/Documentation/networking/devlink/devlink-trap.rst
@@ -405,6 +405,10 @@ be added to the following table:
      - ``control``
      - Traps packets logged during processing of flow action trap (e.g., via
        tc's trap action)
+   * - ``flow_action_trap_fwd``
+     - ``control``
+     - Traps packets logged during processing of flow action trap_fwd (e.g., via
+       tc's trap_fwd action)
    * - ``early_drop``
      - ``drop``
      - Traps packets dropped due to the RED (Random Early Detection) algorithm
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 853420db5d32..967e70363ba9 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -845,6 +845,7 @@ enum devlink_trap_generic_id {
 	DEVLINK_TRAP_GENERIC_ID_PTP_GENERAL,
 	DEVLINK_TRAP_GENERIC_ID_FLOW_ACTION_SAMPLE,
 	DEVLINK_TRAP_GENERIC_ID_FLOW_ACTION_TRAP,
+	DEVLINK_TRAP_GENERIC_ID_FLOW_ACTION_TRAP_FWD,
 	DEVLINK_TRAP_GENERIC_ID_EARLY_DROP,
 	DEVLINK_TRAP_GENERIC_ID_VXLAN_PARSING,
 	DEVLINK_TRAP_GENERIC_ID_LLC_SNAP_PARSING,
@@ -1053,6 +1054,8 @@ enum devlink_trap_group_generic_id {
 	"flow_action_sample"
 #define DEVLINK_TRAP_GENERIC_NAME_FLOW_ACTION_TRAP \
 	"flow_action_trap"
+#define DEVLINK_TRAP_GENERIC_NAME_FLOW_ACTION_TRAP_FWD \
+	"flow_action_trap_fwd"
 #define DEVLINK_TRAP_GENERIC_NAME_EARLY_DROP \
 	"early_drop"
 #define DEVLINK_TRAP_GENERIC_NAME_VXLAN_PARSING \
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 737b61c2976e..478d4bc01a39 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -9744,6 +9744,7 @@ static const struct devlink_trap devlink_trap_generic[] = {
 	DEVLINK_TRAP(PTP_GENERAL, CONTROL),
 	DEVLINK_TRAP(FLOW_ACTION_SAMPLE, CONTROL),
 	DEVLINK_TRAP(FLOW_ACTION_TRAP, CONTROL),
+	DEVLINK_TRAP(FLOW_ACTION_TRAP_FWD, CONTROL),
 	DEVLINK_TRAP(EARLY_DROP, DROP),
 	DEVLINK_TRAP(VXLAN_PARSING, DROP),
 	DEVLINK_TRAP(LLC_SNAP_PARSING, DROP),
-- 
2.26.2

[PATCH net-next 4/7] mlxsw: Propagate extack to mlxsw_afa_block_commit()

[Petr Machata]

In the following patch, attempts to change the next/goto of a flexible
action set from goto to next will be rejected for action sets that contain
a trap_fwd action. Propagate extack to make it possible to communicate the
issue to the user.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 .../net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c  | 9 ++++++---
 .../net/ethernet/mellanox/mlxsw/core_acl_flex_actions.h  | 3 ++-
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h           | 3 ++-
 drivers/net/ethernet/mellanox/mlxsw/spectrum1_acl_tcam.c | 2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c       | 5 +++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c    | 2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_mr_tcam.c   | 2 +-
 7 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c b/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
index 78d9c0196f2b..faa90cc31376 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
@@ -264,7 +264,8 @@ static void mlxsw_afa_set_goto_set(struct mlxsw_afa_set *set,
 }
 
 static void mlxsw_afa_set_next_set(struct mlxsw_afa_set *set,
-				   u32 next_set_kvdl_index)
+				  u32 next_set_kvdl_index,
+				  struct netlink_ext_ack *extack)
 {
 	char *actions = set->ht_key.enc_actions;
 
@@ -455,7 +456,8 @@ void mlxsw_afa_block_destroy(struct mlxsw_afa_block *block)
 }
 EXPORT_SYMBOL(mlxsw_afa_block_destroy);
 
-int mlxsw_afa_block_commit(struct mlxsw_afa_block *block)
+int mlxsw_afa_block_commit(struct mlxsw_afa_block *block,
+			   struct netlink_ext_ack *extack)
 {
 	struct mlxsw_afa_set *set = block->cur_set;
 	struct mlxsw_afa_set *prev_set;
@@ -479,7 +481,8 @@ int mlxsw_afa_block_commit(struct mlxsw_afa_block *block)
 			return PTR_ERR(set);
 		if (prev_set) {
 			prev_set->next = set;
-			mlxsw_afa_set_next_set(prev_set, set->kvdl_index);
+			mlxsw_afa_set_next_set(prev_set, set->kvdl_index,
+					       extack);
 			set = prev_set;
 		}
 	} while (prev_set);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.h b/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.h
index b65bf98eb5ab..24350f9470f8 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.h
@@ -45,7 +45,8 @@ struct mlxsw_afa *mlxsw_afa_create(unsigned int max_acts_per_set,
 void mlxsw_afa_destroy(struct mlxsw_afa *mlxsw_afa);
 struct mlxsw_afa_block *mlxsw_afa_block_create(struct mlxsw_afa *mlxsw_afa);
 void mlxsw_afa_block_destroy(struct mlxsw_afa_block *block);
-int mlxsw_afa_block_commit(struct mlxsw_afa_block *block);
+int mlxsw_afa_block_commit(struct mlxsw_afa_block *block,
+			   struct netlink_ext_ack *extack);
 char *mlxsw_afa_block_first_set(struct mlxsw_afa_block *block);
 char *mlxsw_afa_block_cur_set(struct mlxsw_afa_block *block);
 u32 mlxsw_afa_block_first_kvdl_index(struct mlxsw_afa_block *block);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index f99db88ee884..d74fc7ff8083 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -920,7 +920,8 @@ struct mlxsw_sp_acl_rule_info *
 mlxsw_sp_acl_rulei_create(struct mlxsw_sp_acl *acl,
 			  struct mlxsw_afa_block *afa_block);
 void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp_acl_rule_info *rulei);
-int mlxsw_sp_acl_rulei_commit(struct mlxsw_sp_acl_rule_info *rulei);
+int mlxsw_sp_acl_rulei_commit(struct mlxsw_sp_acl_rule_info *rulei,
+			      struct netlink_ext_ack *extack);
 void mlxsw_sp_acl_rulei_priority(struct mlxsw_sp_acl_rule_info *rulei,
 				 unsigned int priority);
 void mlxsw_sp_acl_rulei_keymask_u32(struct mlxsw_sp_acl_rule_info *rulei,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_acl_tcam.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_acl_tcam.c
index 3a636f753607..cda04bc4453f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_acl_tcam.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_acl_tcam.c
@@ -75,7 +75,7 @@ mlxsw_sp1_acl_ctcam_region_catchall_add(struct mlxsw_sp *mlxsw_sp,
 	err = mlxsw_sp_acl_rulei_act_continue(rulei);
 	if (WARN_ON(err))
 		goto err_rulei_act_continue;
-	err = mlxsw_sp_acl_rulei_commit(rulei);
+	err = mlxsw_sp_acl_rulei_commit(rulei, NULL);
 	if (err)
 		goto err_rulei_commit;
 	err = mlxsw_sp_acl_ctcam_entry_add(mlxsw_sp, &region->cregion,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
index 67cedfa76f78..b9c4c1feba6d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
@@ -341,9 +341,10 @@ void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp_acl_rule_info *rulei)
 	kfree(rulei);
 }
 
-int mlxsw_sp_acl_rulei_commit(struct mlxsw_sp_acl_rule_info *rulei)
+int mlxsw_sp_acl_rulei_commit(struct mlxsw_sp_acl_rule_info *rulei,
+			      struct netlink_ext_ack *extack)
 {
-	return mlxsw_afa_block_commit(rulei->act_block);
+	return mlxsw_afa_block_commit(rulei->act_block, extack);
 }
 
 void mlxsw_sp_acl_rulei_priority(struct mlxsw_sp_acl_rule_info *rulei,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index be3791ca6069..936788f741dd 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -611,7 +611,7 @@ int mlxsw_sp_flower_replace(struct mlxsw_sp *mlxsw_sp,
 	if (err)
 		goto err_flower_parse;
 
-	err = mlxsw_sp_acl_rulei_commit(rulei);
+	err = mlxsw_sp_acl_rulei_commit(rulei, f->common.extack);
 	if (err)
 		goto err_rulei_commit;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_mr_tcam.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_mr_tcam.c
index 221aa6a474eb..f81e8d25987b 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_mr_tcam.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_mr_tcam.c
@@ -241,7 +241,7 @@ mlxsw_sp_mr_tcam_afa_block_create(struct mlxsw_sp *mlxsw_sp,
 		goto err;
 	}
 
-	err = mlxsw_afa_block_commit(afa_block);
+	err = mlxsw_afa_block_commit(afa_block, NULL);
 	if (err)
 		goto err;
 	return afa_block;
-- 
2.26.2

[PATCH net-next 5/7] mlxsw: Offload trap_fwd

[Petr Machata]

Offload the TC action trap_fwd. This is offloaded as a TRAP_ACTION with
forward_action of FORWARD (as opposed to NOP for the trap action). Unlike
trap, trap_fwd needs to be in an "goto"-typed action set, not "next"-typed
one.

Trap_fwd'd traffic is marked with offload_fwd_mark and offload_l3_fwd_mark
to prevent second forwarding in the SW datapath.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 .../mellanox/mlxsw/core_acl_flex_actions.c    | 23 +++++++++++++++----
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  1 +
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    |  6 +++++
 .../ethernet/mellanox/mlxsw/spectrum_flower.c |  7 ++++++
 .../ethernet/mellanox/mlxsw/spectrum_trap.c   |  8 +++++++
 drivers/net/ethernet/mellanox/mlxsw/trap.h    |  2 ++
 6 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c b/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
index faa90cc31376..d7d7e688139f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
@@ -94,7 +94,8 @@ struct mlxsw_afa_set {
 		      * kvdl_index is valid).
 		      */
 	   has_trap:1,
-	   has_police:1;
+	   has_police:1,
+	   has_trap_fwd:1;
 	unsigned int ref_count;
 	struct mlxsw_afa_set *next; /* Pointer to the next set. */
 	struct mlxsw_afa_set *prev; /* Pointer to the previous set,
@@ -263,14 +264,23 @@ static void mlxsw_afa_set_goto_set(struct mlxsw_afa_set *set,
 	mlxsw_afa_set_goto_next_binding_set(actions, group_id);
 }
 
-static void mlxsw_afa_set_next_set(struct mlxsw_afa_set *set,
+static int mlxsw_afa_set_next_set(struct mlxsw_afa_set *set,
 				  u32 next_set_kvdl_index,
 				  struct netlink_ext_ack *extack)
 {
 	char *actions = set->ht_key.enc_actions;
 
+	/* If the forwarding action is not drop, the next/goto record must not
+	 * be a next, it must be a goto.
+	 */
+	if (set->has_trap_fwd) {
+		NL_SET_ERR_MSG_MOD(extack, "Only goto permissible after a trap_fwd action");
+		return -EINVAL;
+	}
+
 	mlxsw_afa_set_type_set(actions, MLXSW_AFA_SET_TYPE_NEXT);
 	mlxsw_afa_set_next_action_set_ptr_set(actions, next_set_kvdl_index);
+	return 0;
 }
 
 static struct mlxsw_afa_set *mlxsw_afa_set_create(bool is_first)
@@ -461,6 +471,7 @@ int mlxsw_afa_block_commit(struct mlxsw_afa_block *block,
 {
 	struct mlxsw_afa_set *set = block->cur_set;
 	struct mlxsw_afa_set *prev_set;
+	int err;
 
 	block->cur_set = NULL;
 	block->finished = true;
@@ -481,8 +492,10 @@ int mlxsw_afa_block_commit(struct mlxsw_afa_block *block,
 			return PTR_ERR(set);
 		if (prev_set) {
 			prev_set->next = set;
-			mlxsw_afa_set_next_set(prev_set, set->kvdl_index,
-					       extack);
+			err = mlxsw_afa_set_next_set(prev_set, set->kvdl_index,
+						     extack);
+			if (err)
+				return err;
 			set = prev_set;
 		}
 	} while (prev_set);
@@ -1346,6 +1359,8 @@ int mlxsw_afa_block_append_trap_and_forward(struct mlxsw_afa_block *block,
 
 	if (IS_ERR(act))
 		return PTR_ERR(act);
+
+	block->cur_set->has_trap_fwd = true;
 	mlxsw_afa_trap_pack(act, MLXSW_AFA_TRAP_TRAP_ACTION_TRAP,
 			    MLXSW_AFA_TRAP_FORWARD_ACTION_FORWARD, trap_id);
 	return 0;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index d74fc7ff8083..6067a049dcf2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -940,6 +940,7 @@ int mlxsw_sp_acl_rulei_act_drop(struct mlxsw_sp_acl_rule_info *rulei,
 				const struct flow_action_cookie *fa_cookie,
 				struct netlink_ext_ack *extack);
 int mlxsw_sp_acl_rulei_act_trap(struct mlxsw_sp_acl_rule_info *rulei);
+int mlxsw_sp_acl_rulei_act_trap_fwd(struct mlxsw_sp_acl_rule_info *rulei);
 int mlxsw_sp_acl_rulei_act_mirror(struct mlxsw_sp *mlxsw_sp,
 				  struct mlxsw_sp_acl_rule_info *rulei,
 				  struct mlxsw_sp_flow_block *block,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
index b9c4c1feba6d..6f7913424bd9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
@@ -401,6 +401,12 @@ int mlxsw_sp_acl_rulei_act_trap(struct mlxsw_sp_acl_rule_info *rulei)
 					   MLXSW_TRAP_ID_ACL0);
 }
 
+int mlxsw_sp_acl_rulei_act_trap_fwd(struct mlxsw_sp_acl_rule_info *rulei)
+{
+	return mlxsw_afa_block_append_trap_and_forward(rulei->act_block,
+						       MLXSW_TRAP_ID_ACL3);
+}
+
 int mlxsw_sp_acl_rulei_act_fwd(struct mlxsw_sp *mlxsw_sp,
 			       struct mlxsw_sp_acl_rule_info *rulei,
 			       struct net_device *out_dev,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 936788f741dd..1f52ea7ba202 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -86,6 +86,13 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 				return err;
 			}
 			break;
+		case FLOW_ACTION_TRAP_FWD:
+			err = mlxsw_sp_acl_rulei_act_trap_fwd(rulei);
+			if (err) {
+				NL_SET_ERR_MSG_MOD(extack, "Cannot append trap_fwd action");
+				return err;
+			}
+			break;
 		case FLOW_ACTION_GOTO: {
 			u32 chain_index = act->chain_index;
 			struct mlxsw_sp_acl_ruleset *ruleset;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c
index 26d01adbedad..504fb7440a1f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c
@@ -1154,6 +1154,14 @@ static const struct mlxsw_sp_trap_item mlxsw_sp_trap_items_arr[] = {
 					     false),
 		},
 	},
+	{
+		.trap = MLXSW_SP_TRAP_CONTROL(FLOW_ACTION_TRAP_FWD,
+					      ACL_TRAP, MIRROR),
+		.listeners_arr = {
+			MLXSW_SP_RXL_L3_MARK(ACL3, FLOW_LOGGING, MIRROR_TO_CPU,
+					     false),
+		},
+	},
 	{
 		.trap = MLXSW_SP_TRAP_DROP(BLACKHOLE_NEXTHOP, L3_DROPS),
 		.listeners_arr = {
diff --git a/drivers/net/ethernet/mellanox/mlxsw/trap.h b/drivers/net/ethernet/mellanox/mlxsw/trap.h
index 9e070ab3ed76..5271d7ad092a 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/trap.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/trap.h
@@ -108,6 +108,8 @@ enum {
 	MLXSW_TRAP_ID_ACL2 = 0x1C2,
 	MLXSW_TRAP_ID_DISCARD_INGRESS_ACL = 0x1C3,
 	MLXSW_TRAP_ID_DISCARD_EGRESS_ACL = 0x1C4,
+	/* Packets trapped due to FLOW_ACTION_TRAP_FWD. */
+	MLXSW_TRAP_ID_ACL3 = 0x1C5,
 	MLXSW_TRAP_ID_MIRROR_SESSION0 = 0x220,
 	MLXSW_TRAP_ID_MIRROR_SESSION1 = 0x221,
 	MLXSW_TRAP_ID_MIRROR_SESSION2 = 0x222,
-- 
2.26.2

[PATCH net-next 6/7] selftests: forwarding: Add a test for TC trapping behavior

[Petr Machata]

Test that trapped packets are forwarded through the SW datapath, whereas
trap_fwd'd ones are not (but are forwarded through HW datapath). For
completeness' sake, also test that "pass" (i.e. lack of trapping) simply
forwards the packets in the HW datapath.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 .../selftests/net/forwarding/tc_trap.sh       | 170 ++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/tc_trap.sh

diff --git a/tools/testing/selftests/net/forwarding/tc_trap.sh b/tools/testing/selftests/net/forwarding/tc_trap.sh
new file mode 100755
index 000000000000..56336cea45a2
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/tc_trap.sh
@@ -0,0 +1,170 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# In the following simple routing scenario, put SW datapath packet probes on
+# $swp1, $swp2 and $h2. Always expect packets to arrive at $h2. Depending on
+# whether, in the HW datapath, $swp1 lets packets pass, traps them, or
+# traps_forwards them, $swp1 and $swp2 probes are expected to give different
+# results.
+#
+# +----------------------+                             +----------------------+
+# | H1                   |                             |                   H2 |
+# |    + $h1             |                             |            $h2 +     |
+# |    | 192.0.2.1/28    |                             |  192.0.2.18/28 |     |
+# +----|-----------------+                             +----------------|-----+
+#      |                                                                |
+# +----|----------------------------------------------------------------|-----+
+# | SW |                                                                |     |
+# |    + $swp1                                                    $swp2 +     |
+# |      192.0.2.2/28                                     192.0.2.17/28       |
+# +---------------------------------------------------------------------------+
+
+
+ALL_TESTS="
+	no_trap_test
+	trap_fwd_test
+	trap_test
+"
+
+NUM_NETIFS=4
+source lib.sh
+source tc_common.sh
+
+h1_create()
+{
+	simple_if_init $h1 192.0.2.1/28
+	ip route add vrf v$h1 192.0.2.16/28 via 192.0.2.2
+}
+
+h1_destroy()
+{
+	ip route del vrf v$h1 192.0.2.16/28 via 192.0.2.2
+	simple_if_fini $h1 192.0.2.1/28
+}
+
+h2_create()
+{
+	simple_if_init $h2 192.0.2.18/28
+	ip route add vrf v$h2 192.0.2.0/28 via 192.0.2.17
+	tc qdisc add dev $h2 clsact
+}
+
+h2_destroy()
+{
+	tc qdisc del dev $h2 clsact
+	ip route del vrf v$h2 192.0.2.0/28 via 192.0.2.17
+	simple_if_fini $h2 192.0.2.18/28
+}
+
+switch_create()
+{
+	simple_if_init $swp1 192.0.2.2/28
+	__simple_if_init $swp2 v$swp1 192.0.2.17/28
+
+	tc qdisc add dev $swp1 clsact
+	tc qdisc add dev $swp2 clsact
+}
+
+switch_destroy()
+{
+	tc qdisc del dev $swp2 clsact
+	tc qdisc del dev $swp1 clsact
+
+	__simple_if_fini $swp2 192.0.2.17/28
+	simple_if_fini $swp1 192.0.2.2/28
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	swp1=${NETIFS[p2]}
+
+	swp2=${NETIFS[p3]}
+	h2=${NETIFS[p4]}
+
+	vrf_prepare
+	forwarding_enable
+
+	h1_create
+	h2_create
+	switch_create
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	switch_destroy
+	h2_destroy
+	h1_destroy
+
+	forwarding_restore
+	vrf_cleanup
+}
+
+__test()
+{
+	local action=$1; shift
+	local ingress_should_fail=$1; shift
+	local egress_should_fail=$1; shift
+
+	tc filter add dev $swp1 ingress protocol ip pref 2 handle 101 \
+		flower skip_sw dst_ip 192.0.2.18 action $action
+	tc filter add dev $swp1 ingress protocol ip pref 1 handle 102 \
+		flower skip_hw dst_ip 192.0.2.18 action pass
+	tc filter add dev $swp2 egress protocol ip pref 1 handle 103 \
+		flower skip_hw dst_ip 192.0.2.18 action pass
+	tc filter add dev $h2 ingress protocol ip pref 1 handle 104 \
+		flower dst_ip 192.0.2.18 action drop
+
+	RET=0
+
+	$MZ $h1 -c 1 -p 64 -a $(mac_get $h1) -b $(mac_get $swp1) \
+		-A 192.0.2.1 -B 192.0.2.18 -q -t ip
+
+	tc_check_packets "dev $swp1 ingress" 102 1
+	check_err_fail $ingress_should_fail $? "ingress should_fail $ingress_should_fail"
+
+	tc_check_packets "dev $swp2 egress" 103 1
+	check_err_fail $egress_should_fail $? "egress should_fail $egress_should_fail"
+
+	tc_check_packets "dev $h2 ingress" 104 1
+	check_err $? "Did not see the packet on host"
+
+	log_test "$action test"
+
+	tc filter del dev $h2 ingress protocol ip pref 1 handle 104 flower
+	tc filter del dev $swp2 egress protocol ip pref 1 handle 103 flower
+	tc filter del dev $swp1 ingress protocol ip pref 1 handle 102 flower
+	tc filter del dev $swp1 ingress protocol ip pref 2 handle 101 flower
+}
+
+no_trap_test()
+{
+	__test pass 1 1
+}
+
+trap_fwd_test()
+{
+	__test trap_fwd 0 1
+}
+
+trap_test()
+{
+	__test trap 0 0
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+if ! tc_offload_check; then
+	check_err 1 "Could not test offloaded functionality"
+	log_test "offloaded tc_trap test"
+	exit
+fi
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.26.2

[PATCH net-next 7/7] selftests: mlxsw: Add a trap_fwd test to devlink_trap_control

[Petr Machata]

Test that trap_fwd'd packets show up under the correct trap.

Signed-off-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 .../drivers/net/mlxsw/devlink_trap_control.sh | 23 ++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh b/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh
index a37273473c1b..8bca4c58819b 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh
@@ -83,6 +83,7 @@ ALL_TESTS="
 	ptp_general_test
 	flow_action_sample_test
 	flow_action_trap_test
+	flow_action_trap_fwd_test
 "
 NUM_NETIFS=4
 source $lib_dir/lib.sh
@@ -663,14 +664,18 @@ flow_action_sample_test()
 	tc qdisc del dev $rp1 clsact
 }
 
-flow_action_trap_test()
+__flow_action_trap_test()
 {
+	local action=$1; shift
+	local trap=$1; shift
+	local description=$1; shift
+
 	# Install a filter that traps a specific flow.
 	tc qdisc add dev $rp1 clsact
 	tc filter add dev $rp1 ingress proto ip pref 1 handle 101 flower \
-		skip_sw ip_proto udp src_port 12345 dst_port 54321 action trap
+		skip_sw ip_proto udp src_port 12345 dst_port 54321 action $action
 
-	devlink_trap_stats_test "Flow Trapping (Logging)" "flow_action_trap" \
+	devlink_trap_stats_test "$description" $trap \
 		$MZ $h1 -c 1 -a own -b $(mac_get $rp1) \
 		-A 192.0.2.1 -B 198.51.100.1 -t udp sp=12345,dp=54321 -p 100 -q
 
@@ -678,6 +683,18 @@ flow_action_trap_test()
 	tc qdisc del dev $rp1 clsact
 }
 
+flow_action_trap_test()
+{
+	__flow_action_trap_test trap flow_action_trap \
+				"Flow Trapping (Logging)"
+}
+
+flow_action_trap_fwd_test()
+{
+	__flow_action_trap_test trap_fwd flow_action_trap_fwd \
+				"Flow Trap-and-forwarding (Logging)"
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.26.2

Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Jamal Hadi Salim]

Hi Petr,

On 2021-04-08 9:38 a.m., Petr Machata wrote:
> The TC action "trap" is used to instruct the HW datapath to drop the
> matched packet and transfer it for processing in the SW pipeline. If
> instead it is desirable to forward the packet and transferring a _copy_ to
> the SW pipeline, there is no practical way to achieve that.
> 
> To that end add a new generic action, trap_fwd. In the software pipeline,
> it is equivalent to an OK. When offloading, it should forward the packet to
> the host, but unlike trap it should not drop the packet.
> 

I am concerned about adding new opcodes which only make sense if you
offload (or make sense only if you are running in s/w).

Those opcodes are intended to be generic abstractions so the dispatcher
can decide what to do next. Adding things that are specific only
to scenarios of hardware offload removes that opaqueness.
I must have missed the discussion on ACT_TRAP because it is the
same issue there i.e shouldnt be an opcode. For details see:
https://people.netfilter.org/pablo/netdev0.1/papers/Linux-Traffic-Control-Classifier-Action-Subsystem-Architecture.pdf

IMO:
It seems to me there are two actions here encapsulated in one.
The first is to "trap" and the second is to "drop".
This is no different semantically than say "mirror and drop"
offload being enunciated by "skip_sw".

Does the spectrum not support multiple actions?
e.g with a policy like:
  match blah action trap action drop skip_sw

cheers,
jamal

Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Jakub Kicinski]

On Thu, 8 Apr 2021 10:05:07 -0400 Jamal Hadi Salim wrote:
> On 2021-04-08 9:38 a.m., Petr Machata wrote:
> > The TC action "trap" is used to instruct the HW datapath to drop the
> > matched packet and transfer it for processing in the SW pipeline. If
> > instead it is desirable to forward the packet and transferring a _copy_ to
> > the SW pipeline, there is no practical way to achieve that.
> > 
> > To that end add a new generic action, trap_fwd. In the software pipeline,
> > it is equivalent to an OK. When offloading, it should forward the packet to
> > the host, but unlike trap it should not drop the packet.
> 
> I am concerned about adding new opcodes which only make sense if you
> offload (or make sense only if you are running in s/w).
> 
> Those opcodes are intended to be generic abstractions so the dispatcher
> can decide what to do next. Adding things that are specific only
> to scenarios of hardware offload removes that opaqueness.
> I must have missed the discussion on ACT_TRAP because it is the
> same issue there i.e shouldnt be an opcode. For details see:
> https://people.netfilter.org/pablo/netdev0.1/papers/Linux-Traffic-Control-Classifier-Action-Subsystem-Architecture.pdf
> 
> IMO:
> It seems to me there are two actions here encapsulated in one.
> The first is to "trap" and the second is to "drop".
> This is no different semantically than say "mirror and drop"
> offload being enunciated by "skip_sw".
> 
> Does the spectrum not support multiple actions?
> e.g with a policy like:
>   match blah action trap action drop skip_sw

To make sure I understand - are you saying that trap should become 
more general and support both "and then drop" as well as "and then 
pass" semantics?

Seems like that ship has sailed, but also - how does it make it any
better WRT not having HW only opcodes? Or are you saying one is better
than two?

Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Petr Machata]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> I am concerned about adding new opcodes which only make sense if you
> offload (or make sense only if you are running in s/w).
>
> Those opcodes are intended to be generic abstractions so the dispatcher
> can decide what to do next. Adding things that are specific only
> to scenarios of hardware offload removes that opaqueness.
> I must have missed the discussion on ACT_TRAP because it is the
> same issue there i.e shouldnt be an opcode. For details see:
> https://people.netfilter.org/pablo/netdev0.1/papers/Linux-Traffic-Control-Classifier-Action-Subsystem-Architecture.pdf

Trap has been in since 4.13, so 2017ish. It's done and dusted at this
point.

> IMO:
> It seems to me there are two actions here encapsulated in one.
> The first is to "trap" and the second is to "drop".
>
> This is no different semantically than say "mirror and drop"
> offload being enunciated by "skip_sw".
>
> Does the spectrum not support multiple actions?
> e.g with a policy like:
>  match blah action trap action drop skip_sw

Trap drops implicitly. We need a "trap, but don't drop". Expressed in
terms of existing actions it would be "mirred egress redirect dev
$cpu_port". But how to express $cpu_port except again by a HW-specific
magic token I don't know.

Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Jamal Hadi Salim]

On 2021-04-08 5:25 p.m., Jakub Kicinski wrote:
> On Thu, 8 Apr 2021 10:05:07 -0400 Jamal Hadi Salim wrote:
>> On 2021-04-08 9:38 a.m., Petr Machata wrote:
>>> The TC action "trap" is used to instruct the HW datapath to drop the
>>> matched packet and transfer it for processing in the SW pipeline. If
>>> instead it is desirable to forward the packet and transferring a _copy_ to
>>> the SW pipeline, there is no practical way to achieve that.
>>>
>>> To that end add a new generic action, trap_fwd. In the software pipeline,
>>> it is equivalent to an OK. When offloading, it should forward the packet to
>>> the host, but unlike trap it should not drop the packet.
>>
>> I am concerned about adding new opcodes which only make sense if you
>> offload (or make sense only if you are running in s/w).
>>
>> Those opcodes are intended to be generic abstractions so the dispatcher
>> can decide what to do next. Adding things that are specific only
>> to scenarios of hardware offload removes that opaqueness.
>> I must have missed the discussion on ACT_TRAP because it is the
>> same issue there i.e shouldnt be an opcode. For details see:
>> https://people.netfilter.org/pablo/netdev0.1/papers/Linux-Traffic-Control-Classifier-Action-Subsystem-Architecture.pdf
>>
>> IMO:
>> It seems to me there are two actions here encapsulated in one.
>> The first is to "trap" and the second is to "drop".
>> This is no different semantically than say "mirror and drop"
>> offload being enunciated by "skip_sw".
>>
>> Does the spectrum not support multiple actions?
>> e.g with a policy like:
>>    match blah action trap action drop skip_sw
> 
> To make sure I understand - are you saying that trap should become
> more general and support both "and then drop" as well as "and then
> pass" semantics?
> 

No.
Main issue is the pollution of the opcodes - whether it is
one or multiple actions is less of a concern.
Those opcodes are intended to be for the core action dispatcher's
consumption. See figure 6 and table 1 of the document i referred to.

Basically:
You dont an action then add an opcode for it even if it is hardware
offloaded (otherwise that opcode space would have grown a lot more by
now for all those actions that are offloaded).
Trap, for example, could have been a dummy action that just returns
the STOLEN/DROP/PASS opcode and does nothing else.
Typically we expect things that are offloaded to have a software
equivalent. It makes for good control consistency etc clean.

> Seems like that ship has sailed, but also - how does it make it any
> better WRT not having HW only opcodes? Or are you saying one is better
> than two?

The opcodes are not tied to whether an action is offloaded or not.
That role belongs to the "skip_sw" axes - which works well
today since we dont offload actions on their own without some
filter rule which specifies the offload option.

I will barf if someone implements 3 actions: "trap", "trap and forward",
"trap and drop" - but that is not messing up with the core architecture
so the barfing is more due to the bad taste of that approach.
A cleaner approach is to code one and change the return code for those
3 to "STOLEN", "PIPE", and "DROP"

cheers,
jamal

Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Jamal Hadi Salim]

On 2021-04-09 7:03 a.m., Petr Machata wrote:
> 
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
> 
>> I am concerned about adding new opcodes which only make sense if you
>> offload (or make sense only if you are running in s/w).
>>
>> Those opcodes are intended to be generic abstractions so the dispatcher
>> can decide what to do next. Adding things that are specific only
>> to scenarios of hardware offload removes that opaqueness.
>> I must have missed the discussion on ACT_TRAP because it is the
>> same issue there i.e shouldnt be an opcode. For details see:
>> https://people.netfilter.org/pablo/netdev0.1/papers/Linux-Traffic-Control-Classifier-Action-Subsystem-Architecture.pdf
> 
> Trap has been in since 4.13, so 2017ish. It's done and dusted at this
> point.
> 

I am afraid that is not a good arguement. With all due respect,
here's how it translates:
"We already made a mistake, therefore, its ok to build on it and
make more mistakes". Touching those opcodes is really dirty; at
least i have seen no convincing arguement _at all_ for it. And,
it is not too late not to make more mistakes.
I dont remember, I may have spoken against TRAP; what i know is had
i seen the patch i would have said something - maybe i did and should
have been louder. Mea culpa.

>> IMO:
>> It seems to me there are two actions here encapsulated in one.
>> The first is to "trap" and the second is to "drop".
>>
>> This is no different semantically than say "mirror and drop"
>> offload being enunciated by "skip_sw".
>>
>> Does the spectrum not support multiple actions?
>> e.g with a policy like:
>>   match blah action trap action drop skip_sw
> 
> Trap drops implicitly. We need a "trap, but don't drop". Expressed in
> terms of existing actions it would be "mirred egress redirect dev
> $cpu_port". But how to express $cpu_port except again by a HW-specific
> magic token I don't know.


Note: mirred was originally intended to send redirect/mirror
packets to user space (the comment is still there in the code).
Infact there is a patch lying around somewhere that does that with
packet sockets (the author hasnt been serious about pushing it
upstream). In that case the semantics are redirecting to a file
descriptor. Could we have something like that here which points
to whatever representation $cpu_port has? Sounds like semantics
for "trap and forward" are just "mirror and forward".

I think there is value in having something like trap action
which generalizes the combinations only to the fact that
it will make it easier to relay the info to the offload without
much transformation.
If i was to do it i would write one action configured by user space:
- to return DROP if you want action trap-and-drop semantics.
- to return STOLEN if you want trap
- to return PIPE if you want trap and forward. You will need a second
action composed to forward.

I said dummy because this action has no value in s/w. Someone could
use it in s/w but it would be no different than gact.
Maybe it could be extended to work also in s/w by adding the "trap to
fd" in userspace.

cheers,
jamal

Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Petr Machata]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> On 2021-04-09 7:03 a.m., Petr Machata wrote:
>> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>> 
>>> I am concerned about adding new opcodes which only make sense if you
>>> offload (or make sense only if you are running in s/w).
>>>
>>> Those opcodes are intended to be generic abstractions so the dispatcher
>>> can decide what to do next.
>>> [...]
>>> For details see:
>>> https://people.netfilter.org/pablo/netdev0.1/papers/Linux-Traffic-Control-Classifier-Action-Subsystem-Architecture.pdf
>>
>> Trap has been in since 4.13, so 2017ish. It's done and dusted at this
>> point.
>
> here's how it translates:
> "We already made a mistake, therefore, its ok to build on it and
> make more mistakes".

I can see how it reads that way, but that was not the intention. I was
actually thinking about whether there might be a way to gradually
migrate all this stuff over to mirred, but at this point, trap is very
much baked in.

>>> IMO:
>>> It seems to me there are two actions here encapsulated in one.
>>> The first is to "trap" and the second is to "drop".
>>>
>>> This is no different semantically than say "mirror and drop"
>>> offload being enunciated by "skip_sw".
>>>
>>> Does the spectrum not support multiple actions?
>>> e.g with a policy like:
>>>   match blah action trap action drop skip_sw
>> Trap drops implicitly. We need a "trap, but don't drop". Expressed in
>> terms of existing actions it would be "mirred egress redirect dev
>> $cpu_port". But how to express $cpu_port except again by a HW-specific
>> magic token I don't know.

(I meant mirred egress mirror, not redirect.)

> Note: mirred was originally intended to send redirect/mirror
> packets to user space (the comment is still there in the code).
> Infact there is a patch lying around somewhere that does that with
> packet sockets (the author hasnt been serious about pushing it
> upstream). In that case the semantics are redirecting to a file
> descriptor. Could we have something like that here which points
> to whatever representation $cpu_port has? Sounds like semantics
> for "trap and forward" are just "mirror and forward".

Hmm, we have devlink ports, the CPU port is exposed there. But that's
the only thing that comes to mind. Those are specific for the given
device though, it doesn't look suitable...

> I think there is value in having something like trap action
> which generalizes the combinations only to the fact that
> it will make it easier to relay the info to the offload without
> much transformation.
> If i was to do it i would write one action configured by user space:
> - to return DROP if you want action trap-and-drop semantics.
> - to return STOLEN if you want trap
> - to return PIPE if you want trap and forward. You will need a second
> action composed to forward.

I think your STOLEN and PIPE are the same behavior. Both are "transfer
the packet to the SW datapath, but keep it in the HW datapath".

In general I have no issue expressing this stuff as a new action,
instead of an opcode. I'll take a look at this.

Re: [PATCH net-next 1/7] net: sched: Add a trap-and-forward action

[Jamal Hadi Salim]

On 2021-04-09 9:43 a.m., Petr Machata wrote:
> 
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
> 

>>>> Does the spectrum not support multiple actions?
>>>> e.g with a policy like:
>>>>    match blah action trap action drop skip_sw
>>> Trap drops implicitly. We need a "trap, but don't drop". Expressed in
>>> terms of existing actions it would be "mirred egress redirect dev
>>> $cpu_port". But how to express $cpu_port except again by a HW-specific
>>> magic token I don't know.
> 
> (I meant mirred egress mirror, not redirect.)
>

Ok.

>> Note: mirred was originally intended to send redirect/mirror
>> packets to user space (the comment is still there in the code).
>> Infact there is a patch lying around somewhere that does that with
>> packet sockets (the author hasnt been serious about pushing it
>> upstream). In that case the semantics are redirecting to a file
>> descriptor. Could we have something like that here which points
>> to whatever representation $cpu_port has? Sounds like semantics
>> for "trap and forward" are just "mirror and forward".
> 
> Hmm, we have devlink ports, the CPU port is exposed there. But that's
> the only thing that comes to mind. Those are specific for the given
> device though, it doesn't look suitable...
> 

If it has an ifindex should be good enough for abstraction
purposes.

>> I think there is value in having something like trap action
>> which generalizes the combinations only to the fact that
>> it will make it easier to relay the info to the offload without
>> much transformation.
>> If i was to do it i would write one action configured by user space:
>> - to return DROP if you want action trap-and-drop semantics.
>> - to return STOLEN if you want trap
>> - to return PIPE if you want trap and forward. You will need a second
>> action composed to forward.
> 
> I think your STOLEN and PIPE are the same behavior. Both are "transfer
> the packet to the SW datapath, but keep it in the HW datapath".
> 
> In general I have no issue expressing this stuff as a new action,
> instead of an opcode. I'll take a look at this.
> 

Ok, thanks.

cheers,
jamal