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

On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:

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

quoted

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.

quoted

If we add a filter with a new added action, we will also fail to offload the

filter.

quoted

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.

quoted

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?

Hi Jamal, for your suggestion, I think it will work for most of the case. But there maybe some kind of actions
that will be assigned different hw_id in different case, such as the gact, we need to think about this case. 
So I will prefer to add a callback in action ops struct to implement the flow_action_init function for the new added
Standalone action.
WDYT?

quoted

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.

I did not mean it is difficult.
Since as my understanding, we will have the same problem in function of tc_setup_flow_action to
Setup the actions for a to be offloaded flower. So my proposal is to add a callback in action ops to implement
Both the function of flow_act_init and tc_setup_flow_action with a flag(maybe bind?) as a distinguish.
What is your opinion?

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.

Ok we will consider to delete the extack parameter.

cheers,
jamal