[PATCH net-next RFC v5 0/4] net/sched: Introduce tc block ports tracking and use

[PATCH net-next RFC v5 0/4] net/sched: Introduce tc block ports tracking and use

[Victor Nogueira]

__context__
The "tc block" is a collection of netdevs/ports which allow qdiscs to share
match-action block instances (as opposed to the traditional tc filter per
netdev/port)[1].

Example setup:
$ tc qdisc add dev ens7 ingress_block 22
$ tc qdisc add dev ens8 ingress_block 22

Once the block is created we can add a filter using the block index:
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action drop

A packet with dst IP matching 192.168.0.0/16 arriving on the ingress of
either ens7 or ens8 is dropped.

__this patchset__
Up to this point in the implementation, the block is unaware of its ports.
This patch makes the tc block ports available to the datapath.

For the datapath we provide a use case of the tc block in an action
we call "blockcast" in patch 4. This action can be used in an example as
such:

$ tc qdisc add dev ens7 ingress_block 22
$ tc qdisc add dev ens8 ingress_block 22
$ tc qdisc add dev ens9 ingress_block 22
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action blockcast blockid 22

When a packet(matching dst IP 192.168.0.0/16) arrives on the ingress of any
of ens7, ens8 or ens9 it will be copied to all ports other than itself.
For example, if it arrives on ens8 then a copy of the packet will be
"blockcasted";-> to both ens7 and ens9 (unmodified), but not to ens8.

We also allow for the packet to be send to all the ports in the block
indiscriminately by specifying the "tx_type all" option. Using the
previous example:

$ tc qdisc add dev ens7 ingress_block 22
$ tc qdisc add dev ens8 ingress_block 22
$ tc qdisc add dev ens9 ingress_block 22
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all

In this case, if the packet arrives on ens8, it will be copied and sent to
all ports in the block including ens8.

Patch 1 separates/exports mirror and redirect functions from act_mirred
Patch 2 introduces the required infra.
Patch 3 exposes the tc block to the tc datapath
Patch 4 implements datapath usage via a new tc action "blockcast".

__Acknowledgements__
Suggestions from Vlad Buslov and Marcelo Ricardo Leitner made this patchset
better. The idea of integrating the ports into the tc block was suggested
by Jiri Pirko.

[1] See commit ca46abd6f89f ("Merge branch 'net-sched-allow-qdiscs-to-share-filter-block-instances'")

Changes in v2:
  - Remove RFC tag
  - Add more details in patch 0(Jiri)
  - When CONFIG_NET_TC_SKB_EXT is selected we have unused qdisc_cb
    Reported-by: kernel test robot <lkp@intel.com> (and horms@kernel.org)
  - Fix bad dev dereference in printk of blockcast action (Simon)

Changes in v3:
  - Add missing xa_destroy (pointed out by Vlad)
  - Remove bugfix pointed by Vlad (will send in separate patch)
  - Removed ports from subject in patch #2 and typos (suggested by Marcelo)
  - Remove net_notice_ratelimited debug messages in error
    cases (suggested by Marcelo)
  - Minor changes to appease sparse's lock context warning

Changes in v4:
  - Avoid code repetition using gotos in cast_one (suggested by Paolo)
  - Fix typo in cover letter (pointed out by Paolo)
  - Create a module description for act_blockcast
    (reported by Paolo and CI)

Changes in v5:
  - Added new patch which separated mirred into mirror and redirect
    functions (suggested by Jiri)
  - Instead of repeating the code to mirror in blockcast use mirror
    exported function by patch1 (tcf_mirror_act)
  - Make Block ID into act_blockcast's parameter passed by user space
    instead of always getting it from SKB (suggested by Jiri)
  - Add tx_type parameter which will specify what transmission behaviour
    we want (as described earlier)

Victor Nogueira (4):
  net/sched: act_mirred: Separate mirror and redirect into two distinct
    functions
  net/sched: Introduce tc block netdev tracking infra
  net/sched: cls_api: Expose tc block to the datapath
  net/sched: act_blockcast: Introduce blockcast tc action

 include/net/act_api.h                    |  85 +++++++
 include/net/sch_generic.h                |   6 +
 include/net/tc_act/tc_blockcast.h        |  16 ++
 include/net/tc_wrapper.h                 |   5 +
 include/uapi/linux/pkt_cls.h             |   1 +
 include/uapi/linux/tc_act/tc_blockcast.h |  32 +++
 net/sched/Kconfig                        |  12 +
 net/sched/Makefile                       |   1 +
 net/sched/act_blockcast.c                | 283 +++++++++++++++++++++++
 net/sched/act_mirred.c                   | 103 +++------
 net/sched/cls_api.c                      |   5 +-
 net/sched/sch_api.c                      |  55 +++++
 net/sched/sch_generic.c                  |  31 ++-
 13 files changed, 557 insertions(+), 78 deletions(-)
 create mode 100644 include/net/tc_act/tc_blockcast.h
 create mode 100644 include/uapi/linux/tc_act/tc_blockcast.h
 create mode 100644 net/sched/act_blockcast.c

-- 
2.25.1

[PATCH net-next RFC v5 1/4] net/sched: act_mirred: Separate mirror and redirect into two distinct functions

[Victor Nogueira]

Separate mirror and redirect code into two into two separate functions
(tcf_mirror_act and tcf_redirect_act). This not only cleans up the code and
improves both readability and maintainability in addition to reducing the
complexity given different expectations for mirroring and redirecting.

This patchset has a use case for the mirror part in action "blockcast".

Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/act_api.h  |  85 ++++++++++++++++++++++++++++++++++
 net/sched/act_mirred.c | 103 +++++++++++------------------------------
 2 files changed, 113 insertions(+), 75 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 4ae0580b63ca..8d288040aeb8 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -12,6 +12,7 @@
 #include <net/pkt_sched.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <net/dst.h>
 
 struct tcf_idrinfo {
 	struct mutex	lock;
@@ -293,5 +294,89 @@ static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
 #endif
 }
 
+static inline int tcf_mirred_forward(bool to_ingress, bool nested_call,
+				     struct sk_buff *skb)
+{
+	int err;
+
+	if (!to_ingress)
+		err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
+	else if (nested_call)
+		err = netif_rx(skb);
+	else
+		err = netif_receive_skb(skb);
+
+	return err;
+}
+
+static inline struct sk_buff *
+tcf_mirred_common(struct sk_buff *skb, bool want_ingress, bool dont_clone,
+		  bool expects_nh, struct net_device *dest_dev)
+{
+	struct sk_buff *skb_to_send = skb;
+	bool at_ingress;
+	int mac_len;
+	bool at_nh;
+	int err;
+
+	if (unlikely(!(dest_dev->flags & IFF_UP)) ||
+	    !netif_carrier_ok(dest_dev)) {
+		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
+				       dest_dev->name);
+		err = -ENODEV;
+		goto err_out;
+	}
+
+	if (!dont_clone) {
+		skb_to_send = skb_clone(skb, GFP_ATOMIC);
+		if (!skb_to_send) {
+			err =  -ENOMEM;
+			goto err_out;
+		}
+	}
+
+	at_ingress = skb_at_tc_ingress(skb);
+
+	/* All mirred/redirected skbs should clear previous ct info */
+	nf_reset_ct(skb_to_send);
+	if (want_ingress && !at_ingress) /* drop dst for egress -> ingress */
+		skb_dst_drop(skb_to_send);
+
+	at_nh = skb->data == skb_network_header(skb);
+	if (at_nh != expects_nh) {
+		mac_len = at_ingress ? skb->mac_len :
+			  skb_network_offset(skb);
+		if (expects_nh) {
+			/* target device/action expect data at nh */
+			skb_pull_rcsum(skb_to_send, mac_len);
+		} else {
+			/* target device/action expect data at mac */
+			skb_push_rcsum(skb_to_send, mac_len);
+		}
+	}
+
+	skb_to_send->skb_iif = skb->dev->ifindex;
+	skb_to_send->dev = dest_dev;
+
+	return skb_to_send;
+
+err_out:
+	return ERR_PTR(err);
+}
+
+static inline int
+tcf_redirect_act(struct sk_buff *skb,
+		 bool nested_call, bool want_ingress)
+{
+	skb_set_redirected(skb, skb->tc_at_ingress);
+
+	return tcf_mirred_forward(want_ingress, nested_call, skb);
+}
+
+static inline int
+tcf_mirror_act(struct sk_buff *skb, bool nested_call, bool want_ingress)
+{
+	return tcf_mirred_forward(want_ingress, nested_call, skb);
+}
 
 #endif
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 0a711c184c29..95d30cb06e54 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -211,38 +211,22 @@ static bool is_mirred_nested(void)
 	return unlikely(__this_cpu_read(mirred_nest_level) > 1);
 }
 
