diff mbox series

[ovs-dev,v5,06/10] tc: Move tunnel_key unset action before output ports

Message ID 1576511601-12348-7-git-send-email-paulb@mellanox.com
State Changes Requested
Headers show
Series Add support for offloading CT datapath rules to TC | expand

Commit Message

Paul Blakey Dec. 16, 2019, 3:53 p.m. UTC 
Since OvS datapath gets packets already decapsulated from tunnel devices,
it doesn't explicitly decapsulate them. So in a recirculation setup,
the tunnel matching continues in the recirculation as the tunnel
metadata still exists on the SKB.

Tunnel key unset action unsets this metadata. Some drivers might rely
on this explicit tunnel key unset to know when to decapsulate the packet
instead of the device type. So instead of removing it completly,
we move it near the output actions.

This way, we also keep SKB metadata through recirculation, and for
non-recirculation rules, the resulting tc rules should remain the same.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>

---

Changelog:
V3->V4:
    Fix: Set flower tunnel attribute to true, if any of the tunnel
         attributes exists (we used to rely on unset action).
V2->V3:
    Actually removed old tunnel set if tunnel exists (instead of just
    adding a new one before a port)
    Moved patch earlier to help git bisect
---
 lib/tc.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Marcelo Leitner Dec. 16, 2019, 5:52 p.m. UTC | #1 
On Mon, Dec 16, 2019 at 05:53:17PM +0200, Paul Blakey wrote:
> diff --git a/lib/tc.c b/lib/tc.c
> index b2d8ca7..7a4acce 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -665,6 +665,12 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)

Not sure why but my 'git am' can't process this patch:
$ git am --show-current | patch -p1
patching file lib/tc.c
patch: **** malformed patch at line 109: c_flower *flower)

git am --show-current  gives:

--- a/lib/tc.c
+++ b/lib/tc.c
@@ -665,6 +665,12 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct t=
                                                          line broken here ^
c_flower *flower)
         flower->mask.tunnel.ttl =3D
                                  ^^ other encoding
             nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ENC_IP_TTL_MASK]);
     }
