tc: Keep header rewrite actions order.

Currently, tc merges all header rewrite actions into one tc pedit
action. So the header rewrite actions order is lost. Save each header
rewrite action into one tc pedit action to keep the order. And only
append one tc csum action to the last pedit action of a series.

Signed-off-by: Chris Mi <cmi@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

3 files changed, 60 insertions, 42 deletions

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 0d0cafc14..5b9759ed9 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -410,10 +410,10 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump)
 
 static void
 parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
-                                       struct tc_flower *flower)
+                                       struct tc_action *action)
 {
-    char *mask = (char *) &flower->rewrite.mask;
-    char *data = (char *) &flower->rewrite.key;
+    char *mask = (char *) &action->rewrite.mask;
+    char *data = (char *) &action->rewrite.key;
 
     for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
         char *put = NULL;
@@ -734,7 +734,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
             }
             break;
             case TC_ACT_PEDIT: {
-                parse_flower_rewrite_to_netlink_action(buf, flower);
+                parse_flower_rewrite_to_netlink_action(buf, action);
             }
             break;
             case TC_ACT_ENCAP: {
@@ -1085,8 +1085,8 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     uint64_t set_stub[1024 / 8];
     struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
     char *set_data, *set_mask;
-    char *key = (char *) &flower->rewrite.key;
-    char *mask = (char *) &flower->rewrite.mask;
+    char *key = (char *) &action->rewrite.key;
+    char *mask = (char *) &action->rewrite.mask;
     const struct nlattr *attr;
     int i, j, type;
     size_t size;
@@ -1128,14 +1128,6 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
         }
     }
 
-    if (!is_all_zeros(&flower->rewrite, sizeof flower->rewrite)) {
-        if (flower->rewrite.rewrite == false) {
-            flower->rewrite.rewrite = true;
-            action->type = TC_ACT_PEDIT;
-            flower->action_count++;
-        }
-    }
-
     if (hasmask && !is_all_zeros(set_mask, size)) {
         VLOG_DBG_RL(&rl, "unsupported sub attribute of set action type %d",
                     type);
@@ -1144,6 +1136,8 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     }
 
     ofpbuf_uninit(&set_buf);
+    action->type = TC_ACT_PEDIT;
+    flower->action_count++;
     return 0;
 }
 