-static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
-{
-	int err;
-
-	if (!want_ingress)
-		err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
-	else if (is_mirred_nested())
-		err = netif_rx(skb);
-	else
-		err = netif_receive_skb(skb);
-
-	return err;
-}
-
 TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
 				     const struct tc_action *a,
 				     struct tcf_result *res)
 {
 	struct tcf_mirred *m = to_mirred(a);
-	struct sk_buff *skb2 = skb;
+	struct sk_buff *skb_to_send;
+	unsigned int nest_level;
 	bool m_mac_header_xmit;
 	struct net_device *dev;
-	unsigned int nest_level;
-	int retval, err = 0;
-	bool use_reinsert;
 	bool want_ingress;
 	bool is_redirect;
 	bool expects_nh;
-	bool at_ingress;
+	bool dont_clone;
 	int m_eaction;
-	int mac_len;
-	bool at_nh;
+	int err = 0;
+	int retval;
 
 	nest_level = __this_cpu_inc_return(mirred_nest_level);
 	if (unlikely(nest_level > MIRRED_NEST_LIMIT)) {
@@ -255,80 +239,49 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
 	tcf_lastuse_update(&m->tcf_tm);
 	tcf_action_update_bstats(&m->common, skb);
 
-	m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
 	m_eaction = READ_ONCE(m->tcfm_eaction);
-	retval = READ_ONCE(m->tcf_action);
+	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+	retval = READ_ONCE(a->tcfa_action);
 	dev = rcu_dereference_bh(m->tcfm_dev);
 	if (unlikely(!dev)) {
 		pr_notice_once("tc mirred: target device is gone\n");
+		err = -ENODEV;
 		goto out;
 	}
 
-	if (unlikely(!(dev->flags & IFF_UP)) || !netif_carrier_ok(dev)) {
-		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
-				       dev->name);
-		goto out;
-	}
+	m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
+	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
+	expects_nh = want_ingress || !m_mac_header_xmit;
 
 	/* we could easily avoid the clone only if called by ingress and clsact;
 	 * since we can't easily detect the clsact caller, skip clone only for
 	 * ingress - that covers the TC S/W datapath.
 	 */
-	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
-	at_ingress = skb_at_tc_ingress(skb);
-	use_reinsert = at_ingress && is_redirect &&
-		       tcf_mirred_can_reinsert(retval);
-	if (!use_reinsert) {
-		skb2 = skb_clone(skb, GFP_ATOMIC);
-		if (!skb2)
-			goto out;
-	}
-
-	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
-
-	/* All mirred/redirected skbs should clear previous ct info */
-	nf_reset_ct(skb2);
-	if (want_ingress && !at_ingress) /* drop dst for egress -> ingress */
-		skb_dst_drop(skb2);
+	dont_clone = skb_at_tc_ingress(skb) && is_redirect &&
+		     tcf_mirred_can_reinsert(retval);
 
-	expects_nh = want_ingress || !m_mac_header_xmit;
-	at_nh = skb->data == skb_network_header(skb);
-	if (at_nh != expects_nh) {
-		mac_len = skb_at_tc_ingress(skb) ? skb->mac_len :
-			  skb_network_offset(skb);
-		if (expects_nh) {
-			/* target device/action expect data at nh */
-			skb_pull_rcsum(skb2, mac_len);
-		} else {
-			/* target device/action expect data at mac */
-			skb_push_rcsum(skb2, mac_len);
-		}
+	skb_to_send = tcf_mirred_common(skb, want_ingress, dont_clone,
+					expects_nh, dev);
+	if (IS_ERR(skb_to_send)) {
+		err = PTR_ERR(skb_to_send);
+		goto out;
 	}
 
-	skb2->skb_iif = skb->dev->ifindex;
-	skb2->dev = dev;
-
-	/* mirror is always swallowed */
 	if (is_redirect) {
-		skb_set_redirected(skb2, skb2->tc_at_ingress);
-
-		/* let's the caller reinsert the packet, if possible */
-		if (use_reinsert) {
-			err = tcf_mirred_forward(want_ingress, skb);
-			if (err)
-				tcf_action_inc_overlimit_qstats(&m->common);
-			__this_cpu_dec(mirred_nest_level);
-			return TC_ACT_CONSUMED;
-		}
+		if (skb == skb_to_send)
+			retval = TC_ACT_CONSUMED;
+
+		err = tcf_redirect_act(skb_to_send, is_mirred_nested(),
+				       want_ingress);
+	} else {
+		err = tcf_mirror_act(skb_to_send, is_mirred_nested(),
+				     want_ingress);
 	}
 
-	err = tcf_mirred_forward(want_ingress, skb2);
-	if (err) {
 out:
+	if (err)
 		tcf_action_inc_overlimit_qstats(&m->common);
-		if (tcf_mirred_is_act_redirect(m_eaction))
-			retval = TC_ACT_SHOT;
-	}
+
 	__this_cpu_dec(mirred_nest_level);
 
 	return retval;
-- 
2.25.1

[PATCH net-next RFC v5 2/4] net/sched: Introduce tc block netdev tracking infra

[Victor Nogueira]

The tc block is a collection of netdevs/ports which allow qdiscs to share
filter block instances (as opposed to the traditional tc filter per port).
Example:
$ tc qdisc add dev ens7 ingress_block 22
$ tc qdisc add dev ens8 ingress_block 22

Now we can add a filter using the block index:
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action drop

Up to this point, the block has been unaware of its ports. This patch makes
tc block aware of its ports. Patch 3 exposes tc block to the datapath.
Patch 4 shows a use case of the blockcast action which uses the tc block in its
datapath and then multicast packets to the tc block ports.

Suggested-by: Jiri Pirko <jiri@nvidia.com>
Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/sch_generic.h |  4 +++
 net/sched/cls_api.c       |  2 ++
 net/sched/sch_api.c       | 55 +++++++++++++++++++++++++++++++++++++++
 net/sched/sch_generic.c   | 31 ++++++++++++++++++++--
 4 files changed, 90 insertions(+), 2 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index dcb9160e6467..cefca55dd4f9 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -19,6 +19,7 @@
 #include <net/gen_stats.h>
 #include <net/rtnetlink.h>
 #include <net/flow_offload.h>
+#include <linux/xarray.h>
 
 struct Qdisc_ops;
 struct qdisc_walker;
@@ -126,6 +127,8 @@ struct Qdisc {
 
 	struct rcu_head		rcu;
 	netdevice_tracker	dev_tracker;
+	netdevice_tracker	in_block_tracker;
+	netdevice_tracker	eg_block_tracker;
 	/* private data */
 	long privdata[] ____cacheline_aligned;
 };
@@ -457,6 +460,7 @@ struct tcf_chain {
 };
 
 struct tcf_block {
+	struct xarray ports; /* datapath accessible */
 	/* Lock protects tcf_block and lifetime-management data of chains
 	 * attached to the block (refcnt, action_refcnt, explicitly_created).
 	 */
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 1976bd163986..42f760ab7e43 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -531,6 +531,7 @@ static void tcf_block_destroy(struct tcf_block *block)
 {
 	mutex_destroy(&block->lock);
 	mutex_destroy(&block->proto_destroy_lock);
+	xa_destroy(&block->ports);
 	kfree_rcu(block, rcu);
 }
 
@@ -1003,6 +1004,7 @@ static struct tcf_block *tcf_block_create(struct net *net, struct Qdisc *q,
 	refcount_set(&block->refcnt, 1);
 	block->net = net;
 	block->index = block_index;
+	xa_init(&block->ports);
 
 	/* Don't store q pointer for blocks which are shared */
 	if (!tcf_block_shared(block))
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index e9eaf637220e..09ec64f2f463 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1180,6 +1180,57 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 	return 0;
 }
 
+static int qdisc_block_add_dev(struct Qdisc *sch, struct net_device *dev,
+			       struct nlattr **tca,
+			       struct netlink_ext_ack *extack)
+{
+	const struct Qdisc_class_ops *cl_ops = sch->ops->cl_ops;
+	struct tcf_block *in_block = NULL;
+	struct tcf_block *eg_block = NULL;
+	int err;
+
+	if (tca[TCA_INGRESS_BLOCK]) {
+		/* works for both ingress and clsact */
+		in_block = cl_ops->tcf_block(sch, TC_H_MIN_INGRESS, NULL);
+		if (!in_block) {
+			NL_SET_ERR_MSG(extack, "Shared ingress block missing");
+			return -EINVAL;
+		}
+
+		err = xa_insert(&in_block->ports, dev->ifindex, dev, GFP_KERNEL);
+		if (err) {
+			NL_SET_ERR_MSG(extack, "Ingress block dev insert failed");
+			return err;
+		}
+
+		netdev_hold(dev, &sch->in_block_tracker, GFP_KERNEL);
+	}
+
+	if (tca[TCA_EGRESS_BLOCK]) {
+		eg_block = cl_ops->tcf_block(sch, TC_H_MIN_EGRESS, NULL);
+		if (!eg_block) {
+			NL_SET_ERR_MSG(extack, "Shared egress block missing");
+			err = -EINVAL;
+			goto err_out;
+		}
+
+		err = xa_insert(&eg_block->ports, dev->ifindex, dev, GFP_KERNEL);
+		if (err) {
+			NL_SET_ERR_MSG(extack, "Egress block dev insert failed");
+			goto err_out;
+		}
+		netdev_hold(dev, &sch->eg_block_tracker, GFP_KERNEL);
+	}
+
+	return 0;
+err_out:
+	if (in_block) {
+		xa_erase(&in_block->ports, dev->ifindex);
+		netdev_put(dev, &sch->in_block_tracker);
+	}
+	return err;
+}
+
 static int qdisc_block_indexes_set(struct Qdisc *sch, struct nlattr **tca,
 				   struct netlink_ext_ack *extack)
 {
@@ -1350,6 +1401,10 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 	qdisc_hash_add(sch, false);
 	trace_qdisc_create(ops, dev, parent);
 
+	err = qdisc_block_add_dev(sch, dev, tca, extack);
+	if (err)
+		goto err_out4;
+
 	return sch;
 
 err_out4:
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 4195a4bc26ca..83bea257904a 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1049,7 +1049,11 @@ static void qdisc_free_cb(struct rcu_head *head)
 
 static void __qdisc_destroy(struct Qdisc *qdisc)
 {
-	const struct Qdisc_ops  *ops = qdisc->ops;
+	struct net_device *dev = qdisc_dev(qdisc);
+	const struct Qdisc_ops *ops = qdisc->ops;
+	const struct Qdisc_class_ops *cops;
+	struct tcf_block *block;
+	u32 block_index;
 
 #ifdef CONFIG_NET_SCHED
 	qdisc_hash_del(qdisc);
@@ -1060,11 +1064,34 @@ static void __qdisc_destroy(struct Qdisc *qdisc)
 
 	qdisc_reset(qdisc);
 
+	cops = ops->cl_ops;
+	if (ops->ingress_block_get) {
+		block_index = ops->ingress_block_get(qdisc);
+		if (block_index) {
+			block = cops->tcf_block(qdisc, TC_H_MIN_INGRESS, NULL);
+			if (block) {
+				if (xa_erase(&block->ports, dev->ifindex))
+					netdev_put(dev, &qdisc->in_block_tracker);
+			}
+		}
+	}
+
+	if (ops->egress_block_get) {
+		block_index = ops->egress_block_get(qdisc);
+		if (block_index) {
+			block = cops->tcf_block(qdisc, TC_H_MIN_EGRESS, NULL);
+			if (block) {
+				if (xa_erase(&block->ports, dev->ifindex))
+					netdev_put(dev, &qdisc->eg_block_tracker);
+			}
+		}
+	}
+
 	if (ops->destroy)
 		ops->destroy(qdisc);
 
 	module_put(ops->owner);
-	netdev_put(qdisc_dev(qdisc), &qdisc->dev_tracker);
+	netdev_put(dev, &qdisc->dev_tracker);
 
 	trace_qdisc_destroy(qdisc);
 
-- 
2.25.1

[PATCH net-next RFC v5 3/4] net/sched: cls_api: Expose tc block to the datapath

[Victor Nogueira]

The datapath can now find the block of the port in which the packet arrived
at.

In the next patch we show a simple action(blockcast) that multicasts to all
ports except for the port in which the packet arrived on.

Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/sch_generic.h | 2 ++
 net/sched/cls_api.c       | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index cefca55dd4f9..479bc195bb0f 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -487,6 +487,8 @@ struct tcf_block {
 	struct mutex proto_destroy_lock; /* Lock for proto_destroy hashtable. */
 };
 
+struct tcf_block *tcf_block_lookup(struct net *net, u32 block_index);
+
 static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
 {
 	return lockdep_is_held(&chain->filter_chain_lock);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 42f760ab7e43..e7015c2dbbbb 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1012,12 +1012,13 @@ static struct tcf_block *tcf_block_create(struct net *net, struct Qdisc *q,
 	return block;
 }
 
-static struct tcf_block *tcf_block_lookup(struct net *net, u32 block_index)
+struct tcf_block *tcf_block_lookup(struct net *net, u32 block_index)
 {
 	struct tcf_net *tn = net_generic(net, tcf_net_id);
 
 	return idr_find(&tn->idr, block_index);
 }
+EXPORT_SYMBOL(tcf_block_lookup);
 
 static struct tcf_block *tcf_block_refcnt_get(struct net *net, u32 block_index)
 {
-- 
2.25.1

[PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Victor Nogueira]

This action takes advantage of the presence of tc block ports set in the
datapath and multicasts a packet to ports on a block. By default, it will
broadcast the packet to a block, that is send to all members of the block except
the port in which the packet arrived on. However, the user may specify
the option "tx_type all", which will send the packet to all members of the
block indiscriminately.

Example usage:
    $ tc qdisc add dev ens7 ingress_block 22
    $ tc qdisc add dev ens8 ingress_block 22

Now we can add a filter to broadcast packets to ports on ingress block id 22:
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action blockcast blockid 22

Or if we wish to send to all ports in the block:
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all

Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 include/net/tc_act/tc_blockcast.h        |  16 ++
 include/net/tc_wrapper.h                 |   5 +
 include/uapi/linux/pkt_cls.h             |   1 +
 include/uapi/linux/tc_act/tc_blockcast.h |  32 +++
 net/sched/Kconfig                        |  12 +
 net/sched/Makefile                       |   1 +
 net/sched/act_blockcast.c                | 283 +++++++++++++++++++++++
 7 files changed, 350 insertions(+)
 create mode 100644 include/net/tc_act/tc_blockcast.h
 create mode 100644 include/uapi/linux/tc_act/tc_blockcast.h
 create mode 100644 net/sched/act_blockcast.c

diff --git a/include/net/tc_act/tc_blockcast.h b/include/net/tc_act/tc_blockcast.h
new file mode 100644
index 000000000000..513d6622db66
--- /dev/null
+++ b/include/net/tc_act/tc_blockcast.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __NET_TC_BLOCKCAST_H
+#define __NET_TC_BLOCKCAST_H
+
+#include <net/act_api.h>
+#include <linux/tc_act/tc_blockcast.h>
+
+struct tcf_blockcast_act {
+	struct tc_action common;
+	u32 blockid;
+	enum tc_blockcast_tx_type tx_type;
+};
+
+#define to_blockcast_act(a) ((struct tcf_blockcast_act *)a)
+
+#endif /* __NET_TC_BLOCKCAST_H */
diff --git a/include/net/tc_wrapper.h b/include/net/tc_wrapper.h
index a6d481b5bcbc..5525544ee6ee 100644
--- a/include/net/tc_wrapper.h
+++ b/include/net/tc_wrapper.h
@@ -28,6 +28,7 @@ TC_INDIRECT_ACTION_DECLARE(tcf_csum_act);
 TC_INDIRECT_ACTION_DECLARE(tcf_ct_act);
 TC_INDIRECT_ACTION_DECLARE(tcf_ctinfo_act);
 TC_INDIRECT_ACTION_DECLARE(tcf_gact_act);
+TC_INDIRECT_ACTION_DECLARE(tcf_blockcast_act);
 TC_INDIRECT_ACTION_DECLARE(tcf_gate_act);
 TC_INDIRECT_ACTION_DECLARE(tcf_ife_act);
 TC_INDIRECT_ACTION_DECLARE(tcf_ipt_act);
@@ -57,6 +58,10 @@ static inline int tc_act(struct sk_buff *skb, const struct tc_action *a,
 	if (a->ops->act == tcf_mirred_act)
 		return tcf_mirred_act(skb, a, res);
 #endif
+#if IS_BUILTIN(CONFIG_NET_ACT_BLOCKCAST)
+	if (a->ops->act == tcf_blockcast_act)
+		return tcf_blockcast_act(skb, a, res);
+#endif
 #if IS_BUILTIN(CONFIG_NET_ACT_PEDIT)
 	if (a->ops->act == tcf_pedit_act)
 		return tcf_pedit_act(skb, a, res);
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index c7082cc60d21..e12fc51c1be1 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -139,6 +139,7 @@ enum tca_id {
 	TCA_ID_MPLS,
 	TCA_ID_CT,
 	TCA_ID_GATE,
+	TCA_ID_BLOCKCAST,
 	/* other actions go here */
 	__TCA_ID_MAX = 255
 };
diff --git a/include/uapi/linux/tc_act/tc_blockcast.h b/include/uapi/linux/tc_act/tc_blockcast.h
new file mode 100644
index 000000000000..fe43d0af439d
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_blockcast.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __LINUX_TC_BLOCKCAST_H
+#define __LINUX_TC_BLOCKCAST_H
+
+#include <linux/types.h>
+#include <linux/pkt_cls.h>
+
+struct tc_blockcast {
+	tc_gen;
+	__u32                   blockid;  /* block ID to which we'll blockcast */
+};
+
+enum {
+	TCA_BLOCKCAST_UNSPEC,
+	TCA_BLOCKCAST_TM,
+	TCA_BLOCKCAST_PARMS,
+	TCA_BLOCKCAST_TX_TYPE,
+	TCA_BLOCKCAST_PAD,
+	__TCA_BLOCKCAST_MAX
+};
+
+#define TCA_BLOCKCAST_MAX (__TCA_BLOCKCAST_MAX - 1)
+
+enum tc_blockcast_tx_type {
+	TCA_BLOCKCAST_TX_TYPE_BROADCAST,
+	TCA_BLOCKCAST_TX_TYPE_ALL,
+	__TCA_BLOCKCAST_TX_TYPE_MAX,
+};
+
+#define TCA_BLOCKCAST_TX_TYPE_MAX (__TCA_BLOCKCAST_TX_TYPE_MAX - 1)
+
+#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index 470c70deffe2..ca1deecdd6ae 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -780,6 +780,18 @@ config NET_ACT_SIMP
 	  To compile this code as a module, choose M here: the
 	  module will be called act_simple.
 
+config NET_ACT_BLOCKCAST
+	tristate "TC block Multicast"
+	depends on NET_CLS_ACT
+	help
+	  Say Y here to add an action that will multicast an skb to egress of
+	  netdevs that belong to a tc block
+
+	  If unsure, say N.
+
+	  To compile this code as a module, choose M here: the
+	  module will be called act_blockcast.
+
 config NET_ACT_SKBEDIT
 	tristate "SKB Editing"
 	depends on NET_CLS_ACT
diff --git a/net/sched/Makefile b/net/sched/Makefile
index b5fd49641d91..2cdcf30645eb 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_NET_ACT_IPT)	+= act_ipt.o
 obj-$(CONFIG_NET_ACT_NAT)	+= act_nat.o
 obj-$(CONFIG_NET_ACT_PEDIT)	+= act_pedit.o
 obj-$(CONFIG_NET_ACT_SIMP)	+= act_simple.o
+obj-$(CONFIG_NET_ACT_BLOCKCAST)	+= act_blockcast.o
 obj-$(CONFIG_NET_ACT_SKBEDIT)	+= act_skbedit.o
 obj-$(CONFIG_NET_ACT_CSUM)	+= act_csum.o
 obj-$(CONFIG_NET_ACT_MPLS)	+= act_mpls.o
diff --git a/net/sched/act_blockcast.c b/net/sched/act_blockcast.c
new file mode 100644
index 000000000000..dc5d1088e534
--- /dev/null
+++ b/net/sched/act_blockcast.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * net/sched/act_blockcast.c	Block Cast action
+ * Copyright (c) 2023, Mojatatu Networks
+ * Authors:     Jamal Hadi Salim <jhs@mojatatu.com>
+ *              Victor Nogueira <victor@mojatatu.com>
+ *              Pedro Tammela <pctammela@mojatatu.com>
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
+#include <linux/if_arp.h>
+#include <net/tc_wrapper.h>
+
+#include <linux/tc_act/tc_blockcast.h>
+#include <net/tc_act/tc_blockcast.h>
+#include <net/tc_act/tc_mirred.h>
+
+static struct tc_action_ops act_blockcast_ops;
+
+#define BLOCKCAST_NEST_LIMIT    4
+static DEFINE_PER_CPU(unsigned int, blockcast_nest_level);
+
+TC_INDIRECT_SCOPE int tcf_blockcast_act(struct sk_buff *skb,
+					const struct tc_action *a,
+					struct tcf_result *res)
+{
+	struct tcf_blockcast_act *p = to_blockcast_act(a);
+	enum tc_blockcast_tx_type tx_type = READ_ONCE(p->tx_type);
+	int action = READ_ONCE(p->tcf_action);
+	unsigned int nest_level;
+	struct tcf_block *block;
+	struct net_device *dev;
+	u32 exception_ifindex;
+	unsigned long index;
+
+	nest_level = __this_cpu_inc_return(blockcast_nest_level);
+	if (unlikely(nest_level > BLOCKCAST_NEST_LIMIT)) {
+		net_warn_ratelimited("Packet exceeded blockcast recursion limit on dev %s\n",
+				     netdev_name(skb->dev));
+		__this_cpu_dec(blockcast_nest_level);
+		return TC_ACT_SHOT;
+	}
+
+	exception_ifindex = skb->dev->ifindex;
+
+	tcf_action_update_bstats(&p->common, skb);
+	tcf_lastuse_update(&p->tcf_tm);
+
+	/* we are already under rcu protection, so can call block lookup directly */
+	block = tcf_block_lookup(dev_net(skb->dev), p->blockid);
+	if (!block || xa_empty(&block->ports)) {
+		__this_cpu_dec(blockcast_nest_level);
+		return action;
+	}
+
+	xa_for_each(&block->ports, index, dev) {
+		struct sk_buff *skb_to_send;
+		struct net_device *dev;
+
+		if (tx_type == TCA_BLOCKCAST_TX_TYPE_BROADCAST &&
+		    index == exception_ifindex)
+			continue;
+
+		dev = dev_get_by_index_rcu(dev_net(skb->dev), index);
+		if (unlikely(!dev)) {
+			tcf_action_inc_overlimit_qstats(&p->common);
+			continue;
+		}
+
+		skb_to_send = tcf_mirred_common(skb, false, false,
+						!dev_is_mac_header_xmit(dev),
+						dev);
+		if (IS_ERR(skb_to_send)) {
+			tcf_action_inc_overlimit_qstats(&p->common);
+			continue;
+		}
+
+		if (tcf_mirror_act(skb_to_send, false, false))
+			tcf_action_inc_overlimit_qstats(&p->common);
+	}
+
+	__this_cpu_dec(blockcast_nest_level);
+	return action;
+}
+
+static const struct nla_policy blockcast_policy[TCA_BLOCKCAST_MAX + 1] = {
+	[TCA_BLOCKCAST_PARMS]	= NLA_POLICY_EXACT_LEN(sizeof(struct tc_blockcast)),
+	[TCA_BLOCKCAST_TX_TYPE]	= NLA_POLICY_RANGE(NLA_U8,
+						   TCA_BLOCKCAST_TX_TYPE_BROADCAST,
+						   TCA_BLOCKCAST_TX_TYPE_MAX),
+};
+
+static int tcf_blockcast_init(struct net *net, struct nlattr *nla,
+			      struct nlattr *est, struct tc_action **a,
+			      struct tcf_proto *tp, u32 flags,
+			      struct netlink_ext_ack *extack)
+{
+	struct tc_action_net *tn = net_generic(net, act_blockcast_ops.net_id);
+	bool bind = flags & TCA_ACT_FLAGS_BIND;
+	struct nlattr *tb[TCA_BLOCKCAST_MAX + 1];
+	struct tcf_chain *goto_ch = NULL;
+	struct tcf_blockcast_act *p;
+	struct tc_blockcast *parm;
+	bool exists = false;
+	int ret = 0, err;
+	u32 index;
+
+	if (!nla)
+		return -EINVAL;
+
+	err = nla_parse_nested(tb, TCA_BLOCKCAST_MAX, nla,
+			       blockcast_policy, extack);
+	if (err < 0)
+		return err;
+
+	if (!tb[TCA_BLOCKCAST_PARMS]) {
+		NL_SET_ERR_MSG_MOD(extack, "Must specify blockcast parms");
+		return -EINVAL;
+	}
+
+	parm = nla_data(tb[TCA_BLOCKCAST_PARMS]);
+	index = parm->index;
+
+	err = tcf_idr_check_alloc(tn, &index, a, bind);
+	if (err < 0)
+		return err;
+
+	exists = err;
+	if (exists && bind)
+		return 0;
+
+	if (!exists) {
+		if (!parm->blockid) {
+			tcf_idr_cleanup(tn, index);
+			NL_SET_ERR_MSG_MOD(extack, "Must specify blockid");
+			return -EINVAL;
+		}
+
+		ret = tcf_idr_create_from_flags(tn, index, est, a,
+						&act_blockcast_ops, bind, flags);
+		if (ret) {
+			tcf_idr_cleanup(tn, index);
+			return ret;
+		}
+
+		ret = ACT_P_CREATED;
+	} else {
+		if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
+			err = -EEXIST;
+			goto release_idr;
+		}
+	}
+	p = to_blockcast_act(*a);
+
+	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
+	if (err < 0)
+		goto release_idr;
+
+	if (exists) {
+		spin_lock_bh(&p->tcf_lock);
+		goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
+
+		if (tb[TCA_BLOCKCAST_TX_TYPE])
+			p->tx_type = nla_get_u8(tb[TCA_BLOCKCAST_TX_TYPE]);
+
+		p->blockid = parm->blockid ?: p->blockid;
+
+		spin_unlock_bh(&p->tcf_lock);
+	} else {
+		goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
+
+		/** Default to broadcast if none specified */
+		if (tb[TCA_BLOCKCAST_TX_TYPE])
+			p->tx_type = nla_get_u8(tb[TCA_BLOCKCAST_TX_TYPE]);
+		else
+			p->tx_type = TCA_BLOCKCAST_TX_TYPE_BROADCAST;
+
+		p->blockid = parm->blockid;
+	}
+
+	if (goto_ch)
+		tcf_chain_put_by_act(goto_ch);
+
+	return ret;
+
+release_idr:
+	tcf_idr_release(*a, bind);
+	return err;
+}
+
+static int tcf_blockcast_dump(struct sk_buff *skb, struct tc_action *a,
+			      int bind, int ref)
+{
+	unsigned char *b = skb_tail_pointer(skb);
+	struct tcf_blockcast_act *p = to_blockcast_act(a);
+	struct tc_blockcast opt = {
+		.index   = p->tcf_index,
+		.refcnt  = refcount_read(&p->tcf_refcnt) - ref,
+		.bindcnt = atomic_read(&p->tcf_bindcnt) - bind,
+	};
+	struct tcf_t t;
+
+	spin_lock_bh(&p->tcf_lock);
+	opt.action = p->tcf_action;
+	opt.blockid = p->blockid;
+	if (nla_put(skb, TCA_BLOCKCAST_PARMS, sizeof(opt), &opt))
+		goto nla_put_failure;
+
+	tcf_tm_dump(&t, &p->tcf_tm);
+	if (nla_put_64bit(skb, TCA_BLOCKCAST_TM, sizeof(t), &t,
+			  TCA_BLOCKCAST_PAD))
+		goto nla_put_failure;
+
+	if (nla_put_u8(skb, TCA_BLOCKCAST_TX_TYPE, p->tx_type))
+		goto nla_put_failure;
+
+	spin_unlock_bh(&p->tcf_lock);
+
+	return skb->len;
+
+nla_put_failure:
+	spin_unlock_bh(&p->tcf_lock);
+	nlmsg_trim(skb, b);
+	return -1;
+}
+
+static struct tc_action_ops act_blockcast_ops = {
+	.kind		=	"blockcast",
+	.id		=	TCA_ID_BLOCKCAST,
+	.owner		=	THIS_MODULE,
+	.act		=	tcf_blockcast_act,
+	.dump		=	tcf_blockcast_dump,
+	.init		=	tcf_blockcast_init,
+	.size		=	sizeof(struct tcf_blockcast_act),
+};
+
+static __net_init int blockcast_init_net(struct net *net)
+{
+	struct tc_action_net *tn = net_generic(net, act_blockcast_ops.net_id);
+
+	return tc_action_net_init(net, tn, &act_blockcast_ops);
+}
+
+static void __net_exit blockcast_exit_net(struct list_head *net_list)
+{
+	tc_action_net_exit(net_list, act_blockcast_ops.net_id);
+}
+
+static struct pernet_operations blockcast_net_ops = {
+	.init = blockcast_init_net,
+	.exit_batch = blockcast_exit_net,
+	.id   = &act_blockcast_ops.net_id,
+	.size = sizeof(struct tc_action_net),
+};
+
+MODULE_AUTHOR("Mojatatu Networks, Inc");
+MODULE_DESCRIPTION("Action to broadcast to devices on a block");
+MODULE_LICENSE("GPL");
+
+static int __init blockcast_init_module(void)
+{
+	int ret = tcf_register_action(&act_blockcast_ops, &blockcast_net_ops);
+
+	if (!ret)
+		pr_info("blockcast TC action Loaded\n");
+	return ret;
+}
+
+static void __exit blockcast_cleanup_module(void)
+{
+	tcf_unregister_action(&act_blockcast_ops, &blockcast_net_ops);
+}
+
+module_init(blockcast_init_module);
+module_exit(blockcast_cleanup_module);
-- 
2.25.1

Re: [PATCH net-next RFC v5 1/4] net/sched: act_mirred: Separate mirror and redirect into two distinct functions

[kernel test robot]

Hi Victor,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Victor-Nogueira/net-sched-act_mirred-Separate-mirror-and-redirect-into-two-distinct-functions/20231111-054809
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231110214618.1883611-2-victor%40mojatatu.com
patch subject: [PATCH net-next RFC v5 1/4] net/sched: act_mirred: Separate mirror and redirect into two distinct functions
config: arc-randconfig-001-20231111 (https://download.01.org/0day-ci/archive/20231111/202311110831.UxXQMpbG-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231111/202311110831.UxXQMpbG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311110831.UxXQMpbG-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/net/pkt_cls.h:8,
                    from drivers/net/netdevsim/netdev.c:23:
   include/net/act_api.h: In function 'tcf_mirred_forward':
   include/net/act_api.h:303:23: error: implicit declaration of function 'tcf_dev_queue_xmit'; did you mean 'dev_queue_xmit'? [-Werror=implicit-function-declaration]
     303 |                 err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
         |                       ^~~~~~~~~~~~~~~~~~
         |                       dev_queue_xmit
   include/net/act_api.h: In function 'tcf_redirect_act':
>> include/net/act_api.h:371:36: error: 'struct sk_buff' has no member named 'tc_at_ingress'
     371 |         skb_set_redirected(skb, skb->tc_at_ingress);
         |                                    ^~
   cc1: some warnings being treated as errors


vim +371 include/net/act_api.h

   366	
   367	static inline int
   368	tcf_redirect_act(struct sk_buff *skb,
   369			 bool nested_call, bool want_ingress)
   370	{
 > 371		skb_set_redirected(skb, skb->tc_at_ingress);
   372	
   373		return tcf_mirred_forward(want_ingress, nested_call, skb);
   374	}
   375	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next RFC v5 1/4] net/sched: act_mirred: Separate mirror and redirect into two distinct functions

[kernel test robot]

Hi Victor,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Victor-Nogueira/net-sched-act_mirred-Separate-mirror-and-redirect-into-two-distinct-functions/20231111-054809
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231110214618.1883611-2-victor%40mojatatu.com
patch subject: [PATCH net-next RFC v5 1/4] net/sched: act_mirred: Separate mirror and redirect into two distinct functions
config: um-allyesconfig (https://download.01.org/0day-ci/archive/20231111/202311110834.bxpemlwC-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231111/202311110834.bxpemlwC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311110834.bxpemlwC-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/core/flow_dissector.c:3:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from net/core/flow_dissector.c:3:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from net/core/flow_dissector.c:3:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   In file included from net/core/flow_dissector.c:30:
   In file included from include/net/pkt_cls.h:8:
>> include/net/act_api.h:303:9: error: implicit declaration of function 'tcf_dev_queue_xmit' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                   err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
                         ^
   include/net/act_api.h:303:9: note: did you mean '__dev_queue_xmit'?
   include/linux/netdevice.h:3107:5: note: '__dev_queue_xmit' declared here
   int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev);
       ^
   In file included from net/core/flow_dissector.c:30:
   In file included from include/net/pkt_cls.h:8:
   include/net/act_api.h:371:31: error: no member named 'tc_at_ingress' in 'struct sk_buff'
           skb_set_redirected(skb, skb->tc_at_ingress);
                                   ~~~  ^
   12 warnings and 2 errors generated.


vim +/tcf_dev_queue_xmit +303 include/net/act_api.h

   296	
   297	static inline int tcf_mirred_forward(bool to_ingress, bool nested_call,
   298					     struct sk_buff *skb)
   299	{
   300		int err;
   301	
   302		if (!to_ingress)
 > 303			err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
   304		else if (nested_call)
   305			err = netif_rx(skb);
   306		else
   307			err = netif_receive_skb(skb);
   308	
   309		return err;
   310	}
   311	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next RFC v5 1/4] net/sched: act_mirred: Separate mirror and redirect into two distinct functions

[Jiri Pirko]

Fri, Nov 10, 2023 at 10:46:15PM CET, victor@mojatatu.com wrote:
>Separate mirror and redirect code into two into two separate functions
>(tcf_mirror_act and tcf_redirect_act). This not only cleans up the code and
>improves both readability and maintainability in addition to reducing the
>complexity given different expectations for mirroring and redirecting.
>
>This patchset has a use case for the mirror part in action "blockcast".

Patchset? Which one? You mean this patch?

>
>Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
>Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
>Signed-off-by: Victor Nogueira <victor@mojatatu.com>
>---
> include/net/act_api.h  |  85 ++++++++++++++++++++++++++++++++++
> net/sched/act_mirred.c | 103 +++++++++++------------------------------
> 2 files changed, 113 insertions(+), 75 deletions(-)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index 4ae0580b63ca..8d288040aeb8 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -12,6 +12,7 @@
> #include <net/pkt_sched.h>
> #include <net/net_namespace.h>
> #include <net/netns/generic.h>
>+#include <net/dst.h>
> 
> struct tcf_idrinfo {
> 	struct mutex	lock;
>@@ -293,5 +294,89 @@ static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
> #endif
> }
> 
>+static inline int tcf_mirred_forward(bool to_ingress, bool nested_call,
>+				     struct sk_buff *skb)
>+{
>+	int err;
>+
>+	if (!to_ingress)
>+		err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
>+	else if (nested_call)
>+		err = netif_rx(skb);
>+	else
>+		err = netif_receive_skb(skb);
>+
>+	return err;
>+}
>+
>+static inline struct sk_buff *
>+tcf_mirred_common(struct sk_buff *skb, bool want_ingress, bool dont_clone,
>+		  bool expects_nh, struct net_device *dest_dev)
>+{
>+	struct sk_buff *skb_to_send = skb;
>+	bool at_ingress;
>+	int mac_len;
>+	bool at_nh;
>+	int err;
>+
>+	if (unlikely(!(dest_dev->flags & IFF_UP)) ||
>+	    !netif_carrier_ok(dest_dev)) {
>+		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
>+				       dest_dev->name);
>+		err = -ENODEV;
>+		goto err_out;
>+	}
>+
>+	if (!dont_clone) {
>+		skb_to_send = skb_clone(skb, GFP_ATOMIC);
>+		if (!skb_to_send) {
>+			err =  -ENOMEM;
>+			goto err_out;
>+		}
>+	}
>+
>+	at_ingress = skb_at_tc_ingress(skb);
>+
>+	/* All mirred/redirected skbs should clear previous ct info */
>+	nf_reset_ct(skb_to_send);
>+	if (want_ingress && !at_ingress) /* drop dst for egress -> ingress */
>+		skb_dst_drop(skb_to_send);
>+
>+	at_nh = skb->data == skb_network_header(skb);
>+	if (at_nh != expects_nh) {
>+		mac_len = at_ingress ? skb->mac_len :
>+			  skb_network_offset(skb);
>+		if (expects_nh) {
>+			/* target device/action expect data at nh */
>+			skb_pull_rcsum(skb_to_send, mac_len);
>+		} else {
>+			/* target device/action expect data at mac */
>+			skb_push_rcsum(skb_to_send, mac_len);
>+		}
>+	}
>+
>+	skb_to_send->skb_iif = skb->dev->ifindex;
>+	skb_to_send->dev = dest_dev;
>+
>+	return skb_to_send;
>+
>+err_out:
>+	return ERR_PTR(err);
>+}

It's odd to see functions this size as inlined in header files. I don't
think that is good idea.


>+
>+static inline int
>+tcf_redirect_act(struct sk_buff *skb,
>+		 bool nested_call, bool want_ingress)
>+{
>+	skb_set_redirected(skb, skb->tc_at_ingress);
>+
>+	return tcf_mirred_forward(want_ingress, nested_call, skb);
>+}
>+
>+static inline int
>+tcf_mirror_act(struct sk_buff *skb, bool nested_call, bool want_ingress)
>+{
>+	return tcf_mirred_forward(want_ingress, nested_call, skb);
>+}
> 
> #endif
>diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>index 0a711c184c29..95d30cb06e54 100644
>--- a/net/sched/act_mirred.c
>+++ b/net/sched/act_mirred.c
>@@ -211,38 +211,22 @@ static bool is_mirred_nested(void)
> 	return unlikely(__this_cpu_read(mirred_nest_level) > 1);
> }
> 
>-static int tcf_mirred_forward(bool want_ingress, struct sk_buff *skb)
>-{
>-	int err;
>-
>-	if (!want_ingress)
>-		err = tcf_dev_queue_xmit(skb, dev_queue_xmit);
>-	else if (is_mirred_nested())
>-		err = netif_rx(skb);
>-	else
>-		err = netif_receive_skb(skb);
>-
>-	return err;
>-}
>-
> TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
> 				     const struct tc_action *a,
> 				     struct tcf_result *res)
> {
> 	struct tcf_mirred *m = to_mirred(a);
>-	struct sk_buff *skb2 = skb;
>+	struct sk_buff *skb_to_send;
>+	unsigned int nest_level;
> 	bool m_mac_header_xmit;
> 	struct net_device *dev;
>-	unsigned int nest_level;
>-	int retval, err = 0;
>-	bool use_reinsert;
> 	bool want_ingress;
> 	bool is_redirect;
> 	bool expects_nh;
>-	bool at_ingress;
>+	bool dont_clone;
> 	int m_eaction;
>-	int mac_len;
>-	bool at_nh;
>+	int err = 0;
>+	int retval;
> 
> 	nest_level = __this_cpu_inc_return(mirred_nest_level);
> 	if (unlikely(nest_level > MIRRED_NEST_LIMIT)) {
>@@ -255,80 +239,49 @@ TC_INDIRECT_SCOPE int tcf_mirred_act(struct sk_buff *skb,
> 	tcf_lastuse_update(&m->tcf_tm);
> 	tcf_action_update_bstats(&m->common, skb);
> 
>-	m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
> 	m_eaction = READ_ONCE(m->tcfm_eaction);
>-	retval = READ_ONCE(m->tcf_action);
>+	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
>+	retval = READ_ONCE(a->tcfa_action);
> 	dev = rcu_dereference_bh(m->tcfm_dev);
> 	if (unlikely(!dev)) {
> 		pr_notice_once("tc mirred: target device is gone\n");
>+		err = -ENODEV;
> 		goto out;
> 	}
> 
>-	if (unlikely(!(dev->flags & IFF_UP)) || !netif_carrier_ok(dev)) {
>-		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
>-				       dev->name);
>-		goto out;
>-	}
>+	m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
>+	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
>+	expects_nh = want_ingress || !m_mac_header_xmit;
> 
> 	/* we could easily avoid the clone only if called by ingress and clsact;
> 	 * since we can't easily detect the clsact caller, skip clone only for
> 	 * ingress - that covers the TC S/W datapath.
> 	 */
>-	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
>-	at_ingress = skb_at_tc_ingress(skb);
>-	use_reinsert = at_ingress && is_redirect &&
>-		       tcf_mirred_can_reinsert(retval);
>-	if (!use_reinsert) {
>-		skb2 = skb_clone(skb, GFP_ATOMIC);
>-		if (!skb2)
>-			goto out;
>-	}
>-
>-	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
>-
>-	/* All mirred/redirected skbs should clear previous ct info */
>-	nf_reset_ct(skb2);
>-	if (want_ingress && !at_ingress) /* drop dst for egress -> ingress */
>-		skb_dst_drop(skb2);
>+	dont_clone = skb_at_tc_ingress(skb) && is_redirect &&
>+		     tcf_mirred_can_reinsert(retval);
> 
>-	expects_nh = want_ingress || !m_mac_header_xmit;
>-	at_nh = skb->data == skb_network_header(skb);
>-	if (at_nh != expects_nh) {
>-		mac_len = skb_at_tc_ingress(skb) ? skb->mac_len :
>-			  skb_network_offset(skb);
>-		if (expects_nh) {
>-			/* target device/action expect data at nh */
>-			skb_pull_rcsum(skb2, mac_len);
>-		} else {
>-			/* target device/action expect data at mac */
>-			skb_push_rcsum(skb2, mac_len);
>-		}
>+	skb_to_send = tcf_mirred_common(skb, want_ingress, dont_clone,
>+					expects_nh, dev);
>+	if (IS_ERR(skb_to_send)) {
>+		err = PTR_ERR(skb_to_send);
>+		goto out;
> 	}
> 
>-	skb2->skb_iif = skb->dev->ifindex;
>-	skb2->dev = dev;
>-
>-	/* mirror is always swallowed */
> 	if (is_redirect) {
>-		skb_set_redirected(skb2, skb2->tc_at_ingress);
>-
>-		/* let's the caller reinsert the packet, if possible */
>-		if (use_reinsert) {
>-			err = tcf_mirred_forward(want_ingress, skb);
>-			if (err)
>-				tcf_action_inc_overlimit_qstats(&m->common);
>-			__this_cpu_dec(mirred_nest_level);
>-			return TC_ACT_CONSUMED;
>-		}
>+		if (skb == skb_to_send)
>+			retval = TC_ACT_CONSUMED;
>+
>+		err = tcf_redirect_act(skb_to_send, is_mirred_nested(),
>+				       want_ingress);
>+	} else {
>+		err = tcf_mirror_act(skb_to_send, is_mirred_nested(),
>+				     want_ingress);
> 	}
> 
>-	err = tcf_mirred_forward(want_ingress, skb2);
>-	if (err) {
> out:
>+	if (err)
> 		tcf_action_inc_overlimit_qstats(&m->common);
>-		if (tcf_mirred_is_act_redirect(m_eaction))
>-			retval = TC_ACT_SHOT;
>-	}
>+
> 	__this_cpu_dec(mirred_nest_level);
> 
> 	return retval;
>-- 
>2.25.1
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>This action takes advantage of the presence of tc block ports set in the
>datapath and multicasts a packet to ports on a block. By default, it will
>broadcast the packet to a block, that is send to all members of the block except
>the port in which the packet arrived on. However, the user may specify
>the option "tx_type all", which will send the packet to all members of the
>block indiscriminately.
>
>Example usage:
>    $ tc qdisc add dev ens7 ingress_block 22
>    $ tc qdisc add dev ens8 ingress_block 22
>
>Now we can add a filter to broadcast packets to ports on ingress block id 22:
>$ tc filter add block 22 protocol ip pref 25 \
>  flower dst_ip 192.168.0.0/16 action blockcast blockid 22

Name the arg "block" so it is consistent with "filter add block". Make
sure this is aligned netlink-wise as well.


>
>Or if we wish to send to all ports in the block:
>$ tc filter add block 22 protocol ip pref 25 \
>  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all

I read the discussion the the previous version again. I suggested this
to be part of mirred. Why exactly that was not addressed?

Instead of:
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
You'd have:
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22

I don't see why we need special action for this.

Regarding "tx_type all":
Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
to have this as "no_src_skip" or some other similar arg, without value
acting as a bool (flag) on netlink level.

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> >This action takes advantage of the presence of tc block ports set in the
> >datapath and multicasts a packet to ports on a block. By default, it will
> >broadcast the packet to a block, that is send to all members of the block except
> >the port in which the packet arrived on. However, the user may specify
> >the option "tx_type all", which will send the packet to all members of the
> >block indiscriminately.
> >
> >Example usage:
> >    $ tc qdisc add dev ens7 ingress_block 22
> >    $ tc qdisc add dev ens8 ingress_block 22
> >
> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> >$ tc filter add block 22 protocol ip pref 25 \
> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>
> Name the arg "block" so it is consistent with "filter add block". Make
> sure this is aligned netlink-wise as well.
>
>
> >
> >Or if we wish to send to all ports in the block:
> >$ tc filter add block 22 protocol ip pref 25 \
> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>
> I read the discussion the the previous version again. I suggested this
> to be part of mirred. Why exactly that was not addressed?
>

I am the one who pushed back (in that discussion). Actions should be
small and specific. Like i had said in that earlier discussion it was
a mistake to make mirred do both mirror and redirect - they should
have been two actions. So i feel like adding a block to mirred is
adding more knobs. We are also going to add dev->group as a way to
select what devices to mirror to. Should that be in mirred as well?

cheers,
jamal

> Instead of:
> $ tc filter add block 22 protocol ip pref 25 \
>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> You'd have:
> $ tc filter add block 22 protocol ip pref 25 \
>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>
> I don't see why we need special action for this.
>
> Regarding "tx_type all":
> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> to have this as "no_src_skip" or some other similar arg, without value
> acting as a bool (flag) on netlink level.
>
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
>On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> >This action takes advantage of the presence of tc block ports set in the
>> >datapath and multicasts a packet to ports on a block. By default, it will
>> >broadcast the packet to a block, that is send to all members of the block except
>> >the port in which the packet arrived on. However, the user may specify
>> >the option "tx_type all", which will send the packet to all members of the
>> >block indiscriminately.
>> >
>> >Example usage:
>> >    $ tc qdisc add dev ens7 ingress_block 22
>> >    $ tc qdisc add dev ens8 ingress_block 22
>> >
>> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> >$ tc filter add block 22 protocol ip pref 25 \
>> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>>
>> Name the arg "block" so it is consistent with "filter add block". Make
>> sure this is aligned netlink-wise as well.
>>
>>
>> >
>> >Or if we wish to send to all ports in the block:
>> >$ tc filter add block 22 protocol ip pref 25 \
>> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>>
>> I read the discussion the the previous version again. I suggested this
>> to be part of mirred. Why exactly that was not addressed?
>>
>
>I am the one who pushed back (in that discussion). Actions should be
>small and specific. Like i had said in that earlier discussion it was
>a mistake to make mirred do both mirror and redirect - they should

For mirror and redirect, I agree. For redirect and redirect, does not
make much sense. It's just confusing for the user.


>have been two actions. So i feel like adding a block to mirred is
>adding more knobs. We are also going to add dev->group as a way to
>select what devices to mirror to. Should that be in mirred as well?

I need more details.


>
>cheers,
>jamal
>
>> Instead of:
>> $ tc filter add block 22 protocol ip pref 25 \
>>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> You'd have:
>> $ tc filter add block 22 protocol ip pref 25 \
>>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>>
>> I don't see why we need special action for this.
>>
>> Regarding "tx_type all":
>> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> to have this as "no_src_skip" or some other similar arg, without value
>> acting as a bool (flag) on netlink level.
>>
>>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Marcelo Ricardo Leitner]

On Thu, Nov 23, 2023 at 08:37:13AM -0500, Jamal Hadi Salim wrote:
> On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >
> > Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> > >This action takes advantage of the presence of tc block ports set in the
> > >datapath and multicasts a packet to ports on a block. By default, it will
> > >broadcast the packet to a block, that is send to all members of the block except
> > >the port in which the packet arrived on. However, the user may specify
> > >the option "tx_type all", which will send the packet to all members of the
> > >block indiscriminately.
> > >
> > >Example usage:
> > >    $ tc qdisc add dev ens7 ingress_block 22
> > >    $ tc qdisc add dev ens8 ingress_block 22
> > >
> > >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> > >$ tc filter add block 22 protocol ip pref 25 \
> > >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >
> > Name the arg "block" so it is consistent with "filter add block". Make
> > sure this is aligned netlink-wise as well.
> >
> >
> > >
> > >Or if we wish to send to all ports in the block:
> > >$ tc filter add block 22 protocol ip pref 25 \
> > >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> >
> > I read the discussion the the previous version again. I suggested this
> > to be part of mirred. Why exactly that was not addressed?
> >
>
> I am the one who pushed back (in that discussion). Actions should be

Me too, and actually I thought Jiri had agreed to it with some
remarks to be addressed (which I don't know if there were, I didn't
read this version yet).

> small and specific. Like i had said in that earlier discussion it was
> a mistake to make mirred do both mirror and redirect - they should
> have been two actions. So i feel like adding a block to mirred is
> adding more knobs. We are also going to add dev->group as a way to
> select what devices to mirror to. Should that be in mirred as well?
>
> cheers,
> jamal
>
> > Instead of:
> > $ tc filter add block 22 protocol ip pref 25 \
> >   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> > You'd have:
> > $ tc filter add block 22 protocol ip pref 25 \
> >   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> >
> > I don't see why we need special action for this.
> >
> > Regarding "tx_type all":
> > Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> > to have this as "no_src_skip" or some other similar arg, without value
> > acting as a bool (flag) on netlink level.
> >
> >
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> >> >This action takes advantage of the presence of tc block ports set in the
> >> >datapath and multicasts a packet to ports on a block. By default, it will
> >> >broadcast the packet to a block, that is send to all members of the block except
> >> >the port in which the packet arrived on. However, the user may specify
> >> >the option "tx_type all", which will send the packet to all members of the
> >> >block indiscriminately.
> >> >
> >> >Example usage:
> >> >    $ tc qdisc add dev ens7 ingress_block 22
> >> >    $ tc qdisc add dev ens8 ingress_block 22
> >> >
> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >>
> >> Name the arg "block" so it is consistent with "filter add block". Make
> >> sure this is aligned netlink-wise as well.
> >>
> >>
> >> >
> >> >Or if we wish to send to all ports in the block:
> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> >>
> >> I read the discussion the the previous version again. I suggested this
> >> to be part of mirred. Why exactly that was not addressed?
> >>
> >
> >I am the one who pushed back (in that discussion). Actions should be
> >small and specific. Like i had said in that earlier discussion it was
> >a mistake to make mirred do both mirror and redirect - they should
>
> For mirror and redirect, I agree. For redirect and redirect, does not
> make much sense. It's just confusing for the user.
>

Blockcast only emulates the mirror part. I agree redirect doesnt make
any sense because once you redirect the packet is gone.

> >have been two actions. So i feel like adding a block to mirred is
> >adding more knobs. We are also going to add dev->group as a way to
> >select what devices to mirror to. Should that be in mirred as well?
>
> I need more details.
>

You set any port you want to be mirrored to using ip link, example:
ip link set dev $DEV1 group 2
ip link set dev $DEV2 group 2
...

Then you can blockcast:
tc filter add devx protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action blockcast group 2

cheers,
jamal

>
> >
> >cheers,
> >jamal
> >
> >> Instead of:
> >> $ tc filter add block 22 protocol ip pref 25 \
> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> You'd have:
> >> $ tc filter add block 22 protocol ip pref 25 \
> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> >>
> >> I don't see why we need special action for this.
> >>
> >> Regarding "tx_type all":
> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> >> to have this as "no_src_skip" or some other similar arg, without value
> >> acting as a bool (flag) on netlink level.
> >>
> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
>On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
>> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> >> >This action takes advantage of the presence of tc block ports set in the
>> >> >datapath and multicasts a packet to ports on a block. By default, it will
>> >> >broadcast the packet to a block, that is send to all members of the block except
>> >> >the port in which the packet arrived on. However, the user may specify
>> >> >the option "tx_type all", which will send the packet to all members of the
>> >> >block indiscriminately.
>> >> >
>> >> >Example usage:
>> >> >    $ tc qdisc add dev ens7 ingress_block 22
>> >> >    $ tc qdisc add dev ens8 ingress_block 22
>> >> >
>> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >>
>> >> Name the arg "block" so it is consistent with "filter add block". Make
>> >> sure this is aligned netlink-wise as well.
>> >>
>> >>
>> >> >
>> >> >Or if we wish to send to all ports in the block:
>> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>> >>
>> >> I read the discussion the the previous version again. I suggested this
>> >> to be part of mirred. Why exactly that was not addressed?
>> >>
>> >
>> >I am the one who pushed back (in that discussion). Actions should be
>> >small and specific. Like i had said in that earlier discussion it was
>> >a mistake to make mirred do both mirror and redirect - they should
>>
>> For mirror and redirect, I agree. For redirect and redirect, does not
>> make much sense. It's just confusing for the user.
>>
>
>Blockcast only emulates the mirror part. I agree redirect doesnt make
>any sense because once you redirect the packet is gone.

How is it mirror? It is redirect to multiple, isn't it?


>
>> >have been two actions. So i feel like adding a block to mirred is
>> >adding more knobs. We are also going to add dev->group as a way to
>> >select what devices to mirror to. Should that be in mirred as well?
>>
>> I need more details.
>>
>
>You set any port you want to be mirrored to using ip link, example:
>ip link set dev $DEV1 group 2
>ip link set dev $DEV2 group 2

That does not looks correct at all. Do tc stuff in tc, no?


>...
>
>Then you can blockcast:
>tc filter add devx protocol ip pref 25 \
>  flower dst_ip 192.168.0.0/16 action blockcast group 2

"blockcasting" to something that is not a block anymore. Not nice.

>
>cheers,
>jamal
>
>>
>> >
>> >cheers,
>> >jamal
>> >
>> >> Instead of:
>> >> $ tc filter add block 22 protocol ip pref 25 \
>> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> You'd have:
>> >> $ tc filter add block 22 protocol ip pref 25 \
>> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >>
>> >> I don't see why we need special action for this.
>> >>
>> >> Regarding "tx_type all":
>> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> >> to have this as "no_src_skip" or some other similar arg, without value
>> >> acting as a bool (flag) on netlink level.
>> >>
>> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Thu, Nov 23, 2023 at 03:29:00PM CET, mleitner@redhat.com wrote:
>On Thu, Nov 23, 2023 at 08:37:13AM -0500, Jamal Hadi Salim wrote:
>> On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> > Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> > >This action takes advantage of the presence of tc block ports set in the
>> > >datapath and multicasts a packet to ports on a block. By default, it will
>> > >broadcast the packet to a block, that is send to all members of the block except
>> > >the port in which the packet arrived on. However, the user may specify
>> > >the option "tx_type all", which will send the packet to all members of the
>> > >block indiscriminately.
>> > >
>> > >Example usage:
>> > >    $ tc qdisc add dev ens7 ingress_block 22
>> > >    $ tc qdisc add dev ens8 ingress_block 22
>> > >
>> > >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> > >$ tc filter add block 22 protocol ip pref 25 \
>> > >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >
>> > Name the arg "block" so it is consistent with "filter add block". Make
>> > sure this is aligned netlink-wise as well.
>> >
>> >
>> > >
>> > >Or if we wish to send to all ports in the block:
>> > >$ tc filter add block 22 protocol ip pref 25 \
>> > >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>> >
>> > I read the discussion the the previous version again. I suggested this
>> > to be part of mirred. Why exactly that was not addressed?
>> >
>>
>> I am the one who pushed back (in that discussion). Actions should be
>
>Me too, and actually I thought Jiri had agreed to it with some
>remarks to be addressed (which I don't know if there were, I didn't
>read this version yet).

I looked today and didn't' find it. If I missed it, sorry. I still don't
understand why new action is needed for redirect to X instead of Y.

>
>> small and specific. Like i had said in that earlier discussion it was
>> a mistake to make mirred do both mirror and redirect - they should
>> have been two actions. So i feel like adding a block to mirred is
>> adding more knobs. We are also going to add dev->group as a way to
>> select what devices to mirror to. Should that be in mirred as well?
>>
>> cheers,
>> jamal
>>
>> > Instead of:
>> > $ tc filter add block 22 protocol ip pref 25 \
>> >   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> > You'd have:
>> > $ tc filter add block 22 protocol ip pref 25 \
>> >   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >
>> > I don't see why we need special action for this.
>> >
>> > Regarding "tx_type all":
>> > Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> > to have this as "no_src_skip" or some other similar arg, without value
>> > acting as a bool (flag) on netlink level.
>> >
>> >
>>
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> >> >> >This action takes advantage of the presence of tc block ports set in the
> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
> >> >> >broadcast the packet to a block, that is send to all members of the block except
> >> >> >the port in which the packet arrived on. However, the user may specify
> >> >> >the option "tx_type all", which will send the packet to all members of the
> >> >> >block indiscriminately.
> >> >> >
> >> >> >Example usage:
> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
> >> >> >
> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >>
> >> >> Name the arg "block" so it is consistent with "filter add block". Make
> >> >> sure this is aligned netlink-wise as well.
> >> >>
> >> >>
> >> >> >
> >> >> >Or if we wish to send to all ports in the block:
> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> >> >>
> >> >> I read the discussion the the previous version again. I suggested this
> >> >> to be part of mirred. Why exactly that was not addressed?
> >> >>
> >> >
> >> >I am the one who pushed back (in that discussion). Actions should be
> >> >small and specific. Like i had said in that earlier discussion it was
> >> >a mistake to make mirred do both mirror and redirect - they should
> >>
> >> For mirror and redirect, I agree. For redirect and redirect, does not
> >> make much sense. It's just confusing for the user.
> >>
> >
> >Blockcast only emulates the mirror part. I agree redirect doesnt make
> >any sense because once you redirect the packet is gone.
>
> How is it mirror? It is redirect to multiple, isn't it?

mirror has been used (so far in mirred action and i believe in the
industry in general) to mean  "send a copy of the packet" - meaning
you can send to many ports and even when you are done sending to all
those ports the packet is still in the pipeline and you can continue
to execute other action on it. Whereas redirect means the packet is
stolen from the pipeline i.e if you redirect to a port the packet is
not available to redirect to the next port or for any other action
after that.
You could argue a loose interpretation of redirect to a block to mean
"mirror to all ports on the block but on the last port redirect".

>
> >
> >> >have been two actions. So i feel like adding a block to mirred is
> >> >adding more knobs. We are also going to add dev->group as a way to
> >> >select what devices to mirror to. Should that be in mirred as well?
> >>
> >> I need more details.
> >>
> >
> >You set any port you want to be mirrored to using ip link, example:
> >ip link set dev $DEV1 group 2
> >ip link set dev $DEV2 group 2
>
> That does not looks correct at all. Do tc stuff in tc, no?
>

We could certainly annotate the dev group via tc but it seems odd ....

cheers,
jamal
>
> >...
> >
> >Then you can blockcast:
> >tc filter add devx protocol ip pref 25 \
> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
>
> "blockcasting" to something that is not a block anymore. Not nice.
>
> >
> >cheers,
> >jamal
> >
> >>
> >> >
> >> >cheers,
> >> >jamal
> >> >
> >> >> Instead of:
> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >> You'd have:
> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> >> >>
> >> >> I don't see why we need special action for this.
> >> >>
> >> >> Regarding "tx_type all":
> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> >> >> to have this as "no_src_skip" or some other similar arg, without value
> >> >> acting as a bool (flag) on netlink level.
> >> >>
> >> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> >> >> >This action takes advantage of the presence of tc block ports set in the
> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
> >> >> >broadcast the packet to a block, that is send to all members of the block except
> >> >> >the port in which the packet arrived on. However, the user may specify
> >> >> >the option "tx_type all", which will send the packet to all members of the
> >> >> >block indiscriminately.
> >> >> >
> >> >> >Example usage:
> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
> >> >> >
> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >>
> >> >> Name the arg "block" so it is consistent with "filter add block". Make
> >> >> sure this is aligned netlink-wise as well.
> >> >>
> >> >>
> >> >> >
> >> >> >Or if we wish to send to all ports in the block:
> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> >> >>
> >> >> I read the discussion the the previous version again. I suggested this
> >> >> to be part of mirred. Why exactly that was not addressed?
> >> >>
> >> >
> >> >I am the one who pushed back (in that discussion). Actions should be
> >> >small and specific. Like i had said in that earlier discussion it was
> >> >a mistake to make mirred do both mirror and redirect - they should
> >>
> >> For mirror and redirect, I agree. For redirect and redirect, does not
> >> make much sense. It's just confusing for the user.
> >>
> >
> >Blockcast only emulates the mirror part. I agree redirect doesnt make
> >any sense because once you redirect the packet is gone.
>
> How is it mirror? It is redirect to multiple, isn't it?
>
>
> >
> >> >have been two actions. So i feel like adding a block to mirred is
> >> >adding more knobs. We are also going to add dev->group as a way to
> >> >select what devices to mirror to. Should that be in mirred as well?
> >>
> >> I need more details.
> >>
> >
> >You set any port you want to be mirrored to using ip link, example:
> >ip link set dev $DEV1 group 2
> >ip link set dev $DEV2 group 2
>
> That does not looks correct at all. Do tc stuff in tc, no?
>
>
> >...
> >
> >Then you can blockcast:
> >tc filter add devx protocol ip pref 25 \
> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
>
> "blockcasting" to something that is not a block anymore. Not nice.
>

Sorry, missed this one. Yes blockcasting is no longer appropriate  -
perhaps a different action altogether.

cheers,
jamal
> >
> >cheers,
> >jamal
> >
> >>
> >> >
> >> >cheers,
> >> >jamal
> >> >
> >> >> Instead of:
> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >> You'd have:
> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> >> >>
> >> >> I don't see why we need special action for this.
> >> >>
> >> >> Regarding "tx_type all":
> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> >> >> to have this as "no_src_skip" or some other similar arg, without value
> >> >> acting as a bool (flag) on netlink level.
> >> >>
> >> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Thu, Nov 23, 2023 at 05:20:27PM CET, hadi@mojatatu.com wrote:
>On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
>> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
>> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> >> >> >This action takes advantage of the presence of tc block ports set in the
>> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
>> >> >> >broadcast the packet to a block, that is send to all members of the block except
>> >> >> >the port in which the packet arrived on. However, the user may specify
>> >> >> >the option "tx_type all", which will send the packet to all members of the
>> >> >> >block indiscriminately.
>> >> >> >
>> >> >> >Example usage:
>> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
>> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
>> >> >> >
>> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> >>
>> >> >> Name the arg "block" so it is consistent with "filter add block". Make
>> >> >> sure this is aligned netlink-wise as well.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >Or if we wish to send to all ports in the block:
>> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>> >> >>
>> >> >> I read the discussion the the previous version again. I suggested this
>> >> >> to be part of mirred. Why exactly that was not addressed?
>> >> >>
>> >> >
>> >> >I am the one who pushed back (in that discussion). Actions should be
>> >> >small and specific. Like i had said in that earlier discussion it was
>> >> >a mistake to make mirred do both mirror and redirect - they should
>> >>
>> >> For mirror and redirect, I agree. For redirect and redirect, does not
>> >> make much sense. It's just confusing for the user.
>> >>
>> >
>> >Blockcast only emulates the mirror part. I agree redirect doesnt make
>> >any sense because once you redirect the packet is gone.
>>
>> How is it mirror? It is redirect to multiple, isn't it?
>
>mirror has been used (so far in mirred action and i believe in the
>industry in general) to mean  "send a copy of the packet" - meaning
>you can send to many ports and even when you are done sending to all
>those ports the packet is still in the pipeline and you can continue
>to execute other action on it. Whereas redirect means the packet is
>stolen from the pipeline i.e if you redirect to a port the packet is
>not available to redirect to the next port or for any other action
>after that.
>You could argue a loose interpretation of redirect to a block to mean
>"mirror to all ports on the block but on the last port redirect".

it's stolen from the pipeline, right? That would be redirect from my
perspective.


>
>>
>> >
>> >> >have been two actions. So i feel like adding a block to mirred is
>> >> >adding more knobs. We are also going to add dev->group as a way to
>> >> >select what devices to mirror to. Should that be in mirred as well?
>> >>
>> >> I need more details.
>> >>
>> >
>> >You set any port you want to be mirrored to using ip link, example:
>> >ip link set dev $DEV1 group 2
>> >ip link set dev $DEV2 group 2
>>
>> That does not looks correct at all. Do tc stuff in tc, no?
>>
>
>We could certainly annotate the dev group via tc but it seems odd ....
>
>cheers,
>jamal
>>
>> >...
>> >
>> >Then you can blockcast:
>> >tc filter add devx protocol ip pref 25 \
>> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
>>
>> "blockcasting" to something that is not a block anymore. Not nice.
>>
>> >
>> >cheers,
>> >jamal
>> >
>> >>
>> >> >
>> >> >cheers,
>> >> >jamal
>> >> >
>> >> >> Instead of:
>> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> >> You'd have:
>> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >> >>
>> >> >> I don't see why we need special action for this.
>> >> >>
>> >> >> Regarding "tx_type all":
>> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> >> >> to have this as "no_src_skip" or some other similar arg, without value
>> >> >> acting as a bool (flag) on netlink level.
>> >> >>
>> >> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@mojatatu.com wrote:
>On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
>> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
>> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> >> >> >This action takes advantage of the presence of tc block ports set in the
>> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
>> >> >> >broadcast the packet to a block, that is send to all members of the block except
>> >> >> >the port in which the packet arrived on. However, the user may specify
>> >> >> >the option "tx_type all", which will send the packet to all members of the
>> >> >> >block indiscriminately.
>> >> >> >
>> >> >> >Example usage:
>> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
>> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
>> >> >> >
>> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> >>
>> >> >> Name the arg "block" so it is consistent with "filter add block". Make
>> >> >> sure this is aligned netlink-wise as well.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >Or if we wish to send to all ports in the block:
>> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>> >> >>
>> >> >> I read the discussion the the previous version again. I suggested this
>> >> >> to be part of mirred. Why exactly that was not addressed?
>> >> >>
>> >> >
>> >> >I am the one who pushed back (in that discussion). Actions should be
>> >> >small and specific. Like i had said in that earlier discussion it was
>> >> >a mistake to make mirred do both mirror and redirect - they should
>> >>
>> >> For mirror and redirect, I agree. For redirect and redirect, does not
>> >> make much sense. It's just confusing for the user.
>> >>
>> >
>> >Blockcast only emulates the mirror part. I agree redirect doesnt make
>> >any sense because once you redirect the packet is gone.
>>
>> How is it mirror? It is redirect to multiple, isn't it?
>>
>>
>> >
>> >> >have been two actions. So i feel like adding a block to mirred is
>> >> >adding more knobs. We are also going to add dev->group as a way to
>> >> >select what devices to mirror to. Should that be in mirred as well?
>> >>
>> >> I need more details.
>> >>
>> >
>> >You set any port you want to be mirrored to using ip link, example:
>> >ip link set dev $DEV1 group 2
>> >ip link set dev $DEV2 group 2
>>
>> That does not looks correct at all. Do tc stuff in tc, no?
>>
>>
>> >...
>> >
>> >Then you can blockcast:
>> >tc filter add devx protocol ip pref 25 \
>> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
>>
>> "blockcasting" to something that is not a block anymore. Not nice.
>>
>
>Sorry, missed this one. Yes blockcasting is no longer appropriate  -
>perhaps a different action altogether.

mirret redirect? :)

With target of:
1) dev (the current one)
2) block
3) group
?