+
+    if (!is_all_zeros(&flower->mask.tunnel, sizeof flower->mask.tunnel) ||
+        !is_all_zeros(&flower->key.tunnel, sizeof flower->key.tunnel)) {
+        flower->tunnel =3D true;
                         ^^ another here

but if I display it with mutt, I can see the patch just fine. Seems
git am is not decoding something here (git-2.23.0-1.fc31).


>          flower->mask.tunnel.ttl =
>              nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ENC_IP_TTL_MASK]);
>      }
> +
> +    if (!is_all_zeros(&flower->mask.tunnel, sizeof flower->mask.tunnel) ||
> +        !is_all_zeros(&flower->key.tunnel, sizeof flower->key.tunnel)) {
> +        flower->tunnel = true;
> +    }
> +
>      if (attrs[TCA_FLOWER_KEY_ENC_OPTS] &&
>          attrs[TCA_FLOWER_KEY_ENC_OPTS_MASK]) {
>           err = nl_parse_flower_tunnel_opts(attrs[TCA_FLOWER_KEY_ENC_OPTS],
Paul Blakey Dec. 17, 2019, 8:31 a.m. UTC | #2 
I have no problem, maybe remove the changelog part?

paulb@reg-r-vrt-019-180 openvswitch (git (detached from origin/master))$ git checkout FETCH_HEAD
Note: checking out 'FETCH_HEAD'.
paulb@reg-r-vrt-019-180 openvswitch (git (detached from origin/master))$ git am 0submit/ovs/ovs/5/0001-match-Add-match_set_ct_zone_masked-helper.patch
Applying: match: Add match_set_ct_zone_masked helper
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0002*
Applying: compat: Add tc ct action and flower matches defines for older kernels
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0003*
Applying: tc: Introduce tcf_id to specify a tc filter
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0004*
Applying: netdev-offload-tc: Implement netdev tc flush via tc filter del
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0005*
Applying: dpif: Add support to set user features
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0006*
Applying: tc: Move tunnel_key unset action before output ports
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0007*
Applying: netdev-offload-tc: Add recirculation support via tc chains
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0008*
Applying: netdev-offload-tc: Add conntrack support
^[[Apaulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0009*
Applying: netdev-offload-tc: Add conntrack label and mark support
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git am 0submit/ovs/ovs/5/0010*
Applying: netdev-offload-tc: Add conntrack nat support
paulb@reg-r-vrt-019-180 openvswitch (git (detached from FETCH_HEAD))$ git log --oneline -11
574b18d (HEAD) netdev-offload-tc: Add conntrack nat support
ce5fb14 netdev-offload-tc: Add conntrack label and mark support
085fc42 netdev-offload-tc: Add conntrack support
4515f8d netdev-offload-tc: Add recirculation support via tc chains
bf1e1c3 tc: Move tunnel_key unset action before output ports
4b4f5c6 dpif: Add support to set user features
aa5e592 netdev-offload-tc: Implement netdev tc flush via tc filter del
015e4d1 tc: Introduce tcf_id to specify a tc filter
ca41aaf compat: Add tc ct action and flower matches defines for older kernels
c9c3fa9 match: Add match_set_ct_zone_masked helper
391b52f rhel: Support RHEL 7.8 kernel module rpm build



Paul.
Marcelo Leitner Dec. 18, 2019, 5:22 p.m. UTC | #3 
On Tue, Dec 17, 2019 at 08:31:20AM +0000, Paul Blakey wrote:
> I have no problem, maybe remove the changelog part?

It was in the patch itself.
Enabling pipe_decode in mutt did it. Now everything applies here.

Thanks,
Marcelo
diff mbox series

Patch

diff --git a/lib/tc.c b/lib/tc.c
index b2d8ca7..7a4acce 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -665,6 +665,12 @@  nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
         flower->mask.tunnel.ttl =
             nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ENC_IP_TTL_MASK]);
     }
+
+    if (!is_all_zeros(&flower->mask.tunnel, sizeof flower->mask.tunnel) ||
+        !is_all_zeros(&flower->key.tunnel, sizeof flower->key.tunnel)) {
+        flower->tunnel = true;
+    }
+
     if (attrs[TCA_FLOWER_KEY_ENC_OPTS] &&
         attrs[TCA_FLOWER_KEY_ENC_OPTS_MASK]) {
          err = nl_parse_flower_tunnel_opts(attrs[TCA_FLOWER_KEY_ENC_OPTS],
@@ -2091,24 +2097,17 @@  nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
 static int
 nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
 {
+    bool ingress, released = false;
     size_t offset;
     size_t act_offset;
     uint16_t act_index = 1;
     struct tc_action *action;
     int i, ifindex = 0;
-    bool ingress;
 
     offset = nl_msg_start_nested(request, TCA_FLOWER_ACT);
     {
         int error;
 
-        if (flower->tunnel) {
-            act_offset = nl_msg_start_nested(request, act_index++);
-            nl_msg_put_act_tunnel_key_release(request);
-            nl_msg_put_act_flags(request);
-            nl_msg_end_nested(request, act_offset);
-        }
-
         action = flower->actions;
         for (i = 0; i < flower->action_count; i++, action++) {
             switch (action->type) {
@@ -2185,6 +2184,13 @@  nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
             }
             break;
             case TC_ACT_OUTPUT: {
+                if (!released && flower->tunnel) {
+                    act_offset = nl_msg_start_nested(request, act_index++);
+                    nl_msg_put_act_tunnel_key_release(request);
+                    nl_msg_end_nested(request, act_offset);
+                    released = true;
+                }
+
                 ingress = action->out.ingress;
                 ifindex = action->out.ifindex_out;
                 if (ifindex < 1) {