Re: [ovs-dev] [PATCH v2 01/26] lib: Separate versioning to its own module.

[Ben Pfaff]

On Fri, Jul 29, 2016 at 11:42:13AM -0700, Jarno Rajahalme wrote:
> 
> > On Jul 29, 2016, at 11:06 AM, Ben Pfaff  wrote:
> > 
> > On Thu, Jul 28, 2016 at 05:55:53PM -0700, Jarno Rajahalme wrote:
> >> Separate rule versioning to lib/versions.h to make it easier to use
> >> versioning for other data types.
> >> 
> >> Signed-off-by: Jarno Rajahalme 
> > 
> > I wish this wasn't called "versions", because that sounds like the
> > version of Open vSwitch, but I don't have a really good name.
> > 
> 
> I struggled with this myself. We could call it "table_versions" but the we 
> might one day use it also for port configuration, for example.
> 
> Other possibility would be to call these "configuration versions" 
> "generations" - I believe this term is used elsewhere with similar meaning.

Hmm.  None of those are really great.

Let's just leave it for now.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 01/26] lib: Separate versioning to its own module.

[Jarno Rajahalme]

> On Jul 29, 2016, at 11:06 AM, Ben Pfaff  wrote:
> 
> On Thu, Jul 28, 2016 at 05:55:53PM -0700, Jarno Rajahalme wrote:
>> Separate rule versioning to lib/versions.h to make it easier to use
>> versioning for other data types.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> I wish this wasn't called "versions", because that sounds like the
> version of Open vSwitch, but I don't have a really good name.
> 

I struggled with this myself. We could call it "table_versions" but the we 
might one day use it also for port configuration, for example.

Other possibility would be to call these "configuration versions" "generations" 
- I believe this term is used elsewhere with similar meaning.

> I think that it would be a good idea to add a top-level explanatory
> comment to versions.h that explains what it is and how one would use
> it.  The functions and data structures in versions.h could also use
> brief comments.
> 

I'll add these.

> Acked-by: Ben Pfaff 
> 

Thanks!

  Jarno
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 01/26] lib: Separate versioning to its own module.

[Ben Pfaff]

On Thu, Jul 28, 2016 at 05:55:53PM -0700, Jarno Rajahalme wrote:
> Separate rule versioning to lib/versions.h to make it easier to use
> versioning for other data types.
> 
> Signed-off-by: Jarno Rajahalme 

I wish this wasn't called "versions", because that sounds like the
version of Open vSwitch, but I don't have a really good name.

I think that it would be a good idea to add a top-level explanatory
comment to versions.h that explains what it is and how one would use
it.  The functions and data structures in versions.h could also use
brief comments.

Acked-by: Ben Pfaff 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2 01/26] lib: Separate versioning to its own module.

[Jarno Rajahalme]

Separate rule versioning to lib/versions.h to make it easier to use
versioning for other data types.

Signed-off-by: Jarno Rajahalme 
---
 lib/automake.mk  |  1 +
 lib/classifier-private.h | 31 +--
 lib/classifier.c | 45 +--
 lib/classifier.h | 29 +++--
 lib/ovs-router.c |  6 ++--
 lib/tnl-ports.c  |  8 ++---
 lib/versions.h   | 74 
 ofproto/ofproto-dpif-xlate.c |  4 +--
 ofproto/ofproto-dpif.c   | 14 -
 ofproto/ofproto-dpif.h   |  4 +--
 ofproto/ofproto-provider.h   |  6 ++--
 ofproto/ofproto.c| 44 +-
 tests/test-classifier.c  | 46 +--
 tests/test-ovn.c |  4 +--
 utilities/ovs-ofctl.c|  2 +-
 15 files changed, 185 insertions(+), 133 deletions(-)
 create mode 100644 lib/versions.h