>
>cheers,
>jamal
>> >
>> >cheers,
>> >jamal
>> >
>> >>
>> >> >
>> >> >cheers,
>> >> >jamal
>> >> >
>> >> >> Instead of:
>> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> >> You'd have:
>> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >> >>
>> >> >> I don't see why we need special action for this.
>> >> >>
>> >> >> Regarding "tx_type all":
>> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> >> >> to have this as "no_src_skip" or some other similar arg, without value
>> >> >> acting as a bool (flag) on netlink level.
>> >> >>
>> >> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@mojatatu.com wrote:
> >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
> >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
> >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >>
> >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> >> >> >> >This action takes advantage of the presence of tc block ports set in the
> >> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
> >> >> >> >broadcast the packet to a block, that is send to all members of the block except
> >> >> >> >the port in which the packet arrived on. However, the user may specify
> >> >> >> >the option "tx_type all", which will send the packet to all members of the
> >> >> >> >block indiscriminately.
> >> >> >> >
> >> >> >> >Example usage:
> >> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
> >> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
> >> >> >> >
> >> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >> >>
> >> >> >> Name the arg "block" so it is consistent with "filter add block". Make
> >> >> >> sure this is aligned netlink-wise as well.
> >> >> >>
> >> >> >>
> >> >> >> >
> >> >> >> >Or if we wish to send to all ports in the block:
> >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> >> >> >>
> >> >> >> I read the discussion the the previous version again. I suggested this
> >> >> >> to be part of mirred. Why exactly that was not addressed?
> >> >> >>
> >> >> >
> >> >> >I am the one who pushed back (in that discussion). Actions should be
> >> >> >small and specific. Like i had said in that earlier discussion it was
> >> >> >a mistake to make mirred do both mirror and redirect - they should
> >> >>
> >> >> For mirror and redirect, I agree. For redirect and redirect, does not
> >> >> make much sense. It's just confusing for the user.
> >> >>
> >> >
> >> >Blockcast only emulates the mirror part. I agree redirect doesnt make
> >> >any sense because once you redirect the packet is gone.
> >>
> >> How is it mirror? It is redirect to multiple, isn't it?
> >>
> >>
> >> >
> >> >> >have been two actions. So i feel like adding a block to mirred is
> >> >> >adding more knobs. We are also going to add dev->group as a way to
> >> >> >select what devices to mirror to. Should that be in mirred as well?
> >> >>
> >> >> I need more details.
> >> >>
> >> >
> >> >You set any port you want to be mirrored to using ip link, example:
> >> >ip link set dev $DEV1 group 2
> >> >ip link set dev $DEV2 group 2
> >>
> >> That does not looks correct at all. Do tc stuff in tc, no?
> >>
> >>
> >> >...
> >> >
> >> >Then you can blockcast:
> >> >tc filter add devx protocol ip pref 25 \
> >> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
> >>
> >> "blockcasting" to something that is not a block anymore. Not nice.
> >>
> >
> >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
> >perhaps a different action altogether.
>
> mirret redirect? :)
>
> With target of:
> 1) dev (the current one)
> 2) block
> 3) group
> ?

