Message ID | 20191127125516.5135-10-roid@mellanox.com |
---|---|
State | Superseded |
Headers | show |
Series | Add support for offloading CT datapath rules to TC | expand |
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 :|
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) {