On Sun, Jan 22, 2023 at 04:55:07PM +0200, Paul Blakey wrote:

For drivers to support partial offload of a filter's action list,
add support for action miss to specify an action instance to
continue from in sw.

CT action in particular can't be fully offloaded, as new connections
need to be handled in software. This imposes other limitations on
the actions that can be offloaded together with the CT action, such
as packet modifications.

Assign each action on a filter's action list a unique miss_cookie
which drivers can then use to fill action_miss part of the tc skb
extension. On getting back this miss_cookie, find the action
instance with relevant cookie and continue classifying from there.

Signed-off-by: Paul Blakey <redacted>
Reviewed-by: Jiri Pirko <redacted>

...

...

nit: I think it would be nicer if there was a dummy implementation
of tcf_exts_init_ex for the !CONFIG_NET_CLS case and #ifdefs
could disappear from tcf_exts_init() entirely.

Likewise, elsewhere in this patch there seems to be new #if/#ifdefs
Which may be in keeping with the style of this file. But I also
think it's a style we ought to be moving away from.

-	exts->action = action;
-	exts->police = police;
-	return 0;
 }
 
 /* Return false if the netns is being destroyed in cleanup_net(). Callers

...

...

+static struct tcf_exts_miss_cookie_node *
+tcf_exts_miss_cookie_lookup(u64 miss_cookie, int *act_index)
+{
+	union tcf_exts_miss_cookie mc = { .miss_cookie = miss_cookie, };
+
+	*act_index = mc.act_index;
+	return xa_load(&tcf_exts_miss_cookies_xa, mc.miss_cookie_base);
+}
+#endif /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */
+
+static u64 tcf_exts_miss_cookie_get(u32 miss_cookie_base, int act_index)
+{
+	union tcf_exts_miss_cookie mc = { .act_index = act_index, };
+
+	if (!miss_cookie_base)
+		return 0;

nit: perhaps the compiler optimises this out, or it is otherwise
unimportant,
but doesn't the assignment
of mc zero it's fields, other than .act_index only for:

1. Any assignment of mc to be unused in the !miss_cookie_base case
2. mc.miss_cookie_base to be reassigned, otherwise

FWIIW, I might have gone for something more like this (*untested*).

	union tcf_exts_miss_cookie mc;

	if (!miss_cookie_base)
		return 0;

	mc.act_index = act_index;
	mc.miss_cookie_base = miss_cookie_base;

	return mc.miss_cookie;

I see that this is probably safe, but it's not entirely obvious
that tp->ops->get_exts will never by NULL here.

1) It is invariant on n, which is set in a different function and is in turn
2) invariant on ext->act_miss being set, several patches away, in the driver.
3) Which is lastly invariant on being a code path relating to for
   hardware offload of a classifier where tp->ops->get_exts is defined.

Or perhaps I mixed it up somehow. But I do think at a minimum this
ought to be documented.

Which brings me to a more central concern with this series: it's very
invasive and sets up a complex relationship between the core and the
driver. Is this the right abstraction for the problem at hand?

+			if (unlikely(!exts || n->exts != exts))
+				return TC_ACT_SHOT;
+
+			n = NULL;
+#ifdef CONFIG_NET_CLS_ACT
+			err = tcf_action_exec(skb, exts->actions + act_index,
+					      exts->nr_actions - act_index,
+					      res);
+#endif
+		} else {
+			if (tp->protocol != protocol &&
+			    tp->protocol != htons(ETH_P_ALL))
+				continue;
+
+			err = tc_classify(skb, tp, res);
+		}
 #ifdef CONFIG_NET_CLS_ACT
 		if (unlikely(err == TC_ACT_RECLASSIFY && !compat_mode)) {
 			first_tp = orig_tp;

...

nit: ext->chain seems to only be used once, as it was before this patch.
Perhaps the chain local variable is an artifact of development
and is best not added?

+
+			if (ext->act_miss) {
+				n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie,
+								&act_index);
+				if (!n)
+					return TC_ACT_SHOT;
 
-			fchain = tcf_chain_lookup_rcu(block, ext->chain);
+				chain = n->chain_index;
+			}
+
+			fchain = tcf_chain_lookup_rcu(block, chain);
 			if (!fchain)
 				return TC_ACT_SHOT;
 

...