I sent a new patch that addresses this and also simplifies the versioning inside the classifier (“classifier: Simplify versioning.”)
This patch is against the current master, not the one that failed on Windows. Jarno > On Jun 11, 2015, at 3:57 PM, Ben Pfaff <b...@nicira.com> wrote: > > On Thu, Jun 11, 2015 at 03:41:53PM -0700, Jarno Rajahalme wrote: >> >>> On Jun 11, 2015, at 3:00 PM, Gurucharan Shetty <shet...@nicira.com> wrote: >>> >>> On Tue, Jun 9, 2015 at 5:24 PM, Jarno Rajahalme <jrajaha...@nicira.com >>> <mailto:jrajaha...@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 <jrajaha...@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. */ > }; _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
