From dc43709014749ed924d4fcff63055320146cd018 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Tue, 11 Feb 2014 09:49:30 -0800 Subject: [PATCH] ofproto: Move 'rule->used' to the provider. Rule's 'used' timestamp is updated at the same time with the other stats. So far the 'used' has been updated without proper protection, which may lead to 'tearing' in 32-bit architectures, resulting in an incorrect 'used' timestamp. While in practice this is highly improbable, it is still better to handle this correctly. This is resolved by moving the 'used' field to the provider's stats, so that whatever protection is used for updating packet and byte counts, can be also used for both reading and writing of the 'used' timestamp. Signed-off-by: Jarno Rajahalme Signed-off-by: Ben Pfaff --- ofproto/ofproto-dpif.c | 61 ++++++++++++++++++++++++------------ ofproto/ofproto-provider.h | 5 +-- ofproto/ofproto.c | 64 ++++++++++++++++++++++---------------- 3 files changed, 79 insertions(+), 51 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index f2ae600cd..08570f1eb 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -89,9 +89,11 @@ struct rule_dpif { struct ovs_mutex stats_mutex; uint64_t packet_count OVS_GUARDED; /* Number of packets received. */ uint64_t byte_count OVS_GUARDED; /* Number of bytes received. */ + long long int used; /* Last used time (msec). */ }; -static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes); +static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes, + long long int *used); static struct rule_dpif *rule_dpif_cast(const struct rule *); static void rule_expire(struct rule_dpif *); @@ -1402,14 +1404,16 @@ get_tables(struct ofproto *ofproto_, struct ofp12_table_stats *ots) struct dpif_dp_stats s; uint64_t n_miss, n_no_pkt_in, n_bytes, n_dropped_frags; uint64_t n_lookup; + long long int used; strcpy(ots->name, "classifier"); dpif_get_dp_stats(ofproto->backer->dpif, &s); - rule_get_stats(&ofproto->miss_rule->up, &n_miss, &n_bytes); - rule_get_stats(&ofproto->no_packet_in_rule->up, &n_no_pkt_in, &n_bytes); - rule_get_stats(&ofproto->drop_frags_rule->up, &n_dropped_frags, &n_bytes); - + rule_get_stats(&ofproto->miss_rule->up, &n_miss, &n_bytes, &used); + rule_get_stats(&ofproto->no_packet_in_rule->up, &n_no_pkt_in, &n_bytes, + &used); + rule_get_stats(&ofproto->drop_frags_rule->up, &n_dropped_frags, &n_bytes, + &used); n_lookup = s.n_hit + s.n_missed - n_dropped_frags; ots->lookup_count = htonll(n_lookup); ots->matched_count = htonll(n_lookup - n_miss - n_no_pkt_in); @@ -2911,24 +2915,39 @@ static void rule_expire(struct rule_dpif *rule) OVS_REQUIRES(ofproto_mutex) { - uint16_t idle_timeout, hard_timeout; + uint16_t hard_timeout, idle_timeout; long long int now = time_msec(); - int reason; + int reason = -1; ovs_assert(!rule->up.pending); - /* Has 'rule' expired? */ - ovs_mutex_lock(&rule->up.mutex); hard_timeout = rule->up.hard_timeout; idle_timeout = rule->up.idle_timeout; - if (hard_timeout && now > rule->up.modified + hard_timeout * 1000) { - reason = OFPRR_HARD_TIMEOUT; - } else if (idle_timeout && now > rule->up.used + idle_timeout * 1000) { - reason = OFPRR_IDLE_TIMEOUT; - } else { - reason = -1; + + /* Has 'rule' expired? */ + if (hard_timeout) { + long long int modified; + + ovs_mutex_lock(&rule->up.mutex); + modified = rule->up.modified; + ovs_mutex_unlock(&rule->up.mutex); + + if (now > modified + hard_timeout * 1000) { + reason = OFPRR_HARD_TIMEOUT; + } + } + + if (reason < 0 && idle_timeout) { + long long int used; + + ovs_mutex_lock(&rule->stats_mutex); + used = rule->used; + ovs_mutex_unlock(&rule->stats_mutex); + + if (now > used + idle_timeout * 1000) { + reason = OFPRR_IDLE_TIMEOUT; + } } - ovs_mutex_unlock(&rule->up.mutex); if (reason >= 0) { COVERAGE_INC(ofproto_dpif_expired); @@ -2992,7 +3011,7 @@ rule_dpif_credit_stats(struct rule_dpif *rule, ovs_mutex_lock(&rule->stats_mutex); rule->packet_count += stats->n_packets; rule->byte_count += stats->n_bytes; - rule->up.used = MAX(rule->up.used, stats->used); + rule->used = MAX(rule->used, stats->used); ovs_mutex_unlock(&rule->stats_mutex); } @@ -3154,13 +3173,13 @@ rule_dealloc(struct rule *rule_) static enum ofperr rule_construct(struct rule *rule_) + OVS_NO_THREAD_SAFETY_ANALYSIS { struct rule_dpif *rule = rule_dpif_cast(rule_); ovs_mutex_init(&rule->stats_mutex); - ovs_mutex_lock(&rule->stats_mutex); rule->packet_count = 0; rule->byte_count = 0; - ovs_mutex_unlock(&rule->stats_mutex); + rule->used = rule->up.modified; return 0; } @@ -3188,13 +3207,15 @@ rule_destruct(struct rule *rule_) } static void -rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes) +rule_get_stats(struct rule *rule_, uint64_t *packets, uint64_t *bytes, + long long int *used) { struct rule_dpif *rule = rule_dpif_cast(rule_); ovs_mutex_lock(&rule->stats_mutex); *packets = rule->packet_count; *bytes = rule->byte_count; + *used = rule->used; ovs_mutex_unlock(&rule->stats_mutex); } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 77ca498cd..08a8ac16f 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -398,9 +398,6 @@ struct rule { /* Must hold 'mutex' for both read/write, 'ofproto_mutex' not needed. */ long long int modified OVS_GUARDED; /* Time of last modification. */ - - /* XXX: Currently updated by provider without protection. */ - long long int used OVS_GUARDED; /* Last use; time created if never used. */ }; void ofproto_rule_ref(struct rule *); @@ -1287,7 +1284,7 @@ struct ofproto_class { * in '*byte_count'. UINT64_MAX indicates that the packet count or byte * count is unknown. */ void (*rule_get_stats)(struct rule *rule, uint64_t *packet_count, - uint64_t *byte_count) + uint64_t *byte_count, long long int *used) /* OVS_EXCLUDED(ofproto_mutex) */; /* Applies the actions in 'rule' to 'packet'. (This implements sending diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 2578f6457..62c97a21b 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -182,7 +182,7 @@ struct eviction_group { static bool choose_rule_to_evict(struct oftable *table, struct rule **rulep); static void ofproto_evict(struct ofproto *) OVS_EXCLUDED(ofproto_mutex); -static uint32_t rule_eviction_priority(struct rule *); +static uint32_t rule_eviction_priority(struct ofproto *ofproto, struct rule *); static void eviction_group_add_rule(struct rule *); static void eviction_group_remove_rule(struct rule *); @@ -3601,20 +3601,20 @@ handle_flow_stats_request(struct ofconn *ofconn, fs.idle_timeout = rule->idle_timeout; fs.hard_timeout = rule->hard_timeout; created = rule->created; - used = rule->used; modified = rule->modified; actions = rule_get_actions__(rule); flags = rule->flags; ovs_mutex_unlock(&rule->mutex); + ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count, + &fs.byte_count, &used); + minimatch_expand(&rule->cr.match, &fs.match); fs.table_id = rule->table_id; calc_duration(created, now, &fs.duration_sec, &fs.duration_nsec); fs.priority = rule->cr.priority; fs.idle_age = age_secs(now - used); fs.hard_age = age_secs(now - modified); - ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count, - &fs.byte_count); fs.ofpacts = actions->ofpacts; fs.ofpacts_len = actions->ofpacts_len; @@ -3637,10 +3637,10 @@ flow_stats_ds(struct rule *rule, struct ds *results) { uint64_t packet_count, byte_count; struct rule_actions *actions; - long long int created; + long long int created, used; - rule->ofproto->ofproto_class->rule_get_stats(rule, - &packet_count, &byte_count); + rule->ofproto->ofproto_class->rule_get_stats(rule, &packet_count, + &byte_count, &used); ovs_mutex_lock(&rule->mutex); actions = rule_get_actions__(rule); @@ -3751,9 +3751,10 @@ handle_aggregate_stats_request(struct ofconn *ofconn, struct rule *rule = rules.rules[i]; uint64_t packet_count; uint64_t byte_count; + long long int used; ofproto->ofproto_class->rule_get_stats(rule, &packet_count, - &byte_count); + &byte_count, &used); if (packet_count == UINT64_MAX) { unknown_packets = true; @@ -4055,7 +4056,7 @@ add_flow(struct ofproto *ofproto, struct ofconn *ofconn, ovs_refcount_init(&rule->ref_count); rule->pending = NULL; rule->flow_cookie = fm->new_cookie; - rule->created = rule->modified = rule->used = time_msec(); + rule->created = rule->modified = time_msec(); ovs_mutex_init(&rule->mutex); ovs_mutex_lock(&rule->mutex); @@ -4345,6 +4346,7 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t reason) OVS_REQUIRES(ofproto_mutex) { struct ofputil_flow_removed fr; + long long int used; if (ofproto_rule_is_hidden(rule) || !(rule->flags & OFPUTIL_FF_SEND_FLOW_REM)) { @@ -4363,7 +4365,7 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t reason) fr.hard_timeout = rule->hard_timeout; ovs_mutex_unlock(&rule->mutex); rule->ofproto->ofproto_class->rule_get_stats(rule, &fr.packet_count, - &fr.byte_count); + &fr.byte_count, &used); connmgr_send_flow_removed(rule->ofproto->connmgr, &fr); } @@ -6235,7 +6237,7 @@ ofopgroup_complete(struct ofopgroup *group) ovs_mutex_lock(&rule->mutex); rule->modified = now; if (op->type == OFOPERATION_REPLACE) { - rule->created = rule->used = now; + rule->created = now; } ovs_mutex_unlock(&rule->mutex); } else { @@ -6600,26 +6602,34 @@ eviction_group_find(struct oftable *table, uint32_t id) /* Returns an eviction priority for 'rule'. The return value should be * interpreted so that higher priorities make a rule more attractive candidates - * for eviction. */ + * for eviction. + * Called only if have a timeout. */ static uint32_t -rule_eviction_priority(struct rule *rule) +rule_eviction_priority(struct ofproto *ofproto, struct rule *rule) OVS_REQUIRES(ofproto_mutex) { - long long int hard_expiration; - long long int idle_expiration; - long long int expiration; + long long int expiration = LLONG_MAX; + long long int modified; uint32_t expiration_offset; - /* Calculate time of expiration. */ + /* 'modified' needs protection even when we hold 'ofproto_mutex'. */ ovs_mutex_lock(&rule->mutex); - hard_expiration = (rule->hard_timeout - ? rule->modified + rule->hard_timeout * 1000 - : LLONG_MAX); - idle_expiration = (rule->idle_timeout - ? rule->used + rule->idle_timeout * 1000 - : LLONG_MAX); - expiration = MIN(hard_expiration, idle_expiration); + modified = rule->modified; ovs_mutex_unlock(&rule->mutex); + + if (rule->hard_timeout) { + expiration = modified + rule->hard_timeout * 1000; + } + if (rule->idle_timeout) { + uint64_t packets, bytes; + long long int used; + long long int idle_expiration; + + ofproto->ofproto_class->rule_get_stats(rule, &packets, &bytes, &used); + idle_expiration = used + rule->idle_timeout * 1000; + expiration = MIN(expiration, idle_expiration); + } + if (expiration == LLONG_MAX) { return 0; } @@ -6649,9 +6659,9 @@ eviction_group_add_rule(struct rule *rule) struct oftable *table = &ofproto->tables[rule->table_id]; bool has_timeout; - ovs_mutex_lock(&rule->mutex); + /* Timeouts may be modified only when holding 'ofproto_mutex'. We have it + * so no additional protection is needed. */ has_timeout = rule->hard_timeout || rule->idle_timeout; - ovs_mutex_unlock(&rule->mutex); if (table->eviction_fields && has_timeout) { struct eviction_group *evg; @@ -6660,7 +6670,7 @@ eviction_group_add_rule(struct rule *rule) rule->eviction_group = evg; heap_insert(&evg->rules, &rule->evg_node, - rule_eviction_priority(rule)); + rule_eviction_priority(ofproto, rule)); eviction_group_resized(table, evg); } } -- 2.43.0