Re: [PATCH v2 net-next 1/6] net: sched: propagate "skip_sw" flag to offload for flower and matchall

From: Ido Schimmel
Date: Sun Oct 20 2024 - 11:59:42 EST


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@xxxxxxxxx/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> ---
> 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
>