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

[Ido Schimmel]

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
>