[ovs-dev] [PATCH v2 05/26] ofproto: Use ofproto_mutex for groups and keep track of referring flows.
Jarno Rajahalme
jarno at ovn.org
Fri Jul 29 20:19:03 UTC 2016
> On Jul 29, 2016, at 12:33 PM, Ben Pfaff <blp at 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 <jarno at 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,
>
More information about the dev
mailing list