tbh, I dont like it - but we need to make progress. I will defer to Marcelo.

cheers,
jamal

>
> >
> >cheers,
> >jamal
> >> >
> >> >cheers,
> >> >jamal
> >> >
> >> >>
> >> >> >
> >> >> >cheers,
> >> >> >jamal
> >> >> >
> >> >> >> Instead of:
> >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> >> >> You'd have:
> >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> >> >> >>
> >> >> >> I don't see why we need special action for this.
> >> >> >>
> >> >> >> Regarding "tx_type all":
> >> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> >> >> >> to have this as "no_src_skip" or some other similar arg, without value
> >> >> >> acting as a bool (flag) on netlink level.
> >> >> >>
> >> >> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Marcelo Ricardo Leitner]

On Mon, Nov 27, 2023 at 10:50:48AM -0500, Jamal Hadi Salim wrote:
> On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >
> > Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@mojatatu.com wrote:
> > >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >>
> > >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
> > >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >> >>
> > >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
> > >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >> >> >>
> > >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> > >> >> >> >This action takes advantage of the presence of tc block ports set in the
> > >> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
> > >> >> >> >broadcast the packet to a block, that is send to all members of the block except
> > >> >> >> >the port in which the packet arrived on. However, the user may specify
> > >> >> >> >the option "tx_type all", which will send the packet to all members of the
> > >> >> >> >block indiscriminately.
> > >> >> >> >
> > >> >> >> >Example usage:
> > >> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
> > >> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
> > >> >> >> >
> > >> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> > >> >> >>
> > >> >> >> Name the arg "block" so it is consistent with "filter add block". Make
> > >> >> >> sure this is aligned netlink-wise as well.
> > >> >> >>
> > >> >> >>
> > >> >> >> >
> > >> >> >> >Or if we wish to send to all ports in the block:
> > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> > >> >> >>
> > >> >> >> I read the discussion the the previous version again. I suggested this
> > >> >> >> to be part of mirred. Why exactly that was not addressed?
> > >> >> >>
> > >> >> >
> > >> >> >I am the one who pushed back (in that discussion). Actions should be
> > >> >> >small and specific. Like i had said in that earlier discussion it was
> > >> >> >a mistake to make mirred do both mirror and redirect - they should
> > >> >>
> > >> >> For mirror and redirect, I agree. For redirect and redirect, does not
> > >> >> make much sense. It's just confusing for the user.
> > >> >>
> > >> >
> > >> >Blockcast only emulates the mirror part. I agree redirect doesnt make
> > >> >any sense because once you redirect the packet is gone.
> > >>
> > >> How is it mirror? It is redirect to multiple, isn't it?
> > >>
> > >>
> > >> >
> > >> >> >have been two actions. So i feel like adding a block to mirred is
> > >> >> >adding more knobs. We are also going to add dev->group as a way to
> > >> >> >select what devices to mirror to. Should that be in mirred as well?
> > >> >>
> > >> >> I need more details.
> > >> >>
> > >> >
> > >> >You set any port you want to be mirrored to using ip link, example:
> > >> >ip link set dev $DEV1 group 2
> > >> >ip link set dev $DEV2 group 2
> > >>
> > >> That does not looks correct at all. Do tc stuff in tc, no?
> > >>
> > >>
> > >> >...
> > >> >
> > >> >Then you can blockcast:
> > >> >tc filter add devx protocol ip pref 25 \
> > >> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
> > >>
> > >> "blockcasting" to something that is not a block anymore. Not nice.

