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

Sorry for reply this message again.
On November 24, 2021 10:11 AM, Baowen Zheng wrote:

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

quoted

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

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

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?

quoted

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.

Maybe I misunderstand what you mean previously, when I look through the implement in
flow_action_init, I did not found we use the extack to make a log before return -EINVAL. 
So could you please figure it out? Maybe I miss something or misunderstand again. 

quoted

cheers,
jamal