RE: [RFC/PATCH net-next v3 3/8] flow_offload: allow user to offload tc action to net device

On November 1, 2021 6:07 PM, Oz Shlomo wrote:

On 11/1/2021 4:30 AM, Baowen Zheng wrote:

quoted

On 10/31/2021 5:50 PM, Oz Shlomo wrote:

quoted

On 10/28/2021 2:06 PM, Simon Horman wrote:

quoted

From: Baowen Zheng <redacted>

Use flow_indr_dev_register/flow_indr_dev_setup_offload to offload tc
action.

How will device drivers reference the offloaded actions when
offloading a flow?
Perhaps the flow_action_entry structure should also include the action

index.

quoted

quoted

We have set action index in flow_offload_action to offload the action, also

there are > already some actions in flow_action_entry include index which we
want to offload.

quoted

If the driver wants to support action that needs index, I think it can
add the index later, it may not include in this patch, WDYT?

What do you mean by "action that needs index"?

Currently only the police and gate actions have an action index parameter.
However, with this series the user can create any action using the tc action API
and then reference it from any filter.
Do you see a reason not to expose the action index as a flow_action_entry
attribute?

What I mean is currently the action is created along with the filter, then the index is not needed.
With this patch, we intend to offload the police action which already includes action index. 
I think your suggestion makes sense to us, we will consider to move the index to the
flow_action_entry structure instead of current in single action structure, thanks.

quoted

quoted

quoted

We need to call tc_cleanup_flow_action to clean up tc action entry
since in tc_setup_action, some actions may hold dev refcnt, especially
the mirror action.

Signed-off-by: Baowen Zheng <redacted>
Signed-off-by: Louis Peens <redacted>
Signed-off-by: Simon Horman <redacted>
---
   include/linux/netdevice.h  |   1 +
   include/net/act_api.h      |   2 +-
   include/net/flow_offload.h |  17 ++++
   include/net/pkt_cls.h      |  15 ++++
   net/core/flow_offload.c    |  43 ++++++++--
   net/sched/act_api.c        | 166

+++++++++++++++++++++++++++++++++++++

quoted

quoted

quoted

   net/sched/cls_api.c        |  29 ++++++-
   7 files changed, 260 insertions(+), 13 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3ec42495a43a..9815c3a058e9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -916,6 +916,7 @@ enum tc_setup_type {
   	TC_SETUP_QDISC_TBF,
   	TC_SETUP_QDISC_FIFO,
   	TC_SETUP_QDISC_HTB,
+	TC_SETUP_ACT,
   };

...

quoted

quoted

quoted

   int tcf_action_init(struct net *net, struct tcf_proto *tp, struct
nlattr *nla, @@ -1103,6 +1267,8 @@ int tcf_action_init(struct net *net,

struct tcf_proto *tp, struct nlattr *nla,

quoted

   		sz += tcf_action_fill_size(act);
   		/* Start from index 0 */
   		actions[i - 1] = act;
+		if (!(flags & TCA_ACT_FLAGS_BIND))
+			tcf_action_offload_add(act, extack);

Why is this restricted to actions created without the

TCA_ACT_FLAGS_BIND

quoted

quoted

flag?
How are actions instantiated by the filters different from those that are
created by "tc actions"?

Our patch aims to offload tc action that is created independent of any flow.

It is usually

quoted

offloaded when it is added or replaced.
This patch is to implement a process of reoffloading the actions when driver

is

quoted

inserted or removed, so it will still offload the independent actions.

I see.

quoted

quoted

quoted

   	}

   	/* We have to commit them all together, because if any error
happened in diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2ef8f5a6205a..351d93988b8b 100644

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

@@ -3544,8 +3544,8 @@ static enum flow_action_hw_stats

tc_act_hw_stats(u8 hw_stats)

quoted

   	return hw_stats;
   }

-int tc_setup_flow_action(struct flow_action *flow_action,
-			 const struct tcf_exts *exts)
+int tc_setup_action(struct flow_action *flow_action,
+		    struct tc_action *actions[])
   {
   	struct tc_action *act;
   	int i, j, k, err = 0;

@@ -3554,11 +3554,11 @@ int tc_setup_flow_action(struct flow_action

*flow_action,

quoted

   	BUILD_BUG_ON(TCA_ACT_HW_STATS_IMMEDIATE !=

FLOW_ACTION_HW_STATS_IMMEDIATE);

quoted

   	BUILD_BUG_ON(TCA_ACT_HW_STATS_DELAYED !=
FLOW_ACTION_HW_STATS_DELAYED);

-	if (!exts)
+	if (!actions)
   		return 0;

   	j = 0;
-	tcf_exts_for_each_action(i, act, exts) {
+	tcf_act_for_each_action(i, act, actions) {
   		struct flow_action_entry *entry;

   		entry = &flow_action->entries[j];

@@ -3725,7 +3725,19 @@ int tc_setup_flow_action(struct flow_action

*flow_action,

quoted

   	spin_unlock_bh(&act->tcfa_lock);
   	goto err_out;
   }
+EXPORT_SYMBOL(tc_setup_action);
+
+#ifdef CONFIG_NET_CLS_ACT
+int tc_setup_flow_action(struct flow_action *flow_action,
+			 const struct tcf_exts *exts)
+{
+	if (!exts)
+		return 0;
+
+	return tc_setup_action(flow_action, exts->actions); }
   EXPORT_SYMBOL(tc_setup_flow_action);
+#endif

   unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
   {

@@ -3743,6 +3755,15 @@ unsigned int tcf_exts_num_actions(struct

tcf_exts *exts)

quoted

   }
   EXPORT_SYMBOL(tcf_exts_num_actions);

+unsigned int tcf_act_num_actions_single(struct tc_action *act) {
+	if (is_tcf_pedit(act))
+		return tcf_pedit_nkeys(act);
+	else
+		return 1;
+}
+EXPORT_SYMBOL(tcf_act_num_actions_single);
+
   #ifdef CONFIG_NET_CLS_ACT
   static int tcf_qevent_parse_block_index(struct nlattr *block_index_attr,
   					u32 *p_block_index,