+1

> > >>
> > >
> > >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
> > >perhaps a different action altogether.
> >
> > mirret redirect? :)
> >
> > With target of:
> > 1) dev (the current one)
> > 2) block
> > 3) group
> > ?
>
> tbh, I dont like it - but we need to make progress. I will defer to Marcelo.

With the addition of a new output type that I didn't foresee, that
AFAICS will use the same parameters as the block output, creating a
new action for it is a lot of boilerplate for just having a different
name. If these new two actions can share parsing code and everything,
then it's not too far for mirred also use. And if we stick to the
concept of one single action for outputting to multiple interfaces,
even just deciding on the new name became quite challenging now.
"groupcast" is misleading. "multicast" no good, "multimirred" not
intuitive, "supermirred" what? and so on..

I still think that it will become a very complex action, but well,
hopefully the man page can be updated in a way to minimize the
confusion.

Cheers,
Marcelo

>
> cheers,
> jamal
>
> >
> > >
> > >cheers,
> > >jamal
> > >> >
> > >> >cheers,
> > >> >jamal
> > >> >
> > >> >>
> > >> >> >
> > >> >> >cheers,
> > >> >> >jamal
> > >> >> >
> > >> >> >> Instead of:
> > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> > >> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> > >> >> >> You'd have:
> > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> > >> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> > >> >> >>
> > >> >> >> I don't see why we need special action for this.
> > >> >> >>
> > >> >> >> Regarding "tx_type all":
> > >> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> > >> >> >> to have this as "no_src_skip" or some other similar arg, without value
> > >> >> >> acting as a bool (flag) on netlink level.
> > >> >> >>
> > >> >> >>
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Mon, Nov 27, 2023 at 1:52 PM Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
>
> On Mon, Nov 27, 2023 at 10:50:48AM -0500, Jamal Hadi Salim wrote:
> > On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >
> > > Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@mojatatu.com wrote:
> > > >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > > >>
> > > >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
> > > >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > > >> >>
> > > >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
> > > >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > > >> >> >>
> > > >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> > > >> >> >> >This action takes advantage of the presence of tc block ports set in the
> > > >> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
> > > >> >> >> >broadcast the packet to a block, that is send to all members of the block except
> > > >> >> >> >the port in which the packet arrived on. However, the user may specify
> > > >> >> >> >the option "tx_type all", which will send the packet to all members of the
> > > >> >> >> >block indiscriminately.
> > > >> >> >> >
> > > >> >> >> >Example usage:
> > > >> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
> > > >> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
> > > >> >> >> >
> > > >> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> > > >> >> >>
> > > >> >> >> Name the arg "block" so it is consistent with "filter add block". Make
> > > >> >> >> sure this is aligned netlink-wise as well.
> > > >> >> >>
> > > >> >> >>
> > > >> >> >> >
> > > >> >> >> >Or if we wish to send to all ports in the block:
> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> > > >> >> >>
> > > >> >> >> I read the discussion the the previous version again. I suggested this
> > > >> >> >> to be part of mirred. Why exactly that was not addressed?
> > > >> >> >>
> > > >> >> >
> > > >> >> >I am the one who pushed back (in that discussion). Actions should be
> > > >> >> >small and specific. Like i had said in that earlier discussion it was
> > > >> >> >a mistake to make mirred do both mirror and redirect - they should
> > > >> >>
> > > >> >> For mirror and redirect, I agree. For redirect and redirect, does not
> > > >> >> make much sense. It's just confusing for the user.
> > > >> >>
> > > >> >
> > > >> >Blockcast only emulates the mirror part. I agree redirect doesnt make
> > > >> >any sense because once you redirect the packet is gone.
> > > >>
> > > >> How is it mirror? It is redirect to multiple, isn't it?
> > > >>
> > > >>
> > > >> >
> > > >> >> >have been two actions. So i feel like adding a block to mirred is
> > > >> >> >adding more knobs. We are also going to add dev->group as a way to
> > > >> >> >select what devices to mirror to. Should that be in mirred as well?
> > > >> >>
> > > >> >> I need more details.
> > > >> >>
> > > >> >
> > > >> >You set any port you want to be mirrored to using ip link, example:
> > > >> >ip link set dev $DEV1 group 2
> > > >> >ip link set dev $DEV2 group 2
> > > >>
> > > >> That does not looks correct at all. Do tc stuff in tc, no?
> > > >>
> > > >>
> > > >> >...
> > > >> >
> > > >> >Then you can blockcast:
> > > >> >tc filter add devx protocol ip pref 25 \
> > > >> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
> > > >>
> > > >> "blockcasting" to something that is not a block anymore. Not nice.
>
> +1
>
> > > >>
> > > >
> > > >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
> > > >perhaps a different action altogether.
> > >
> > > mirret redirect? :)
> > >
> > > With target of:
> > > 1) dev (the current one)
> > > 2) block
> > > 3) group
> > > ?
> >
> > tbh, I dont like it - but we need to make progress. I will defer to Marcelo.
>
> With the addition of a new output type that I didn't foresee, that
> AFAICS will use the same parameters as the block output, creating a
> new action for it is a lot of boilerplate for just having a different
> name. If these new two actions can share parsing code and everything,
> then it's not too far for mirred also use. And if we stick to the
> concept of one single action for outputting to multiple interfaces,
> even just deciding on the new name became quite challenging now.
> "groupcast" is misleading. "multicast" no good, "multimirred" not
> intuitive, "supermirred" what? and so on..
>
> I still think that it will become a very complex action, but well,
> hopefully the man page can be updated in a way to minimize the
> confusion.

