[ovs-dev,2/2] odp-util: Do not rewrite fields with the same values as matched

Message ID 20190110085110.4900-3-elibr@mellanox.com
State Awaiting Upstream
Headers show
Series
  • Do not rewrite fields with the same values as
Related show

Commit Message

Eli Britstein Jan. 10, 2019, 8:51 a.m.
To improve performance and avoid wasting resources for HW offloaded
flows, do not rewrite fields that are matched with the same value.

Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 lib/odp-util.c        | 110 +++++++++++++++++++++++++++++++++++++++++++++-----
 tests/mpls-xlate.at   |   2 +-
 tests/ofproto-dpif.at |  14 +++----
 3 files changed, 108 insertions(+), 18 deletions(-)

Comments

Yifeng Sun Jan. 10, 2019, 7:35 p.m. | #1
Hi,

When I try to understand this patch, I feel there may be some issue in this
loop below.
It looks like each loop is doing the same thing. Can you please take a look?

+        for (i = 0; i < size; i++)
+            if (memcmp(pkey0, pkey1, size) == 0)
+                memset(pmask, 0, size);
+            else
+                differ = true;

Thanks,
Yifeng

On Thu, Jan 10, 2019 at 12:52 AM Eli Britstein <elibr@mellanox.com> wrote:

> To improve performance and avoid wasting resources for HW offloaded
> flows, do not rewrite fields that are matched with the same value.
>
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> ---
>  lib/odp-util.c        | 110
> +++++++++++++++++++++++++++++++++++++++++++++-----
>  tests/mpls-xlate.at   |   2 +-
>  tests/ofproto-dpif.at |  14 +++----
>  3 files changed, 108 insertions(+), 18 deletions(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 0491bed38..7e916f9d9 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -7086,12 +7086,50 @@ commit_odp_tunnel_action(const struct flow *flow,
> struct flow *base,
>      }
>  }
>
> +struct ovs_key_field_properties {
> +    int offset;
> +    int size;
> +};
> +
> +/* Compare the keys similary to memcmp, but each field separately.
> + * The offsets and sizes of each field is provided by field_properties
> + * argument.
> + * For fields with the same value, zero out their mask part in order not
> to
> + * rewrite them as it's unnecessary */
> +static bool
> +keycmp_mask(const void *key0, const void *key1,
> +            struct ovs_key_field_properties *field_properties, void *mask)
> +{
> +    int field, i;
> +    bool differ = false;
> +
> +    for (field = 0 ; ; field++) {
> +        int size = field_properties[field].size;
> +        int offset = field_properties[field].offset;
> +        char *pkey0 = ((char *)key0) + offset;
> +        char *pkey1 = ((char *)key1) + offset;
> +        char *pmask = ((char *)mask) + offset;
> +
> +        if (size == 0)
> +            break;
> +
> +        for (i = 0; i < size; i++)
> +            if (memcmp(pkey0, pkey1, size) == 0)
> +                memset(pmask, 0, size);
> +            else
> +                differ = true;
> +    }
> +
> +    return differ;
> +}
> +
>  static bool
>  commit(enum ovs_key_attr attr, bool use_masked_set,
>         const void *key, void *base, void *mask, size_t size,
> +       struct ovs_key_field_properties *field_properties,
>         struct ofpbuf *odp_actions)
>  {
> -    if (memcmp(key, base, size)) {
> +    if (keycmp_mask(key, base, field_properties, mask)) {
>          bool fully_masked = odp_mask_is_exact(attr, mask, size);
>
>          if (use_masked_set && !fully_masked) {
> @@ -7133,6 +7171,12 @@ commit_set_ether_action(const struct flow *flow,
> struct flow *base_flow,
>                          bool use_masked)
>  {
>      struct ovs_key_ethernet key, base, mask;
> +    struct ovs_key_field_properties ovs_key_ethernet_field_properties[] =
> {
> +#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ethernet,
> name), sizeof(type)},
> +        OVS_KEY_ETHERNET_FIELDS
> +        {0, 0}
> +#undef OVS_KEY_FIELD
> +    };
>
>      if (flow->packet_type != htonl(PT_ETH)) {
>          return;
> @@ -7143,7 +7187,8 @@ commit_set_ether_action(const struct flow *flow,
> struct flow *base_flow,
>      get_ethernet_key(&wc->masks, &mask);
>
>      if (commit(OVS_KEY_ATTR_ETHERNET, use_masked,
> -               &key, &base, &mask, sizeof key, odp_actions)) {
> +               &key, &base, &mask, sizeof key,
> +               ovs_key_ethernet_field_properties, odp_actions)) {
>          put_ethernet_key(&base, base_flow);
>          put_ethernet_key(&mask, &wc->masks);
>      }
> @@ -7269,6 +7314,12 @@ commit_set_ipv4_action(const struct flow *flow,
> struct flow *base_flow,
>                         bool use_masked)
>  {
>      struct ovs_key_ipv4 key, mask, base;
> +    struct ovs_key_field_properties ovs_key_ipv4_field_properties[] = {
> +#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ipv4, name),
> sizeof(type)},
> +        OVS_KEY_IPV4_FIELDS
> +        {0, 0}
> +#undef OVS_KEY_FIELD
> +    };
>
>      /* Check that nw_proto and nw_frag remain unchanged. */
>      ovs_assert(flow->nw_proto == base_flow->nw_proto &&
> @@ -7286,7 +7337,7 @@ commit_set_ipv4_action(const struct flow *flow,
> struct flow *base_flow,
>      }
>
>      if (commit(OVS_KEY_ATTR_IPV4, use_masked, &key, &base, &mask, sizeof
> key,
> -               odp_actions)) {
> +               ovs_key_ipv4_field_properties, odp_actions)) {
>          put_ipv4_key(&base, base_flow, false);
>          if (mask.ipv4_proto != 0) { /* Mask was changed by commit(). */
>              put_ipv4_key(&mask, &wc->masks, true);
> @@ -7324,6 +7375,12 @@ commit_set_ipv6_action(const struct flow *flow,
> struct flow *base_flow,
>                         bool use_masked)
>  {
>      struct ovs_key_ipv6 key, mask, base;
> +    struct ovs_key_field_properties ovs_key_ipv6_field_properties[] = {
> +#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ipv6, name),
> sizeof(type)},
> +        OVS_KEY_IPV6_FIELDS
> +        {0, 0}
> +#undef OVS_KEY_FIELD
> +    };
>
>      /* Check that nw_proto and nw_frag remain unchanged. */
>      ovs_assert(flow->nw_proto == base_flow->nw_proto &&
> @@ -7342,7 +7399,7 @@ commit_set_ipv6_action(const struct flow *flow,
> struct flow *base_flow,
>      }
>
>      if (commit(OVS_KEY_ATTR_IPV6, use_masked, &key, &base, &mask, sizeof
> key,
> -               odp_actions)) {
> +               ovs_key_ipv6_field_properties, odp_actions)) {
>          put_ipv6_key(&base, base_flow, false);
>          if (mask.ipv6_proto != 0) { /* Mask was changed by commit(). */
>              put_ipv6_key(&mask, &wc->masks, true);
> @@ -7378,13 +7435,19 @@ commit_set_arp_action(const struct flow *flow,
> struct flow *base_flow,
>                        struct ofpbuf *odp_actions, struct flow_wildcards
> *wc)
>  {
>      struct ovs_key_arp key, mask, base;
> +    struct ovs_key_field_properties ovs_key_arp_field_properties[] = {
> +#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_arp, name),
> sizeof(type)},
> +        OVS_KEY_ARP_FIELDS
> +        {0, 0}
> +#undef OVS_KEY_FIELD
> +    };
>
>      get_arp_key(flow, &key);
>      get_arp_key(base_flow, &base);
>      get_arp_key(&wc->masks, &mask);
>
>      if (commit(OVS_KEY_ATTR_ARP, true, &key, &base, &mask, sizeof key,
> -               odp_actions)) {
> +               ovs_key_arp_field_properties, odp_actions)) {
>          put_arp_key(&base, base_flow);
>          put_arp_key(&mask, &wc->masks);
>          return SLOW_ACTION;
> @@ -7413,6 +7476,12 @@ commit_set_icmp_action(const struct flow *flow,
> struct flow *base_flow,
>                         struct ofpbuf *odp_actions, struct flow_wildcards
> *wc)
>  {
>      struct ovs_key_icmp key, mask, base;
> +    struct ovs_key_field_properties ovs_key_icmp_field_properties[] = {
> +#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_icmp, name),
> sizeof(type)},
> +        OVS_KEY_ICMP_FIELDS
> +        {0, 0}
> +#undef OVS_KEY_FIELD
> +    };
>      enum ovs_key_attr attr;
>
>      if (is_icmpv4(flow, NULL)) {
> @@ -7427,7 +7496,8 @@ commit_set_icmp_action(const struct flow *flow,
> struct flow *base_flow,
>      get_icmp_key(base_flow, &base);
>      get_icmp_key(&wc->masks, &mask);
>
> -    if (commit(attr, false, &key, &base, &mask, sizeof key, odp_actions))
> {
> +    if (commit(attr, false, &key, &base, &mask, sizeof key,
> +               ovs_key_icmp_field_properties, odp_actions)) {
>          put_icmp_key(&base, base_flow);
>          put_icmp_key(&mask, &wc->masks);
>          return SLOW_ACTION;
> @@ -7459,13 +7529,19 @@ commit_set_nd_action(const struct flow *flow,
> struct flow *base_flow,
>                       struct flow_wildcards *wc, bool use_masked)
>  {
>      struct ovs_key_nd key, mask, base;
> +    struct ovs_key_field_properties ovs_key_nd_field_properties[] = {
> +#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_nd, name),
> sizeof(type)},
> +        OVS_KEY_ND_FIELDS
> +        {0, 0}
> +#undef OVS_KEY_FIELD
> +    };
>
>      get_nd_key(flow, &key);
>      get_nd_key(base_flow, &base);
>      get_nd_key(&wc->masks, &mask);
>
>      if (commit(OVS_KEY_ATTR_ND, use_masked, &key, &base, &mask, sizeof
> key,
> -               odp_actions)) {
> +               ovs_key_nd_field_properties, odp_actions)) {
>          put_nd_key(&base, base_flow);
>          put_nd_key(&mask, &wc->masks);
>          return SLOW_ACTION;
> @@ -7672,6 +7748,12 @@ commit_set_port_action(const struct flow *flow,
> struct flow *base_flow,
>  {
>      enum ovs_key_attr key_type;
>      union ovs_key_tp key, mask, base;
> +    struct ovs_key_field_properties ovs_key_tp_field_properties[] = {
> +#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_tcp, name),
> sizeof(type)},
> +        OVS_KEY_TCP_FIELDS
> +        {0, 0}
> +#undef OVS_KEY_FIELD
> +    };
>
>      /* Check if 'flow' really has an L3 header. */
>      if (!flow->nw_proto) {
> @@ -7697,7 +7779,7 @@ commit_set_port_action(const struct flow *flow,
> struct flow *base_flow,
>      get_tp_key(&wc->masks, &mask);
>
>      if (commit(key_type, use_masked, &key, &base, &mask, sizeof key,
> -               odp_actions)) {
> +               ovs_key_tp_field_properties, odp_actions)) {
>          put_tp_key(&base, base_flow);
>          put_tp_key(&mask, &wc->masks);
>      }
> @@ -7710,13 +7792,17 @@ commit_set_priority_action(const struct flow
> *flow, struct flow *base_flow,
>                             bool use_masked)
>  {
>      uint32_t key, mask, base;
> +    struct ovs_key_field_properties ovs_key_prio_field_properties[] = {
> +        {0, sizeof(uint32_t)},
> +        {0, 0}
> +    };
>
>      key = flow->skb_priority;
>      base = base_flow->skb_priority;
>      mask = wc->masks.skb_priority;
>
>      if (commit(OVS_KEY_ATTR_PRIORITY, use_masked, &key, &base, &mask,
> -               sizeof key, odp_actions)) {
> +               sizeof key, ovs_key_prio_field_properties, odp_actions)) {
>          base_flow->skb_priority = base;
>          wc->masks.skb_priority = mask;
>      }
> @@ -7729,13 +7815,17 @@ commit_set_pkt_mark_action(const struct flow
> *flow, struct flow *base_flow,
>                             bool use_masked)
>  {
>      uint32_t key, mask, base;
> +    struct ovs_key_field_properties ovs_key_pkt_mark_field_properties[] =
> {
> +        {0, sizeof(uint32_t)},
> +        {0, 0}
> +    };
>
>      key = flow->pkt_mark;
>      base = base_flow->pkt_mark;
>      mask = wc->masks.pkt_mark;
>
>      if (commit(OVS_KEY_ATTR_SKB_MARK, use_masked, &key, &base, &mask,
> -               sizeof key, odp_actions)) {
> +               sizeof key, ovs_key_pkt_mark_field_properties,
> odp_actions)) {
>          base_flow->pkt_mark = base;
>          wc->masks.pkt_mark = mask;
>      }
> diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
> index 4392f7baa..a9ba62215 100644
> --- a/tests/mpls-xlate.at
> +++ b/tests/mpls-xlate.at
> @@ -182,7 +182,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1
> in_port=1,ip,actions=dec_ttl,push
>  dnl MPLS push+pop
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'],
> [0], [stdout])
>  AT_CHECK([tail -1 stdout], [0],
> -  [Datapath actions:
> set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(tos=0/0xfc,ttl=64)),1,1
> +  [Datapath actions:
> set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(ttl=64)),1,1
>  ])
>
>  OVS_VSWITCHD_STOP
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index ded2ef013..4c69d09be 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -4346,7 +4346,7 @@ do
>      elif test $type = later; then
>          echo "Datapath actions: $exp_output" >> expout
>      else
> -        echo "Datapath actions: set(tcp(src=80,dst=80)),$exp_output" >>
> expout
> +        echo "Datapath actions: set(tcp(src=80)),$exp_output" >> expout
>      fi
>      AT_CHECK([grep 'IP fragments' stdout; tail -1 stdout], [0], [expout])
>    done
> @@ -7252,7 +7252,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0],
> [ignore])
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
> [0], [stdout])
>
>  AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions:
> set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
> +Datapath actions:
> set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
>  ])
>
>  dnl Test flow xlate openflow clone action without using datapath clone
> action.
> @@ -7261,14 +7261,14 @@ AT_CHECK([ovs-appctl dpif/set-dp-features br0
> clone false], [0], [ignore])
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
> [0], [stdout])
>
>  AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions:
> set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
> +Datapath actions:
> set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
>  ])
>
>  AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0],
> [ignore])
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
> [0], [stdout])
>
>  AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions:
> set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
> +Datapath actions:
> set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
>  ])
>
>  dnl Mixing reversible and irreversible open flow clone actions. Datapath
> clone action
> @@ -7286,7 +7286,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0],
> [ignore])
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
> [0], [stdout])
>
>  AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions:
> set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),clone(ct(commit),3),4
> +Datapath actions:
> set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),clone(ct(commit),3),4
>  ])
>
>  AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
> @@ -7294,14 +7294,14 @@ AT_CHECK([ovs-appctl dpif/set-dp-features br0
> clone false], [0], [ignore])
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
> [0], [stdout])
>
>  AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions:
> set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),sample(sample=100.0%,actions(ct(commit),3)),4
> +Datapath actions:
> set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),sample(sample=100.0%,actions(ct(commit),3)),4
>  ])
>
>  AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0],
> [ignore])
>  AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'],
> [0], [stdout])
>
>  AT_CHECK([tail -1 stdout], [0], [dnl
> -Datapath actions:
> set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
> +Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),4
>  ])
>  AT_CHECK([grep "Failed to compose clone action" stdout], [0], [ignore])
>
> --
> 2.14.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Eli Britstein Jan. 11, 2019, 5:13 a.m. | #2
You are right. I'll fix it.

