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

2019-01-10 Thread Eli Britstein 
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 
mailto:el...@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 mailto:el...@mellanox.com>>
Reviewed-by: Roi Dayan mailto:r...@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))

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

2019-01-10 Thread Yifeng Sun 
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  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 
> ---
>  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_properti

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

2019-01-10 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| 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) { /