Ok, so we are moving forward with mirred "mirror" option only for this then...

cheers,
jamal

> Cheers,
> Marcelo
>
> >
> > cheers,
> > jamal
> >
> > >
> > > >
> > > >cheers,
> > > >jamal
> > > >> >
> > > >> >cheers,
> > > >> >jamal
> > > >> >
> > > >> >>
> > > >> >> >
> > > >> >> >cheers,
> > > >> >> >jamal
> > > >> >> >
> > > >> >> >> Instead of:
> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> > > >> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> > > >> >> >> You'd have:
> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> > > >> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> > > >> >> >>
> > > >> >> >> I don't see why we need special action for this.
> > > >> >> >>
> > > >> >> >> Regarding "tx_type all":
> > > >> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> > > >> >> >> to have this as "no_src_skip" or some other similar arg, without value
> > > >> >> >> acting as a bool (flag) on netlink level.
> > > >> >> >>
> > > >> >> >>
> >
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
>On Mon, Nov 27, 2023 at 1:52 PM Marcelo Ricardo Leitner
><mleitner@redhat.com> wrote:
>>
>> On Mon, Nov 27, 2023 at 10:50:48AM -0500, Jamal Hadi Salim wrote:
>> > On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> > >
>> > > Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@mojatatu.com wrote:
>> > > >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> > > >>
>> > > >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
>> > > >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> > > >> >>
>> > > >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
>> > > >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> > > >> >> >>
>> > > >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> > > >> >> >> >This action takes advantage of the presence of tc block ports set in the
>> > > >> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
>> > > >> >> >> >broadcast the packet to a block, that is send to all members of the block except
>> > > >> >> >> >the port in which the packet arrived on. However, the user may specify
>> > > >> >> >> >the option "tx_type all", which will send the packet to all members of the
>> > > >> >> >> >block indiscriminately.
>> > > >> >> >> >
>> > > >> >> >> >Example usage:
>> > > >> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
>> > > >> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
>> > > >> >> >> >
>> > > >> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> > > >> >> >>
>> > > >> >> >> Name the arg "block" so it is consistent with "filter add block". Make
>> > > >> >> >> sure this is aligned netlink-wise as well.
>> > > >> >> >>
>> > > >> >> >>
>> > > >> >> >> >
>> > > >> >> >> >Or if we wish to send to all ports in the block:
>> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>> > > >> >> >>
>> > > >> >> >> I read the discussion the the previous version again. I suggested this
>> > > >> >> >> to be part of mirred. Why exactly that was not addressed?
>> > > >> >> >>
>> > > >> >> >
>> > > >> >> >I am the one who pushed back (in that discussion). Actions should be
>> > > >> >> >small and specific. Like i had said in that earlier discussion it was
>> > > >> >> >a mistake to make mirred do both mirror and redirect - they should
>> > > >> >>
>> > > >> >> For mirror and redirect, I agree. For redirect and redirect, does not
>> > > >> >> make much sense. It's just confusing for the user.
>> > > >> >>
>> > > >> >
>> > > >> >Blockcast only emulates the mirror part. I agree redirect doesnt make
>> > > >> >any sense because once you redirect the packet is gone.
>> > > >>
>> > > >> How is it mirror? It is redirect to multiple, isn't it?
>> > > >>
>> > > >>
>> > > >> >
>> > > >> >> >have been two actions. So i feel like adding a block to mirred is
>> > > >> >> >adding more knobs. We are also going to add dev->group as a way to
>> > > >> >> >select what devices to mirror to. Should that be in mirred as well?
>> > > >> >>
>> > > >> >> I need more details.
>> > > >> >>
>> > > >> >
>> > > >> >You set any port you want to be mirrored to using ip link, example:
>> > > >> >ip link set dev $DEV1 group 2
>> > > >> >ip link set dev $DEV2 group 2
>> > > >>
>> > > >> That does not looks correct at all. Do tc stuff in tc, no?
>> > > >>
>> > > >>
>> > > >> >...
>> > > >> >
>> > > >> >Then you can blockcast:
>> > > >> >tc filter add devx protocol ip pref 25 \
>> > > >> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
>> > > >>
>> > > >> "blockcasting" to something that is not a block anymore. Not nice.
>>
>> +1
>>
>> > > >>
>> > > >
>> > > >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
>> > > >perhaps a different action altogether.
>> > >
>> > > mirret redirect? :)
>> > >
>> > > With target of:
>> > > 1) dev (the current one)
>> > > 2) block
>> > > 3) group
>> > > ?
>> >
>> > tbh, I dont like it - but we need to make progress. I will defer to Marcelo.
>>
>> With the addition of a new output type that I didn't foresee, that
>> AFAICS will use the same parameters as the block output, creating a
>> new action for it is a lot of boilerplate for just having a different
>> name. If these new two actions can share parsing code and everything,
>> then it's not too far for mirred also use. And if we stick to the
>> concept of one single action for outputting to multiple interfaces,
>> even just deciding on the new name became quite challenging now.
>> "groupcast" is misleading. "multicast" no good, "multimirred" not
>> intuitive, "supermirred" what? and so on..
>>
>> I still think that it will become a very complex action, but well,
>> hopefully the man page can be updated in a way to minimize the
>> confusion.
>
>Ok, so we are moving forward with mirred "mirror" option only for this then...

Could you remind me why mirror and not redirect? Does the packet
continue through the stack?


