subject:"\[ovs\-dev\] \[PATCH\] Revert \"pvector\: Expose non\-concurrent priority vector.\""

Re: [ovs-dev] [PATCH] Revert "pvector: Expose non-concurrent priority vector."

2016-08-10 Thread Jarno Rajahalme

Thanks for the review, pushed to master,

  Jarno

> On Aug 9, 2016, at 2:43 PM, Daniele Di Proietto  wrote:
> 
> Simple revert, looks good to me, thanks
> 
> Acked-by: Daniele Di Proietto  >
> 
> 
> 2016-08-09 13:59 GMT-07:00 Jarno Rajahalme  >:
> This reverts commit 8bdfe1313894047d44349fa4cf4402970865950f.
> 
> I failed to see that lib/dpif-netdev.c actually needs the concurrency
> provided by pvector prior to this change.  More specifically, when a
> subtable is removed, concurrent lookups may skip over another subtable
> swapped in to the place of the removed subtable in the vector.
> 
> Since this was the only use of the non-concurrent pvector, it is
> cleaner to revert the whole patch.
> 
> Reported-by: Jan Scheurich  >
> Signed-off-by: Jarno Rajahalme >
> ---
>  lib/classifier.c|  30 
>  lib/classifier.h|   6 +-
>  lib/dpif-netdev.c   |  14 ++--
>  lib/pvector.c   | 190 
> +++-
>  lib/pvector.h   | 187 +++
>  tests/test-classifier.c |  12 +--
>  6 files changed, 182 insertions(+), 257 deletions(-)
> 
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 8f195d5..0551146 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -325,7 +325,7 @@ classifier_init(struct classifier *cls, const uint8_t 
> *flow_segments)
>  {
>  cls->n_rules = 0;
>  cmap_init(>subtables_map);
> -cpvector_init(>subtables);
> +pvector_init(>subtables);
>  cls->n_flow_segments = 0;
>  if (flow_segments) {
>  while (cls->n_flow_segments < CLS_MAX_INDICES
> @@ -359,7 +359,7 @@ classifier_destroy(struct classifier *cls)
>  }
>  cmap_destroy(>subtables_map);
> 
> -cpvector_destroy(>subtables);
> +pvector_destroy(>subtables);
>  }
>  }
> 
> @@ -658,20 +658,20 @@ classifier_replace(struct classifier *cls, const struct 
> cls_rule *rule,
>  if (n_rules == 1) {
>  subtable->max_priority = rule->priority;
>  subtable->max_count = 1;
> -cpvector_insert(>subtables, subtable, rule->priority);
> +pvector_insert(>subtables, subtable, rule->priority);
>  } else if (rule->priority == subtable->max_priority) {
>  ++subtable->max_count;
>  } else if (rule->priority > subtable->max_priority) {
>  subtable->max_priority = rule->priority;
>  subtable->max_count = 1;
> -cpvector_change_priority(>subtables, subtable, rule->priority);
> +pvector_change_priority(>subtables, subtable, rule->priority);
>  }
> 
>  /* Nothing was replaced. */
>  cls->n_rules++;
> 
>  if (cls->publish) {
> -cpvector_publish(>subtables);
> +pvector_publish(>subtables);
>  }
> 
>  return NULL;
> @@ -803,12 +803,12 @@ check_priority:
>  }
>  }
>  subtable->max_priority = max_priority;
> -cpvector_change_priority(>subtables, subtable, 
> max_priority);
> +pvector_change_priority(>subtables, subtable, max_priority);
>  }
>  }
> 
>  if (cls->publish) {
> -cpvector_publish(>subtables);
> +pvector_publish(>subtables);
>  }
> 
>  /* free the rule. */
> @@ -959,8 +959,8 @@ classifier_lookup__(const struct classifier *cls, 
> ovs_version_t version,
> 
>  /* Main loop. */
>  struct cls_subtable *subtable;
> -CPVECTOR_FOR_EACH_PRIORITY (subtable, hard_pri + 1, 2, sizeof *subtable,
> ->subtables) {
> +PVECTOR_FOR_EACH_PRIORITY (subtable, hard_pri + 1, 2, sizeof *subtable,
> +   >subtables) {
>  struct cls_conjunction_set *conj_set;
> 
>  /* Skip subtables with no match, or where the match is lower-priority
> @@ -1231,8 +1231,8 @@ classifier_rule_overlaps(const struct classifier *cls,
>  struct cls_subtable *subtable;
> 
>  /* Iterate subtables in the descending max priority order. */
> -CPVECTOR_FOR_EACH_PRIORITY (subtable, target->priority, 2,
> -sizeof(struct cls_subtable), 
> >subtables) {
> +PVECTOR_FOR_EACH_PRIORITY (subtable, target->priority, 2,
> +   sizeof(struct cls_subtable), >subtables) 
> {
>  struct {
>  struct minimask mask;
>  uint64_t storage[FLOW_U64S];
> @@ -1350,8 +1350,8 @@ cls_cursor_start(const struct classifier *cls, const 
> struct cls_rule *target,
>  cursor.rule = NULL;
> 
>  /* Find first rule. */
> -CPVECTOR_CURSOR_FOR_EACH (subtable, ,
> -  >subtables) {
> +PVECTOR_CURSOR_FOR_EACH (subtable, ,
> + >subtables) {
>  const struct cls_rule

Re: [ovs-dev] [PATCH] Revert "pvector: Expose non-concurrent priority vector."

2016-08-09 Thread Daniele Di Proietto

Simple revert, looks good to me, thanks

Acked-by: Daniele Di Proietto 


2016-08-09 13:59 GMT-07:00 Jarno Rajahalme :

> This reverts commit 8bdfe1313894047d44349fa4cf4402970865950f.
>
> I failed to see that lib/dpif-netdev.c actually needs the concurrency
> provided by pvector prior to this change.  More specifically, when a
> subtable is removed, concurrent lookups may skip over another subtable
> swapped in to the place of the removed subtable in the vector.
>
> Since this was the only use of the non-concurrent pvector, it is
> cleaner to revert the whole patch.
>
> Reported-by: Jan Scheurich 
> Signed-off-by: Jarno Rajahalme 
> ---
>  lib/classifier.c|  30 
>  lib/classifier.h|   6 +-
>  lib/dpif-netdev.c   |  14 ++--
>  lib/pvector.c   | 190 +++---
> --
>  lib/pvector.h   | 187 +++---
> -
>  tests/test-classifier.c |  12 +--
>  6 files changed, 182 insertions(+), 257 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 8f195d5..0551146 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -325,7 +325,7 @@ classifier_init(struct classifier *cls, const uint8_t
> *flow_segments)
>  {
>  cls->n_rules = 0;
>  cmap_init(>subtables_map);
> -cpvector_init(>subtables);
> +pvector_init(>subtables);
>  cls->n_flow_segments = 0;
>  if (flow_segments) {
>  while (cls->n_flow_segments < CLS_MAX_INDICES
> @@ -359,7 +359,7 @@ classifier_destroy(struct classifier *cls)
>  }
>  cmap_destroy(>subtables_map);
>
> -cpvector_destroy(>subtables);
> +pvector_destroy(>subtables);
>  }
>  }
>
> @@ -658,20 +658,20 @@ classifier_replace(struct classifier *cls, const
> struct cls_rule *rule,
>  if (n_rules == 1) {
>  subtable->max_priority = rule->priority;
>  subtable->max_count = 1;
> -cpvector_insert(>subtables, subtable, rule->priority);
> +pvector_insert(>subtables, subtable, rule->priority);
>  } else if (rule->priority == subtable->max_priority) {
>  ++subtable->max_count;
>  } else if (rule->priority > subtable->max_priority) {
>  subtable->max_priority = rule->priority;
>  subtable->max_count = 1;
> -cpvector_change_priority(>subtables, subtable,
> rule->priority);
> +pvector_change_priority(>subtables, subtable,
> rule->priority);
>  }
>
>  /* Nothing was replaced. */
>  cls->n_rules++;
>
>  if (cls->publish) {
> -cpvector_publish(>subtables);
> +pvector_publish(>subtables);
>  }
>
>  return NULL;
> @@ -803,12 +803,12 @@ check_priority:
>  }
>  }
>  subtable->max_priority = max_priority;
> -cpvector_change_priority(>subtables, subtable,
> max_priority);
> +pvector_change_priority(>subtables, subtable,
> max_priority);
>  }
>  }
>
>  if (cls->publish) {
> -cpvector_publish(>subtables);
> +pvector_publish(>subtables);
>  }
>
>  /* free the rule. */
> @@ -959,8 +959,8 @@ classifier_lookup__(const struct classifier *cls,
> ovs_version_t version,
>
>  /* Main loop. */
>  struct cls_subtable *subtable;
> -CPVECTOR_FOR_EACH_PRIORITY (subtable, hard_pri + 1, 2, sizeof
> *subtable,
> ->subtables) {
> +PVECTOR_FOR_EACH_PRIORITY (subtable, hard_pri + 1, 2, sizeof
> *subtable,
> +   >subtables) {
>  struct cls_conjunction_set *conj_set;
>
>  /* Skip subtables with no match, or where the match is
> lower-priority
> @@ -1231,8 +1231,8 @@ classifier_rule_overlaps(const struct classifier
> *cls,
>  struct cls_subtable *subtable;
>
>  /* Iterate subtables in the descending max priority order. */
> -CPVECTOR_FOR_EACH_PRIORITY (subtable, target->priority, 2,
> -sizeof(struct cls_subtable),
> >subtables) {
> +PVECTOR_FOR_EACH_PRIORITY (subtable, target->priority, 2,
> +   sizeof(struct cls_subtable),
> >subtables) {
>  struct {
>  struct minimask mask;
>  uint64_t storage[FLOW_U64S];
> @@ -1350,8 +1350,8 @@ cls_cursor_start(const struct classifier *cls, const
> struct cls_rule *target,
>  cursor.rule = NULL;
>
>  /* Find first rule. */
> -CPVECTOR_CURSOR_FOR_EACH (subtable, ,
> -  >subtables) {
> +PVECTOR_CURSOR_FOR_EACH (subtable, ,
> + >subtables) {
>  const struct cls_rule *rule = search_subtable(subtable, );
>
>  if (rule) {
> @@ -1378,7 +1378,7 @@ cls_cursor_next(struct cls_cursor *cursor)
>  }
>  }
>
> -CPVECTOR_CURSOR_FOR_EACH_CONTINUE (subtable, >subtables) {
> +PVECTOR_CURSOR_FOR_EACH_CONTINUE (subtable,

[ovs-dev] [PATCH] Revert "pvector: Expose non-concurrent priority vector."

2016-08-09 Thread Jarno Rajahalme

This reverts commit 8bdfe1313894047d44349fa4cf4402970865950f.

I failed to see that lib/dpif-netdev.c actually needs the concurrency
provided by pvector prior to this change.  More specifically, when a
subtable is removed, concurrent lookups may skip over another subtable
swapped in to the place of the removed subtable in the vector.

Since this was the only use of the non-concurrent pvector, it is
cleaner to revert the whole patch.

Reported-by: Jan Scheurich 
Signed-off-by: Jarno Rajahalme 
---
 lib/classifier.c|  30 
 lib/classifier.h|   6 +-
 lib/dpif-netdev.c   |  14 ++--
 lib/pvector.c   | 190 +++-
 lib/pvector.h   | 187 +++
 tests/test-classifier.c |  12 +--
 6 files changed, 182 insertions(+), 257 deletions(-)

diff --git a/lib/classifier.c b/lib/classifier.c
index 8f195d5..0551146 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -325,7 +325,7 @@ classifier_init(struct classifier *cls, const uint8_t 
*flow_segments)
 {
 cls->n_rules = 0;
 cmap_init(>subtables_map);
-cpvector_init(>subtables);
+pvector_init(>subtables);
 cls->n_flow_segments = 0;
 if (flow_segments) {
 while (cls->n_flow_segments < CLS_MAX_INDICES
@@ -359,7 +359,7 @@ classifier_destroy(struct classifier *cls)
 }
 cmap_destroy(>subtables_map);
 
-cpvector_destroy(>subtables);
+pvector_destroy(>subtables);
 }
 }
 
@@ -658,20 +658,20 @@ classifier_replace(struct classifier *cls, const struct 
cls_rule *rule,
 if (n_rules == 1) {
 subtable->max_priority = rule->priority;
 subtable->max_count = 1;
-cpvector_insert(>subtables, subtable, rule->priority);
+pvector_insert(>subtables, subtable, rule->priority);
 } else if (rule->priority == subtable->max_priority) {
 ++subtable->max_count;
 } else if (rule->priority > subtable->max_priority) {
 subtable->max_priority = rule->priority;
 subtable->max_count = 1;
-cpvector_change_priority(>subtables, subtable, rule->priority);
+pvector_change_priority(>subtables, subtable, rule->priority);
 }
 
 /* Nothing was replaced. */
 cls->n_rules++;
 
 if (cls->publish) {
-cpvector_publish(>subtables);
+pvector_publish(>subtables);
 }
 
 return NULL;
@@ -803,12 +803,12 @@ check_priority:
 }
 }
 subtable->max_priority = max_priority;
-cpvector_change_priority(>subtables, subtable, max_priority);
+pvector_change_priority(>subtables, subtable, max_priority);
 }
 }
 
 if (cls->publish) {
-cpvector_publish(>subtables);
+pvector_publish(>subtables);
 }
 
 /* free the rule. */
@@ -959,8 +959,8 @@ classifier_lookup__(const struct classifier *cls, 
ovs_version_t version,
 
 /* Main loop. */
 struct cls_subtable *subtable;
-CPVECTOR_FOR_EACH_PRIORITY (subtable, hard_pri + 1, 2, sizeof *subtable,
->subtables) {
+PVECTOR_FOR_EACH_PRIORITY (subtable, hard_pri + 1, 2, sizeof *subtable,
+   >subtables) {
 struct cls_conjunction_set *conj_set;
 
 /* Skip subtables with no match, or where the match is lower-priority
@@ -1231,8 +1231,8 @@ classifier_rule_overlaps(const struct classifier *cls,
 struct cls_subtable *subtable;
 
 /* Iterate subtables in the descending max priority order. */
-CPVECTOR_FOR_EACH_PRIORITY (subtable, target->priority, 2,
-sizeof(struct cls_subtable), >subtables) {
+PVECTOR_FOR_EACH_PRIORITY (subtable, target->priority, 2,
+   sizeof(struct cls_subtable), >subtables) {
 struct {
 struct minimask mask;
 uint64_t storage[FLOW_U64S];
@@ -1350,8 +1350,8 @@ cls_cursor_start(const struct classifier *cls, const 
struct cls_rule *target,
 cursor.rule = NULL;
 
 /* Find first rule. */
-CPVECTOR_CURSOR_FOR_EACH (subtable, ,
-  >subtables) {
+PVECTOR_CURSOR_FOR_EACH (subtable, ,
+ >subtables) {
 const struct cls_rule *rule = search_subtable(subtable, );
 
 if (rule) {
@@ -1378,7 +1378,7 @@ cls_cursor_next(struct cls_cursor *cursor)
 }
 }
 
-CPVECTOR_CURSOR_FOR_EACH_CONTINUE (subtable, >subtables) {
+PVECTOR_CURSOR_FOR_EACH_CONTINUE (subtable, >subtables) {
 rule = search_subtable(subtable, cursor);
 if (rule) {
 cursor->subtable = subtable;
@@ -1510,7 +1510,7 @@ destroy_subtable(struct classifier *cls, struct 
cls_subtable *subtable)
 {
 int i;
 
-cpvector_remove(>subtables, subtable);
+pvector_remove(>subtables, subtable);
 cmap_remove(>subtables_map, >cmap_node,

Re: [ovs-dev] [PATCH] Revert "pvector: Expose non-concurrent priority vector."

Re: [ovs-dev] [PATCH] Revert "pvector: Expose non-concurrent priority vector."

[ovs-dev] [PATCH] Revert "pvector: Expose non-concurrent priority vector."

3 matches

Site Navigation

Mail list logo

Footer information