| Message ID | 20180426055448.3742-1-jianbol@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] odp-util: Remove unnecessary TOS ECN bits rewrite for tunnels | expand |
Thanks Jianbo, On 26 April 2018 at 07:54, Jianbo Liu <jianbol@mellanox.com> wrote: > For tunnels, TOS ECN bits are never wildcard for the reason that they > are always inherited. OVS will create a rewrite action if we add rule > to modify other IP headers. But it also adds an extra ECN rewrite for > the action because of this ECN un-wildcarding. > > It seems no error because the ECN bits to be changed are same in this > case. But as rule can't be offloaded to hardware, the unnecssary ECN > rewrite should be removed. > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > Reviewed-by: Paul Blakey <paulb@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> > I'd like to take a little time to test this one.
On Thu, Apr 26, 2018 at 05:54:48AM +0000, Jianbo Liu wrote: > For tunnels, TOS ECN bits are never wildcard for the reason that they > are always inherited. OVS will create a rewrite action if we add rule > to modify other IP headers. But it also adds an extra ECN rewrite for > the action because of this ECN un-wildcarding. > > It seems no error because the ECN bits to be changed are same in this > case. But as rule can't be offloaded to hardware, the unnecssary ECN > rewrite should be removed. > > Signed-off-by: Jianbo Liu <jianbol@mellanox.com> > Reviewed-by: Paul Blakey <paulb@mellanox.com> > Reviewed-by: Roi Dayan <roid@mellanox.com> Thanks for working on improving OVS. Please add {} around conditional statements, according to OVS style. This isn't really a review because I didn't consider anything beyond style. Thanks, Ben.
On 26 April 2018 at 14:50, Simon Horman <simon.horman@netronome.com> wrote: > Thanks Jianbo, > > On 26 April 2018 at 07:54, Jianbo Liu <jianbol@mellanox.com> wrote: > >> For tunnels, TOS ECN bits are never wildcard for the reason that they >> are always inherited. OVS will create a rewrite action if we add rule >> to modify other IP headers. But it also adds an extra ECN rewrite for >> the action because of this ECN un-wildcarding. >> >> It seems no error because the ECN bits to be changed are same in this >> case. But as rule can't be offloaded to hardware, the unnecssary ECN >> rewrite should be removed. >> >> Signed-off-by: Jianbo Liu <jianbol@mellanox.com> >> Reviewed-by: Paul Blakey <paulb@mellanox.com> >> Reviewed-by: Roi Dayan <roid@mellanox.com> >> > > I'd like to take a little time to test this one. > Hi Jinbao, thanks again for your patch. We have tested this patch and from a functional point of view it looks good to me. Could you post a v2 which addresses the style concerns raised separately by Ben? Thanks!
diff --git a/lib/odp-util.c b/lib/odp-util.c index 6db241a..91bf5d6 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -6962,6 +6962,10 @@ commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow, mask.ipv4_proto = 0; /* Not writeable. */ mask.ipv4_frag = 0; /* Not writable. */ + if (flow_tnl_dst_is_set(&base_flow->tunnel) && + ((base_flow->nw_tos ^ flow->nw_tos) & IP_ECN_MASK) == 0) + mask.ipv4_tos &= ~IP_ECN_MASK; + if (commit(OVS_KEY_ATTR_IPV4, use_masked, &key, &base, &mask, sizeof key, odp_actions)) { put_ipv4_key(&base, base_flow, false); @@ -7012,6 +7016,10 @@ commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow, mask.ipv6_proto = 0; /* Not writeable. */ mask.ipv6_frag = 0; /* Not writable. */ + if (flow_tnl_dst_is_set(&base_flow->tunnel) && + ((base_flow->nw_tos ^ flow->nw_tos) & IP_ECN_MASK) == 0) + mask.ipv6_tclass &= ~IP_ECN_MASK; + if (commit(OVS_KEY_ATTR_IPV6, use_masked, &key, &base, &mask, sizeof key, odp_actions)) { put_ipv6_key(&base, base_flow, false);