diff --git a/lib/tc.c b/lib/tc.c
index 5337f104c..31c046790 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -881,14 +881,14 @@ static const struct nl_policy pedit_policy[] = {
 static int
 nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
 {
-    struct tc_action *action;
+    struct tc_action *action = &flower->actions[flower->action_count++];
     struct nlattr *pe_attrs[ARRAY_SIZE(pedit_policy)];
     const struct tc_pedit *pe;
     const struct tc_pedit_key *keys;
     const struct nlattr *nla, *keys_ex, *ex_type;
     const void *keys_attr;
-    char *rewrite_key = (void *) &flower->rewrite.key;
-    char *rewrite_mask = (void *) &flower->rewrite.mask;
+    char *rewrite_key = (void *) &action->rewrite.key;
+    char *rewrite_mask = (void *) &action->rewrite.mask;
     size_t keys_ex_size, left;
     int type, i = 0, err;
 
@@ -967,7 +967,6 @@ nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
         i++;
     }
 
-    action = &flower->actions[flower->action_count++];
     action->type = TC_ACT_PEDIT;
 
     return 0;
@@ -2243,14 +2242,14 @@ nl_msg_put_act_flags(struct ofpbuf *request) {
  * first_word_mask/last_word_mask - the mask to use for the first/last read
  * (as we read entire words). */
 static void
-calc_offsets(struct tc_flower *flower, struct flower_key_to_pedit *m,
+calc_offsets(struct tc_action *action, struct flower_key_to_pedit *m,
              int *cur_offset, int *cnt, ovs_be32 *last_word_mask,
              ovs_be32 *first_word_mask, ovs_be32 **mask, ovs_be32 **data)
 {
     int start_offset, max_offset, total_size;
     int diff, right_zero_bits, left_zero_bits;
-    char *rewrite_key = (void *) &flower->rewrite.key;
-    char *rewrite_mask = (void *) &flower->rewrite.mask;
+    char *rewrite_key = (void *) &action->rewrite.key;
+    char *rewrite_mask = (void *) &action->rewrite.mask;
 
     max_offset = m->offset + m->size;
     start_offset = ROUND_DOWN(m->offset, 4);
@@ -2317,7 +2316,8 @@ csum_update_flag(struct tc_flower *flower,
 
 static int
 nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
-                                 struct tc_flower *flower)
+                                 struct tc_flower *flower,
+                                 struct tc_action *action)
 {
     struct {
         struct tc_pedit sel;
@@ -2341,7 +2341,7 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
             continue;
         }
 
-        calc_offsets(flower, m, &cur_offset, &cnt, &last_word_mask,
+        calc_offsets(action, m, &cur_offset, &cnt, &last_word_mask,
                      &first_word_mask, &mask, &data);
 
         for (j = 0; j < cnt; j++,  mask++, data++, cur_offset += 4) {
@@ -2400,6 +2400,29 @@ nl_msg_put_flower_acts_release(struct ofpbuf *request, uint16_t act_index)
     nl_msg_end_nested(request, act_offset);
 }
 
+/* Aggregates all previous successive pedit actions csum_update_flags
+ * to flower->csum_update_flags. Only append one csum action to the
+ * last pedit action. */
+static void
+nl_msg_put_csum_act(struct ofpbuf *request, struct tc_flower *flower,
+                    uint16_t *act_index)
+{
+    size_t act_offset;
+
+    /* No pedit actions or processed already. */
+    if (!flower->csum_update_flags) {
+        return;
+    }
+
+    act_offset = nl_msg_start_nested(request, (*act_index)++);
+    nl_msg_put_act_csum(request, flower->csum_update_flags);
+    nl_msg_put_act_flags(request);
+    nl_msg_end_nested(request, act_offset);
+
+    /* Clear it. So we can have another series of pedit actions. */
+    flower->csum_update_flags = 0;
+}
+
 static int
 nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
 {
@@ -2416,20 +2439,22 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
 
         action = flower->actions;
         for (i = 0; i < flower->action_count; i++, action++) {
+            if (action->type != TC_ACT_PEDIT) {
+                nl_msg_put_csum_act(request, flower, &act_index);
+            }
             switch (action->type) {
             case TC_ACT_PEDIT: {
                 act_offset = nl_msg_start_nested(request, act_index++);
-                error = nl_msg_put_flower_rewrite_pedits(request, flower);
+                error = nl_msg_put_flower_rewrite_pedits(request, flower,
+                                                         action);
                 if (error) {
                     return error;
                 }
                 nl_msg_end_nested(request, act_offset);
 
-                if (flower->csum_update_flags) {
-                    act_offset = nl_msg_start_nested(request, act_index++);
-                    nl_msg_put_act_csum(request, flower->csum_update_flags);
-                    nl_msg_put_act_flags(request);
-                    nl_msg_end_nested(request, act_offset);
+                if (i == flower->action_count - 1) {
+                    /* If this is the last action check csum calc again. */
+                    nl_msg_put_csum_act(request, flower, &act_index);
                 }
             }
             break;
diff --git a/lib/tc.h b/lib/tc.h
index e5657e52d..b75af1a41 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -245,11 +245,23 @@ struct tc_action {
             bool force;
             bool commit;
         } ct;
+
+        struct {
+            struct tc_flower_key key;
+            struct tc_flower_key mask;
+        } rewrite;
      };
 
      enum tc_action_type type;
 };
 
+/* assert that if we overflow with a masked write of uint32_t to the last byte
+ * of action.rewrite we overflow inside struct tc_action.
+ * shouldn't happen unless someone moves rewrite to the end of action */
+BUILD_ASSERT_DECL(offsetof(struct tc_action, rewrite)
+                  + MEMBER_SIZEOF(struct tc_action, rewrite)
+                  + sizeof(uint32_t) - 2 < sizeof(struct tc_action));
+
 enum tc_offloaded_state {
     TC_OFFLOADED_STATE_UNDEFINED,
     TC_OFFLOADED_STATE_IN_HW,
@@ -323,12 +335,6 @@ struct tc_flower {
     struct ovs_flow_stats stats_hw;
     uint64_t lastused;
 
-    struct {
-        bool rewrite;
-        struct tc_flower_key key;
-        struct tc_flower_key mask;
-    } rewrite;
-
     uint32_t csum_update_flags;
 
     bool tunnel;
@@ -342,13 +348,6 @@ struct tc_flower {
     enum tc_offload_policy tc_policy;
 };
 
-/* assert that if we overflow with a masked write of uint32_t to the last byte
- * of flower.rewrite we overflow inside struct flower.
- * shouldn't happen unless someone moves rewrite to the end of flower */
-BUILD_ASSERT_DECL(offsetof(struct tc_flower, rewrite)
-                  + MEMBER_SIZEOF(struct tc_flower, rewrite)
-                  + sizeof(uint32_t) - 2 < sizeof(struct tc_flower));
-
 int tc_replace_flower(struct tcf_id *id, struct tc_flower *flower);
 int tc_del_filter(struct tcf_id *id);
 int tc_get_flower(struct tcf_id *id, struct tc_flower *flower);