[ovs-dev] [PATCH v4 03/12] classifier: Support table versioning

[Ben Pfaff]

On Thu, Jun 11, 2015 at 03:41:53PM -0700, Jarno Rajahalme wrote:
> 
> > On Jun 11, 2015, at 3:00 PM, Gurucharan Shetty <shettyg at nicira.com> wrote:
> > 
> > On Tue, Jun 9, 2015 at 5:24 PM, Jarno Rajahalme <jrajahalme at nicira.com <mailto:jrajahalme at nicira.com>> wrote:
> >> This patch allows classifier rules to become visible and invisible in
> >> specific versions.  A 'version' is defined as a positive monotonically
> >> increasing integer, which never wraps around.
> >> 
> >> The new 'visibility' attribute replaces the prior 'to_be_removed' and
> >> 'visible' attributes.
> >> 
> >> When versioning is not used, the 'version' parameter should be passed
> >> as 'CLS_MIN_VERSION' when creating rules, and 'CLS_MAX_VERSION' when
> >> looking up flows.
> >> 
> >> This feature enables the support for atomic OpenFlow bundles without
> >> significant performance penalty on 64-bit systems. There is a
> >> performance decrease in 32-bit systems due to 64-bit atomics used.
> >> 
> >> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> > 
> > Looks like this patch causes ovs-vswitchd to crash in unit tests of
> > Windows with the following backtrace:
> > 
> >> ovs-vswitchd.exe!abort() Line 88 C
> >  ovs-vswitchd.exe!ofproto_dpif_add_internal_flow(ofproto_dpif *
> > ofproto, const match * match, int priority, unsigned short
> > idle_timeout, const ofpbuf * ofpacts, rule * * rulep) Line 5454 C
> 
> On master line ofproto-dpif.c:5452 is the OVS_NOT_REACHED();
> 
> I’ve seen this happen earlier while coding this, but this should not happen on master. The cause was that the internal rule was added for a next version, but the lookup was done with the old version, so the new rule was invisible. But on master the rule is added in version CLS_MIN_VERSION (= 1) and the lookup (that seems to fail) is done in CLS_MAX_VERSION (LLONG_MAX), so it should work.
> 
> You could try to modify the classifier_lookup line in ofproto-dpif.c to use a much lower number, in case something goes wrong with the atomics used within the classifier:
> 
> -        cls_rule = classifier_lookup(cls, CLS_MAX_VERSION, flow, wc);
> +        cls_rule = classifier_lookup(cls, 10, flow, wc);
> 
> In master the version number is not yet incremented, so this should work.

In C I think that enums always have type "int", but CLS_MAX_VERSION
is bigger than int:

enum {
    CLS_MIN_VERSION = 1,   /* Default version number to use. */
    CLS_MAX_VERSION = LLONG_MAX, /* Last possible version number. */
    CLS_MAX_INDICES = 3,   /* Maximum number of lookup indices per subtable. */
    CLS_MAX_TRIES = 3      /* Maximum number of prefix trees per classifier. */
};

Does it make any difference if you do something like this instead:
enum {
    CLS_MIN_VERSION = 1,   /* Default version number to use. */
#define CLS_MAX_VERSION LLONG_MAX /* Last possible version number. */
    CLS_MAX_INDICES = 3,   /* Maximum number of lookup indices per subtable. */
    CLS_MAX_TRIES = 3      /* Maximum number of prefix trees per classifier. */
};