>
>cheers,
>jamal
>
>> Cheers,
>> Marcelo
>>
>> >
>> > cheers,
>> > jamal
>> >
>> > >
>> > > >
>> > > >cheers,
>> > > >jamal
>> > > >> >
>> > > >> >cheers,
>> > > >> >jamal
>> > > >> >
>> > > >> >>
>> > > >> >> >
>> > > >> >> >cheers,
>> > > >> >> >jamal
>> > > >> >> >
>> > > >> >> >> Instead of:
>> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> > > >> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> > > >> >> >> You'd have:
>> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> > > >> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> > > >> >> >>
>> > > >> >> >> I don't see why we need special action for this.
>> > > >> >> >>
>> > > >> >> >> Regarding "tx_type all":
>> > > >> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> > > >> >> >> to have this as "no_src_skip" or some other similar arg, without value
>> > > >> >> >> acting as a bool (flag) on netlink level.
>> > > >> >> >>
>> > > >> >> >>
>> >
>>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
> >On Mon, Nov 27, 2023 at 1:52 PM Marcelo Ricardo Leitner
> ><mleitner@redhat.com> wrote:
> >>
> >> On Mon, Nov 27, 2023 at 10:50:48AM -0500, Jamal Hadi Salim wrote:
> >> > On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> > >
> >> > > Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@mojatatu.com wrote:
> >> > > >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> > > >>
> >> > > >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
> >> > > >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> > > >> >>
> >> > > >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
> >> > > >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> > > >> >> >>
> >> > > >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
> >> > > >> >> >> >This action takes advantage of the presence of tc block ports set in the
> >> > > >> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
> >> > > >> >> >> >broadcast the packet to a block, that is send to all members of the block except
> >> > > >> >> >> >the port in which the packet arrived on. However, the user may specify
> >> > > >> >> >> >the option "tx_type all", which will send the packet to all members of the
> >> > > >> >> >> >block indiscriminately.
> >> > > >> >> >> >
> >> > > >> >> >> >Example usage:
> >> > > >> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
> >> > > >> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
> >> > > >> >> >> >
> >> > > >> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
> >> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> > > >> >> >>
> >> > > >> >> >> Name the arg "block" so it is consistent with "filter add block". Make
> >> > > >> >> >> sure this is aligned netlink-wise as well.
> >> > > >> >> >>
> >> > > >> >> >>
> >> > > >> >> >> >
> >> > > >> >> >> >Or if we wish to send to all ports in the block:
> >> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
> >> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
> >> > > >> >> >>
> >> > > >> >> >> I read the discussion the the previous version again. I suggested this
> >> > > >> >> >> to be part of mirred. Why exactly that was not addressed?
> >> > > >> >> >>
> >> > > >> >> >
> >> > > >> >> >I am the one who pushed back (in that discussion). Actions should be
> >> > > >> >> >small and specific. Like i had said in that earlier discussion it was
> >> > > >> >> >a mistake to make mirred do both mirror and redirect - they should
> >> > > >> >>
> >> > > >> >> For mirror and redirect, I agree. For redirect and redirect, does not
> >> > > >> >> make much sense. It's just confusing for the user.
> >> > > >> >>
> >> > > >> >
> >> > > >> >Blockcast only emulates the mirror part. I agree redirect doesnt make
> >> > > >> >any sense because once you redirect the packet is gone.
> >> > > >>
> >> > > >> How is it mirror? It is redirect to multiple, isn't it?
> >> > > >>
> >> > > >>
> >> > > >> >
> >> > > >> >> >have been two actions. So i feel like adding a block to mirred is
> >> > > >> >> >adding more knobs. We are also going to add dev->group as a way to
> >> > > >> >> >select what devices to mirror to. Should that be in mirred as well?
> >> > > >> >>
> >> > > >> >> I need more details.
> >> > > >> >>
> >> > > >> >
> >> > > >> >You set any port you want to be mirrored to using ip link, example:
> >> > > >> >ip link set dev $DEV1 group 2
> >> > > >> >ip link set dev $DEV2 group 2
> >> > > >>
> >> > > >> That does not looks correct at all. Do tc stuff in tc, no?
> >> > > >>
> >> > > >>
> >> > > >> >...
> >> > > >> >
> >> > > >> >Then you can blockcast:
> >> > > >> >tc filter add devx protocol ip pref 25 \
> >> > > >> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
> >> > > >>
> >> > > >> "blockcasting" to something that is not a block anymore. Not nice.
> >>
> >> +1
> >>
> >> > > >>
> >> > > >
> >> > > >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
> >> > > >perhaps a different action altogether.
> >> > >
> >> > > mirret redirect? :)
> >> > >
> >> > > With target of:
> >> > > 1) dev (the current one)
> >> > > 2) block
> >> > > 3) group
> >> > > ?
> >> >
> >> > tbh, I dont like it - but we need to make progress. I will defer to Marcelo.
> >>
> >> With the addition of a new output type that I didn't foresee, that
> >> AFAICS will use the same parameters as the block output, creating a
> >> new action for it is a lot of boilerplate for just having a different
> >> name. If these new two actions can share parsing code and everything,
> >> then it's not too far for mirred also use. And if we stick to the
> >> concept of one single action for outputting to multiple interfaces,
> >> even just deciding on the new name became quite challenging now.
> >> "groupcast" is misleading. "multicast" no good, "multimirred" not
> >> intuitive, "supermirred" what? and so on..
> >>
> >> I still think that it will become a very complex action, but well,
> >> hopefully the man page can be updated in a way to minimize the
> >> confusion.
> >
> >Ok, so we are moving forward with mirred "mirror" option only for this then...
>
> Could you remind me why mirror and not redirect? Does the packet
> continue through the stack?

For mirror it is _a copy_ of the packet so it continues up the stack
and you can have other actions follow it (including multiple mirrors
after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
so removed from the stack processing (and cant be sent to more ports).
That is how mirred has always worked and i believe thats how most
hardware works as well.
So sending to multiple ports has to be mirroring semantics (most
hardware assumes the same semantics).

cheers,
jamal

>
> >
> >cheers,
> >jamal
> >
> >> Cheers,
> >> Marcelo
> >>
> >> >
> >> > cheers,
> >> > jamal
> >> >
> >> > >
> >> > > >
> >> > > >cheers,
> >> > > >jamal
> >> > > >> >
> >> > > >> >cheers,
> >> > > >> >jamal
> >> > > >> >
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> >cheers,
> >> > > >> >> >jamal
> >> > > >> >> >
> >> > > >> >> >> Instead of:
> >> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> > > >> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
> >> > > >> >> >> You'd have:
> >> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
> >> > > >> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
> >> > > >> >> >>
> >> > > >> >> >> I don't see why we need special action for this.
> >> > > >> >> >>
> >> > > >> >> >> Regarding "tx_type all":
> >> > > >> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
> >> > > >> >> >> to have this as "no_src_skip" or some other similar arg, without value
> >> > > >> >> >> acting as a bool (flag) on netlink level.
> >> > > >> >> >>
> >> > > >> >> >>
> >> >
> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Mon, Dec 04, 2023 at 09:10:18PM CET, jhs@mojatatu.com wrote:
>On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
>> >On Mon, Nov 27, 2023 at 1:52 PM Marcelo Ricardo Leitner
>> ><mleitner@redhat.com> wrote:
>> >>
>> >> On Mon, Nov 27, 2023 at 10:50:48AM -0500, Jamal Hadi Salim wrote:
>> >> > On Thu, Nov 23, 2023 at 11:52 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > >
>> >> > > Thu, Nov 23, 2023 at 05:21:51PM CET, hadi@mojatatu.com wrote:
>> >> > > >On Thu, Nov 23, 2023 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > > >>
>> >> > > >> Thu, Nov 23, 2023 at 03:38:35PM CET, jhs@mojatatu.com wrote:
>> >> > > >> >On Thu, Nov 23, 2023 at 9:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > > >> >>
>> >> > > >> >> Thu, Nov 23, 2023 at 02:37:13PM CET, jhs@mojatatu.com wrote:
>> >> > > >> >> >On Thu, Nov 23, 2023 at 3:51 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > > >> >> >>
>> >> > > >> >> >> Fri, Nov 10, 2023 at 10:46:18PM CET, victor@mojatatu.com wrote:
>> >> > > >> >> >> >This action takes advantage of the presence of tc block ports set in the
>> >> > > >> >> >> >datapath and multicasts a packet to ports on a block. By default, it will
>> >> > > >> >> >> >broadcast the packet to a block, that is send to all members of the block except
>> >> > > >> >> >> >the port in which the packet arrived on. However, the user may specify
>> >> > > >> >> >> >the option "tx_type all", which will send the packet to all members of the
>> >> > > >> >> >> >block indiscriminately.
>> >> > > >> >> >> >
>> >> > > >> >> >> >Example usage:
>> >> > > >> >> >> >    $ tc qdisc add dev ens7 ingress_block 22
>> >> > > >> >> >> >    $ tc qdisc add dev ens8 ingress_block 22
>> >> > > >> >> >> >
>> >> > > >> >> >> >Now we can add a filter to broadcast packets to ports on ingress block id 22:
>> >> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> > > >> >> >>
>> >> > > >> >> >> Name the arg "block" so it is consistent with "filter add block". Make
>> >> > > >> >> >> sure this is aligned netlink-wise as well.
>> >> > > >> >> >>
>> >> > > >> >> >>
>> >> > > >> >> >> >
>> >> > > >> >> >> >Or if we wish to send to all ports in the block:
>> >> > > >> >> >> >$ tc filter add block 22 protocol ip pref 25 \
>> >> > > >> >> >> >  flower dst_ip 192.168.0.0/16 action blockcast blockid 22 tx_type all
>> >> > > >> >> >>
>> >> > > >> >> >> I read the discussion the the previous version again. I suggested this
>> >> > > >> >> >> to be part of mirred. Why exactly that was not addressed?
>> >> > > >> >> >>
>> >> > > >> >> >
>> >> > > >> >> >I am the one who pushed back (in that discussion). Actions should be
>> >> > > >> >> >small and specific. Like i had said in that earlier discussion it was
>> >> > > >> >> >a mistake to make mirred do both mirror and redirect - they should
>> >> > > >> >>
>> >> > > >> >> For mirror and redirect, I agree. For redirect and redirect, does not
>> >> > > >> >> make much sense. It's just confusing for the user.
>> >> > > >> >>
>> >> > > >> >
>> >> > > >> >Blockcast only emulates the mirror part. I agree redirect doesnt make
>> >> > > >> >any sense because once you redirect the packet is gone.
>> >> > > >>
>> >> > > >> How is it mirror? It is redirect to multiple, isn't it?
>> >> > > >>
>> >> > > >>
>> >> > > >> >
>> >> > > >> >> >have been two actions. So i feel like adding a block to mirred is
>> >> > > >> >> >adding more knobs. We are also going to add dev->group as a way to
>> >> > > >> >> >select what devices to mirror to. Should that be in mirred as well?
>> >> > > >> >>
>> >> > > >> >> I need more details.
>> >> > > >> >>
>> >> > > >> >
>> >> > > >> >You set any port you want to be mirrored to using ip link, example:
>> >> > > >> >ip link set dev $DEV1 group 2
>> >> > > >> >ip link set dev $DEV2 group 2
>> >> > > >>
>> >> > > >> That does not looks correct at all. Do tc stuff in tc, no?
>> >> > > >>
>> >> > > >>
>> >> > > >> >...
>> >> > > >> >
>> >> > > >> >Then you can blockcast:
>> >> > > >> >tc filter add devx protocol ip pref 25 \
>> >> > > >> >  flower dst_ip 192.168.0.0/16 action blockcast group 2
>> >> > > >>
>> >> > > >> "blockcasting" to something that is not a block anymore. Not nice.
>> >>
>> >> +1
>> >>
>> >> > > >>
>> >> > > >
>> >> > > >Sorry, missed this one. Yes blockcasting is no longer appropriate  -
>> >> > > >perhaps a different action altogether.
>> >> > >
>> >> > > mirret redirect? :)
>> >> > >
>> >> > > With target of:
>> >> > > 1) dev (the current one)
>> >> > > 2) block
>> >> > > 3) group
>> >> > > ?
>> >> >
>> >> > tbh, I dont like it - but we need to make progress. I will defer to Marcelo.
>> >>
>> >> With the addition of a new output type that I didn't foresee, that
>> >> AFAICS will use the same parameters as the block output, creating a
>> >> new action for it is a lot of boilerplate for just having a different
>> >> name. If these new two actions can share parsing code and everything,
>> >> then it's not too far for mirred also use. And if we stick to the
>> >> concept of one single action for outputting to multiple interfaces,
>> >> even just deciding on the new name became quite challenging now.
>> >> "groupcast" is misleading. "multicast" no good, "multimirred" not
>> >> intuitive, "supermirred" what? and so on..
>> >>
>> >> I still think that it will become a very complex action, but well,
>> >> hopefully the man page can be updated in a way to minimize the
>> >> confusion.
>> >
>> >Ok, so we are moving forward with mirred "mirror" option only for this then...
>>
>> Could you remind me why mirror and not redirect? Does the packet
>> continue through the stack?
>
>For mirror it is _a copy_ of the packet so it continues up the stack
>and you can have other actions follow it (including multiple mirrors
>after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
>so removed from the stack processing (and cant be sent to more ports).
>That is how mirred has always worked and i believe thats how most
>hardware works as well.
>So sending to multiple ports has to be mirroring semantics (most
>hardware assumes the same semantics).

You assume cloning (sending to multiple ports) means mirror,
that is I believe a mistake. Look at it from the perspective of
replacing device by target for each action. Currently we have:

1) mirred mirror TARGET_DEVICE
   Clones, sends to TARGET_DEVICE and continues up the stack
2) mirred redirect TARGET_DEVICE
   Sends to TARGET_DEVICE, nothing is sent up the stack

For block target, there should be exacly the same semantics:

1) mirred mirror TARGET_BLOCK
   Clones (multiple times, for each block member), sends to TARGET_BLOCK
   and continues up the stack
2) mirred redirect TARGET_BLOCK
   Clones (multiple times, for each block member - 1), sends to
   TARGET_BLOCK, nothing is sent up the stack



>
>cheers,
>jamal
>
>>
>> >
>> >cheers,
>> >jamal
>> >
>> >> Cheers,
>> >> Marcelo
>> >>
>> >> >
>> >> > cheers,
>> >> > jamal
>> >> >
>> >> > >
>> >> > > >
>> >> > > >cheers,
>> >> > > >jamal
>> >> > > >> >
>> >> > > >> >cheers,
>> >> > > >> >jamal
>> >> > > >> >
>> >> > > >> >>
>> >> > > >> >> >
>> >> > > >> >> >cheers,
>> >> > > >> >> >jamal
>> >> > > >> >> >
>> >> > > >> >> >> Instead of:
>> >> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> > > >> >> >>   flower dst_ip 192.168.0.0/16 action blockcast blockid 22
>> >> > > >> >> >> You'd have:
>> >> > > >> >> >> $ tc filter add block 22 protocol ip pref 25 \
>> >> > > >> >> >>   flower dst_ip 192.168.0.0/16 action mirred egress redirect block 22
>> >> > > >> >> >>
>> >> > > >> >> >> I don't see why we need special action for this.
>> >> > > >> >> >>
>> >> > > >> >> >> Regarding "tx_type all":
>> >> > > >> >> >> Do you expect to have another "tx_type"? Seems to me a bit odd. Why not
>> >> > > >> >> >> to have this as "no_src_skip" or some other similar arg, without value
>> >> > > >> >> >> acting as a bool (flag) on netlink level.
>> >> > > >> >> >>
>> >> > > >> >> >>
>> >> >
>> >>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Marcelo Ricardo Leitner]

