> On Jul 29, 2016, at 12:33 PM, Ben Pfaff <b...@ovn.org> wrote: > > On Thu, Jul 28, 2016 at 05:55:57PM -0700, Jarno Rajahalme wrote: >> Adding groups support for bundles is simpler if also groups are >> modified under ofproto_mutex. >> >> Eliminate the search for rules when deleting a group so that we will >> not keep the mutex for too long. >> >> Signed-off-by: Jarno Rajahalme <ja...@ovn.org> > > It's confusing that rule_collection_remove() and > rule_collection_remove_postponed() have unrelated effects. >
We want a verb that is the opposite of "add", how about rule_collection_subtract(), rule_collection_retract() or rule_collection_withdraw()? Of should we be more careful about naming functions that change change the rule collection vs. operate on the rules in the collection? E.g., rename rule_collection_add() as rule_collection_add_rule() and rule_collection_remove() as rule_collection_remove_rule(). Functions named like "rule_collection_VERB()" would then be reserved for functions that VERB on each rule? > In rule_collection_remove(), regarding the comment below, I think that > the comparison is to ensure hysteresis, not to avoid it, e.g. we want > the kind of behavior described at > https://en.wikipedia.org/wiki/Hysteresis#Control_systems: > > /* Shrink? Watermark at '/ 4' to avoid hysteresis, but leave spare > * capacity. */ > Right. I flipped the meaning in my head. > This adds an OVS_EXCLUDED annotation to ofproto_group_delete_all(), > which is a non-static function, so it should also add the annotation to > its prototype in ofproto-provider.h. There might be other functions in > the same boat, didn't notice. (This also applies to static functions if > there's a prototype at the top of the .c file; Clang is not smart enough > to merge all the annotations and so you miss warnings if you don't > annotate every prototype and definition.) > Will fix and check all annotation changes for consistency. > Here's an incremental to make OFPACT_FOR_EACH_TYPE(_FLATTENED) more > type-safe: > I like this, will fold in! Jarno > diff --git a/include/openvswitch/ofp-actions.h > b/include/openvswitch/ofp-actions.h > index 6355eed..42764b1 100644 > --- a/include/openvswitch/ofp-actions.h > +++ b/include/openvswitch/ofp-actions.h > @@ -227,6 +227,9 @@ ofpact_find_type(const struct ofpact *a, enum ofpact_type > type, > return NULL; > } > > +#define OFPACT_FIND_TYPE(A, TYPE, END) \ > + ofpact_get_##TYPE##_nullable(ofpact_find_type(A, OFPACT_##TYPE, END)) > + > static inline const struct ofpact * > ofpact_find_type_flattened(const struct ofpact *a, enum ofpact_type type, > const struct ofpact * const end) > @@ -240,6 +243,10 @@ ofpact_find_type_flattened(const struct ofpact *a, enum > ofpact_type type, > return NULL; > } > > +#define OFPACT_FIND_TYPE_FLATTENED(A, TYPE, END) \ > + ofpact_get_##TYPE##_nullable( \ > + ofpact_find_type_flattened(A, OFPACT_##TYPE, END)) > + > /* Assigns POS to each ofpact, in turn, in the OFPACTS_LEN bytes of ofpacts > * starting at OFPACTS. */ > #define OFPACT_FOR_EACH(POS, OFPACTS, OFPACTS_LEN) \ > @@ -247,10 +254,10 @@ ofpact_find_type_flattened(const struct ofpact *a, enum > ofpact_type type, > (POS) = ofpact_next(POS)) > > #define OFPACT_FOR_EACH_TYPE(POS, TYPE, OFPACTS, OFPACTS_LEN) \ > - for ((POS) = ofpact_find_type(OFPACTS, TYPE, \ > + for ((POS) = OFPACT_FIND_TYPE(OFPACTS, TYPE, \ > ofpact_end(OFPACTS, OFPACTS_LEN)); \ > (POS); \ > - (POS) = ofpact_find_type(ofpact_next(POS), TYPE, \ > + (POS) = OFPACT_FIND_TYPE(ofpact_next(&(POS)->ofpact), TYPE, \ > ofpact_end(OFPACTS, OFPACTS_LEN))) > > /* Assigns POS to each ofpact, in turn, in the OFPACTS_LEN bytes of ofpacts > @@ -263,11 +270,12 @@ ofpact_find_type_flattened(const struct ofpact *a, enum > ofpact_type type, > (POS) = ofpact_next_flattened(POS)) > > #define OFPACT_FOR_EACH_TYPE_FLATTENED(POS, TYPE, OFPACTS, OFPACTS_LEN) \ > - for ((POS) = ofpact_find_type_flattened(OFPACTS, TYPE, \ > + for ((POS) = OFPACT_FIND_TYPE_FLATTENED(OFPACTS, TYPE, \ > ofpact_end(OFPACTS, OFPACTS_LEN)); \ > (POS); \ > - (POS) = ofpact_find_type_flattened(ofpact_next_flattened(POS), > TYPE, \ > - ofpact_end(OFPACTS, OFPACTS_LEN))) > + (POS) = OFPACT_FIND_TYPE_FLATTENED( \ > + ofpact_next_flattened(&(POS)->ofpact), TYPE, \ > + ofpact_end(OFPACTS, OFPACTS_LEN))) > > /* Action structure for each OFPACT_*. */ > > @@ -1017,6 +1025,13 @@ void *ofpact_finish(struct ofpbuf *, struct ofpact *); > } \ > \ > static inline struct STRUCT * \ > + ofpact_get_##ENUM##_nullable(const struct ofpact *ofpact) \ > + { \ > + ovs_assert(!ofpact || ofpact->type == OFPACT_##ENUM); \ > + return ALIGNED_CAST(struct STRUCT *, ofpact); \ > + } \ > + \ > + static inline struct STRUCT * \ > ofpact_put_##ENUM(struct ofpbuf *ofpacts) \ > { \ > return ofpact_put(ofpacts, OFPACT_##ENUM, \ > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 7542fc3..e7fd6bb 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -3445,7 +3445,6 @@ enum ofperr > ofproto_check_ofpacts(struct ofproto *ofproto, > const struct ofpact ofpacts[], size_t ofpacts_len) > { > - const struct ofpact *a; > uint32_t mid; > > mid = ofpacts_get_meter(ofpacts, ofpacts_len); > @@ -3453,9 +3452,9 @@ ofproto_check_ofpacts(struct ofproto *ofproto, > return OFPERR_OFPMMFC_INVALID_METER; > } > > - OFPACT_FOR_EACH_TYPE_FLATTENED (a, OFPACT_GROUP, ofpacts, ofpacts_len) { > - if (!ofproto_group_exists(ofproto, > - ofpact_get_GROUP(a)->group_id)) { > + const struct ofpact_group *a; > + OFPACT_FOR_EACH_TYPE_FLATTENED (a, GROUP, ofpacts, ofpacts_len) { > + if (!ofproto_group_exists(ofproto, a->group_id)) { > return OFPERR_OFPBAC_BAD_OUT_GROUP; > } > } > @@ -8022,14 +8021,12 @@ ofproto_rule_insert__(struct ofproto *ofproto, struct > rule *rule) > meter_insert_rule(rule); > } > if (actions->has_groups) { > - const struct ofpact *a; > - > - OFPACT_FOR_EACH_TYPE_FLATTENED (a, OFPACT_GROUP, actions->ofpacts, > + const struct ofpact_group *a; > + OFPACT_FOR_EACH_TYPE_FLATTENED (a, GROUP, actions->ofpacts, > actions->ofpacts_len) { > struct ofgroup *group; > > - group = ofproto_group_lookup(ofproto, > - ofpact_get_GROUP(a)->group_id, > false); > + group = ofproto_group_lookup(ofproto, a->group_id, false); > ovs_assert(group != NULL); > group_add_rule(group, rule); > } > @@ -8062,14 +8059,13 @@ ofproto_rule_remove__(struct ofproto *ofproto, struct > rule *rule) > const struct rule_actions *actions = rule_get_actions(rule); > > if (actions->has_groups) { > - const struct ofpact *a; > + const struct ofpact_group *a; > > - OFPACT_FOR_EACH_TYPE_FLATTENED (a, OFPACT_GROUP, actions->ofpacts, > + OFPACT_FOR_EACH_TYPE (a, GROUP, actions->ofpacts, > actions->ofpacts_len) { > struct ofgroup *group; > > - group = ofproto_group_lookup(ofproto, > - ofpact_get_GROUP(a)->group_id, > false); > + group = ofproto_group_lookup(ofproto, a->group_id, false); > ovs_assert(group); > > /* Leave the rule for the group that is being deleted, if any, > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
