[ovs-dev] [PATCH v5 1/4] classifier: Simplify versioning.

[Ben Pfaff]

On Fri, Jun 12, 2015 at 02:36:21PM -0700, Jarno Rajahalme wrote:
> After all, there are some cases in which both the insertion version
> and removal version of a rule need to be considered.  This makes the
> cls_match a bit bigger, but makes classifier versioning much simpler
> to understand.
> 
> Also, avoid using type larger than int in an enum, as it is not
> portable C.
> 
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

You could use this to avoid having to change CLS_MAX_VERSION and
CLS_NOT_REMOVED_VERSION if you change cls_version_t:

diff --git a/lib/classifier.h b/lib/classifier.h
index 9aec8b2..a25764e 100644
--- a/lib/classifier.h
+++ b/lib/classifier.h
@@ -308,6 +308,7 @@
 #include "meta-flow.h"
 #include "pvector.h"
 #include "rculist.h"
+#include "type-props.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -330,8 +331,8 @@ typedef uint64_t cls_version_t;
 
 #define CLS_NO_VERSION 0
 #define CLS_MIN_VERSION 1                  /* Default version number to use. */
-#define CLS_MAX_VERSION (UINT64_MAX - 1)
-#define CLS_NOT_REMOVED_VERSION UINT64_MAX
+#define CLS_MAX_VERSION (TYPE_MAXIMUM(cls_version_t) - 1)
+#define CLS_NOT_REMOVED_VERSION TYPE_MAXIMUM(cls_version_t)
 
 enum {
     CLS_MAX_INDICES = 3,   /* Maximum number of lookup indices per subtable. */

cls_match has an *almost*-invariant that add_version <= remove_version.
If we did the following, it would be a full invariant, I believe, and we
could remove CLS_NO_VERSION.  Using add_version == remove_version
implies to my mind that the rule was added and removed at the same time,
which seems a lot like it was never there.  Is it correct codewise too
(it passes the unit tests at least)?

diff --git a/lib/classifier.c b/lib/classifier.c
index 6487426..d76b3fb 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -100,7 +100,7 @@ cls_match_alloc(const struct cls_rule *rule,
     *CONST_CAST(const struct cls_rule **, &cls_match->cls_rule) = rule;
     *CONST_CAST(int *, &cls_match->priority) = rule->priority;
     *CONST_CAST(cls_version_t *, &cls_match->add_version) = rule->version;
-    atomic_init(&cls_match->remove_version, CLS_NO_VERSION);  /* Initially
+    atomic_init(&cls_match->remove_version, rule->version);  /* Initially
                                                                  invisible. */
     miniflow_clone_inline(CONST_CAST(struct miniflow *, &cls_match->flow),
                           &rule->match.flow, count);

The 'version' member of cls_rule seems to have only marginal utility.
Maybe we can remove it.  No need to do that now though.

Acked-by: Ben Pfaff <blp at nicira.com>