On Tue, Dec 05, 2023 at 09:41:02AM +0100, Jiri Pirko wrote:
> Mon, Dec 04, 2023 at 09:10:18PM CET, jhs@mojatatu.com wrote:
> >On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
...
> >> >Ok, so we are moving forward with mirred "mirror" option only for this then...
> >>
> >> Could you remind me why mirror and not redirect? Does the packet
> >> continue through the stack?
> >
> >For mirror it is _a copy_ of the packet so it continues up the stack
> >and you can have other actions follow it (including multiple mirrors
> >after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
> >so removed from the stack processing (and cant be sent to more ports).
> >That is how mirred has always worked and i believe thats how most
> >hardware works as well.
> >So sending to multiple ports has to be mirroring semantics (most
> >hardware assumes the same semantics).
>
> You assume cloning (sending to multiple ports) means mirror,
> that is I believe a mistake. Look at it from the perspective of
> replacing device by target for each action. Currently we have:
>
> 1) mirred mirror TARGET_DEVICE
>    Clones, sends to TARGET_DEVICE and continues up the stack
> 2) mirred redirect TARGET_DEVICE
>    Sends to TARGET_DEVICE, nothing is sent up the stack
>
> For block target, there should be exacly the same semantics:
>
> 1) mirred mirror TARGET_BLOCK
>    Clones (multiple times, for each block member), sends to TARGET_BLOCK
>    and continues up the stack
> 2) mirred redirect TARGET_BLOCK
>    Clones (multiple times, for each block member - 1), sends to
>    TARGET_BLOCK, nothing is sent up the stack

This makes sense to me as well. When I first read Jamal's email I
didn't spot any confusion, but now I see there can be some. I think he
meant pretty much the same thing, referencing cascading other outputs
after blockcast (and not the inner outputs, lets say), but that's just
my interpretation. :)

  Marcelo

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Tue, Dec 5, 2023 at 9:52 AM Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
>
> On Tue, Dec 05, 2023 at 09:41:02AM +0100, Jiri Pirko wrote:
> > Mon, Dec 04, 2023 at 09:10:18PM CET, jhs@mojatatu.com wrote:
> > >On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >>
> > >> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
> ...
> > >> >Ok, so we are moving forward with mirred "mirror" option only for this then...
> > >>
> > >> Could you remind me why mirror and not redirect? Does the packet
> > >> continue through the stack?
> > >
> > >For mirror it is _a copy_ of the packet so it continues up the stack
> > >and you can have other actions follow it (including multiple mirrors
> > >after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
> > >so removed from the stack processing (and cant be sent to more ports).
> > >That is how mirred has always worked and i believe thats how most
> > >hardware works as well.
> > >So sending to multiple ports has to be mirroring semantics (most
> > >hardware assumes the same semantics).
> >
> > You assume cloning (sending to multiple ports) means mirror,
> > that is I believe a mistake. Look at it from the perspective of
> > replacing device by target for each action. Currently we have:
> >
> > 1) mirred mirror TARGET_DEVICE
> >    Clones, sends to TARGET_DEVICE and continues up the stack
> > 2) mirred redirect TARGET_DEVICE
> >    Sends to TARGET_DEVICE, nothing is sent up the stack
> >
> > For block target, there should be exacly the same semantics:
> >
> > 1) mirred mirror TARGET_BLOCK
> >    Clones (multiple times, for each block member), sends to TARGET_BLOCK
> >    and continues up the stack
> > 2) mirred redirect TARGET_BLOCK
> >    Clones (multiple times, for each block member - 1), sends to
> >    TARGET_BLOCK, nothing is sent up the stack
>
> This makes sense to me as well. When I first read Jamal's email I
> didn't spot any confusion, but now I see there can be some. I think he
> meant pretty much the same thing, referencing cascading other outputs
> after blockcast (and not the inner outputs, lets say), but that's just
> my interpretation. :)

In my (shall i say long experience) I have never seen the prescribed
behavior of redirect meaning mirror to (all - last one) then redirect
on last one.. Jiri, does spectrum work like this?
Neither in s/w nor in h/w. From h/w - example, the nvidia CX6 you have
to give explicit mirror, mirror, mirror, redirect. IOW, i dont think
the hardware can be told "here's a list of ports, please mirror to all
of them and for the last one steal the packet and redirect".
Having said that i am not opposed to it - it will just make the code
slightly more complex and i am sure slightly slower in the datapath.

cheers,
jamal

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Marcelo Ricardo Leitner]

On Tue, Dec 05, 2023 at 10:27:31AM -0500, Jamal Hadi Salim wrote:
> On Tue, Dec 5, 2023 at 9:52 AM Marcelo Ricardo Leitner
> <mleitner@redhat.com> wrote:
> >
> > On Tue, Dec 05, 2023 at 09:41:02AM +0100, Jiri Pirko wrote:
> > > Mon, Dec 04, 2023 at 09:10:18PM CET, jhs@mojatatu.com wrote:
> > > >On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > > >>
> > > >> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
> > ...
> > > >> >Ok, so we are moving forward with mirred "mirror" option only for this then...
> > > >>
> > > >> Could you remind me why mirror and not redirect? Does the packet
> > > >> continue through the stack?
> > > >
> > > >For mirror it is _a copy_ of the packet so it continues up the stack
> > > >and you can have other actions follow it (including multiple mirrors
> > > >after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
> > > >so removed from the stack processing (and cant be sent to more ports).
> > > >That is how mirred has always worked and i believe thats how most
> > > >hardware works as well.
> > > >So sending to multiple ports has to be mirroring semantics (most
> > > >hardware assumes the same semantics).
> > >
> > > You assume cloning (sending to multiple ports) means mirror,
> > > that is I believe a mistake. Look at it from the perspective of
> > > replacing device by target for each action. Currently we have:
> > >
> > > 1) mirred mirror TARGET_DEVICE
> > >    Clones, sends to TARGET_DEVICE and continues up the stack
> > > 2) mirred redirect TARGET_DEVICE
> > >    Sends to TARGET_DEVICE, nothing is sent up the stack
> > >
> > > For block target, there should be exacly the same semantics:
> > >
> > > 1) mirred mirror TARGET_BLOCK
> > >    Clones (multiple times, for each block member), sends to TARGET_BLOCK
> > >    and continues up the stack
> > > 2) mirred redirect TARGET_BLOCK
> > >    Clones (multiple times, for each block member - 1), sends to
> > >    TARGET_BLOCK, nothing is sent up the stack
> >
> > This makes sense to me as well. When I first read Jamal's email I
> > didn't spot any confusion, but now I see there can be some. I think he
> > meant pretty much the same thing, referencing cascading other outputs
> > after blockcast (and not the inner outputs, lets say), but that's just
> > my interpretation. :)
>
> In my (shall i say long experience) I have never seen the prescribed
> behavior of redirect meaning mirror to (all - last one) then redirect
> on last one.. Jiri, does spectrum work like this?
> Neither in s/w nor in h/w. From h/w - example, the nvidia CX6 you have
> to give explicit mirror, mirror, mirror, redirect. IOW, i dont think
> the hardware can be told "here's a list of ports, please mirror to all
> of them and for the last one steal the packet and redirect".

Precisely. I/(we?) were talking about tc sw/user expectations, not how
to offload it.

From a tc user perspective, the user should still be able to do this:
1) mirred mirror TARGET_BLOCK
2) mirred redirect TARGET_BLOCK
regardless of how the implementation actually works. Because ovs and
other users will rely on this semantic.

As for the actual implementation, as you said, it will have to somehow
unpack that into "[mirror, mirror, ...,] <mirror/redirect>", depending
on what the user requested, as I doubt there will be hw support for
outputting to multiple ports in one action.

> Having said that i am not opposed to it - it will just make the code
> slightly more complex and i am sure slightly slower in the datapath.
>
> cheers,
> jamal
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jiri Pirko]

Tue, Dec 05, 2023 at 11:12:23PM CET, mleitner@redhat.com wrote:
>On Tue, Dec 05, 2023 at 10:27:31AM -0500, Jamal Hadi Salim wrote:
>> On Tue, Dec 5, 2023 at 9:52 AM Marcelo Ricardo Leitner
>> <mleitner@redhat.com> wrote:
>> >
>> > On Tue, Dec 05, 2023 at 09:41:02AM +0100, Jiri Pirko wrote:
>> > > Mon, Dec 04, 2023 at 09:10:18PM CET, jhs@mojatatu.com wrote:
>> > > >On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> > > >>
>> > > >> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
>> > ...
>> > > >> >Ok, so we are moving forward with mirred "mirror" option only for this then...
>> > > >>
>> > > >> Could you remind me why mirror and not redirect? Does the packet
>> > > >> continue through the stack?
>> > > >
>> > > >For mirror it is _a copy_ of the packet so it continues up the stack
>> > > >and you can have other actions follow it (including multiple mirrors
>> > > >after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
>> > > >so removed from the stack processing (and cant be sent to more ports).
>> > > >That is how mirred has always worked and i believe thats how most
>> > > >hardware works as well.
>> > > >So sending to multiple ports has to be mirroring semantics (most
>> > > >hardware assumes the same semantics).
>> > >
>> > > You assume cloning (sending to multiple ports) means mirror,
>> > > that is I believe a mistake. Look at it from the perspective of
>> > > replacing device by target for each action. Currently we have:
>> > >
>> > > 1) mirred mirror TARGET_DEVICE
>> > >    Clones, sends to TARGET_DEVICE and continues up the stack
>> > > 2) mirred redirect TARGET_DEVICE
>> > >    Sends to TARGET_DEVICE, nothing is sent up the stack
>> > >
>> > > For block target, there should be exacly the same semantics:
>> > >
>> > > 1) mirred mirror TARGET_BLOCK
>> > >    Clones (multiple times, for each block member), sends to TARGET_BLOCK
>> > >    and continues up the stack
>> > > 2) mirred redirect TARGET_BLOCK
>> > >    Clones (multiple times, for each block member - 1), sends to
>> > >    TARGET_BLOCK, nothing is sent up the stack
>> >
>> > This makes sense to me as well. When I first read Jamal's email I
>> > didn't spot any confusion, but now I see there can be some. I think he
>> > meant pretty much the same thing, referencing cascading other outputs
>> > after blockcast (and not the inner outputs, lets say), but that's just
>> > my interpretation. :)
>>
>> In my (shall i say long experience) I have never seen the prescribed
>> behavior of redirect meaning mirror to (all - last one) then redirect
>> on last one.. Jiri, does spectrum work like this?
>> Neither in s/w nor in h/w. From h/w - example, the nvidia CX6 you have
>> to give explicit mirror, mirror, mirror, redirect. IOW, i dont think
>> the hardware can be told "here's a list of ports, please mirror to all
>> of them and for the last one steal the packet and redirect".
>
>Precisely. I/(we?) were talking about tc sw/user expectations, not how
>to offload it.
>
>From a tc user perspective, the user should still be able to do this:
>1) mirred mirror TARGET_BLOCK
>2) mirred redirect TARGET_BLOCK
>regardless of how the implementation actually works. Because ovs and
>other users will rely on this semantic.

Exactly. Forget about hw for now.


>
>As for the actual implementation, as you said, it will have to somehow
>unpack that into "[mirror, mirror, ...,] <mirror/redirect>", depending
>on what the user requested, as I doubt there will be hw support for
>outputting to multiple ports in one action.
>
>> Having said that i am not opposed to it - it will just make the code
>> slightly more complex and i am sure slightly slower in the datapath.
>>
>> cheers,
>> jamal
>>
>

Re: [PATCH net-next RFC v5 4/4] net/sched: act_blockcast: Introduce blockcast tc action

[Jamal Hadi Salim]

On Wed, Dec 6, 2023 at 2:55 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Dec 05, 2023 at 11:12:23PM CET, mleitner@redhat.com wrote:
> >On Tue, Dec 05, 2023 at 10:27:31AM -0500, Jamal Hadi Salim wrote:
> >> On Tue, Dec 5, 2023 at 9:52 AM Marcelo Ricardo Leitner
> >> <mleitner@redhat.com> wrote:
> >> >
> >> > On Tue, Dec 05, 2023 at 09:41:02AM +0100, Jiri Pirko wrote:
> >> > > Mon, Dec 04, 2023 at 09:10:18PM CET, jhs@mojatatu.com wrote:
> >> > > >On Mon, Dec 4, 2023 at 4:49 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> > > >>
> >> > > >> Fri, Dec 01, 2023 at 07:45:47PM CET, jhs@mojatatu.com wrote:
> >> > ...
> >> > > >> >Ok, so we are moving forward with mirred "mirror" option only for this then...
> >> > > >>
> >> > > >> Could you remind me why mirror and not redirect? Does the packet
> >> > > >> continue through the stack?
> >> > > >
> >> > > >For mirror it is _a copy_ of the packet so it continues up the stack
> >> > > >and you can have other actions follow it (including multiple mirrors
> >> > > >after the first mirror). For redirect the packet is TC_ACT_CONSUMED -
> >> > > >so removed from the stack processing (and cant be sent to more ports).
> >> > > >That is how mirred has always worked and i believe thats how most
> >> > > >hardware works as well.
> >> > > >So sending to multiple ports has to be mirroring semantics (most
> >> > > >hardware assumes the same semantics).
> >> > >
> >> > > You assume cloning (sending to multiple ports) means mirror,
> >> > > that is I believe a mistake. Look at it from the perspective of
> >> > > replacing device by target for each action. Currently we have:
> >> > >
> >> > > 1) mirred mirror TARGET_DEVICE
> >> > >    Clones, sends to TARGET_DEVICE and continues up the stack
> >> > > 2) mirred redirect TARGET_DEVICE
> >> > >    Sends to TARGET_DEVICE, nothing is sent up the stack
> >> > >
> >> > > For block target, there should be exacly the same semantics:
> >> > >
> >> > > 1) mirred mirror TARGET_BLOCK
> >> > >    Clones (multiple times, for each block member), sends to TARGET_BLOCK
> >> > >    and continues up the stack
> >> > > 2) mirred redirect TARGET_BLOCK
> >> > >    Clones (multiple times, for each block member - 1), sends to
> >> > >    TARGET_BLOCK, nothing is sent up the stack
> >> >
> >> > This makes sense to me as well. When I first read Jamal's email I
> >> > didn't spot any confusion, but now I see there can be some. I think he
> >> > meant pretty much the same thing, referencing cascading other outputs
> >> > after blockcast (and not the inner outputs, lets say), but that's just
> >> > my interpretation. :)
> >>
> >> In my (shall i say long experience) I have never seen the prescribed
> >> behavior of redirect meaning mirror to (all - last one) then redirect
> >> on last one.. Jiri, does spectrum work like this?
> >> Neither in s/w nor in h/w. From h/w - example, the nvidia CX6 you have
> >> to give explicit mirror, mirror, mirror, redirect. IOW, i dont think
> >> the hardware can be told "here's a list of ports, please mirror to all
> >> of them and for the last one steal the packet and redirect".
> >
> >Precisely. I/(we?) were talking about tc sw/user expectations, not how
> >to offload it.
> >
> >From a tc user perspective, the user should still be able to do this:
> >1) mirred mirror TARGET_BLOCK
> >2) mirred redirect TARGET_BLOCK
> >regardless of how the implementation actually works. Because ovs and
> >other users will rely on this semantic.
>
> Exactly. Forget about hw for now.

Ok, Lets go!

cheers,
jamal

>
> >
> >As for the actual implementation, as you said, it will have to somehow
> >unpack that into "[mirror, mirror, ...,] <mirror/redirect>", depending
> >on what the user requested, as I doubt there will be hw support for
> >outputting to multiple ports in one action.
> >
> >> Having said that i am not opposed to it - it will just make the code
> >> slightly more complex and i am sure slightly slower in the datapath.
> >>
> >> cheers,
> >> jamal
> >>
> >