diff --git a/lib/automake.mk b/lib/automake.mk
index 4110e5f..f420a9c 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -279,6 +279,7 @@ lib_libopenvswitch_la_SOURCES = \
lib/vconn-provider.h \
lib/vconn-stream.c \
lib/vconn.c \
+   lib/versions.h \
lib/vlan-bitmap.c \
lib/vlan-bitmap.h \
lib/vlog.c \
diff --git a/lib/classifier-private.h b/lib/classifier-private.h
index eb47a4c..1d5ee00 100644
--- a/lib/classifier-private.h
+++ b/lib/classifier-private.h
@@ -72,13 +72,8 @@ struct cls_match {
 /* Accessed by all readers. */
 struct cmap_node cmap_node; /* Within struct cls_subtable 'rules'. */
 
-/* Rule versioning.
- *
- * CLS_NOT_REMOVED_VERSION has a special meaning for 'remove_version',
- * meaning that the rule has been added but not yet removed.
- */
-const cls_version_t add_version;/* Version rule was added in. */
-ATOMIC(cls_version_t) remove_version;   /* Version rule is removed in. */
+/* Rule versioning. */
+struct versions versions;
 
 const struct cls_rule *cls_rule;
 const struct miniflow flow; /* Matching rule. Mask is in the subtable. */
@@ -102,34 +97,22 @@ get_cls_match(const struct cls_rule *rule)
 void cls_match_free_cb(struct cls_match *);
 
 static inline void
-cls_match_set_remove_version(struct cls_match *rule, cls_version_t version)
+cls_match_set_remove_version(struct cls_match *rule, ovs_version_t version)
 {
-atomic_store_relaxed(>remove_version, version);
+versions_set_remove_version(>versions, version);
 }
 
 static inline bool
 cls_match_visible_in_version(const struct cls_match *rule,
- cls_version_t version)
+ ovs_version_t version)
 {
-cls_version_t remove_version;
-
-/* C11 does not want to access an atomic via a const object pointer. */
-atomic_read_relaxed(_CAST(struct cls_match *, rule)->remove_version,
-_version);
-
-return rule->add_version <= version && version < remove_version;
+return versions_visible_in_version(>versions, version);
 }
 
 static inline bool
 cls_match_is_eventually_invisible(const struct cls_match *rule)
 {
-cls_version_t remove_version;
-
-/* C11 does not want to access an atomic via a const object pointer. */
-atomic_read_relaxed(_CAST(struct cls_match *, rule)->remove_version,
-_version);
-
-return remove_version <= CLS_MAX_VERSION;
+return versions_is_eventually_invisible(>versions);
 }
 
 
diff --git a/lib/classifier.c b/lib/classifier.c
index 2b24724..d5fd759 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -84,7 +84,7 @@ cls_conjunction_set_alloc(struct cls_match *match,
 }
 
 static struct cls_match *
-cls_match_alloc(const struct cls_rule *rule, cls_version_t version,
+cls_match_alloc(const struct cls_rule *rule, ovs_version_t version,
 const struct cls_conjunction conj[], size_t n)
 {
 size_t count = miniflow_n_values(rule->match.flow);
@@ -95,9 +95,8 @@ cls_match_alloc(const struct cls_rule *rule, cls_version_t 
version,
 ovsrcu_init(_match->next, NULL);
 *CONST_CAST(const struct cls_rule **, _match->cls_rule) = rule;
 *CONST_CAST(int *, _match->priority) = rule->priority;
-*CONST_CAST(cls_version_t *, _match->add_version) = version;
-atomic_init(_match->remove_version, version);   /* Initially
- * invisible. */
+/* Make rule initially invisible. */
+cls_match->versions = VERSIONS_INITIALIZER(version, version);
 miniflow_clone(CONST_CAST(struct miniflow *, _match->flow),
rule->match.flow, count);
 ovsrcu_set_hidden(_match->conj_set,
@@ -113,7 +112,7 @@ static struct cls_subtable *insert_subtable(struct 
classifier *cls,
 static void destroy_subtable(struct classifier *cls, struct cls_subtable *);
 
 static const struct cls_match *find_match_wc(const struct cls_subtable *,
-