Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device

On 2021-11-23 03:23, Baowen Zheng wrote:

On November 22, 2021 8:25 PM, Jamal Hadi Salim wrote:

quoted

On 2021-11-18 08:07, Simon Horman wrote:

[..]

quoted

--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -21,6 +21,19 @@
+#include <net/tc_act/tc_pedit.h>
+#include <net/tc_act/tc_mirred.h>
+#include <net/tc_act/tc_vlan.h>
+#include <net/tc_act/tc_tunnel_key.h> #include <net/tc_act/tc_csum.h>
+#include <net/tc_act/tc_gact.h> #include <net/tc_act/tc_police.h>
+#include <net/tc_act/tc_sample.h> #include <net/tc_act/tc_skbedit.h>
+#include <net/tc_act/tc_ct.h> #include <net/tc_act/tc_mpls.h>
+#include <net/tc_act/tc_gate.h> #include <net/flow_offload.h>

   #ifdef CONFIG_INET
   DEFINE_STATIC_KEY_FALSE(tcf_frag_xmit_count);

@@ -129,8 +142,157 @@ static void free_tcf(struct tc_action *p)
   	kfree(p);
   }

+static int flow_action_init(struct flow_offload_action *fl_action,
+			    struct tc_action *act,
+			    enum flow_act_command cmd,
+			    struct netlink_ext_ack *extack) {
+	if (!fl_action)
+		return -EINVAL;
+
+	fl_action->extack = extack;
+	fl_action->command = cmd;
+	fl_action->index = act->tcfa_index;
+
+	if (is_tcf_gact_ok(act)) {
+		fl_action->id = FLOW_ACTION_ACCEPT;
+	} else if (is_tcf_gact_shot(act)) {
+		fl_action->id = FLOW_ACTION_DROP;
+	} else if (is_tcf_gact_trap(act)) {
+		fl_action->id = FLOW_ACTION_TRAP;
+	} else if (is_tcf_gact_goto_chain(act)) {
+		fl_action->id = FLOW_ACTION_GOTO;
+	} else if (is_tcf_mirred_egress_redirect(act)) {
+		fl_action->id = FLOW_ACTION_REDIRECT;
+	} else if (is_tcf_mirred_egress_mirror(act)) {
+		fl_action->id = FLOW_ACTION_MIRRED;
+	} else if (is_tcf_mirred_ingress_redirect(act)) {
+		fl_action->id = FLOW_ACTION_REDIRECT_INGRESS;
+	} else if (is_tcf_mirred_ingress_mirror(act)) {
+		fl_action->id = FLOW_ACTION_MIRRED_INGRESS;
+	} else if (is_tcf_vlan(act)) {
+		switch (tcf_vlan_action(act)) {
+		case TCA_VLAN_ACT_PUSH:
+			fl_action->id = FLOW_ACTION_VLAN_PUSH;
+			break;
+		case TCA_VLAN_ACT_POP:
+			fl_action->id = FLOW_ACTION_VLAN_POP;
+			break;
+		case TCA_VLAN_ACT_MODIFY:
+			fl_action->id = FLOW_ACTION_VLAN_MANGLE;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+	} else if (is_tcf_tunnel_set(act)) {
+		fl_action->id = FLOW_ACTION_TUNNEL_ENCAP;
+	} else if (is_tcf_tunnel_release(act)) {
+		fl_action->id = FLOW_ACTION_TUNNEL_DECAP;
+	} else if (is_tcf_csum(act)) {
+		fl_action->id = FLOW_ACTION_CSUM;
+	} else if (is_tcf_skbedit_mark(act)) {
+		fl_action->id = FLOW_ACTION_MARK;
+	} else if (is_tcf_sample(act)) {
+		fl_action->id = FLOW_ACTION_SAMPLE;
+	} else if (is_tcf_police(act)) {
+		fl_action->id = FLOW_ACTION_POLICE;
+	} else if (is_tcf_ct(act)) {
+		fl_action->id = FLOW_ACTION_CT;
+	} else if (is_tcf_mpls(act)) {
+		switch (tcf_mpls_action(act)) {
+		case TCA_MPLS_ACT_PUSH:
+			fl_action->id = FLOW_ACTION_MPLS_PUSH;
+			break;
+		case TCA_MPLS_ACT_POP:
+			fl_action->id = FLOW_ACTION_MPLS_POP;
+			break;
+		case TCA_MPLS_ACT_MODIFY:
+			fl_action->id = FLOW_ACTION_MPLS_MANGLE;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+	} else if (is_tcf_skbedit_ptype(act)) {
+		fl_action->id = FLOW_ACTION_PTYPE;
+	} else if (is_tcf_skbedit_priority(act)) {
+		fl_action->id = FLOW_ACTION_PRIORITY;
+	} else if (is_tcf_gate(act)) {
+		fl_action->id = FLOW_ACTION_GATE;
+	} else {
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+

The challenge with this is now it is impossible to write an action as a
standalone module (which works today).
One resolution to this is to either reuse or introduce a new ops in struct
tc_action_ops.
Then flow_action_init() would just invoke this act->ops() which will do action
specific setup.

Thanks for bringing this to us.
As my understanding, for this issue, we are facing the same fact with What we do in function tc_setup_flow_action.
If we add a filter with a new added action, we will also fail to offload the filter.
For a new added action, if we aim to offload the action to hardware, then we definitely need a
init fction and setup function for action/filter offload. We can add a ops for the new added action to init or setup the action.

The simplest approach seems to be adding a field in ops struct and call
it hw_id (we already have id which represents the s/w side).
All your code in flow_action_init() then becomes something like:

         if (!fl_action)
                 return -EINVAL;

         fl_action->extack = extack;
         fl_action->command = cmd;
         fl_action->index = act->tcfa_index;


         fl_action->id = act->hwid;

And modules continue to work. Did i miss something?

Do you think it is proper to include this implement in our patch series or we can delivery a new patch for this?

Unless I am missing something basic, I dont see this as hard to do as
explained above in this patch series.

BTW: shouldnt extack be used here instead of returning just -EINVAL?
I didnt stare long enough but it seems extack is not passed when
deleting from hardware? I saw a NULL being passed in one of the patches.

cheers,
jamal