> On Jun 12, 2015, at 3:21 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> 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 <jrajaha...@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.
> */
>
I like it when I learn new things during reviews :-)
Applied.
> 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);
>
Always nice to have tighter invariants, thanks!
This was actually the only use of CLS_NO_VERSION, so I removed it and made
CLS_MIN_VERSION to be 0.
> The 'version' member of cls_rule seems to have only marginal utility.
> Maybe we can remove it. No need to do that now though.
>
This thought occurred to me as well earlier today. Removing version from
cls_rule leads to a version parameter being necessary in many more classifier
API calls. However, that might be more informative, too.
I’ll see about this later.
> Acked-by: Ben Pfaff <b...@nicira.com>
Thanks for the review!
Applied to master,
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
