On 2022-02-16 12:15 PM, Eelco Chaudron wrote:


On 16 Feb 2022, at 10:48, Roi Dayan wrote:

On 2022-02-16 10:37 AM, Eelco Chaudron wrote:


On 15 Feb 2022, at 10:52, Roi Dayan wrote:

From: Chris Mi <[email protected]>

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.

I’m on training all of this week, so will try to look at this later.
But in the meantime, do you think it’s worth adding a test case for this?

//Eelco

maybe. but the fix is already ready for some time while it looks like we
could add many potential cases related to tc.
We should and will find the time later to look at adding more unit tests
into the repo.
thanks

The reason for asking for a test case is that when reviewing TC-related patches 
I no longer just do a visual review, I test them. This is because a lot of 
times a small change results in the introduction of problems, for example with 
the dpctl/dump-flows output. So without any test cases, I need to reverse 
engineer how to replicate the problem, and then test it.

So my request would be if you are not supplying a test case, at least supply a 
simple replicator in the commit description.


I understand. I thought in this case it's clear.

before this commit all header rewrite actions are saved into
flower->rewrite and from there translated to single tc pedit action.

This patch saves header rewrite in action->rewrite per action instance
so translating actions to tc ends up with multiple/separated tc pedit
actions.

as we already agreed we will try to improve the testsuite
in the ovs repo in near future.

We can reproduce the issue like this:
Config ovs with hw-offload=true.
Create bridge with 2 ports.

execute:

ovs-appctl dpctl/add-flow "ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5,recirc_id(0),in_port(2),eth_type(0x0800),eth(),ipv4()" "set(eth(dst=76:59:99:fb:aa:aa)),3,set(eth(dst=76:59:99:fb:ff:ff))" ; tc filter show dev enp8s0f0 ingress;


before this commit you will see in TC pedit,mirred and after the commit
you will see pedit,mirred,pedit.

do you want me to add the steps to the commit msg?

Signed-off-by: Chris Mi <[email protected]>
Reviewed-by: Roi Dayan <[email protected]>
---

v2:
- rebased to fix conflict.

   lib/netdev-offload-tc.c | 22 ++++++++------------
   lib/tc.c                | 54 
+++++++++++++++++++++++++++++++++----------------
   lib/tc.h                | 25 +++++++++++------------
   3 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 9845e8d3feae..ab2a678c6923 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -481,10 +481,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;
@@ -877,7 +877,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: {
@@ -1222,8 +1222,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;
@@ -1265,14 +1265,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);
@@ -1281,6 +1273,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 adb2d3182ad4..b637f4c17ed9 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1006,14 +1006,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;

@@ -1092,7 +1092,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;
@@ -2399,14 +2398,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);
@@ -2473,7 +2472,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;
@@ -2497,7 +2497,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) {
@@ -2556,6 +2556,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)
   {
@@ -2572,21 +2595,18 @@ 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);
-                }
               }
               break;
               case TC_ACT_ENCAP: {
diff --git a/lib/tc.h b/lib/tc.h
index a147ca461dc5..f08786b72528 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -256,11 +256,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,
@@ -333,12 +345,6 @@ struct tc_flower {
       struct ovs_flow_stats stats;
       uint64_t lastused;

-    struct {
-        bool rewrite;
-        struct tc_flower_key key;
-        struct tc_flower_key mask;
-    } rewrite;
-
       uint32_t csum_update_flags;

       bool tunnel;
@@ -352,13 +358,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);
--
2.8.0


_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to