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

2019-03-25 Thread Ben Pfaff
On Thu, Mar 21, 2019 at 07:44:16AM +, Eli Britstein 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 
> Reviewed-by: Roi Dayan 

Thanks.  I applied this series to master.

I folded in an improvement to the comment on keycmp_mask() and minor
style fixes to the same function:

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 291c05f84a48..b6552c5c208a 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7367,29 +7367,27 @@ struct offsetof_sizeof {
 int size;
 };
 
-/* Compare the keys similary to memcmp, but each field separately.
- * The offsets and sizes of each field is provided by offsetof_sizeof_arr
- * argument.
- * For fields with the same value, zero out their mask part in order not to
- * rewrite them as it's unnecessary */
+/* Compares each of the fields in 'key0' and 'key1'.  The fields are specified
+ * in 'offsetof_sizeof_arr', which is an array terminated by a 0-size field.
+ * Returns true if all of the fields are equal, false if at least one differs.
+ * As a side effect, for each field that is the same in 'key0' and 'key1',
+ * zeros the corresponding bytes in 'mask'. */
 static bool
 keycmp_mask(const void *key0, const void *key1,
 struct offsetof_sizeof *offsetof_sizeof_arr, void *mask)
 {
-int field;
 bool differ = false;
 
-for (field = 0 ; ; field++) {
+for (int field = 0 ; ; field++) {
 int size = offsetof_sizeof_arr[field].size;
 int offset = offsetof_sizeof_arr[field].offset;
-char *pkey0 = ((char *)key0) + offset;
-char *pkey1 = ((char *)key1) + offset;
-char *pmask = ((char *)mask) + offset;
-
 if (size == 0) {
 break;
 }
 
+char *pkey0 = ((char *)key0) + offset;
+char *pkey1 = ((char *)key1) + offset;
+char *pmask = ((char *)mask) + offset;
 if (memcmp(pkey0, pkey1, size) == 0) {
 memset(pmask, 0, size);
 } else {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


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

2019-03-21 Thread Eli Britstein
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 
Reviewed-by: Roi Dayan 
---
 lib/odp-util.c| 93 +--
 tests/mpls-xlate.at   |  2 +-
 tests/ofproto-dpif.at | 14 +++
 3 files changed, 88 insertions(+), 21 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 0716161dd..291c05f84 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -43,6 +43,7 @@
 #include "uuid.h"
 #include "openvswitch/vlog.h"
 #include "openvswitch/match.h"
+#include "odp-netlink-macros.h"
 
 VLOG_DEFINE_THIS_MODULE(odp_util);
 
@@ -7361,12 +7362,51 @@ commit_odp_tunnel_action(const struct flow *flow, 
struct flow *base,
 }
 }
 
+struct offsetof_sizeof {
+int offset;
+int size;
+};
+
+/* Compare the keys similary to memcmp, but each field separately.
+ * The offsets and sizes of each field is provided by offsetof_sizeof_arr
+ * 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 offsetof_sizeof *offsetof_sizeof_arr, void *mask)
+{
+int field;
+bool differ = false;
+
+for (field = 0 ; ; field++) {
+int size = offsetof_sizeof_arr[field].size;
+int offset = offsetof_sizeof_arr[field].offset;
+char *pkey0 = ((char *)key0) + offset;
+char *pkey1 = ((char *)key1) + offset;
+char *pmask = ((char *)mask) + offset;
+
+if (size == 0) {
+break;
+}
+
+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 offsetof_sizeof *offsetof_sizeof_arr,
struct ofpbuf *odp_actions)
 {
-if (memcmp(key, base, size)) {
+if (keycmp_mask(key, base, offsetof_sizeof_arr, mask)) {
 bool fully_masked = odp_mask_is_exact(attr, mask, size);
 
 if (use_masked_set && !fully_masked) {
@@ -7408,7 +7448,8 @@ commit_set_ether_action(const struct flow *flow, struct 
flow *base_flow,
 bool use_masked)
 {
 struct ovs_key_ethernet key, base, mask;
-
+struct offsetof_sizeof ovs_key_ethernet_offsetof_sizeof_arr[] =
+OVS_KEY_ETHERNET_OFFSETOF_SIZEOF_ARR;
 if (flow->packet_type != htonl(PT_ETH)) {
 return;
 }
@@ -7418,7 +7459,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_offsetof_sizeof_arr, odp_actions)) {
 put_ethernet_key(&base, base_flow);
 put_ethernet_key(&mask, &wc->masks);
 }
@@ -7544,6 +7586,8 @@ commit_set_ipv4_action(const struct flow *flow, struct 
flow *base_flow,
bool use_masked)
 {
 struct ovs_key_ipv4 key, mask, base;
+struct offsetof_sizeof ovs_key_ipv4_offsetof_sizeof_arr[] =
+OVS_KEY_IPV4_OFFSETOF_SIZEOF_ARR;
 
 /* Check that nw_proto and nw_frag remain unchanged. */
 ovs_assert(flow->nw_proto == base_flow->nw_proto &&
@@ -7561,7 +7605,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_offsetof_sizeof_arr, 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);
@@ -7599,6 +7643,8 @@ commit_set_ipv6_action(const struct flow *flow, struct 
flow *base_flow,
bool use_masked)
 {
 struct ovs_key_ipv6 key, mask, base;
+struct offsetof_sizeof ovs_key_ipv6_offsetof_sizeof_arr[] =
+OVS_KEY_IPV6_OFFSETOF_SIZEOF_ARR;
 
 /* Check that nw_proto and nw_frag remain unchanged. */
 ovs_assert(flow->nw_proto == base_flow->nw_proto &&
@@ -7617,7 +7663,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_offsetof_sizeof_arr, 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);
@@ -7653,13 +7699,15 @@ commit_set_arp_action(const struct flow *flow, struct 
flow *base_flow,
   struct ofpbuf *odp_actions, struct flow_wild