Acked-by: Ethan Jackson <et...@nicira.com>
On Thu, Sep 12, 2013 at 12:39 AM, Ben Pfaff <b...@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 <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
