[ovs-dev] [threaded-learning v2 20/25] ofproto: Mark immutable members of struct rule 'const'.

[Ethan Jackson]

Acked-by: Ethan Jackson <ethan at nicira.com>


On Thu, Sep 12, 2013 at 12:39 AM, Ben Pfaff <blp at nicira.com> wrote:
> One difficult part of make flow_mods thread-safe is sorting out which
> members of each structure can be modified under which locks and,
> especially, documenting this in a way that makes it hard for programmers to
> get it wrong later.  The compiler provides some tools for us, however, and
> 'const' is one of the nicer ones since it is standardized rather than part
> of a compiler extension.
>
> This commit makes use of 'const' to mark the immutable members of struct
> rule, which is definitely the most confusing structure regarding thread
> safety simply because it has so many members that use different forms of
> synchronization.  It also adds a bunch of CONST_CASTs to allow these
> members to be initialized and destroyed where necessary.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ofproto/ofproto-dpif.c     |    2 +-
>  ofproto/ofproto-provider.h |   10 +++++++---
>  ofproto/ofproto.c          |   12 ++++++------
>  3 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 81adb0a..5b60e36 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4777,7 +4777,7 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
>                            const struct flow *flow, struct flow_wildcards *wc,
>                            uint8_t table_id, struct rule_dpif **rule)
>  {
> -    struct cls_rule *cls_rule;
> +    const struct cls_rule *cls_rule;
>      struct classifier *cls;
>      bool frag;
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 6583294e..a3640bd 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -227,8 +227,13 @@ struct oftable {
>   * With few exceptions, ofproto implementations may look at these fields but
>   * should not modify them. */
>  struct rule {
> -    struct ofproto *ofproto;     /* The ofproto that contains this rule. */
> -    struct cls_rule cr;          /* In owning ofproto's classifier. */
> +    /* Where this rule resides in an OpenFlow switch.
> +     *
> +     * These are immutable once the rule is constructed, hence 'const'. */
> +    struct ofproto *const ofproto; /* The ofproto that contains this rule. */
> +    const struct cls_rule cr;      /* In owning ofproto's classifier. */
> +    const uint8_t table_id;        /* Index in ofproto's 'tables' array. */
> +
>      atomic_uint ref_count;
>
>      struct ofoperation *pending; /* Operation now in progress, if nonnull. */
> @@ -240,7 +245,6 @@ struct rule {
>      long long int created;       /* Creation time. */
>      long long int modified;      /* Time of last modification. */
>      long long int used;          /* Last use; time created if never used. */
> -    uint8_t table_id;            /* Index in ofproto's 'tables' array. */
>      enum ofputil_flow_mod_flags flags;
>
>      uint16_t hard_timeout OVS_GUARDED; /* In seconds from ->modified. */
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 882d503..160d6b0 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2377,7 +2377,7 @@ ofproto_rule_unref(struct rule *rule)
>  static void
>  ofproto_rule_destroy__(struct rule *rule)
>  {
> -    cls_rule_destroy(&rule->cr);
> +    cls_rule_destroy(CONST_CAST(struct cls_rule *, &rule->cr));
>      rule_actions_unref(rule->actions);
>      ovs_mutex_destroy(&rule->mutex);
>      rule->ofproto->ofproto_class->rule_dealloc(rule);
> @@ -3759,8 +3759,8 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
>      }
>
>      /* Initialize base state. */
> -    rule->ofproto = ofproto;
> -    cls_rule_move(&rule->cr, &cr);
> +    *CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto;
> +    cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), &cr);
>      atomic_init(&rule->ref_count, 1);
>      rule->pending = NULL;
>      rule->flow_cookie = fm->new_cookie;
> @@ -3772,7 +3772,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn,
>      rule->hard_timeout = fm->hard_timeout;
>      ovs_mutex_unlock(&rule->mutex);
>
> -    rule->table_id = table - ofproto->tables;
> +    *CONST_CAST(uint8_t *, &rule->table_id) = table - ofproto->tables;
>      rule->flags = fm->flags & OFPUTIL_FF_STATE;
>      rule->actions = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
>      list_init(&rule->meter_list_node);
> @@ -6335,7 +6335,7 @@ oftable_remove_rule__(struct ofproto *ofproto, struct classifier *cls,
>                        struct rule *rule)
>      OVS_REQ_WRLOCK(cls->rwlock) OVS_RELEASES(rule->mutex)
>  {
> -    classifier_remove(cls, &rule->cr);
> +    classifier_remove(cls, CONST_CAST(struct cls_rule *, &rule->cr));
>
>      ovs_mutex_lock(&ofproto_mutex);
>      cookies_remove(ofproto, rule);
> @@ -6393,7 +6393,7 @@ oftable_insert_rule(struct rule *rule)
>          list_insert(&meter->rules, &rule->meter_list_node);
>      }
>      ovs_rwlock_wrlock(&table->cls.rwlock);
> -    classifier_insert(&table->cls, &rule->cr);
> +    classifier_insert(&table->cls, CONST_CAST(struct cls_rule *, &rule->cr));
>      ovs_rwlock_unlock(&table->cls.rwlock);
>      eviction_group_add_rule(rule);
>  }
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev