| Message ID | 20241017165215.3709000-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Mirroring to DSA CPU port | expand |
On Thu, Oct 17, 2024 at 07:52:10PM +0300, Vladimir Oltean wrote: > Background: switchdev ports offload the Linux bridge, and most of the > packets they handle will never see the CPU. The ports between which > there exists no hardware data path are considered 'foreign' to switchdev. > These can either be normal physical NICs without switchdev offload, or > incompatible switchdev ports, or virtual interfaces like veth/dummy/etc. > > In some cases, an offloaded filter can only do half the work, and the > rest must be handled by software. Redirecting/mirroring from the ingress > of a switchdev port towards a foreign interface is one example of > combined hardware/software data path. The most that the switchdev port > can do is to extract the matching packets from its offloaded data path > and send them to the CPU. From there on, the software filter runs > (a second time, after the first run in hardware) on the packet and > performs the mirred action. > > It makes sense for switchdev drivers which allow this kind of "half > offloading" to sense the "skip_sw" flag of the filter/action pair, and > deny attempts from the user to install a filter that does not run in > software, because that simply won't work. > > In fact, a mirred action on a switchdev port towards a dummy interface > appears to be a valid way of (selectively) monitoring offloaded traffic > that flows through it. IFF_PROMISC was also discussed years ago, but > (despite initial disagreement) there seems to be consensus that this > flag should not affect the destination taken by packets, but merely > whether or not the NIC discards packets with unknown MAC DA for local > processing. > > Only the flower and matchall classifiers are of interest to me for > purely pragmatic reasons: these are offloaded by DSA currently. Possibly a stupid question given I don't remember all the details of the TC offload, but is there a reason not to put the 'skip_sw' indication in 'struct flow_cls_common_offload' and initialize the new field as part of tc_cls_common_offload_init()? Seems like it won't require patching every classifier and will also work for the re-offload case (e.g., fl_reoffload())? Something like: diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 292cd8f4b762..596ab9791e4d 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -685,6 +685,7 @@ struct flow_cls_common_offload { u32 chain_index; __be16 protocol; u32 prio; + bool skip_sw; struct netlink_ext_ack *extack; }; diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 4880b3a7aced..cf199af85c52 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -755,6 +755,7 @@ tc_cls_common_offload_init(struct flow_cls_common_offload *cls_common, cls_common->chain_index = tp->chain->index; cls_common->protocol = tp->protocol; cls_common->prio = tp->prio >> 16; + cls_common->skip_sw = tc_skip_sw(flags); if (tc_skip_sw(flags) || flags & TCA_CLS_FLAGS_VERBOSE) cls_common->extack = extack; } > > [1] https://lore.kernel.org/netdev/20190830092637.7f83d162@ceranb/ > [2] https://lore.kernel.org/netdev/20191002233750.13566-1-olteanv@gmail.com/ > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > v1->v2: rewrite commit message > > include/net/flow_offload.h | 1 + > include/net/pkt_cls.h | 1 + > net/sched/cls_flower.c | 1 + > net/sched/cls_matchall.c | 1 + > 4 files changed, 4 insertions(+) > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index 292cd8f4b762..a2f688dd0447 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -692,6 +692,7 @@ struct flow_cls_offload { > struct flow_cls_common_offload common; > enum flow_cls_command command; > bool use_act_stats; > + bool skip_sw; > unsigned long cookie; > struct flow_rule *rule; > struct flow_stats stats; > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > index 4880b3a7aced..7b9f41f33c33 100644 > --- a/include/net/pkt_cls.h > +++ b/include/net/pkt_cls.h > @@ -782,6 +782,7 @@ struct tc_cls_matchall_offload { > struct flow_rule *rule; > struct flow_stats stats; > bool use_act_stats; > + bool skip_sw; > unsigned long cookie; > }; > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index e280c27cb9f9..8f7c60805f85 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -480,6 +480,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp, > cls_flower.rule->match.mask = &f->mask->key; > cls_flower.rule->match.key = &f->mkey; > cls_flower.classid = f->res.classid; > + cls_flower.skip_sw = skip_sw; > > err = tc_setup_offload_action(&cls_flower.rule->action, &f->exts, > cls_flower.common.extack); > diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c > index 9f1e62ca508d..9bd598f8a46c 100644 > --- a/net/sched/cls_matchall.c > +++ b/net/sched/cls_matchall.c > @@ -98,6 +98,7 @@ static int mall_replace_hw_filter(struct tcf_proto *tp, > tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, extack); > cls_mall.command = TC_CLSMATCHALL_REPLACE; > cls_mall.cookie = cookie; > + cls_mall.skip_sw = skip_sw; > > err = tc_setup_offload_action(&cls_mall.rule->action, &head->exts, > cls_mall.common.extack); > -- > 2.43.0 >
On Sun, Oct 20, 2024 at 06:59:12PM +0300, Ido Schimmel wrote: > Possibly a stupid question given I don't remember all the details of the > TC offload, but is there a reason not to put the 'skip_sw' indication in > 'struct flow_cls_common_offload' and initialize the new field as part of > tc_cls_common_offload_init()? > > Seems like it won't require patching every classifier and will also work > for the re-offload case (e.g., fl_reoffload())? I think you forgot more about tc than I ever knew. The answer isn't more complicated than "I didn't think about it". I've tested this simpler proposal and will send v3 using it. Thanks.
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 292cd8f4b762..a2f688dd0447 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -692,6 +692,7 @@ struct flow_cls_offload { struct flow_cls_common_offload common; enum flow_cls_command command; bool use_act_stats; + bool skip_sw; unsigned long cookie; struct flow_rule *rule; struct flow_stats stats; diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 4880b3a7aced..7b9f41f33c33 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -782,6 +782,7 @@ struct tc_cls_matchall_offload { struct flow_rule *rule; struct flow_stats stats; bool use_act_stats; + bool skip_sw; unsigned long cookie; }; diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index e280c27cb9f9..8f7c60805f85 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -480,6 +480,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp, cls_flower.rule->match.mask = &f->mask->key; cls_flower.rule->match.key = &f->mkey; cls_flower.classid = f->res.classid; + cls_flower.skip_sw = skip_sw; err = tc_setup_offload_action(&cls_flower.rule->action, &f->exts, cls_flower.common.extack); diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c index 9f1e62ca508d..9bd598f8a46c 100644 --- a/net/sched/cls_matchall.c +++ b/net/sched/cls_matchall.c @@ -98,6 +98,7 @@ static int mall_replace_hw_filter(struct tcf_proto *tp, tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, extack); cls_mall.command = TC_CLSMATCHALL_REPLACE; cls_mall.cookie = cookie; + cls_mall.skip_sw = skip_sw; err = tc_setup_offload_action(&cls_mall.rule->action, &head->exts, cls_mall.common.extack);
Background: switchdev ports offload the Linux bridge, and most of the packets they handle will never see the CPU. The ports between which there exists no hardware data path are considered 'foreign' to switchdev. These can either be normal physical NICs without switchdev offload, or incompatible switchdev ports, or virtual interfaces like veth/dummy/etc. In some cases, an offloaded filter can only do half the work, and the rest must be handled by software. Redirecting/mirroring from the ingress of a switchdev port towards a foreign interface is one example of combined hardware/software data path. The most that the switchdev port can do is to extract the matching packets from its offloaded data path and send them to the CPU. From there on, the software filter runs (a second time, after the first run in hardware) on the packet and performs the mirred action. It makes sense for switchdev drivers which allow this kind of "half offloading" to sense the "skip_sw" flag of the filter/action pair, and deny attempts from the user to install a filter that does not run in software, because that simply won't work. In fact, a mirred action on a switchdev port towards a dummy interface appears to be a valid way of (selectively) monitoring offloaded traffic that flows through it. IFF_PROMISC was also discussed years ago, but (despite initial disagreement) there seems to be consensus that this flag should not affect the destination taken by packets, but merely whether or not the NIC discards packets with unknown MAC DA for local processing. Only the flower and matchall classifiers are of interest to me for purely pragmatic reasons: these are offloaded by DSA currently. [1] https://lore.kernel.org/netdev/20190830092637.7f83d162@ceranb/ [2] https://lore.kernel.org/netdev/20191002233750.13566-1-olteanv@gmail.com/ Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: rewrite commit message include/net/flow_offload.h | 1 + include/net/pkt_cls.h | 1 + net/sched/cls_flower.c | 1 + net/sched/cls_matchall.c | 1 + 4 files changed, 4 insertions(+)