RE: [PATCH v6 net-next 06/12] flow_offload: allow user to offload tc action to net device

On December 12, 2021 3:42 AM, Jamal Hadi Salim wrote:

On 2021-12-09 04:28, Simon Horman wrote:

quoted

From: Baowen Zheng <redacted>

quoted

  /* These structures hold the attributes of bpf state that are being
passed diff --git a/include/net/flow_offload.h
b/include/net/flow_offload.h index f6970213497a..15662cad5bca 100644

--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h

@@ -551,6 +551,23 @@ struct flow_cls_offload {
  	u32 classid;
  };

+enum flow_act_command {

Readability:
flow_offload_act_command?

Ok, we will make the change.
maybe it is more proper for "offload_act_command " as we discussed in previous patch?

quoted

+
+struct flow_offload_action *flow_action_alloc(unsigned int
+num_actions);

Same here:
s/flow_action_alloc/offload_action_alloc

quoted

+struct flow_offload_action *flow_action_alloc(unsigned int
+num_actions) {

quoted

+static unsigned int tcf_act_num_actions_single(struct tc_action *act)
+{
+	if (is_tcf_pedit(act))
+		return tcf_pedit_nkeys(act);
+	else
+		return 1;
+}

Again - above only seems needed for offload. Could we name this
appropriately?

quoted

+
+static int flow_action_init(struct flow_offload_action *fl_action,

I think i mentioned this earlier:

quoted

+
+static int tcf_action_offload_cmd(struct flow_offload_action *fl_act,
+				  struct netlink_ext_ack *extack)\

nice

quoted

+
+/* offload the tc command after inserted */

"after it is inserted"

Thanks, we will make the change.

quoted

+static int tcf_action_offload_add(struct tc_action *action,

nice.

\

quoted

+	err = tcf_action_offload_cmd(fl_action, extack);
+	tc_cleanup_flow_action(&fl_action->action);

tc_cleanup_offload_action()?

quoted

+static int tcf_action_offload_del(struct tc_action *action)

nice.

quoted

+{
+	struct flow_offload_action fl_act = {};
+	int err = 0;
+
+	err = flow_action_init(&fl_act, action, FLOW_ACT_DESTROY, NULL);
+	if (err)
+		return err;
+
+	return tcf_action_offload_cmd(&fl_act, NULL); }
+
  static void tcf_action_cleanup(struct tc_action *p)
  {

mention offload somewhere there?

For this function of tcf_action_cleanup, it is not only related to offload process, it will also recycle resource for the software action. 
So it is better to keep it as it is now?

quoted

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index

33b81c867ac0..2a1cc7fe2dd9 100644

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

@@ -3488,8 +3488,8 @@ static int tc_setup_flow_act(struct tc_action *act,
  #endif
  }

-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[])
  {
  	int i, j, index, err = 0;
  	struct tc_action *act;

@@ -3498,11 +3498,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];

@@ -3531,6 +3531,19 @@ int tc_setup_flow_action(struct flow_action

*flow_action,

quoted

  	spin_unlock_bh(&act->tcfa_lock);
  	goto err_out;
  }
+
+int tc_setup_flow_action(struct flow_action *flow_action,
+			 const struct tcf_exts *exts)
+{

I think i mentioned this one earlier:
tc_setup_offload_action()

Thanks, we will make the change for the renaming function as your suggestion.

cheers,
jamal