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. 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. */ 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.) Here's an incremental to make OFPACT_FOR_EACH_TYPE(_FLATTENED) more type-safe: 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