On 1/10/2019 9:35 PM, Yifeng Sun wrote:
Hi,

When I try to understand this patch, I feel there may be some issue in this loop below.
It looks like each loop is doing the same thing. Can you please take a look?

+        for (i = 0; i < size; i++)
+            if (memcmp(pkey0, pkey1, size) == 0)
+                memset(pmask, 0, size);
+            else
+                differ = true;

Thanks,
Yifeng

On Thu, Jan 10, 2019 at 12:52 AM Eli Britstein <elibr@mellanox.com<mailto:elibr@mellanox.com>> wrote:
To improve performance and avoid wasting resources for HW offloaded
flows, do not rewrite fields that are matched with the same value.

Signed-off-by: Eli Britstein <elibr@mellanox.com<mailto:elibr@mellanox.com>>
Reviewed-by: Roi Dayan <roid@mellanox.com<mailto:roid@mellanox.com>>
---
 lib/odp-util.c        | 110 +++++++++++++++++++++++++++++++++++++++++++++-----
 tests/mpls-xlate.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmpls-xlate.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423592048&sdata=z9SB16ZNpp2UA7LjYXZR02TED%2BsSY%2FAgCTSG5hh8b2U%3D&reserved=0>   |   2 +-
 tests/ofproto-dpif.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fofproto-dpif.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423592048&sdata=w%2FdeqIvO3ay%2FO6zi9L%2BPyRglmEI0krk4CtylHXyyVwk%3D&reserved=0> |  14 +++----
 3 files changed, 108 insertions(+), 18 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 0491bed38..7e916f9d9 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7086,12 +7086,50 @@ commit_odp_tunnel_action(const struct flow *flow, struct flow *base,
     }
 }

