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

From: Paul Blakey <paulb@mellanox.com>

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>
---
 lib/tc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

On Wed, Nov 27, 2019 at 02:55:15PM +0200, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> 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.

Hm, you said 'move', but the patch is only 'adding' stuff.

Did you intend to:

diff --git a/lib/tc.c b/lib/tc.c
index 1541629a58e8..00d7f2f2239d 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2375,13 +2375,6 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
     {
         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) {

?

> 
> 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>
> ---
>  lib/tc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/tc.c b/lib/tc.c
> index 83ee0f6473aa..1541629a58e8 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2364,12 +2364,12 @@ 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);
>      {
> @@ -2458,6 +2458,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) {
> -- 
> 2.8.4
>

On 11/27/2019 5:33 PM, Marcelo Ricardo Leitner wrote:
> On Wed, Nov 27, 2019 at 02:55:15PM +0200, Roi Dayan wrote: >> From: Paul Blakey <paulb@mellanox.com><mailto:paulb@mellanox.com> >> >> 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. > > Hm, you said 'move', but the patch is only 'adding' stuff. > > Did you intend to: > > diff --git a/lib/tc.c b/lib/tc.c index 1541629a58e8..00d7f2f2239d > 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -2375,13 +2375,6 @@ > nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower > *flower) { int error; > > - if (flower->tunnel) { - act_of
 fset = >
  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) { > > ? > >

Yes I did, thanks.

not sure how it got added back in :|