On 2022-02-20 1:47 PM, Roi Dayan wrote:


On 2022-02-17 5:40 PM, Eelco Chaudron wrote:


On 17 Feb 2022, at 8:41, Roi Dayan wrote:

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?

Thanks for the details, and yes, it might be handy to have this in the commit message. Let's say this commit is causing a problem (there might be one, see below). Someone trying to understand why this commit was added, has a quick way to test it.

Now for the problem I see when doing some simple testing (I did not review the code, so maybe it's obvious), but with the patch applied I, execute the following:

   ovs-appctl dpctl/add-flow "ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5,recirc_id(0),in_port(2),eth_type(0x0800),eth(),ipv4(proto=6),tcp()" "set(ipv4(ttl=3)),3,set(ipv4(ttl=4))" && ovs-appctl dpctl/dump-flows type=tc && tc filter show dev enp4s0f0 ingress

This is leaving the revalidators in some kind of a loop. Without the patch, I do not see the problem. This is a log snippet:

2022-02-17T15:38:52.990Z|00050|tc|WARN|expected act csum with flags: 0x9
2022-02-17T15:38:52.990Z|00051|tc|WARN|Kernel flower acknowledgment does not match request!  Set dpif_netlink to dbg to see which rule caused this error.
2022-02-17T15:38:52.993Z|00052|tc|WARN|expected act csum with flags: 0x9
2022-02-17T15:38:53.052Z|00001|tc(revalidator18)|WARN|expected act csum with flags: 0x9
2022-02-17T15:38:53.052Z|00002|netdev_offload_tc(revalidator18)|ERR|flow get 
failed (dev ovs-system prio 3 handle 1): Invalid argument
2022-02-17T15:38:53.052Z|00003|tc(revalidator18)|WARN|expected act csum with flags: 0x9
2022-02-17T15:38:53.052Z|00004|netdev_offload_tc(revalidator18)|ERR|flow get 
failed (dev enp4s0f1 prio 3 handle 1): Invalid argument
2022-02-17T15:38:53.052Z|00005|netdev_offload_tc(revalidator18)|ERR|flow get 
failed (dev enp4s0f0 prio 3 handle 1): Invalid argument
2022-02-17T15:38:53.052Z|00006|netdev_offload_tc(revalidator18)|ERR|flow get 
failed (dev ovs_pvp_br0 prio 3 handle 1): Invalid argument
2022-02-17T15:38:53.052Z|00007|dpif(revalidator18)|WARN|system@ovs-system: failed to 
flow_get (No such file or directory) ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5 
<empty>, packets:0, bytes:0, used:never
2022-02-17T15:38:53.052Z|00008|ofproto_dpif_upcall(revalidator18)|WARN|Failed 
to acquire udpif_key corresponding to unexpected flow (No such file or 
directory): ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5

So I guess this patch needs some fixing… I will do a popper code review for v3, and maybe you can add those test cases :)

//Eelco


hmm ok I see what's going on.

before this commit all pedit actions merged into single one and then
there was an additon of csum action after that single pedit.
so csum always existed and the code zero flower->csum_update_flags.

after this commit there are multiple pedits as the order is important
and csum being calculated on the next non pedit action.
since your last action in the example is pedit we now dont add the last
csum action.

when ovs dumps and parse the tc rules it hits this rule with last
pedit action and expect a csum action after it but doesnt find it.

so we basically missing calling nl_msg_put_csum_act() one more time
after the loop of actions in nl_msg_put_flower_acts().

my test case of editing macs didn't update csum_update_flags which
didn't trigger this error. your case changing ip header did.
you case is enough and covers my case so no need for both.

it's a small issue as real case wont have a rule ending with pedit
and not a forward action like redirect, mirror, goto..

thanks for catching


I have added the fix and tested manually and its ok but as for the test
in ovs testsuite i have a problem.
Adding tc rule with ovd-dpctl is deleted right away. manually it works
as you and I tested but I added in the testsuite and most of the times
its faster and ovs deletes the rule and the tc filter show doesn't see
the rule anymore and fails the test.

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