+struct ovs_key_field_properties {
+    int offset;
+    int size;
+};
+
+/* Compare the keys similary to memcmp, but each field separately.
+ * The offsets and sizes of each field is provided by field_properties
+ * argument.
+ * For fields with the same value, zero out their mask part in order not to
+ * rewrite them as it's unnecessary */
+static bool
+keycmp_mask(const void *key0, const void *key1,
+            struct ovs_key_field_properties *field_properties, void *mask)
+{
+    int field, i;
+    bool differ = false;
+
+    for (field = 0 ; ; field++) {
+        int size = field_properties[field].size;
+        int offset = field_properties[field].offset;
+        char *pkey0 = ((char *)key0) + offset;
+        char *pkey1 = ((char *)key1) + offset;
+        char *pmask = ((char *)mask) + offset;
+
+        if (size == 0)
+            break;
+
+        for (i = 0; i < size; i++)
+            if (memcmp(pkey0, pkey1, size) == 0)
+                memset(pmask, 0, size);
+            else
+                differ = true;
+    }
+
+    return differ;
+}
+
 static bool
 commit(enum ovs_key_attr attr, bool use_masked_set,
        const void *key, void *base, void *mask, size_t size,
+       struct ovs_key_field_properties *field_properties,
        struct ofpbuf *odp_actions)
 {
-    if (memcmp(key, base, size)) {
+    if (keycmp_mask(key, base, field_properties, mask)) {
         bool fully_masked = odp_mask_is_exact(attr, mask, size);

         if (use_masked_set && !fully_masked) {
@@ -7133,6 +7171,12 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow,
                         bool use_masked)
 {
     struct ovs_key_ethernet key, base, mask;
+    struct ovs_key_field_properties ovs_key_ethernet_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ethernet, name), sizeof(type)},
+        OVS_KEY_ETHERNET_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };

     if (flow->packet_type != htonl(PT_ETH)) {
         return;
@@ -7143,7 +7187,8 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow,
     get_ethernet_key(&wc->masks, &mask);

     if (commit(OVS_KEY_ATTR_ETHERNET, use_masked,
-               &key, &base, &mask, sizeof key, odp_actions)) {
+               &key, &base, &mask, sizeof key,
+               ovs_key_ethernet_field_properties, odp_actions)) {
         put_ethernet_key(&base, base_flow);
         put_ethernet_key(&mask, &wc->masks);
     }
@@ -7269,6 +7314,12 @@ commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow,
                        bool use_masked)
 {
     struct ovs_key_ipv4 key, mask, base;
+    struct ovs_key_field_properties ovs_key_ipv4_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ipv4, name), sizeof(type)},
+        OVS_KEY_IPV4_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };

     /* Check that nw_proto and nw_frag remain unchanged. */
     ovs_assert(flow->nw_proto == base_flow->nw_proto &&
@@ -7286,7 +7337,7 @@ commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow,
     }

     if (commit(OVS_KEY_ATTR_IPV4, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_ipv4_field_properties, odp_actions)) {
         put_ipv4_key(&base, base_flow, false);
         if (mask.ipv4_proto != 0) { /* Mask was changed by commit(). */
             put_ipv4_key(&mask, &wc->masks, true);
@@ -7324,6 +7375,12 @@ commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow,
                        bool use_masked)
 {
     struct ovs_key_ipv6 key, mask, base;
+    struct ovs_key_field_properties ovs_key_ipv6_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ipv6, name), sizeof(type)},
+        OVS_KEY_IPV6_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };

     /* Check that nw_proto and nw_frag remain unchanged. */
     ovs_assert(flow->nw_proto == base_flow->nw_proto &&
@@ -7342,7 +7399,7 @@ commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow,
     }

     if (commit(OVS_KEY_ATTR_IPV6, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_ipv6_field_properties, odp_actions)) {
         put_ipv6_key(&base, base_flow, false);
         if (mask.ipv6_proto != 0) { /* Mask was changed by commit(). */
             put_ipv6_key(&mask, &wc->masks, true);
@@ -7378,13 +7435,19 @@ commit_set_arp_action(const struct flow *flow, struct flow *base_flow,
                       struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
     struct ovs_key_arp key, mask, base;
+    struct ovs_key_field_properties ovs_key_arp_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_arp, name), sizeof(type)},
+        OVS_KEY_ARP_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };

     get_arp_key(flow, &key);
     get_arp_key(base_flow, &base);
     get_arp_key(&wc->masks, &mask);

     if (commit(OVS_KEY_ATTR_ARP, true, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_arp_field_properties, odp_actions)) {
         put_arp_key(&base, base_flow);
         put_arp_key(&mask, &wc->masks);
         return SLOW_ACTION;
@@ -7413,6 +7476,12 @@ commit_set_icmp_action(const struct flow *flow, struct flow *base_flow,
                        struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
     struct ovs_key_icmp key, mask, base;
+    struct ovs_key_field_properties ovs_key_icmp_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_icmp, name), sizeof(type)},
+        OVS_KEY_ICMP_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
     enum ovs_key_attr attr;

     if (is_icmpv4(flow, NULL)) {
@@ -7427,7 +7496,8 @@ commit_set_icmp_action(const struct flow *flow, struct flow *base_flow,
     get_icmp_key(base_flow, &base);
     get_icmp_key(&wc->masks, &mask);

-    if (commit(attr, false, &key, &base, &mask, sizeof key, odp_actions)) {
+    if (commit(attr, false, &key, &base, &mask, sizeof key,
+               ovs_key_icmp_field_properties, odp_actions)) {
         put_icmp_key(&base, base_flow);
         put_icmp_key(&mask, &wc->masks);
         return SLOW_ACTION;
@@ -7459,13 +7529,19 @@ commit_set_nd_action(const struct flow *flow, struct flow *base_flow,
                      struct flow_wildcards *wc, bool use_masked)
 {
     struct ovs_key_nd key, mask, base;
+    struct ovs_key_field_properties ovs_key_nd_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_nd, name), sizeof(type)},
+        OVS_KEY_ND_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };

     get_nd_key(flow, &key);
     get_nd_key(base_flow, &base);
     get_nd_key(&wc->masks, &mask);

     if (commit(OVS_KEY_ATTR_ND, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_nd_field_properties, odp_actions)) {
         put_nd_key(&base, base_flow);
         put_nd_key(&mask, &wc->masks);
         return SLOW_ACTION;
@@ -7672,6 +7748,12 @@ commit_set_port_action(const struct flow *flow, struct flow *base_flow,
 {
     enum ovs_key_attr key_type;
     union ovs_key_tp key, mask, base;
+    struct ovs_key_field_properties ovs_key_tp_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_tcp, name), sizeof(type)},
+        OVS_KEY_TCP_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };

     /* Check if 'flow' really has an L3 header. */
     if (!flow->nw_proto) {
@@ -7697,7 +7779,7 @@ commit_set_port_action(const struct flow *flow, struct flow *base_flow,
     get_tp_key(&wc->masks, &mask);

     if (commit(key_type, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_tp_field_properties, odp_actions)) {
         put_tp_key(&base, base_flow);
         put_tp_key(&mask, &wc->masks);
     }
@@ -7710,13 +7792,17 @@ commit_set_priority_action(const struct flow *flow, struct flow *base_flow,
                            bool use_masked)
 {
     uint32_t key, mask, base;
+    struct ovs_key_field_properties ovs_key_prio_field_properties[] = {
+        {0, sizeof(uint32_t)},
+        {0, 0}
+    };

     key = flow->skb_priority;
     base = base_flow->skb_priority;
     mask = wc->masks.skb_priority;

     if (commit(OVS_KEY_ATTR_PRIORITY, use_masked, &key, &base, &mask,
-               sizeof key, odp_actions)) {
+               sizeof key, ovs_key_prio_field_properties, odp_actions)) {
         base_flow->skb_priority = base;
         wc->masks.skb_priority = mask;
     }
@@ -7729,13 +7815,17 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base_flow,
                            bool use_masked)
 {
     uint32_t key, mask, base;
+    struct ovs_key_field_properties ovs_key_pkt_mark_field_properties[] = {
+        {0, sizeof(uint32_t)},
+        {0, 0}
+    };

     key = flow->pkt_mark;
     base = base_flow->pkt_mark;
     mask = wc->masks.pkt_mark;

     if (commit(OVS_KEY_ATTR_SKB_MARK, use_masked, &key, &base, &mask,
-               sizeof key, odp_actions)) {
+               sizeof key, ovs_key_pkt_mark_field_properties, odp_actions)) {
         base_flow->pkt_mark = base;
         wc->masks.pkt_mark = mask;
     }
diff --git a/tests/mpls-xlate.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmpls-xlate.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423602053&sdata=A1DvxGyhSteeFmTk0xJSc0z%2BZYnzNjgZqol6PQrMt2w%3D&reserved=0> b/tests/mpls-xlate.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmpls-xlate.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423602053&sdata=A1DvxGyhSteeFmTk0xJSc0z%2BZYnzNjgZqol6PQrMt2w%3D&reserved=0>
index 4392f7baa..a9ba62215 100644
--- a/tests/mpls-xlate.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmpls-xlate.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423612058&sdata=W8DxhNxtNi1XX7X1TLHiJVR%2BEHQ9JEorLgOnV3Ab4ys%3D&reserved=0>
+++ b/tests/mpls-xlate.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmpls-xlate.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423612058&sdata=W8DxhNxtNi1XX7X1TLHiJVR%2BEHQ9JEorLgOnV3Ab4ys%3D&reserved=0>
@@ -182,7 +182,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 in_port=1,ip,actions=dec_ttl,push
 dnl MPLS push+pop
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'], [0], [stdout])
 AT_CHECK([tail -1 stdout], [0],
-  [Datapath actions: set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(tos=0/0xfc,ttl=64)),1,1
+  [Datapath actions: set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(ttl=64)),1,1
 ])

 OVS_VSWITCHD_STOP
diff --git a/tests/ofproto-dpif.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fofproto-dpif.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423622067&sdata=Td5t%2FHhAdtoXMH6jBhzOwPOM813xUwj5%2FiZ%2B%2Fcu7fnc%3D&reserved=0> b/tests/ofproto-dpif.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fofproto-dpif.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423622067&sdata=Td5t%2FHhAdtoXMH6jBhzOwPOM813xUwj5%2FiZ%2B%2Fcu7fnc%3D&reserved=0>
index ded2ef013..4c69d09be 100644
--- a/tests/ofproto-dpif.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fofproto-dpif.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423632072&sdata=HetybxbX%2B0JlmZ%2B4%2B6wUKlJvmutI6kCpVWrffnh0ZhY%3D&reserved=0>
+++ b/tests/ofproto-dpif.at<https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fofproto-dpif.at&data=02%7C01%7Celibr%40mellanox.com%7C8adf0dc78b1e4ac8de3608d67732cd09%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636827457423632072&sdata=HetybxbX%2B0JlmZ%2B4%2B6wUKlJvmutI6kCpVWrffnh0ZhY%3D&reserved=0>
@@ -4346,7 +4346,7 @@ do
     elif test $type = later; then
         echo "Datapath actions: $exp_output" >> expout
     else
-        echo "Datapath actions: set(tcp(src=80,dst=80)),$exp_output" >> expout
+        echo "Datapath actions: set(tcp(src=80)),$exp_output" >> expout
     fi
     AT_CHECK([grep 'IP fragments' stdout; tail -1 stdout], [0], [expout])
   done
@@ -7252,7 +7252,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])

 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
 ])

 dnl Test flow xlate openflow clone action without using datapath clone action.
