[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