@@ -7261,14 +7261,14 @@ AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])

 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
 ])

 AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])

 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
 ])

 dnl Mixing reversible and irreversible open flow clone actions. Datapath clone action
@@ -7286,7 +7286,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])

 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),clone(ct(commit),3),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),clone(ct(commit),3),4
 ])

 AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
@@ -7294,14 +7294,14 @@ AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])

 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),sample(sample=100.0%,actions(ct(commit),3)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),sample(sample=100.0%,actions(ct(commit),3)),4
 ])

 AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])

 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),4
 ])
 AT_CHECK([grep "Failed to compose clone action" stdout], [0], [ignore])

--
2.14.5

Patch

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 0491bed38..7e916f9d9 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7086,12 +7086,50 @@  commit_odp_tunnel_action(const struct flow *flow, struct flow *base,
     }
 }
 
+struct ovs_key_field_properties {
+    int offset;
+    int size;
+};
+
+/* Compare the keys similary to memcmp, but each field separately.
+ * The offsets and sizes of each field is provided by field_properties
+ * argument.
+ * For fields with the same value, zero out their mask part in order not to
+ * rewrite them as it's unnecessary */
+static bool
+keycmp_mask(const void *key0, const void *key1,
+            struct ovs_key_field_properties *field_properties, void *mask)
+{
+    int field, i;
+    bool differ = false;
+
+    for (field = 0 ; ; field++) {
+        int size = field_properties[field].size;
+        int offset = field_properties[field].offset;
+        char *pkey0 = ((char *)key0) + offset;
+        char *pkey1 = ((char *)key1) + offset;
+        char *pmask = ((char *)mask) + offset;
+
+        if (size == 0)
+            break;
+
+        for (i = 0; i < size; i++)
+            if (memcmp(pkey0, pkey1, size) == 0)
+                memset(pmask, 0, size);
+            else
+                differ = true;
+    }
+
+    return differ;
+}
+
 static bool
 commit(enum ovs_key_attr attr, bool use_masked_set,
        const void *key, void *base, void *mask, size_t size,
+       struct ovs_key_field_properties *field_properties,
        struct ofpbuf *odp_actions)
 {
-    if (memcmp(key, base, size)) {
+    if (keycmp_mask(key, base, field_properties, mask)) {
         bool fully_masked = odp_mask_is_exact(attr, mask, size);
 
         if (use_masked_set && !fully_masked) {
@@ -7133,6 +7171,12 @@  commit_set_ether_action(const struct flow *flow, struct flow *base_flow,
                         bool use_masked)
 {
     struct ovs_key_ethernet key, base, mask;
+    struct ovs_key_field_properties ovs_key_ethernet_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ethernet, name), sizeof(type)},
+        OVS_KEY_ETHERNET_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
 
     if (flow->packet_type != htonl(PT_ETH)) {
         return;
@@ -7143,7 +7187,8 @@  commit_set_ether_action(const struct flow *flow, struct flow *base_flow,
     get_ethernet_key(&wc->masks, &mask);
 
     if (commit(OVS_KEY_ATTR_ETHERNET, use_masked,
-               &key, &base, &mask, sizeof key, odp_actions)) {
+               &key, &base, &mask, sizeof key,
+               ovs_key_ethernet_field_properties, odp_actions)) {
         put_ethernet_key(&base, base_flow);
         put_ethernet_key(&mask, &wc->masks);
     }
@@ -7269,6 +7314,12 @@  commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow,
                        bool use_masked)
 {
     struct ovs_key_ipv4 key, mask, base;
+    struct ovs_key_field_properties ovs_key_ipv4_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ipv4, name), sizeof(type)},
+        OVS_KEY_IPV4_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
 
     /* Check that nw_proto and nw_frag remain unchanged. */
     ovs_assert(flow->nw_proto == base_flow->nw_proto &&
@@ -7286,7 +7337,7 @@  commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow,
     }
 
     if (commit(OVS_KEY_ATTR_IPV4, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_ipv4_field_properties, odp_actions)) {
         put_ipv4_key(&base, base_flow, false);
         if (mask.ipv4_proto != 0) { /* Mask was changed by commit(). */
             put_ipv4_key(&mask, &wc->masks, true);
@@ -7324,6 +7375,12 @@  commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow,
                        bool use_masked)
 {
     struct ovs_key_ipv6 key, mask, base;
+    struct ovs_key_field_properties ovs_key_ipv6_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_ipv6, name), sizeof(type)},
+        OVS_KEY_IPV6_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
 
     /* Check that nw_proto and nw_frag remain unchanged. */
     ovs_assert(flow->nw_proto == base_flow->nw_proto &&
@@ -7342,7 +7399,7 @@  commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow,
     }
 
     if (commit(OVS_KEY_ATTR_IPV6, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_ipv6_field_properties, odp_actions)) {
         put_ipv6_key(&base, base_flow, false);
         if (mask.ipv6_proto != 0) { /* Mask was changed by commit(). */
             put_ipv6_key(&mask, &wc->masks, true);
@@ -7378,13 +7435,19 @@  commit_set_arp_action(const struct flow *flow, struct flow *base_flow,
                       struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
     struct ovs_key_arp key, mask, base;
+    struct ovs_key_field_properties ovs_key_arp_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_arp, name), sizeof(type)},
+        OVS_KEY_ARP_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
 
     get_arp_key(flow, &key);
     get_arp_key(base_flow, &base);
     get_arp_key(&wc->masks, &mask);
 
     if (commit(OVS_KEY_ATTR_ARP, true, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_arp_field_properties, odp_actions)) {
         put_arp_key(&base, base_flow);
         put_arp_key(&mask, &wc->masks);
         return SLOW_ACTION;
@@ -7413,6 +7476,12 @@  commit_set_icmp_action(const struct flow *flow, struct flow *base_flow,
                        struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
     struct ovs_key_icmp key, mask, base;
+    struct ovs_key_field_properties ovs_key_icmp_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_icmp, name), sizeof(type)},
+        OVS_KEY_ICMP_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
     enum ovs_key_attr attr;
 
     if (is_icmpv4(flow, NULL)) {
@@ -7427,7 +7496,8 @@  commit_set_icmp_action(const struct flow *flow, struct flow *base_flow,
     get_icmp_key(base_flow, &base);
     get_icmp_key(&wc->masks, &mask);
 
-    if (commit(attr, false, &key, &base, &mask, sizeof key, odp_actions)) {
+    if (commit(attr, false, &key, &base, &mask, sizeof key,
+               ovs_key_icmp_field_properties, odp_actions)) {
         put_icmp_key(&base, base_flow);
         put_icmp_key(&mask, &wc->masks);
         return SLOW_ACTION;
@@ -7459,13 +7529,19 @@  commit_set_nd_action(const struct flow *flow, struct flow *base_flow,
                      struct flow_wildcards *wc, bool use_masked)
 {
     struct ovs_key_nd key, mask, base;
+    struct ovs_key_field_properties ovs_key_nd_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_nd, name), sizeof(type)},
+        OVS_KEY_ND_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
 
     get_nd_key(flow, &key);
     get_nd_key(base_flow, &base);
     get_nd_key(&wc->masks, &mask);
 
     if (commit(OVS_KEY_ATTR_ND, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_nd_field_properties, odp_actions)) {
         put_nd_key(&base, base_flow);
         put_nd_key(&mask, &wc->masks);
         return SLOW_ACTION;
@@ -7672,6 +7748,12 @@  commit_set_port_action(const struct flow *flow, struct flow *base_flow,
 {
     enum ovs_key_attr key_type;
     union ovs_key_tp key, mask, base;
+    struct ovs_key_field_properties ovs_key_tp_field_properties[] = {
+#define OVS_KEY_FIELD(type, name) {offsetof(struct ovs_key_tcp, name), sizeof(type)},
+        OVS_KEY_TCP_FIELDS
+        {0, 0}
+#undef OVS_KEY_FIELD
+    };
 
     /* Check if 'flow' really has an L3 header. */
     if (!flow->nw_proto) {
@@ -7697,7 +7779,7 @@  commit_set_port_action(const struct flow *flow, struct flow *base_flow,
     get_tp_key(&wc->masks, &mask);
 
     if (commit(key_type, use_masked, &key, &base, &mask, sizeof key,
-               odp_actions)) {
+               ovs_key_tp_field_properties, odp_actions)) {
         put_tp_key(&base, base_flow);
         put_tp_key(&mask, &wc->masks);
     }
@@ -7710,13 +7792,17 @@  commit_set_priority_action(const struct flow *flow, struct flow *base_flow,
                            bool use_masked)
 {
     uint32_t key, mask, base;
+    struct ovs_key_field_properties ovs_key_prio_field_properties[] = {
+        {0, sizeof(uint32_t)},
+        {0, 0}
+    };
 
     key = flow->skb_priority;
     base = base_flow->skb_priority;
     mask = wc->masks.skb_priority;
 
     if (commit(OVS_KEY_ATTR_PRIORITY, use_masked, &key, &base, &mask,
-               sizeof key, odp_actions)) {
+               sizeof key, ovs_key_prio_field_properties, odp_actions)) {
         base_flow->skb_priority = base;
         wc->masks.skb_priority = mask;
     }
@@ -7729,13 +7815,17 @@  commit_set_pkt_mark_action(const struct flow *flow, struct flow *base_flow,
                            bool use_masked)
 {
     uint32_t key, mask, base;
+    struct ovs_key_field_properties ovs_key_pkt_mark_field_properties[] = {
+        {0, sizeof(uint32_t)},
+        {0, 0}
+    };
 
     key = flow->pkt_mark;
     base = base_flow->pkt_mark;
     mask = wc->masks.pkt_mark;
 
     if (commit(OVS_KEY_ATTR_SKB_MARK, use_masked, &key, &base, &mask,
-               sizeof key, odp_actions)) {
+               sizeof key, ovs_key_pkt_mark_field_properties, odp_actions)) {
         base_flow->pkt_mark = base;
         wc->masks.pkt_mark = mask;
     }
diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
index 4392f7baa..a9ba62215 100644
--- a/tests/mpls-xlate.at
+++ b/tests/mpls-xlate.at
@@ -182,7 +182,7 @@  AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 in_port=1,ip,actions=dec_ttl,push
 dnl MPLS push+pop
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'], [0], [stdout])
 AT_CHECK([tail -1 stdout], [0],
-  [Datapath actions: set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(tos=0/0xfc,ttl=64)),1,1
+  [Datapath actions: set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(ttl=64)),1,1
 ])
 
 OVS_VSWITCHD_STOP
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index ded2ef013..4c69d09be 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -4346,7 +4346,7 @@  do
     elif test $type = later; then
         echo "Datapath actions: $exp_output" >> expout
     else
-        echo "Datapath actions: set(tcp(src=80,dst=80)),$exp_output" >> expout
+        echo "Datapath actions: set(tcp(src=80)),$exp_output" >> expout
     fi
     AT_CHECK([grep 'IP fragments' stdout; tail -1 stdout], [0], [expout])
   done
@@ -7252,7 +7252,7 @@  AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 
 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
 ])
 
 dnl Test flow xlate openflow clone action without using datapath clone action.
@@ -7261,14 +7261,14 @@  AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 
 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
 ])
 
 AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 
 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(src=10.10.10.2,dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(eth(src=80:81:81:81:81:81)),set(ipv4(dst=192.168.5.5)),3,set(eth(src=50:54:00:00:00:09)),set(ipv4(dst=10.10.10.1)),4
 ])
 
 dnl Mixing reversible and irreversible open flow clone actions. Datapath clone action
@@ -7286,7 +7286,7 @@  AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 
 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),clone(ct(commit),3),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),clone(ct(commit),3),4
 ])
 
 AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
@@ -7294,14 +7294,14 @@  AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone false], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 
 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),sample(sample=100.0%,actions(ct(commit),3)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),sample(sample=100.0%,actions(ct(commit),3)),4
 ])
 
 AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 2], [0], [ignore])
 AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
 
 AT_CHECK([tail -1 stdout], [0], [dnl
-Datapath actions: set(ipv4(src=10.10.10.2,dst=192.168.4.4)),2,set(ipv4(src=10.10.10.2,dst=10.10.10.1)),4
+Datapath actions: set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),4
 ])
 AT_CHECK([grep "Failed to compose clone action" stdout], [0], [ignore])